PoolTogether Aave v3 contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

Hacker can do this two action:

• Perform a DOS attack and continuously deny users from supplying their tokens to YeildSource (deposits less than a high amount will be rejected and attacker can control this amount, for example set it to 10K and every deposit with less than 100K token will be rejected by contract)
• Perform Sandwich Attack and continuously steal user supplied funds in every block.

When One of this conditions are met:

• totalSupply() of YeildSource contract is still zero. (_decimal can be anything) (when contract created or when everybody redeemed their funds totalSupply() will be zero)
• totalSupply() of YeildSource is low and _decimalcan is low too (for example _decimals<3 and totalSupply()<100)

Proof of Concept

YeildSource contract uses this code to convert token amount to share amount. if _supply>0 then to convert the amounts it multiply token amount to supply() and divide by aToken.balance().

  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)));
  }

and this function is for supplying tokens and minting share in YeildSource for user: (which will revert if user tries to mint _share==0)

  function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant {
    uint256 _shares = _tokenToShares(_depositAmount);
    require(_shares > 0, "AaveV3YS/shares-gt-zero");

    address _underlyingAssetAddress = _tokenAddress();
    IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount);
    _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE);

    _mint(_to, _shares);

    emit SuppliedTokenTo(msg.sender, _shares, _depositAmount, _to);
  }

let's assume totalSupply() == 0. To exploit this attacker will do this steps (attack can put this logic in a contract so they all execute in one transaction) (token has 18 _precision):

1. first attacker will approves YeildSource address and supply 1 token to YeildSource with supplyTokenTo(). (not 10e18 token just 1 token).
2. because totalSupply() is 0 so YeildSource will accept attacker supply and deposit attacker token to Aave Pool and mint 1 share for attacker.
3. attacker will deposit 1000 * 10e18 token into Aave protocol and get 1000 * 10e18 aToken. (10e18 is 10 power 18)
4. attacker will transfer 1000 * 10e18 - 1 aToken to YeildSource address. with this action totalSupply() will not change but aToken.balanceOf(YeildSource.address) will increase.
5. right now totalSupply() of shares in YeildSource is 1 and aToken.balanceOf(YeildSource.address) is 1000 * 10e18. so it's obvious that _tokenToShares(uint256 _tokens) will return zero for any value less than 1000 * 10e18 so attacker was able to deny other from depositing less than 1K token and also 100% of the shares int the YeildSource belongs to attacker. Becasue of this limitation no one can supply to YeildSource with amount less than 1000 * 10e18 as long as attacker wants. (attacker can choose higher amount too)

Attacker can continue his attack to steal user's tokens too. to do this let's assume some users send transaction to supplyTokenTo of YeildScource to deposit and get share of pool. so this transactions happen:

6. YeildSource.connect(user1).supplyTokenTo(1500 * 10e18). because right now totalSupply() of shares in YeildSource is 1 and aToken.balanceOf(YeildSource.address) is 1000 * 10e18 so _tokenToShares() will return 1 and user1 will get 1 share for his deposit.
7. YeildSource.connect(user2).supplyTokenTo(2000 * 10e18). because right now totalSupply() of shares in YeildSource is 2 and aToken.balanceOf(YeildSource.address) is 2500 * 10e18 so _tokenToShares() will return 1 and user2 will get 1 share for his deposit.
8. Right now totalSupply() of shares in YeildSource is 3 and aToken.balanceOf(YeildSource.address) is 4500 * 10e18 so for any amount lower than 1500 * 10e18 the return of _tokenToShares() will be 0.
9. After users deposit their transactions, attacker will call YeildSource.redeemToken(1400 * 10e18) and will get 1400 * 10e18 token but his share amount(balanceOf(attacker)) will not change in the YeildSource becasue return value of _tokenToShares(1400 * 10e18) is 0. below is redeemToken() code which confirms this:

  function redeemToken(uint256 _redeemAmount) external override nonReentrant returns (uint256) {
    address _underlyingAssetAddress = _tokenAddress();
    IERC20 _assetToken = IERC20(_underlyingAssetAddress);

    uint256 _shares = _tokenToShares(_redeemAmount);
    _burn(msg.sender, _shares);

    uint256 _beforeBalance = _assetToken.balanceOf(address(this));
    _pool().withdraw(_underlyingAssetAddress, _redeemAmount, address(this));
    uint256 _afterBalance = _assetToken.balanceOf(address(this));

    uint256 _balanceDiff = _afterBalance.sub(_beforeBalance);
    _assetToken.safeTransfer(msg.sender, _balanceDiff);

    emit RedeemedToken(msg.sender, _shares, _redeemAmount);
    return _balanceDiff;
  }

