PoolTogether Aave v3 contest - Tadashi's results

Originally part of a QA report by Tadashi https://github.com/code-423n4/2022-04-pooltogether-findings/issues/69

Griefing attack by transferring aTokens to the contract

Summary:

_tokenToShares uses the following formula for computing the total shares of an user:

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

An attacker wishing to harm users could perform the following attack:

1. The attacker listens for transactions on mempool that are calling supplyTokenTo.
2. When he listen the victim (or victims), the attacker sends X aTokens to the contract and cause a re-order (e.g. by bribing a miner) to guarantee that the transfer of aToken is the first transaction in the block.

Then one of the following damaging situations can occur:

• If X is large enough, _shares will be 0 and will cause a revert in L233. This prevents the user(s) of adding tokens to the pool.
• If X is not large enough to cause a revert, it can also cause a damage since the attacker decreases the expected shares that the user will receive. Indeed, when the user try to redeem the same tokens using redeemToken he may get blocked since he won’t have sufficient token to burn and L256 will revert.

Impact: The attacker can target just a subset of users, but with enough capital can degrade the experience of all users that are trying to supply tokens to Pool Together.

Some tokens require the approval to be zero before calling approve with a non-zero value

Summary: Some ERC20 tokens require approving to 0 first before approving a new value.

Details: Some tokens (such as USDT) do not work when changing the allowance from an existing non-zero allowance value.

Mitigation: Change L182 to

IERC20(_tokenAddress()).safeApprove(address(_pool()), 0);
IERC20(_tokenAddress()).safeApprove(address(_pool()), type(uint256).max);

Impact: It can break the contract composability if _tokenAddress() correspond to address of such tokens (e.g. USDT). Otherwise, it won’t cause any issue.

Unnecessary usage of SafeMath

Details:

The functions from SafeMath used in AaveV3YieldSource.sol is not needed since the compiler has built in overflow checking for Solidity versions above 0.8. Consider removing SafeMath and replacing its operations for the corresponding native operations.

In particular the following changes must be applied:

• Remove L14 and L26

• Change L262 to

  uint256 _balanceDiff = _afterBalance - _beforeBalance;

• Change L361 to

  return _supply == 0 ? _tokens : (_tokens*_supply)/(aToken.balanceOf(address(this)));

• Change L373 to

  return _supply == 0 ? _shares : _shares*aToken.balanceOf(address(this))/_supply;

Gas saved:

• Before change:

  ·-------------------------------------------------------|---------------------------|-------------|-----------------------------·
  |                 Solc version: 0.8.10                  ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
  ························································|···························|·············|······························
  |  Methods                                              ·              100 gwei/gas               ·       2794.03 usd/eth       │
  ·····························|··························|·············|·············|·············|···············|··············
  |  Contract                  ·  Method                  ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  claimRewards            ·      54735  ·      56896  ·      55816  ·            4  ·      15.60  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  decreaseERC20Allowance  ·      37465  ·      39638  ·      38910  ·            3  ·      10.87  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  increaseERC20Allowance  ·      59133  ·      61618  ·      61212  ·            7  ·      17.10  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  mint                    ·      51381  ·      68493  ·      61904  ·           13  ·      17.30  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  redeemToken             ·          -  ·          -  ·     110713  ·            1  ·      30.93  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  setManager              ·          -  ·          -  ·      47982  ·            4  ·      13.41  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  supplyTokenTo           ·     110668  ·     153720  ·     142217  ·            6  ·      39.74  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  transferERC20           ·      58318  ·      60467  ·      59393  ·            2  ·      16.59  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  ERC20Mintable             ·  approve                 ·      26692  ·      46592  ·      43275  ·            6  ·      12.09  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  ERC20Mintable             ·  mint                    ·      51408  ·      68820  ·      65715  ·           12  ·      18.36  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  ERC20Mintable             ·  transferFrom            ·          -  ·          -  ·      49823  ·            1  ·      13.92  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  Deployments                                          ·                                         ·  % of limit   ·             │
  ························································|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness                             ·    2505760  ·    2505772  ·    2505770  ·        8.4 %  ·     700.12  │
  ························································|·············|·············|·············|···············|··············
  |  ERC20Mintable                                        ·          -  ·          -  ·    1154099  ·        3.8 %  ·     322.46  │
  ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·
  

• After change:

  ·-------------------------------------------------------|---------------------------|-------------|-----------------------------·
  |                 Solc version: 0.8.10                  ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
  ························································|···························|·············|······························
  |  Methods                                              ·              100 gwei/gas               ·       2791.55 usd/eth       │
  ·····························|··························|·············|·············|·············|···············|··············
  |  Contract                  ·  Method                  ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  claimRewards            ·      54735  ·      56896  ·      55816  ·            4  ·      15.58  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  decreaseERC20Allowance  ·      37465  ·      39638  ·      38910  ·            3  ·      10.86  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  increaseERC20Allowance  ·      59133  ·      61618  ·      61212  ·            7  ·      17.09  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  mint                    ·      51381  ·      68493  ·      61904  ·           13  ·      17.28  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  redeemToken             ·          -  ·          -  ·     110584  ·            1  ·      30.87  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  setManager              ·          -  ·          -  ·      47982  ·            4  ·      13.39  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  supplyTokenTo           ·     110584  ·     153720  ·     142189  ·            6  ·      39.69  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness  ·  transferERC20           ·      58318  ·      60467  ·      59393  ·            2  ·      16.58  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  ERC20Mintable             ·  approve                 ·      26692  ·      46592  ·      43275  ·            6  ·      12.08  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  ERC20Mintable             ·  mint                    ·      51408  ·      68820  ·      65715  ·           12  ·      18.34  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  ERC20Mintable             ·  transferFrom            ·          -  ·          -  ·      49823  ·            1  ·      13.91  │
  ·····························|··························|·············|·············|·············|···············|··············
  |  Deployments                                          ·                                         ·  % of limit   ·             │
  ························································|·············|·············|·············|···············|··············
  |  AaveV3YieldSourceHarness                             ·    2497329  ·    2497341  ·    2497339  ·        8.3 %  ·     697.14  │
  ························································|·············|·············|·············|···············|··············
  |  ERC20Mintable                                        ·          -  ·          -  ·    1154099  ·        3.8 %  ·     322.17  │
  ·-------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·