Mimo DeFi contest - ilan's results

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L271 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L283 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/supervaults/contracts/SuperVault.sol#L218

Vulnerability details

Impact

when calling transferFrom or transfer of an external token a bool indicating success / fail is returned. Not checking this return value can cause the transaction to proceed even when the transfer has failed.

for example in SuperVault.depositToVault token.transferFrom can fail but the asset will still get deposited.

Proof Of Concept

From the ERC20 docs:

transfer(address recipient, uint256 amount) → bool external Moves amount tokens from the caller’s account to recipient.

Returns a boolean value indicating whether the operation succeeded.

Emits a Transfer event.

transferFrom(address sender, address >recipient, uint256 amount) → bool external

Moves amount tokens from sender to recipient using the allowance mechanism. amount is then deducted from the caller’s allowance.

Returns a boolean value indicating whether the operation succeeded.

Emits a Transfer event.

In SuperValut.depositToVault, SuperValut.depositAndBorrowFromVault, SuperValut.emptyVault transferFrom (or transfer) return value is not checked. an asset which returns false in transferFrom (or transfer) can be passed to these functions.

An example for such asset is the Basic Attention Token which implements this transfer() and this transferFrom()

Tools Used

Manual Inspection with VSCode.

Recommended Mitigation Steps

check the return values of these calls: token.transferFrom(msg.sender, address(this), amount); -> require(token.transferFrom(msg.sender, address(this), amount), "transfer failed");

and the same for transfer()