Nested Finance contest - cryptphi's results

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