search

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

1 stars 0 forks source link

Forcing `IERC20` when calling `transfer()`/`transferFrom()` reverts when used with some ERC20 tokens #38

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/vault_and_oracles/UniV3LpVault.sol#L366 https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/vault_and_oracles/FlashLoan.sol#L57

Vulnerability details

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made revert.

Impact

The code as currently implemented does not handle these sorts of tokens properly when they're a Uniswap pool asset, which would prevent USDT, the sixth largest pool, from being used by this project. This project relies heavily on Uniswap, so this would hamper future growth and availability of the protocol.

Proof of Concept

  1. File: contracts/vault_and_oracles/UniV3LpVault.sol (line 366)

        IERC20Detailed(params.asset).transferFrom(msg.sender, address(this), params.amount);

  2. File: contracts/vault_and_oracles/FlashLoan.sol (line 57)

        IERC20(assets[0]).transferFrom(address(LP_VAULT), address(this), amountOwing);

The call to transfer() in contracts/compound_rari_fork/CErc20.sol (line 144) is safe because it uses EIP20NonStandardInterface and doesn't process the return if there is one.

Tools Used

Code inspection

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20's safeTransfer() instead

0xdramaone commented 2 years ago

Duplicate of #40

jack-the-pug commented 2 years ago

While this issue is indeed not inherently the same as "Return value of transfer()/transferFrom() is not checked". I do believe it can be classified as part of a wider, more of a best practice issue: "SafeERC20 should be used", which has been reported in multiple QA reports (#4, #8, #33, #41).

Non-standard ERC20 tokens exist and will continue to exist in the wild, it's true that certain features of the contract may be broken when it interacts with one. This is because the contract was built on the assumption that the token would properly implement the ERC20 protocol.

Furthermore, in the context of this issue, 'FlashLoan' is one of the protocol's features (and I would argue that it isn't an essential feature; the same feature can be implemented as an external contact and work just as well), and no funds are at risk; it's just this particular built in flash loan leverage feature that won't work for those tokens.

All in all, I believe it is more fair to combine all "SafeERC20 should be used"-related issues into one, and it should be ranked as 'QA' unless there is a very clear path to a more severe impact.