QA Report

[code423n4]

1. Obsolete usage of SafeMath

Risk

Low

Impact

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 {}.

Proof of Concept

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

Used Tools

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove SafeMath from the contract.

2. Invalid setting decimals

Risk

Low

Impact

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.

Proof of Concept

• https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L179-L180
• https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L216-L222

Used Tools

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to set _decimals by making external call to aToken.decimals().

3. Use immutable storage variables

Risk

Non-Critical

Impact

Contract AaveV3YieldSource has multiple storage addresses that are only set in constructor but are not marked as immutable.

Proof of Concept

• IAToken public aToken - https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L127
• IRewardsController public rewardsController - https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L130
• IPoolAddressesProviderRegistry public poolAddressesProviderRegistry - https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L133

Used Tools

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to mark listed storage variables as immutable.

4. Redundant code for checking aToken address

Risk

Non-Critical

Impact

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.

Proof of Concept

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

Used Tools

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use _requireNotAToken in AaveV3YieldSource.transferERC20 function.

[PierrickGT]

1. Obsolete usage of SafeMath

Duplicate of #11

2. 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.

3. Use immutable storage variables

Duplicate of #1

4. Redundant code for checking aToken address

Duplicate of #4