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: 1/40
Findings: 5
Award: $5,204.03
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: MaratCerby
Also found by: CertoraInc, IllIllI, berndartmueller, cccz, reassor
Some ERC20 tokens, such as USDT, allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance. Note that Aave currently supports USDT.
The current implementation reverts whenever a fee-on-transfer asset token is deposited.
The amount specified during deposit successfully gets transferred to the AaveV3YieldSource but if a fee is charged, the balance actually held will be less than _depositAmount and the call to _pool().supply() will revert due to lack of balance.
File: contracts/AaveV3YieldSource.sol 231 function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant { 232 uint256 _shares = _tokenToShares(_depositAmount); 233 require(_shares > 0, "AaveV3YS/shares-gt-zero"); 234 235 address _underlyingAssetAddress = _tokenAddress(); 236 IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount); 237 _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE);
Code inspection
Measure the balance before and after the call to safeTransferFrom(), and use the difference between the two as the amount, rather than the amount stated
#0 - PierrickGT
2022-05-04T19:37:34Z
A malicious owner or manager can steal all Aave rewards that are meant for PoolTogether users
Even if the user is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation.
File: contracts/AaveV3YieldSource.sol #X 275 function claimRewards(address _to) external onlyManagerOrOwner returns (bool) { 276 require(_to != address(0), "AaveV3YS/payee-not-zero-address"); 277 278 address[] memory _assets = new address[](1); 279 _assets[0] = address(aToken); 280 281 (address[] memory _rewardsList, uint256[] memory _claimedAmounts) = rewardsController 282 .claimAllRewards(_assets, _to);
the claimRewards() function allows the caller to send the rewards to an arbitrary address
Code inspection
Use a poolAddressesProviderRegistry-like contract to determine where the rewards should go, instead of letting an address be passed in
#0 - PierrickGT
2022-05-04T19:38:33Z
🌟 Selected for report: IllIllI
3141.4256 USDC - $3,141.43
The aTokens’ value is pegged to the value of the corresponding supplied asset at a 1:1 ratio and can be safely stored, transferred or traded. All yield collected by the aTokens' reserves are distributed to aToken holders directly by continuously increasing their wallet balance.
https://docs.aave.com/developers/tokens/atoken
Incorrect share conversions lead to incorrect pricig of assets and loss of principal. aTokens are rebasing tokens, which means that holders of the token have their balanceof() increase over time, but each token is still redeemable for exactly one underlying asset. Any formula that does not return one out for one in is incorrect.
File: contracts/AaveV3YieldSource.sol #X 352 /** 353 * @notice Calculates the number of shares that should be minted or burnt when a user deposit or withdraw. 354 * @param _tokens Amount of asset tokens 355 * @return Number of shares. 356 */ 357 function _tokenToShares(uint256 _tokens) internal view returns (uint256) { 358 uint256 _supply = totalSupply(); 359 360 // shares = (tokens * totalShares) / yieldSourceATokenTotalSupply 361 return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this))); 362 } 363 364 /** 365 * @notice Calculates the number of asset tokens a user has in the yield source. 366 * @param _shares Amount of shares 367 * @return Number of asset tokens. 368 */ 369 function _sharesToToken(uint256 _shares) internal view returns (uint256) { 370 uint256 _supply = totalSupply(); 371 372 // tokens = (shares * yieldSourceATokenTotalSupply) / totalShares 373 return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply); 374 }
The above code is used for both supplyTokenTo() and redeemToken() and does not return one for one. Consider the following chain of events:
balanceOf(this)(tokens{1} * totalSupply(){1}) / aToken.balanceOf(this){2} is zeroCode inspection
There does not need to be a conversion function - one share must always equal one token
#0 - PierrickGT
2022-05-04T19:31:39Z
This is a plausible but unlikely scenario since it assumes that only Alice has deposited into the yield source and that the interest rate is so high that 1 more wBTC is now in the yield source.
Also, the calculation is wrong since 1 wBTC would represent 1 ether or 1000000000000000000 wei. So the actual calculation would return (1000000000000000000 * 1000000000000000000) / 2000000000000000000 = 500000000000000000 wei or 0.5 wBTC.
We also periodically capture the interest accumulated in the yield source by calling the PrizeFlush contract: https://github.com/pooltogether/v4-periphery/blob/8cc717af440aa23e41e0f24d699647320efd7b04/contracts/PrizeFlush.sol#L105 The capture of the interest happens here: https://github.com/pooltogether/v4-core/blob/e6f2d9ddb303cc4fb1a64582b12ba3e9df85e21c/contracts/prize-strategy/PrizeSplitStrategy.sol#L51
For these reasons, I've acknowledged the issue but we won't change how shares are calculated.
#1 - gititGoro
2022-05-19T02:27:12Z
Downgraded severity as this is more of a value leakage situation, especially given the unlikely edge case of an ERC20 token that sets decimals to 0 and uses low base values.
68.2909 USDC - $68.29
safeApprove() is deprecatedDeprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead
File: contracts/AaveV3YieldSource.sol #1 183 IERC20(_tokenAddress()).safeApprove(address(_pool()), type(uint256).max);
require()sThe comment below should be enforced with require(decimals_ == _aToken.decimals()). If this seems excessive, then why require decimals_ be passed in at all? Why isn't _aToken.decimals() stored instead?
File: contracts/AaveV3YieldSource.sol #1 156 * @param decimals_ Number of decimals the shares (inhereted ERC20) will have. Same as underlying asset to ensure sane exchange rates for shares.
File: contracts/AaveV3YieldSource.sol #1 360 // shares = (tokens * totalShares) / yieldSourceATokenTotalSupply 361 return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this)));
should be // shares = (tokens * totalSupply) / yieldSourceBalanceOfAToken
https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L360-L361
File: contracts/AaveV3YieldSource.sol #2 372 // tokens = (shares * yieldSourceATokenTotalSupply) / totalShares 373 return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply);
should be // tokens = (shares * yieldSourceBalanceOfAToken) / totalSupply
https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L372-L373
There is already a check in one of the functions that the final token amount is not zero, but it would be better to check the input amount first in all functions that take in an amount
File: contracts/AaveV3YieldSource.sol #1 231 function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant {
File: contracts/AaveV3YieldSource.sol #2 251 function redeemToken(uint256 _redeemAmount) external override nonReentrant returns (uint256) {
File: contracts/AaveV3YieldSource.sol #3 332 function transferERC20( 333 IERC20 _token, 334 address _to, 335 uint256 _amount 336 ) external onlyManagerOrOwner {
safeApprove() be based on a constructor argumentApprovals are only needed if doing flash loans or liquidations. If these are not used by the strategy, there is no need for the approval, which will lower the attack surface.
File: contracts/AaveV3YieldSource.sol #1 183 IERC20(_tokenAddress()).safeApprove(address(_pool()), type(uint256).max);
The compiler warns about this issue during compilation. Add the view visibility to resolve the warning
File: contracts/AaveV3YieldSource.sol #1 203 function balanceOfToken(address _user) external override returns (uint256) {
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
File: contracts/AaveV3YieldSource.sol #1 211 function depositToken() public view override returns (address) {
In the case below _token is an address whereas in all other instances, _token is an IERC20. Changing the name of the variable to something like _tokenAddr will make the code more readable and consistent
File: contracts/AaveV3YieldSource.sol #1 348 function _requireNotAToken(address _token) internal view {
File: contracts/AaveV3YieldSource.sol #1 38 * @param decimals Number of decimals the shares (inhereted ERC20) will have. Same as underlying asset to ensure sane exchange rates for shares.
File: contracts/AaveV3YieldSource.sol #2 156 * @param decimals_ Number of decimals the shares (inhereted ERC20) will have. Same as underlying asset to ensure sane exchange rates for shares.
A lot of the NatSpec/comments add a period to the end of fragments. Periods should only be used when there is both a noun phrase and a verb phrase
File: contracts/AaveV3YieldSource.sol (various lines) #1
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
File: contracts/AaveV3YieldSource.sol #1 3 pragma solidity 0.8.10;
The line below should use _requireNotAToken() but it doesn't because that function's revert() string specifically refers to the 'allowance' functions. The function NatSpec doesn't mention this fact. If the function wants different strings based on where it's called from, it can use msg.sig to choose the right one. An even better approach would be to have a custom error instead of a revert string, and include the msg.sig as an argument to the error.
File: contracts/AaveV3YieldSource.sol #1 337 require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer");
The transferERC20() function is sufficient for handling unexpected tokens; the increase/decrease allowance functions aren't useful. Approval isn't required for the contract itself to do the transfer when told to do it, but the increase function requires a second operation to actually do the transfer. Even if there is a case where funds can be moved by an existing contract's functionality, that contract might pass along its own token to this contract, starting another issue. The increase/decrease functions just add an extra attack surface and should just be removed.
File: contracts/AaveV3YieldSource.sol #1 315 function increaseERC20Allowance( 316 IERC20 _token, 317 address _spender, 318 uint256 _amount 319 ) external onlyManagerOrOwner { 320 _requireNotAToken(address(_token)); 321 _token.safeIncreaseAllowance(_spender, _amount); 322 emit IncreasedERC20Allowance(msg.sender, _spender, _amount, _token); 323 } 324 325 /** 326 * @notice Transfer ERC20 tokens other than the aTokens held by this contract to the recipient address. 327 * @dev This function is only callable by the owner or asset manager. 328 * @param _token Address of the ERC20 token to transfer 329 * @param _to Address of the recipient of the tokens 330 * @param _amount Amount of tokens to transfer 331 */ 332 function transferERC20( 333 IERC20 _token, 334 address _to, 335 uint256 _amount 336 ) external onlyManagerOrOwner { 337 require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer"); 338 _token.safeTransfer(_to, _amount); 339 emit TransferredERC20(msg.sender, _to, _amount, _token); 340 }
The instances below say that the argument is an address but they're in fact all variables of type contract. Internally solidity translates contracts to addresses when passing them to abi calls and when emitting events, but the compiler requires the specific user-defined type and errors if a simple address is provided without a cast instead.
File: contracts/AaveV3YieldSource.sol #1 33 * @param aToken Aave aToken address 34 * @param rewardsController Aave rewardsController address 35 * @param poolAddressesProviderRegistry Aave poolAddressesProviderRegistry address
File: contracts/AaveV3YieldSource.sol #2 87 * @param token Address of the ERC20 token to decrease allowance for
File: contracts/AaveV3YieldSource.sol #3 101 * @param token Address of the ERC20 token to increase allowance for
File: contracts/AaveV3YieldSource.sol #4 115 * @param token Address of the ERC20 token transferred
File: contracts/AaveV3YieldSource.sol #5 126 /// @notice Yield-bearing Aave aToken address.
File: contracts/AaveV3YieldSource.sol #6 129 /// @notice Aave RewardsController address.
File: contracts/AaveV3YieldSource.sol #7 132 /// @notice Aave poolAddressesProviderRegistry address.
File: contracts/AaveV3YieldSource.sol #8 151 * @param _aToken Aave aToken address
File: contracts/AaveV3YieldSource.sol #9 152 * @param _rewardsController Aave rewardsController address
indexed fieldsEach event should use three indexed fields if there are three or more fields
File: contracts/AaveV3YieldSource.sol #1 41 event AaveV3YieldSourceInitialized( 42 IAToken indexed aToken, 43 IRewardsController rewardsController, 44 IPoolAddressesProviderRegistry poolAddressesProviderRegistry, 45 string name, 46 string symbol, 47 uint8 decimals, 48 address owner 49 );
File: contracts/AaveV3YieldSource.sol #2 58 event SuppliedTokenTo(address indexed from, uint256 shares, uint256 amount, address indexed to);
File: contracts/AaveV3YieldSource.sol #3 66 event RedeemedToken(address indexed from, uint256 shares, uint256 amount);
File: contracts/AaveV3YieldSource.sol #4 75 event Claimed( 76 address indexed from, 77 address indexed to, 78 address[] rewardsList, 79 uint256[] claimedAmounts 80 );
Code should follow the best-practice of check-effects-interaction
Reentrancy in AaveV3YieldSource.supplyTokenTo(uint256,address) (contracts/AaveV3YieldSource.sol#231-242): #1 External calls: - IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender,address(this),_depositAmount) (contracts/AaveV3YieldSource.sol#236) - _pool().supply(_underlyingAssetAddress,_depositAmount,address(this),REFERRAL_CODE) (contracts/AaveV3YieldSource.sol#237) State variables written after the call(s): - _mint(_to,_shares) (contracts/AaveV3YieldSource.sol#239) - _totalSupply += amount (node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#262)
Reentrancy in AaveV3YieldSource.supplyTokenTo(uint256,address) (contracts/AaveV3YieldSource.sol#231-242): #2 External calls: - IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender,address(this),_depositAmount) (contracts/AaveV3YieldSource.sol#236) - _pool().supply(_underlyingAssetAddress,_depositAmount,address(this),REFERRAL_CODE) (contracts/AaveV3YieldSource.sol#237) State variables written after the call(s): - _mint(_to,_shares) (contracts/AaveV3YieldSource.sol#239) - _balances[account] += amount (node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#263)
Reentrancy in AaveV3YieldSource.claimRewards(address) (contracts/AaveV3YieldSource.sol#275-286): #3 External calls: - (_rewardsList,_claimedAmounts) = rewardsController.claimAllRewards(_assets,_to) (contracts/AaveV3YieldSource.sol#281-282) Event emitted after the call(s): - Claimed(msg.sender,_to,_rewardsList,_claimedAmounts) (contracts/AaveV3YieldSource.sol#284)
Reentrancy in AaveV3YieldSource.decreaseERC20Allowance(IERC20,address,uint256) (contracts/AaveV3YieldSource.sol#296-304): #4 External calls: - _token.safeDecreaseAllowance(_spender,_amount) (contracts/AaveV3YieldSource.sol#302) Event emitted after the call(s): - DecreasedERC20Allowance(msg.sender,_spender,_amount,_token) (contracts/AaveV3YieldSource.sol#303)
Reentrancy in AaveV3YieldSource.increaseERC20Allowance(IERC20,address,uint256) (contracts/AaveV3YieldSource.sol#315-323): #5 External calls: - _token.safeIncreaseAllowance(_spender,_amount) (contracts/AaveV3YieldSource.sol#321) Event emitted after the call(s): - IncreasedERC20Allowance(msg.sender,_spender,_amount,_token) (contracts/AaveV3YieldSource.sol#322)
Reentrancy in AaveV3YieldSource.redeemToken(uint256) (contracts/AaveV3YieldSource.sol#251-267): #6 External calls: - _pool().withdraw(_underlyingAssetAddress,_redeemAmount,address(this)) (contracts/AaveV3YieldSource.sol#259) - _assetToken.safeTransfer(msg.sender,_balanceDiff) (contracts/AaveV3YieldSource.sol#263) Event emitted after the call(s): - RedeemedToken(msg.sender,_shares,_redeemAmount) (contracts/AaveV3YieldSource.sol#265)
Reentrancy in AaveV3YieldSource.supplyTokenTo(uint256,address) (contracts/AaveV3YieldSource.sol#231-242): #7 External calls: - IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender,address(this),_depositAmount) (contracts/AaveV3YieldSource.sol#236) - _pool().supply(_underlyingAssetAddress,_depositAmount,address(this),REFERRAL_CODE) (contracts/AaveV3YieldSource.sol#237) Event emitted after the call(s): - SuppliedTokenTo(msg.sender,_shares,_depositAmount,_to) (contracts/AaveV3YieldSource.sol#241) - Transfer(address(0),account,amount) (node_modules/@openzeppelin/contracts/token/ERC20/ERC20.sol#264) - _mint(_to,_shares) (contracts/AaveV3YieldSource.sol#239)
Reentrancy in AaveV3YieldSource.transferERC20(IERC20,address,uint256) (contracts/AaveV3YieldSource.sol#332-340): #8 External calls: - _token.safeTransfer(_to,_amount) (contracts/AaveV3YieldSource.sol#338) Event emitted after the call(s): - TransferredERC20(msg.sender,_to,_amount,_token) (contracts/AaveV3YieldSource.sol#339)
#0 - PierrickGT
2022-05-04T18:47:47Z
- safeApprove() is deprecated
Perfectly safe to use to approve from a zero allowance.
- Comments should be enforced with require()s
As explained previously, we set it in our deploy script. Seem redundant to add this require.
- Formula does not match what the code is doing
Will keep totalShares since it's more descriptive than total supply.
PR: https://github.com/pooltogether/aave-v3-yield-source/pull/13
- Revert if amount is zero
This is a redundant check.
- Consider making whether to safeApprove() be based on a constructor argument
Needed to avoid having to approve each time a user deposits.
- Function state mutability can be restricted to view
- public functions not called by the contract should be declared external instead
We can't because we inherit from the yield source interface.
- Inconsistent variable-name-to-variable-type usage
The address keyword is enough to know that an address needs to be passed.
- Typos
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/22
- Grammar
- Natspec descriptions incorrect
Picky suggestions. We won't fix.
- Use a more recent version of solidity
The version used is the one used by Aave
- Function behavior doesn't match name
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/4
- Unneeded functions
This functions are needed to retrieve eventual ERC20 token that would be stuck in the yield source and also increase the Aave pool allowance.
- Event is missing indexed fields
No need to index everything
- Non-exploitable re-entrancies
Looks like a generated report with no clear solution provided.
🌟 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
271.514 USDC - $271.51
immutableAvoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas)
File: contracts/AaveV3YieldSource.sol #1 127 IAToken public aToken;
File: contracts/AaveV3YieldSource.sol #2 130 IRewardsController public rewardsController;
File: contracts/AaveV3YieldSource.sol #3 133 IPoolAddressesProviderRegistry public poolAddressesProviderRegistry;
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..1d229ff 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -124,13 +124,13 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { /* ============ Variables ============ */ /// @notice Yield-bearing Aave aToken address. - IAToken public aToken; + IAToken public immutable aToken; /// @notice Aave RewardsController address. - IRewardsController public rewardsController; + IRewardsController public immutable rewardsController; /// @notice Aave poolAddressesProviderRegistry address. - IPoolAddressesProviderRegistry public poolAddressesProviderRegistry; + IPoolAddressesProviderRegistry public immutable poolAddressesProviderRegistry; /// @notice ERC20 token decimals. uint8 private immutable _decimals;
diff --git a/gas.orig b/gas.new index d87edc2..a6cd51d 100644 --- a/gas.orig +++ b/gas.new @@ -5,21 +5,21 @@ ·····························|··························|·············|·············|·············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · usd (avg) │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · claimRewards · 54735 · 56896 · 55816 · 4 · - │ +| AaveV3YieldSourceHarness · claimRewards · 50521 · 52682 · 51602 · 4 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · decreaseERC20Allowance · 37465 · 39638 · 38910 · 3 · - │ +| AaveV3YieldSourceHarness · decreaseERC20Allowance · 35374 · 37547 · 36819 · 3 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · increaseERC20Allowance · 59133 · 61618 · 61212 · 7 · - │ +| AaveV3YieldSourceHarness · increaseERC20Allowance · 57042 · 59527 · 59121 · 7 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · mint · 51381 · 68493 · 61904 · 13 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · redeemToken · - · - · 110713 · 1 · - │ +| AaveV3YieldSourceHarness · redeemToken · - · - · 106359 · 1 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · setManager · - · - · 47982 · 4 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · supplyTokenTo · 110668 · 153720 · 142217 · 6 · - │ +| AaveV3YieldSourceHarness · supplyTokenTo · 106314 · 149466 · 137930 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · transferERC20 · 58318 · 60467 · 59393 · 2 · - │ +| AaveV3YieldSourceHarness · transferERC20 · 56227 · 58376 · 57302 · 2 · - │ ·····························|··························|·············|·············|·············|···············|·············· | ERC20Mintable · approve · 26692 · 46592 · 43275 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2497750 · 2497762 · 2497760 · 8.3 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
File: contracts/AaveV3YieldSource.sol #1 369 function _sharesToToken(uint256 _shares) internal view returns (uint256) {
File: contracts/AaveV3YieldSource.sol #2 388 function _poolProvider() internal view returns (IPoolAddressesProvider) {
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..d7ec7ec 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -201,7 +201,12 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { * @return The underlying balance of asset tokens. */ function balanceOfToken(address _user) external override returns (uint256) { - return _sharesToToken(balanceOf(_user)); + uint256 _shares = balanceOf(_user); + uint256 _supply = totalSupply(); + + // tokens = (shares * yieldSourceATokenTotalSupply) / totalShares + return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply); + } /** @@ -361,18 +366,6 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this))); } - /** - * @notice Calculates the number of asset tokens a user has in the yield source. - * @param _shares Amount of shares - * @return Number of asset tokens. - */ - function _sharesToToken(uint256 _shares) internal view returns (uint256) { - uint256 _supply = totalSupply(); - - // tokens = (shares * yieldSourceATokenTotalSupply) / totalShares - return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply); - } - /** * @notice Returns the underlying asset token address. * @return Underlying asset token address. @@ -381,22 +374,13 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { return aToken.UNDERLYING_ASSET_ADDRESS(); } - /** - * @notice Retrieves Aave PoolAddressesProvider address. - * @return A reference to PoolAddressesProvider interface. - */ - function _poolProvider() internal view returns (IPoolAddressesProvider) { - return - IPoolAddressesProvider( - poolAddressesProviderRegistry.getAddressesProvidersList()[ADDRESSES_PROVIDER_ID] - ); - } - /** * @notice Retrieves Aave Pool address. * @return A reference to Pool interface. */ function _pool() internal view returns (IPool) { - return IPool(_poolProvider().getPool()); + return IPool(IPoolAddressesProvider( + poolAddressesProviderRegistry.getAddressesProvidersList()[ADDRESSES_PROVIDER_ID] + ).getPool()); } }
diff --git a/gas.orig b/gas.new index d87edc2..6948266 100644 --- a/gas.orig +++ b/gas.new @@ -13,11 +13,11 @@ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · mint · 51381 · 68493 · 61904 · 13 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · redeemToken · - · - · 110713 · 1 · - │ +| AaveV3YieldSourceHarness · redeemToken · - · - · 110678 · 1 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · setManager · - · - · 47982 · 4 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · supplyTokenTo · 110668 · 153720 · 142217 · 6 · - │ +| AaveV3YieldSourceHarness · supplyTokenTo · 110633 · 153685 · 142182 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · transferERC20 · 58318 · 60467 · 59393 · 2 · - │ ·····························|··························|·············|·············|·············|···············|·············· @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2559043 · 2559055 · 2559053 · 8.5 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
> 0 costs more gas than != 0 when used on a uint in a require() statementThis change saves 6 gas per instance
File: contracts/AaveV3YieldSource.sol #1 179 require(decimals_ > 0, "AaveV3YS/decimals-gt-zero");
File: contracts/AaveV3YieldSource.sol #2 233 require(_shares > 0, "AaveV3YS/shares-gt-zero");
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..e00ad47 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -176,7 +176,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { require(_owner != address(0), "AaveV3YS/owner-not-zero-address"); - require(decimals_ > 0, "AaveV3YS/decimals-gt-zero"); + require(decimals_ != 0, "AaveV3YS/decimals-gt-zero"); _decimals = decimals_; // Approve once for max amount @@ -230,7 +230,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { */ function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant { uint256 _shares = _tokenToShares(_depositAmount); - require(_shares > 0, "AaveV3YS/shares-gt-zero"); + require(_shares != 0, "AaveV3YS/shares-gt-zero"); address _underlyingAssetAddress = _tokenAddress(); IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount);
diff --git a/gas.orig b/gas.new index d87edc2..9a90ad3 100644 --- a/gas.orig +++ b/gas.new @@ -17,7 +17,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · setManager · - · - · 47982 · 4 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · supplyTokenTo · 110668 · 153720 · 142217 · 6 · - │ +| AaveV3YieldSourceHarness · supplyTokenTo · 110662 · 153714 · 142211 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · transferERC20 · 58318 · 60467 · 59393 · 2 · - │ ·····························|··························|·············|·············|·············|···············|·············· @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2505106 · 2505118 · 2505116 · 8.4 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: contracts/AaveV3YieldSource.sol #1 47 uint8 decimals,
File: contracts/AaveV3YieldSource.sol #2 136 uint8 private immutable _decimals;
File: contracts/AaveV3YieldSource.sol #3 145 uint16 private constant REFERRAL_CODE = uint16(188);
File: contracts/AaveV3YieldSource.sol #4 165 uint8 decimals_,
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..0dfa477 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -44,7 +44,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { IPoolAddressesProviderRegistry poolAddressesProviderRegistry, string name, string symbol, - uint8 decimals, + uint256 decimals, address owner ); @@ -133,7 +133,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { IPoolAddressesProviderRegistry public poolAddressesProviderRegistry; /// @notice ERC20 token decimals. - uint8 private immutable _decimals; + uint256 private immutable _decimals; /** * @dev Aave genesis market PoolAddressesProvider's ID. @@ -142,7 +142,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { uint256 private constant ADDRESSES_PROVIDER_ID = uint256(0); /// @dev PoolTogether's Aave Referral Code - uint16 private constant REFERRAL_CODE = uint16(188); + uint256 private constant REFERRAL_CODE = 188; /* ============ Constructor ============ */ @@ -162,7 +162,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { IPoolAddressesProviderRegistry _poolAddressesProviderRegistry, string memory _name, string memory _symbol, - uint8 decimals_, + uint256 decimals_, address _owner ) Ownable(_owner) ERC20(_name, _symbol) ReentrancyGuard() { require(address(_aToken) != address(0), "AaveV3YS/aToken-not-zero-address"); @@ -218,7 +218,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { * @return The number of decimals. */ function decimals() public view virtual override returns (uint8) { - return _decimals; + return uint8(_decimals); } /** @@ -234,7 +234,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { address _underlyingAssetAddress = _tokenAddress(); IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount); - _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE); + _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), uint16(REFERRAL_CODE)); _mint(_to, _shares);
diff --git a/gas.orig b/gas.new index d87edc2..367daee 100644 --- a/gas.orig +++ b/gas.new @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2505659 · 2505671 · 2505669 · 8.4 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
SafeMath once the solidity version is 0.8.0 or greaterVersion 0.8.0 introduces internal overflow checks, so using SafeMath is redundant and adds overhead
File: contracts/AaveV3YieldSource.sol #1 262 uint256 _balanceDiff = _afterBalance.sub(_beforeBalance);
File: contracts/AaveV3YieldSource.sol #2 361 return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this)));
File: contracts/AaveV3YieldSource.sol #3 373 return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply);
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..6346344 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -11,7 +11,6 @@ import { IRewardsController } from "@aave/periphery-v3/contracts/rewards/interfa import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import { SafeMath } from "@openzeppelin/contracts/utils/math/SafeMath.sol"; import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; import { Manageable, Ownable } from "@pooltogether/owner-manager-contracts/contracts/Manageable.sol"; @@ -23,7 +22,6 @@ import { IYieldSource } from "@pooltogether/yield-source-interface/contracts/IYi * @notice Yield Source for a PoolTogether prize pool that generates yield by depositing into Aave V3. */ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { - using SafeMath for uint256; using SafeERC20 for IERC20; /* ============ Events ============ */ @@ -259,7 +257,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { _pool().withdraw(_underlyingAssetAddress, _redeemAmount, address(this)); uint256 _afterBalance = _assetToken.balanceOf(address(this)); - uint256 _balanceDiff = _afterBalance.sub(_beforeBalance); + uint256 _balanceDiff = _afterBalance - _beforeBalance; _assetToken.safeTransfer(msg.sender, _balanceDiff); emit RedeemedToken(msg.sender, _shares, _redeemAmount); @@ -358,7 +356,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { uint256 _supply = totalSupply(); // shares = (tokens * totalShares) / yieldSourceATokenTotalSupply - return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this))); + return _supply == 0 ? _tokens : _tokens * _supply / aToken.balanceOf(address(this)); } /** @@ -370,7 +368,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { uint256 _supply = totalSupply(); // tokens = (shares * yieldSourceATokenTotalSupply) / totalShares - return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply); + return _supply == 0 ? _shares : _shares * aToken.balanceOf(address(this)) / _supply; } /**
diff --git a/gas.orig b/gas.new index d87edc2..1c91b72 100644 --- a/gas.orig +++ b/gas.new @@ -13,11 +13,11 @@ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · mint · 51381 · 68493 · 61904 · 13 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · redeemToken · - · - · 110713 · 1 · - │ +| AaveV3YieldSourceHarness · redeemToken · - · - · 110584 · 1 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · setManager · - · - · 47982 · 4 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · supplyTokenTo · 110668 · 153720 · 142217 · 6 · - │ +| AaveV3YieldSourceHarness · supplyTokenTo · 110584 · 153720 · 142189 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · transferERC20 · 58318 · 60467 · 59393 · 2 · - │ ·····························|··························|·············|·············|·············|···············|·············· @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2497329 · 2497341 · 2497339 · 8.3 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
require() or revert() statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables
File: contracts/AaveV3YieldSource.sol #1 177 require(_owner != address(0), "AaveV3YS/owner-not-zero-address");
File: contracts/AaveV3YieldSource.sol #2 179 require(decimals_ > 0, "AaveV3YS/decimals-gt-zero");
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..5d7d9fe 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -166,17 +166,16 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { address _owner ) Ownable(_owner) ERC20(_name, _symbol) ReentrancyGuard() { require(address(_aToken) != address(0), "AaveV3YS/aToken-not-zero-address"); + require(_owner != address(0), "AaveV3YS/owner-not-zero-address"); + require(decimals_ > 0, "AaveV3YS/decimals-gt-zero"); + require(address(_rewardsController) != address(0), "AaveV3YS/RC-not-zero-address"); + require(address(_poolAddressesProviderRegistry) != address(0), "AaveV3YS/PR-not-zero-address"); aToken = _aToken; - require(address(_rewardsController) != address(0), "AaveV3YS/RC-not-zero-address"); rewardsController = _rewardsController; - require(address(_poolAddressesProviderRegistry) != address(0), "AaveV3YS/PR-not-zero-address"); poolAddressesProviderRegistry = _poolAddressesProviderRegistry; - require(_owner != address(0), "AaveV3YS/owner-not-zero-address"); - - require(decimals_ > 0, "AaveV3YS/decimals-gt-zero"); _decimals = decimals_; // Approve once for max amount
diff --git a/gas.orig b/gas.new index d87edc2..e76a03d 100644 --- a/gas.orig +++ b/gas.new @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2505548 · 2505560 · 2505558 · 8.4 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
revert()/require() strings to save gasFile: contracts/AaveV3YieldSource.sol #1 168 require(address(_aToken) != address(0), "AaveV3YS/aToken-not-zero-address");
File: contracts/AaveV3YieldSource.sol #2 171 require(address(_rewardsController) != address(0), "AaveV3YS/RC-not-zero-address");
File: contracts/AaveV3YieldSource.sol #3 174 require(address(_poolAddressesProviderRegistry) != address(0), "AaveV3YS/PR-not-zero-address");
File: contracts/AaveV3YieldSource.sol #4 177 require(_owner != address(0), "AaveV3YS/owner-not-zero-address");
File: contracts/AaveV3YieldSource.sol #5 179 require(decimals_ > 0, "AaveV3YS/decimals-gt-zero");
File: contracts/AaveV3YieldSource.sol #6 233 require(_shares > 0, "AaveV3YS/shares-gt-zero");
File: contracts/AaveV3YieldSource.sol #7 276 require(_to != address(0), "AaveV3YS/payee-not-zero-address");
File: contracts/AaveV3YieldSource.sol #8 337 require(_token != address(aToken), "AaveV3YS/forbid-aToken-transfer");
File: contracts/AaveV3YieldSource.sol #9 349 require(_token != address(aToken), "AaveV3YS/forbid-aToken-allowance");
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..0a01635 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -26,6 +26,17 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { using SafeMath for uint256; using SafeERC20 for IERC20; + error ATokenNotZeroAddress(); + error RCNotZeroAddress(); + error PRNotZeroAddress(); + error OwnerNotZeroAddress(); + error DecimalsGtZero(); + error SharesGtZero(); + error PayeeNotZeroAddress(); + error ForbidATokenTransfer(); + error ForbidATokenAllowance(); + + /* ============ Events ============ */ /** @@ -165,18 +176,18 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { uint8 decimals_, address _owner ) Ownable(_owner) ERC20(_name, _symbol) ReentrancyGuard() { - require(address(_aToken) != address(0), "AaveV3YS/aToken-not-zero-address"); + if (address(_aToken) == address(0)) revert ATokenNotZeroAddress(); aToken = _aToken; - require(address(_rewardsController) != address(0), "AaveV3YS/RC-not-zero-address"); + if (address(_rewardsController) == address(0)) revert RCNotZeroAddress(); rewardsController = _rewardsController; - require(address(_poolAddressesProviderRegistry) != address(0), "AaveV3YS/PR-not-zero-address"); + if (address(_poolAddressesProviderRegistry) == address(0)) revert PRNotZeroAddress(); poolAddressesProviderRegistry = _poolAddressesProviderRegistry; - require(_owner != address(0), "AaveV3YS/owner-not-zero-address"); + if (_owner == address(0)) revert OwnerNotZeroAddress(); - require(decimals_ > 0, "AaveV3YS/decimals-gt-zero"); + if (decimals_ == 0) revert DecimalsGtZero(); _decimals = decimals_; // Approve once for max amount @@ -230,7 +241,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { */ function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant { uint256 _shares = _tokenToShares(_depositAmount); - require(_shares > 0, "AaveV3YS/shares-gt-zero"); + if (_shares == 0) revert SharesGtZero(); address _underlyingAssetAddress = _tokenAddress(); IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount); @@ -273,7 +284,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { * @return True if operation was successful. */ function claimRewards(address _to) external onlyManagerOrOwner returns (bool) { - require(_to != address(0), "AaveV3YS/payee-not-zero-address"); + if (_to == address(0)) revert PayeeNotZeroAddress(); address[] memory _assets = new address[](1); _assets[0] = address(aToken); @@ -334,7 +345,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { address _to, uint256 _amount ) external onlyManagerOrOwner { - require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer"); + if (address(_token) == address(aToken)) revert ForbidATokenTransfer(); _token.safeTransfer(_to, _amount); emit TransferredERC20(msg.sender, _to, _amount, _token); } @@ -346,7 +357,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { * @param _token Address of the ERC20 token to check */ function _requireNotAToken(address _token) internal view { - require(_token != address(aToken), "AaveV3YS/forbid-aToken-allowance"); + if (_token == address(aToken)) revert ForbidATokenTransfer(); } /**
diff --git a/gas.orig b/gas.new index d87edc2..3aa8ff3 100644 --- a/gas.orig +++ b/gas.new @@ -17,7 +17,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · setManager · - · - · 47982 · 4 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · supplyTokenTo · 110668 · 153720 · 142217 · 6 · - │ +| AaveV3YieldSourceHarness · supplyTokenTo · 110662 · 153714 · 142211 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · transferERC20 · 58318 · 60467 · 59393 · 2 · - │ ·····························|··························|·············|·············|·············|···············|·············· @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2461083 · 2461095 · 2461093 · 8.2 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
File: contracts/AaveV3YieldSource.sol #1 275 function claimRewards(address _to) external onlyManagerOrOwner returns (bool) {
File: contracts/AaveV3YieldSource.sol #2 296 function decreaseERC20Allowance( 297 IERC20 _token, 298 address _spender, 299 uint256 _amount 300 ) external onlyManagerOrOwner {
File: contracts/AaveV3YieldSource.sol #3 315 function increaseERC20Allowance( 316 IERC20 _token, 317 address _spender, 318 uint256 _amount 319 ) external onlyManagerOrOwner {
File: contracts/AaveV3YieldSource.sol #4 332 function transferERC20( 333 IERC20 _token, 334 address _to, 335 uint256 _amount 336 ) external onlyManagerOrOwner {
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..b16b82a 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -272,7 +272,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { * @param _to Address where the claimed rewards will be sent * @return True if operation was successful. */ - function claimRewards(address _to) external onlyManagerOrOwner returns (bool) { + function claimRewards(address _to) payable external onlyManagerOrOwner returns (bool) { require(_to != address(0), "AaveV3YS/payee-not-zero-address"); address[] memory _assets = new address[](1); @@ -297,7 +297,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { IERC20 _token, address _spender, uint256 _amount - ) external onlyManagerOrOwner { + ) payable external onlyManagerOrOwner { _requireNotAToken(address(_token)); _token.safeDecreaseAllowance(_spender, _amount); emit DecreasedERC20Allowance(msg.sender, _spender, _amount, _token); @@ -316,7 +316,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { IERC20 _token, address _spender, uint256 _amount - ) external onlyManagerOrOwner { + ) payable external onlyManagerOrOwner { _requireNotAToken(address(_token)); _token.safeIncreaseAllowance(_spender, _amount); emit IncreasedERC20Allowance(msg.sender, _spender, _amount, _token); @@ -333,7 +333,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { IERC20 _token, address _to, uint256 _amount - ) external onlyManagerOrOwner { + ) payable external onlyManagerOrOwner { require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer"); _token.safeTransfer(_to, _amount); emit TransferredERC20(msg.sender, _to, _amount, _token);
diff --git a/gas.orig b/gas.new index d87edc2..b10b8b3 100644 --- a/gas.orig +++ b/gas.new @@ -5,11 +5,11 @@ ·····························|··························|·············|·············|·············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · usd (avg) │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · claimRewards · 54735 · 56896 · 55816 · 4 · - │ +| AaveV3YieldSourceHarness · claimRewards · 54711 · 56872 · 55792 · 4 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · decreaseERC20Allowance · 37465 · 39638 · 38910 · 3 · - │ +| AaveV3YieldSourceHarness · decreaseERC20Allowance · 37441 · 39614 · 38886 · 3 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · increaseERC20Allowance · 59133 · 61618 · 61212 · 7 · - │ +| AaveV3YieldSourceHarness · increaseERC20Allowance · 59109 · 61594 · 61188 · 7 · - │ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · mint · 51381 · 68493 · 61904 · 13 · - │ ·····························|··························|·············|·············|·············|···············|·············· @@ -19,7 +19,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | AaveV3YieldSourceHarness · supplyTokenTo · 110668 · 153720 · 142217 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · transferERC20 · 58318 · 60467 · 59393 · 2 · - │ +| AaveV3YieldSourceHarness · transferERC20 · 58294 · 60443 · 59369 · 2 · - │ ·····························|··························|·············|·············|·············|···············|·············· | ERC20Mintable · approve · 26692 · 46592 · 43275 · 6 · - │ ·····························|··························|·············|·············|·············|···············|·············· @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2586991 · 2587003 · 2587001 · 8.6 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
See this page for details
File: contracts/AaveV3YieldSource.sol (various lines) #1
diff --git a/AaveV3YieldSource.sol.orig b/AaveV3YieldSource.sol.new index 3975311..dc04818 100644 --- a/AaveV3YieldSource.sol.orig +++ b/AaveV3YieldSource.sol.new @@ -180,7 +180,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { _decimals = decimals_; // Approve once for max amount - IERC20(_tokenAddress()).safeApprove(address(_pool()), type(uint256).max); + IERC20(_tokenAddress()).safeApprove(address(_pool_Jo$()), type(uint256).max); emit AaveV3YieldSourceInitialized( _aToken, @@ -234,7 +234,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { address _underlyingAssetAddress = _tokenAddress(); IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount); - _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE); + _pool_Jo$().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE); _mint(_to, _shares); @@ -256,7 +256,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { _burn(msg.sender, _shares); uint256 _beforeBalance = _assetToken.balanceOf(address(this)); - _pool().withdraw(_underlyingAssetAddress, _redeemAmount, address(this)); + _pool_Jo$().withdraw(_underlyingAssetAddress, _redeemAmount, address(this)); uint256 _afterBalance = _assetToken.balanceOf(address(this)); uint256 _balanceDiff = _afterBalance.sub(_beforeBalance); @@ -385,7 +385,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { * @notice Retrieves Aave PoolAddressesProvider address. * @return A reference to PoolAddressesProvider interface. */ - function _poolProvider() internal view returns (IPoolAddressesProvider) { + function _poolProvider_uaF() internal view returns (IPoolAddressesProvider) { return IPoolAddressesProvider( poolAddressesProviderRegistry.getAddressesProvidersList()[ADDRESSES_PROVIDER_ID] @@ -396,7 +396,7 @@ contract AaveV3YieldSource is ERC20, IYieldSource, Manageable, ReentrancyGuard { * @notice Retrieves Aave Pool address. * @return A reference to Pool interface. */ - function _pool() internal view returns (IPool) { - return IPool(_poolProvider().getPool()); + function _pool_Jo$() internal view returns (IPool) { + return IPool(_poolProvider_uaF().getPool()); } }
diff --git a/gas.orig b/gas.new index d87edc2..72bf242 100644 --- a/gas.orig +++ b/gas.new @@ -29,7 +29,7 @@ ·····························|··························|·············|·············|·············|···············|·············· | Deployments · · % of limit · │ ························································|·············|·············|·············|···············|·············· -| AaveV3YieldSourceHarness · 2505760 · 2505772 · 2505770 · 8.4 % · - │ +| AaveV3YieldSourceHarness · 2505748 · 2505760 · 2505758 · 8.4 % · - │ ························································|·············|·············|·············|···············|·············· | ERC20Mintable · - · - · 1154099 · 3.8 % · - │ ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
#0 - PierrickGT
2022-05-04T16:38:37Z
Great report by this warden, he should get extra points.
#1 - PierrickGT
2022-05-04T16:53:07Z
- State variables only set in the constructor should be declared immutable
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/1
- internal functions only called once can be inlined to save gas
We prefer to keep the internal function _sharesToToken since it's a pretty important one with several lines of code in it.
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/17
- Using > 0 costs more gas than != 0 when used on a uint in a require() statement
- Don't use SafeMath once the solidity version is 0.8.0 or greater
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/11
- Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead
We prefer to cast them first instead to have to downcast them in each functions.
- require() or revert() statements that check input arguments should be at the top of the function
PR: https://github.com/pooltogether/aave-v3-yield-source/pull/12
- Use custom errors rather than revert()/require() strings to save gas
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13
- Functions guaranteed to revert when called by normal users can be marked payable
Interesting but since these functions don't accept native assets, we shouldn't mark them as payable
- Method IDs can be fiddled with to reduce gas costs
Seems overly complicate to save 12 gas.