search

code-423n4 / 2022-04-pooltogether-findings

0 stars 0 forks source link

QA Report #78

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

C4-001 : The Contract Should approve(0) first

Impact - LOW

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

IERC20(token).safeApprove(address(operator), 0);
IERC20(token).safeApprove(address(operator), amount);

Proof of Concept

  1. Navigate to the following contracts.
https://github.com/pooltogether/aave-v3-yield-source/blob/e63d1b0e396a5bce89f093630c282ca1c6627e44/contracts/AaveV3YieldSource.sol#L183
  1. When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount. Consider use safeIncreaseAllowance and safeDecreaseAllowance.

C4-002 : Check if amount > 0 before token transfer

Impact

Since _amount can be 0. Checking if (_amount != 0) before the transfer can potentially save an external call and the unnecessary gas cost of a 0 token transfer.

Proof of Concept

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

All Contracts

Tools Used

None

Recommended Mitigation Steps

Consider checking amount != 0.

PierrickGT commented 2 years ago

C4-001 : The Contract Should approve(0) first

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

C4-002 : Check if amount > 0 before token transfer

As stated in a previous issue, it would be a redundant check since this function is only callable by the owner or manager, which will be a multisig, so someone would see before signing if the input amount is 0.

gititGoro commented 2 years ago

This issue will be marked as invalid. For C4-001, the attack vector is for ongoing changes to approval, not once off max approvals from zero. I see this issue brought up often but unless the problem appears in the correct context, it's out of scope. C4-002 is also out of scope. For owner managed functions, it would be advisable for wardens to query the use of the functions in forums with the sponsor before making a claim since this area dances between governance and QA and no approach is universally better.