PoolTogether Aave v3 contest - Dravee's results

Table of Contents:

• [L-01] Add a timelock to transferERC20
• [L-02] Prevent accidentally burning tokens
• [L-03] Deprecated safeApprove() function
• [N-01] Missing comment section saying "public" instead of "external"

[L-01] Add a timelock to transferERC20

A Malicious manager or owner could transfer any amount of token to any address.

File: AaveV3YieldSource.sol
332:   function transferERC20(
333:     IERC20 _token,
334:     address _to,
335:     uint256 _amount
336:   ) external onlyManagerOrOwner { //@audit: timelock needed here
337:     require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer");
338:     _token.safeTransfer(_to, _amount);
339:     emit TransferredERC20(msg.sender, _to, _amount, _token);
340:   }

To give more trust to users: this function should be put behind a timelock.

[L-02] Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

File: AaveV3YieldSource.sol
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); //@audit to should be address(0) checked
339:     emit TransferredERC20(msg.sender, _to, _amount, _token);
340:   }

[L-03] Deprecated safeApprove() function

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

File: AaveV3YieldSource.sol
182:     // Approve once for max amount
183:     IERC20(_tokenAddress()).safeApprove(address(_pool()), type(uint256).max);

[N-01] Missing comment section saying "public" instead of "external"

The following functions are under the external section:

File: AaveV3YieldSource.sol
196:   /* ============ External Functions ============ */
...
207:   /**
208:    * @notice Returns the ERC20 asset token used for deposits.
209:    * @return The ERC20 asset token address.
210:    */
211:   function depositToken() public view override returns (address) { //@audit should be under /* ============ Public Functions ============ */
212:     return _tokenAddress();
213:   }
214: 
215:   /**
216:    * @notice Returns the Yield Source ERC20 token decimals.
217:    * @dev This value should be equal to the decimals of the token used to deposit into the pool.
218:    * @return The number of decimals.
219:    */
220:   function decimals() public view virtual override returns (uint8) { //@audit should be under /* ============ Public Functions ============ */
221:     return _decimals;
222:   }

Consider adding a comment mentioning the public section: /* ============ Public Functions ============ */.

Table of Contents:

• Some variables should be immutable
• > 0 is less efficient than != 0 for unsigned integers
• Amounts should be checked for 0 before calling a transfer
• SafeMath is not needed when using Solidity version 0.8.+
• Use Custom Errors instead of Revert Strings to save Gas

Some variables should be immutable

These variables are only set in the constructor and are never edited after that:

File: AaveV3YieldSource.sol
126:   /// @notice Yield-bearing Aave aToken address.
127:   IAToken public aToken;
128: 
129:   /// @notice Aave RewardsController address.
130:   IRewardsController public rewardsController;
131: 
132:   /// @notice Aave poolAddressesProviderRegistry address.
133:   IPoolAddressesProviderRegistry public poolAddressesProviderRegistry;
...
169:   aToken = _aToken;
172:   rewardsController = _rewardsController;
175:   poolAddressesProviderRegistry = _poolAddressesProviderRegistry;

Consider marking them as immutable.

> 0 is less efficient than != 0 for unsigned integers

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

AaveV3YieldSource.sol:179:    require(decimals_ > 0, "AaveV3YS/decimals-gt-zero");
AaveV3YieldSource.sol:233:    require(_shares > 0, "AaveV3YS/shares-gt-zero");

Also, please enable the Optimizer.

Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

I suggest adding a non-zero-value check here:

AaveV3YieldSource.sol:338:    _token.safeTransfer(_to, _amount);

SafeMath is not needed when using Solidity version 0.8.+

Solidity version 0.8.+ already implements overflow and underflow checks by default. Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8.* overflow checks) is therefore redundant.

Affected code:

14:     import { SafeMath } from "@openzeppelin/contracts/utils/math/SafeMath.sol";
26:     using SafeMath for uint256;
262:    uint256 _balanceDiff = _afterBalance.sub(_beforeBalance);
361:    return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this)));
373:    return _supply == 0 ? _shares : _shares.mul(aToken.balanceOf(address(this))).div(_supply);

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

AaveV3YieldSource.sol:168:    require(address(_aToken) != address(0), "AaveV3YS/aToken-not-zero-address");
AaveV3YieldSource.sol:171:    require(address(_rewardsController) != address(0), "AaveV3YS/RC-not-zero-address");
AaveV3YieldSource.sol:174:    require(address(_poolAddressesProviderRegistry) != address(0), "AaveV3YS/PR-not-zero-address");
AaveV3YieldSource.sol:177:    require(_owner != address(0), "AaveV3YS/owner-not-zero-address");
AaveV3YieldSource.sol:179:    require(decimals_ > 0, "AaveV3YS/decimals-gt-zero");
AaveV3YieldSource.sol:233:    require(_shares > 0, "AaveV3YS/shares-gt-zero");
AaveV3YieldSource.sol:276:    require(_to != address(0), "AaveV3YS/payee-not-zero-address");
AaveV3YieldSource.sol:337:    require(address(_token) != address(aToken), "AaveV3YS/forbid-aToken-transfer");
AaveV3YieldSource.sol:349:    require(_token != address(aToken), "AaveV3YS/forbid-aToken-allowance");

I suggest replacing revert strings with custom errors.