10. Right now totalSupply() of shares in YeildSource is 3 and aToken.balanceOf(YeildSource.address) is 3100 * 10e18 and attacker can repeat step #9 with lower amounts(999 * 10e18 then 700 * 10e18 then 450 * 10e18 and ....) multiple times and in each time his balance in YeildSource will not change and he can drain YeildSource's most of aTokens. (in each step amount needs to be smaller than aToken.balanceOf(YeildSource.address) / totalSupply() so return value of _tokenToShares() will be zero and attacker balance will not change)

Attacker can perform all steps of this attack in multiple ways he just need to be first one who deposits token to YeildSource to manipulate aToken.balanceOf(YeildSource.address) / totalSupply() ratio. he can do this with:

• Sandwich Attacking first deposit. (to change the Ratio and get user deposited assets)
• Sandiwitch Attacking contract creation. (to change the Ratio and wait for others suplly to steal them)
• Front-running first deposit. (to change the Ratio and w8 for others deposit to steal them)
• Sandwich Attacking in every block. (to continue stealing funds)

If totalSupply() is low and token's _decimals is low too, It is still possible to perform this attacks to some degree(the impact will be lower if _decimal get higher, but with _decimal=1 and totalSupply() < 1000 attacker can effectively steal users founds).

Tools Used

VIM

Recommended Mitigation Steps

To resolve this issue the logic of _tokenToShares() needs to be changed. for example something like this:

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

so attacker could not manipulate balance/supply ratio so easily.

Lines of code

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

Vulnerability details

Impact

When user use supplyTokenTo() to deposit his tokens and get share in FeildSource because of rounding in division user gets lower amount of share. for example if token's _decimal was 1 and totalSupply() was 1000 and aToken.balanceOf(FieldSource.address) was 2100 (becasue of profits in Aave Pool balance is higher than supply), then if user deposit 4 token to the contract with supplyTokenTo(), contract is going to mint only 1 share for that user and if user calls YeildToken.balanceOf(user) the return value is going to be 2 and user already lost half of his deposit. Of course if _precision was high this loss is going to be low enough to ignore but in case of low _precision and high price token and high balance / supply ratio this loss is going to be noticeable.

Proof of Concept

This is the code of supplyTokenTo():

  function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant {
    uint256 _shares = _tokenToShares(_depositAmount);
    require(_shares > 0, "AaveV3YS/shares-gt-zero");

    address _underlyingAssetAddress = _tokenAddress();
    IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount);
    _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE);

    _mint(_to, _shares);

    emit SuppliedTokenTo(msg.sender, _shares, _depositAmount, _to);
  }

which in line: _shares = _tokenToShares(_depositAmount) trying to calculated shares corresponding to the number of tokens supplied. and then transfer _depositAmount from user and mint shares amount for user. the problem is that if user convert _shares to token, he is going to receive lower amount because in most cases:

_depositAmount > _sharesToToken(_tokenToShares(_depositAmount))

and that's because of rounding in division. Value of _shares is less than _depositAmount. so YeildSource should only take part of _depositAmount that equals to _sharesToToken(_tokenToShares(_depositAmount)) and mint _share for user.

Of course if _precision was high and aToken.balanceOf(FieldSource.address) / totalSupply() was low, then this amount will be insignificant, but for some cases it can be harmful for users. for example this conditions:

• _perecision is low like 1 or 2.
• token value is very high like BTC.
• aToken.balanceOf(FieldSource.address) / totalSupply() is high due to manipulation or profit in Aave pool.

Tools Used

VIM

Recommended Mitigation Steps

To resolve this issue this can be done:

  function supplyTokenTo(uint256 _depositAmount, address _to) external override nonReentrant {
    uint256 _shares = _tokenToShares(_depositAmount);
    require(_shares > 0, "AaveV3YS/shares-gt-zero");
    
    _depositAmount = _sharesToToken(_shares); // added hero to only take correct amount of user tokens
    address _underlyingAssetAddress = _tokenAddress();
    IERC20(_underlyingAssetAddress).safeTransferFrom(msg.sender, address(this), _depositAmount);
    _pool().supply(_underlyingAssetAddress, _depositAmount, address(this), REFERRAL_CODE);

    _mint(_to, _shares);

    emit SuppliedTokenTo(msg.sender, _shares, _depositAmount, _to);
  }