PoolTogether Aave v3 contest - CertoraInc's results

Lines of code

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

Vulnerability details

Impact

If a user manages to be the first user to deposit into the contract, he will be minted shares and he can steal all the other users' deposits.

Proof of Concept

1. The attacker deposits 1 token into the contract and 1 share is minted to him (totalSupply and the aToken balance of the contract is zero).
2. A user deposits y tokens to the contract (y > 0).
3. The attacker front runs the user's transactions and transfers y tokens to the pool (so the pool aToken balance after the transfer will be y + 1) - this is a token transfer and not a regular deposit through the contract.
4. The user deposit transaction is executed - y tokens are transferred from the user to the contract and 0 shares are minted to the user - the calculation of the share amount is y * 1 / (y + 1) which is equal to zero.
5. The attacker can redeem the tokens that are in the contract by calling the redeemToken function which returns all the tokens to the attacker because he has all the shares.

The attacker can perform this attack for every user that tries to deposit to the contract. The attacker doesn't profit from this attack, but he steals all the users' funds (they are actually left in the contract). The owner can get the funds from the contract using the transferERC20 function and transfer them back to the users, but this can be prevented from the first place.

function _tokenToShares(uint256 _tokens) internal view returns (uint256) {
    uint256 _supply = totalSupply();

    // shares = (tokens * totalShares) / yieldSourceATokenTotalSupply
    return _supply == 0 ? _tokens : _tokens.mul(_supply).div(aToken.balanceOf(address(this)));
}

Tools Used

VS Code & Remix

Recommended Mitigation Steps

Save the deposited balance as an updated variable and use it instead aToken.balanceOf(address(this)) in the _tokenToShares function.

Lines of code

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

Vulnerability details

Impact

If the underlying token is a fee-on-transfer token, the amount of tokens that will be transferred to the contract isn't equal to the amount the supplyTokenTo tries to supply to the aave protocol, so the function will revert because the contract won't have enough balance of the token.

Proof of Concept

Let's assume that the underlying token is a n fee-on-transfer token (0 < n <= 1). If a user deposits x tokens, x*(1-n) tokens are actually transferred to the contract, and the supplyTokenTo function will try to deposit x tokens to the aave pool, which will trigger the transfer of the tokens and revert because the contract won't have enough token balance.

Tools Used

VS Code & Remix

Recommended Mitigation Steps

calculate the deposited amount by the difference between the balance of the contract before and after the transfer