Return value of `transfer()`/`transferFrom()` is not checked

[code423n4]

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 https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/compound_rari_fork/CErc20.sol#L144

Vulnerability details

Impact

Some tokens do not revert() on failure and instead return false. If the caller does not check the return value, accounting that depends on the transferred amounts breaks.

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);

3. File: contracts/compound_rari_fork/CErc20.sol (line 144)

       token.transfer(admin, balance);

Tools Used

Code inspection

Recommended Mitigation Steps

Use OpenZeppelin's safeTransfer()/safeTransferFrom()

[0xdramaone]

Agree that it would be best to use Safe Erc20 everywhere

[jack-the-pug]

Downgraded to low as the impact is low in this particular context as the token will not be an arbitrary token and even if it reverts, it will not result in any major impact.