PoolTogether Aave v3 contest - GimelSec's results

Lines of code

https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L275

Vulnerability details

Impact

Although claimRewards() is supposed to be called by the owner or managers to claim the rewards, it still could be a "rug risk". The owner or managers can take all the rewards unconditionally.

Proof of Concept

https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L275

  function claimRewards(address _to) external onlyManagerOrOwner returns (bool) {
    require(_to != address(0), "AaveV3YS/payee-not-zero-address");

    address[] memory _assets = new address[](1);
    _assets[0] = address(aToken);

    (address[] memory _rewardsList, uint256[] memory _claimedAmounts) = rewardsController
      .claimAllRewards(_assets, _to);

    emit Claimed(msg.sender, _to, _rewardsList, _claimedAmounts);
    return true;
  }

Tools Used

vim

Recommended Mitigation Steps

Try to add some limitations or a timelock on claimRewards().

Summary

We list 1 low-critical finding and 1 non-critical finding:

• (Low) _requireNotAToken should check the token address != address(0)
• (Non) Duplicate code of address check

(Low) _requireNotAToken should check the token address != address(0)

Impact

The decreaseERC20Allowance, increaseERC20Allowance and transferERC20 functions should check the token address != address(0).

Proof of Concept

https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L296 https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L315 https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L332

Tools Used

vim

Recommended Mitigation Steps

Check address(_token) != address(0).

(Non) Duplicate code of address check

Impact

In transferERC20, it checks that the _token address should not be aToken, which is implemented in _requireNotAToken.

Proof of Concept

https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L337

Tools Used

vim

Recommended Mitigation Steps

Use _requireNotAToken in the transferERC20 function.

internal functions which are only used once can be inlined

The internal functions which are only used once can be inlined to save gas.

Proof of Concept

https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L369 https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L388

Recommendation

Remove internal functions and add these code inlined.