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: 4/12
Findings: 6
Award: $2,002.31
🌟 Selected for report: 3
🚀 Solo Findings: 0
565.8571 USDC - $565.86
cmichel
The SwappableYieldSource.redeemToken function transfers tokens from the contract back to the sender, however, it uses the ERC20.transferFrom(address(this), msg.sender, redeemableBalance) function for this.
Some deposit token implementations might fail as transferFrom checks if the contract approved itself for the redeemableBalance instead of skipping the allowance check in case the sender is the from address.
This can make the transaction revert and the deposited funds will be unrecoverable for the user.
It's recommended to use _depositToken.safeTransfer(msg.sender, redeemableBalance) instead.
#0 - PierrickGT
2021-08-06T16:18:15Z
#1 - 0xean
2021-08-24T16:37:03Z
re-opening this issue and marking #25 as a duplicate of this issue which clearly articulates the potential severity of unrecoverable user funds.
#2 - PierrickGT
2021-08-30T15:37:14Z
This issue has been fixed and we are now using safeTransfer: https://github.com/pooltogether/swappable-yield-source/blob/bf943b3818b81d5f5cb9d8ecc6f13ffecd33a1ff/contracts/SwappableYieldSource.sol#L235
169.7571 USDC - $169.76
cmichel
Description: safeApprove is now deprecated, see this comment.
As per OpenZepplin documentation “whenever possible, use safeIncreaseAllowance and safeDecreaseAllowance instead”.
#0 - PierrickGT
2021-08-06T16:41:11Z
419.1534 USDC - $419.15
cmichel
There are ERC20 tokens that may make certain customizations to their ERC20 contracts.
One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().
Others are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).
The MStableYieldSource/SwappableYieldSource.supplyTokenTo() function will fail when the underlying token is a fee-on transfer token.
One possible mitigation is to measure the asset change right before and after the asset-transferring routines
#0 - PierrickGT
2021-08-11T23:23:41Z
🌟 Selected for report: tensors
Also found by: GalloDaSballo, cmichel, hickuphh3
169.7571 USDC - $169.76
cmichel
The SwappableYieldSource._setYieldSource function approves the new yield source but does not reset the old yield source's approval to zero.
It could be the case that the old yield source is compromised or has a bug and could therefore transfer and steal any deposit token in the contract.
It's recommended to reset the approval for the old yield source to zero.
#0 - PierrickGT
2021-08-06T16:10:31Z
139.7178 USDC - $139.72
cmichel
The SwappableYieldSource.freeze function does not do anything.
Unused code can hint at programming or architectural errors.
#0 - PierrickGT
2021-08-06T16:21:10Z
🌟 Selected for report: cmichel
310.484 USDC - $310.48
cmichel
The _requireYieldSource function performs a low-level status code and parses the return data even if the call failed as it does not check the first return value (success).
It could be the case that non-zero data is returned even though the call failed, and the function would return true.
Check the return value or perform a high-level call using the _yieldSource interface.
#0 - PierrickGT
2021-08-12T21:29:06Z
As we noticed while testing the contract, staticcall will return the first return value bool success at true, even if we pass a random wallet address instead of a yield source.
That's why we have decided to check for depositTokenAddressData.length and the address returned instead of simply relying on success.
isValidYieldSource being initialized at false and the fact that we check the return value, I doubt the function would return true if non zero data is returned from the staticcall and the call failed.
#1 - 0xean
2021-08-24T17:12:50Z
would recommend following best practices with staticcall regardless and still check the boolean return value. Its a trivial amount of gas in a function that wont be called frequently.
#2 - PierrickGT
2021-08-30T22:45:16Z
This issue has been fixed in the following commit: https://github.com/pooltogether/swappable-yield-source/pull/9/commits/f4cfedc4665dad4635e92a9f96ec9130055dd44d
🌟 Selected for report: hickuphh3
Also found by: 0xRajeev, GalloDaSballo, cmichel, pauliax
26.3992 USDC - $26.40
cmichel
The SwappableYieldSource contract could cache the deposit token in a storage variable once in initialize and when the yield source changes, instead of always doing an external call to get it IERC20Upgradeable(yieldSource.depositToken()):
supplyTokenToredeemToken_setYieldSource#0 - PierrickGT
2021-08-06T16:31:03Z
🌟 Selected for report: cmichel
201.183 USDC - $201.18
cmichel
The SwappableYieldSource.swapYieldSource function receives a _newYieldSource as a parameter and reads a _currentYieldSource from storage. A single storage read should therefore be enough for the entire function and sub-calls.
However, the _transferFunds function reads the new yield source from storage again, performing a second storage read. This can be optimized by _transferFunds taking an oldYieldSource and newYieldSource as parameters instead.
#0 - PierrickGT
2021-08-12T22:10:14Z