QA Report

[code423n4]

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.

[PierrickGT]

Griefing attack by transferring aTokens to the contract

Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/44

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

It will be 0 when first deploying the contract.

[gititGoro]

Changing to a high risk bug. The approval to zero issue isn't valid in this context so only the aToken attack vector will be considered.

[gititGoro]

Attack vector moved to https://github.com/code-423n4/2022-04-pooltogether-findings/issues/99