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: 6/40
Findings: 3
Award: $893.04
🌟 Selected for report: 2
🚀 Solo Findings: 0
🌟 Selected for report: MaratCerby
Also found by: CertoraInc, IllIllI, berndartmueller, cccz, reassor
309.1634 USDC - $309.16
Every time transferFrom or transfer function in ERC20 standard is called there is a possibility that underlying smart contract did not transfer the exact amount entered. It is required to find out contract balance increase/decrease after the transfer. This pattern also prevents from re-entrancy attack vector.
Recommended code: function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant { uint256 _shares = _tokenToShares(_depositAmount); require(_shares > 0, "AaveV3YS/shares-gt-zero");
address _underlyingAssetAddress = _tokenAddress(); uint256 balanceBefore = IERC20(_underlyingAssetAddress).balanceOf(address(this)); // remembering asset balance before the transfer IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount); _depositAmount = IERC20(_underlyingAssetAddress).balanceOf(address(this)) - balanceBefore; // updating actual amount to the contract balance increase _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE); _mint(_to, _shares); emit SuppliedTokenTo(msg.sender, _shares, _depositAmount, _to);
}
#0 - PierrickGT
2022-05-02T21:01:51Z
This scenario would only happen if we use fee on transfer tokens. We don't plan to support fee on transfer token, so for this reason, I have acknowledged the issue but we won't implement the code suggestion.
#1 - gititGoro
2022-05-18T03:08:53Z
This could lead to protocol leakage rather than risk of total funds loss. Downgrading to Medium Risk. Reentrancy isn't an issue since the function is marked nonReentrant.
uint256 is assigned to zero by default, additional reassignment to zero is unnecessary Affected code: https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L142
https://docs.soliditylang.org/en/v0.8.13/control-structures.html#default-value
Recommended code: uint256 private constant ADDRESSES_PROVIDER_ID;
Since solidity v0.8.0 SafeMath library is used by default in arithmetic operations. Using external SafeMath libraries is unnecessary.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L361
Recommended code: return _supply == 0 ? _tokens : (_tokens * _supply) / aToken.balanceOf(address(this));
Since solidity v0.8.0 SafeMath library is used by default in arithmetic operations. Using external SafeMath libraries is unnecessary.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L373
Recommended code: return _supply == 0 ? _shares : (_shares * aToken.balanceOf(address(this))) / _supply;
#0 - PierrickGT
2022-05-02T22:37:09Z
uint256 is assigned to zero by default, additional reassignment to zero is unnecessary Constant variables need to be initialized, so it is not possible to declare the variable without assigning a value.
SafeMath has been removed in the following PR: https://github.com/pooltogether/aave-v3-yield-source/pull/5
Great report by this warden that should get extra point for his recommandation to remove SafeMath.
#1 - PierrickGT
2022-05-02T22:37:20Z
#2 - gititGoro
2022-05-24T22:29:06Z
There were two issues reported. Both were non-critical
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xf15ers, 0xkatana, 242, Dravee, GimelSec, MaratCerby, Tadashi, TrungOre, WatchPug, defsec, fatherOfBlocks, gzeon, hake, horsefacts, joestakey, miguelmtzinf, pauliax, pedroais, peritoflores, rotcivegaf, simon135, slywaters, tabish, throttle, z3s
27.8625 USDC - $27.86
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L168
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error RC_NotZeroAddress(); .. if(address(_aToken) == address(0)) { revert RC_NotZeroAddress(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L171
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error RC_NotZeroAddress(); .. if(address(_rewardsController) == address(0)) { revert RC_NotZeroAddress(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L174
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error RC_NotZeroAddress(); .. if(address(_poolAddressesProviderRegistry) == address(0)) { revert RC_NotZeroAddress(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L177
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error Owner_NotZeroAddress(); .. if(_owner == address(0)) { revert Owner_NotZeroAddress(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L179
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error DecimalsGtZero(); .. if(decimals_ == 0) { revert DecimalsGtZero(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L233
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error SharesGtZero(); .. if(_shares == 0) { revert SharesGtZero(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L276
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error PayeeNotZeroAddress(); .. if(_to == address(0)) { revert PayeeNotZeroAddress(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L337
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error Forbid_aToken_Transfer(); .. if(address(_token) == address(aToken)) { revert Forbid_aToken_Transfer(); }
As per 0.8.4 solidity version it supports new custom errors. Custom errors are reducing 38 gas if condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Affected code: https://github.com/pooltogether/aave-v3-yield-source/tree/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L349
https://blog.soliditylang.org/2021/04/21/custom-errors/
Recommended code: error Forbid_aToken_Allowance(); .. if(_token == address(aToken)) { revert Forbid_aToken_Allowance(); }
#0 - PierrickGT
2022-05-02T22:39:50Z
We prefer to use require on one line instead of custom errors on several lines. For this reason, I've acknowledged the issue but we won't use custom errors.