Platform: Code4rena
Start Date: 29/04/2022
Pot Size: $22,000 USDC
Total HM: 6
Participants: 40
Period: 3 days
Judge: Justin Goro
Total Solo HM: 2
Id: 114
League: ETH
Rank: 14/40
Findings: 2
Award: $377.45
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: MaratCerby
Also found by: CertoraInc, IllIllI, berndartmueller, cccz, reassor
Contract AaveV3YieldSource allows depositing tokens via supplyTokenTo function. Amount of tokens to transfer is based on passed argument _depositAmount and is missing support for tokens with built-in fees. One of the popular tokens that implements such a fee (but currently is set to 0) is USDT.
The severity of vulnerability has been raised to medium since the "areas of concerns" includes shares/tokens peg at 1:1 ratio.
Manual Review / VSCode
It is recommended to add support for ERC20 tokens with built-in fees. Example of the implementation:
(..) uint256 _beforeBalance = _underlyingAssetAddress.balanceOf(address(this)); IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount); uint256 _afterBalance = _underlyingAssetAddress.balanceOf(address(this)); uint256 receivedTokens = _afterBalance - _beforeBalance (..)
#0 - PierrickGT
2022-05-03T22:57:39Z
Low
Contract AaveV3YieldSource is using SafeMath library for uint256. The contract is prepared for solidity 0.8.10 and using SafeMath is obsolete since the overflow/underflow security checks are built-in and done automatically for solidity versions >= 0.8.0 for all calculations unless marked with unchecked {}.
Manual Review / VSCode
It is recommended to remove SafeMath from the contract.
Low
Constructor AaveV3YieldSource.constructor allows setting decimals value in constructor. Based on the comments in the code the value of decimals should be equal to the decimals of the token used to deposit into the pool. Setting it by the user who is deploying the contract makes the process prone to errors.
Comment in AaveV3YieldSource contract:
* @dev This value should be equal to the decimals of the token used to deposit into the pool.
Manual Review / VSCode
It is recommended to set _decimals by making external call to aToken.decimals().
Non-Critical
Contract AaveV3YieldSource has multiple storage addresses that are only set in constructor but are not marked as immutable.
IAToken public aToken - https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L127IRewardsController public rewardsController - https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L130IPoolAddressesProviderRegistry public poolAddressesProviderRegistry - https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L133Manual Review / VSCode
It is recommended to mark listed storage variables as immutable.
Non-Critical
Function AaveV3YieldSource.transferERC20 is checking if the passed _token address argument is not a aToken address. Since there is already defined function _requireNotAToken that performs such a check it is better to reuse existing implementation.
Manual Review / VSCode
It is recommended to use _requireNotAToken in AaveV3YieldSource.transferERC20 function.
#0 - PierrickGT
2022-05-04T16:03:43Z
- Obsolete usage of SafeMath
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/11
- Invalid setting decimals
As mentioned in a previous issue, we set this value in our deploy script, this way we are sure the Ticket contract and the yield source share the same number of decimals.
- Use immutable storage variables
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/1
- Redundant code for checking aToken address
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/4