PoolTogether Aave v3 contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

A malicious, but generous, early depositor can DOS all future deposits. This is accomplished by directly sending aTokens to the AaveV3YieldSource.sol contract after making their first deposit. The amount of aTokens sent to the contract will manipulate the share distribution for future depositors. Future depositors will need to deposit at least the amount of aTokens sent directly to the contract to receive even a single share.

The griefer could be assumed to be motivated by a desire to undermine the success of PoolTogether by DOSing contract interactions.

Proof of Concept

Steps to exploit:

• Griefer deposits 1 wei USDC
• Griefer receives 1 wei share based on the following formula:

return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this)));

Since supply==0, 1 wei shares is minted to the griefer.

• Next, the griefer directly sends 10,000 ether of USDC to AaveV3YieldSource contract. This artificially inflates the denominator of the above function: aToken.balanceOf(address(this)
• A prospective user attempts to deposit 5,000 ether USDC but the transaction will revert because of the following check: require(_shares > 0, "AaveV3YS/shares-gt-zero"); Calculation below without safeMath for ease of understanding:

shares = 5,000 ether * 1 wei / 10,000 ether = 0 shares due to precision loss

Therefore, any depositor that attempts to deposit less than 10,000 ether USDC will be unable. Please note that 10,000 USDC is arbitrary. The griefer can send any value.

Finally, it should be noted that the AaveV3YieldSource contract does not have any way to transfer out aTokens to correct the issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of relying on aToken.balanceOf(address(this), use internal accounting variables to keep track of the protocol's aToken balance.

In the transferERC20() function, the owner is not allowed to send aTokens. Removing this restriction would help out in this situation.

Issue #1 - Code Consistency: Use _requireNotAToken function everywhere

This modifier checks that the input token is not aToken. The check is manually performed in the transferERC20() function. If the purpose of the manual check is to provide a different require message, consider parameterizing the require message in the _requireNotAToken() function.

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

Issue #1 - aToken can be set to immuatable

Since the contract is compiled with Solidity version 0.8.10, aToken can be declared as immutable and set in the constructor, similar to _decimals. There is no setter function for the aToken address other than the constructor.

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