Platform: Code4rena
Start Date: 29/07/2021
Pot Size: $20,000 USDC
Total HM: 8
Participants: 12
Period: 3 days
Judge: LSDan
Total Solo HM: 2
Id: 24
League: ETH
Rank: 5/12
Findings: 4
Award: $1,606.53
🌟 Selected for report: 2
🚀 Solo Findings: 0
565.8571 USDC - $565.86
pauliax
function redeemToken sends tokens to the msg.sender by using safeTransferFrom: _depositToken.safeTransferFrom(address(this), msg.sender, redeemableBalance); For safeTransferFrom to work it needs to have an enough approval. In this case, obviously this contract does not approve every msg.sender for redeemableBalance, so I expect this function to fail in practice. When the sender is address(this) it is best to use safeTransfer function: _depositToken.safeTransfer(msg.sender, redeemableBalance);
#0 - PierrickGT
2021-08-06T16:16:20Z
#1 - 0xean
2021-08-24T16:39:11Z
duplicate of #61
169.7571 USDC - $169.76
pauliax
function approveMax uses safeApprove. This function only works if the current approval is 0. Consider clearing previous approval ( safeApprove(0) ) before setting the max value again. The same issue can happen with SwappableYieldSource if, for example, source A is set but later changed to source B (_setYieldSource) and later you want to set source A again. safeApprove should fail as A already has approval. I think it would also make sense to clear approval of the old yield source when _setYieldSource is invoked as this old source becomes inactive so you don't want it to still have the approval to transfer the tokens.
#0 - PierrickGT
2021-08-06T16:43:38Z
139.7178 USDC - $139.72
pauliax
Function setYieldSource should use _requireYieldSource to ensure that the new yield source is valid. Now _requireYieldSource is only used in the constructor when setting the initial yield source.
#0 - PierrickGT
2021-08-06T15:56:10Z
83.8307 USDC - $83.83
pauliax
isInvalidYieldSource in function _requireYieldSource actually has a reverted meaning, when it is true, the source is treated as valid: require(isInvalidYieldSource, "SwappableYieldSource/invalid-yield-source"); Rename it to isValidYieldSource.
#0 - PierrickGT
2021-08-06T15:52:12Z
🌟 Selected for report: pauliax
310.484 USDC - $310.48
pauliax
Tokens sent directly to the MStableYieldSource will be stuck forever. Consider adding a function that allows an admin to retrieve stuck tokens:
#0 - PierrickGT
2021-08-17T00:14:14Z
#1 - kamescg
2021-08-17T04:20:00Z
LGTM
🌟 Selected for report: pauliax
310.484 USDC - $310.48
pauliax
function supplyTokenTo should check that mAssetAmount and creditsIssued > 0 and to != address(0) or if empty to address is provided, it can replace it with msg.sender to prevent potential burn of funds. function redeemToken should check that mAssetAmount and creditsBurned > 0. function transferERC20 should similarly validate erc20Token, to and amount parameters. function _mintShares requires that shares > 0, while _burnShares lacks such requirement.
#0 - PierrickGT
2021-08-16T20:53:46Z
This report barely describes which functions or contract should be fixed. This is why I disputed the issue. A proof of concept should have at least been written down and it would have been perfect if recommended mitigation steps were provided in a clear and precise manner.
#1 - 0xean
2021-08-24T16:18:44Z
Issue is very poorly written. Warden should take time to clearly articulate the issue and impact.
That being said, I do agree that a check on MStableYieldSource.supplyTokenTo to ensure the to != address(0) is reasonable. The mAsset may or may not implement this check, but it would be useful to avoid potential loss of funds.
#2 - PierrickGT
2021-08-30T15:24:18Z
This issue has been fixed in the following PR: https://github.com/pooltogether/pooltogether-mstable/pull/10
🌟 Selected for report: hickuphh3
Also found by: 0xRajeev, GalloDaSballo, cmichel, pauliax
26.3992 USDC - $26.40
pauliax
function _setYieldSource can cache _newYieldSource.depositToken() to eliminate a duplicate call and thus save some gas.
#0 - PierrickGT
2021-08-06T16:31:22Z