search

code-423n4 / 2022-06-nested-findings

0 stars 1 forks source link

Unchecked return value of transferFrom can allow a user to withdraw native token for free #8

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nested/blob/main/contracts/Withdrawer.sol#L26

Vulnerability details

Impact

The Withdrawer contract has a function withdraw() which calls an unsafe transferFrom(). A call to transferFrom is frequently done without checking the results. For certain ERC20 tokens, if insufficient tokens are present, no revert occurs but a result of "false" is returned. As explained in https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

And in this function case, if the weth.transferFrom() returns false, it would continue the call to withdraw token from the contract and send it to the caller. Thus a user could withdraw free tokens, and eventually some users will be unable to withdraw their tokens.

Proof of Concept

https://github.com/code-423n4/2022-06-nested/blob/main/contracts/Withdrawer.sol#L26

  1. Alice calls Withdrawer.withdraw() with 100 as input.
  2. Assume weth.transferFrom() fails, returns false but does not revert.
  3. weth.withdraw() will run since there was no revert.
  4. Alice receives 100 eth for free.

This may be possible in a direct call by Alice or a call from YearnCurveVaultOperator contract in https://github.com/code-423n4/2022-06-nested/blob/main/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L81

Tools Used

Manual review

Recommended Mitigation Steps

Check the result of transferFrom and transfer. Or making use of SafeERC20 library: safeTransfer and safeTransferFrom would be recommended

kartoonjoy commented 2 years ago

Per warden cryptphi, items 1-4, etc need to be added to the PoC section.

Yashiru commented 2 years ago

Unchecked return value of transferFrom can allow a user to withdraw native token for free (Duplicated)

Duplicated of #24 at Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

Disagree with severity, must be a QA because Alice can't call Withdrawer.withdraw() via the Yearn operator without providing her funds from the NestedReserve to the NestedFactory.

NestedFactory does not have any funds.

jack-the-pug commented 2 years ago

Will downgrade to QA as the pre-condition for the High severity won't stand