Return values are not checked for transferFrom and transfer calls to external tokens

[code423n4]

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

[m19]

If the transferFrom has failed in SuperVault, the call to VaultsCore will fail

[gzeoneth]

Since the transfer should be one that accepted by the core protocol and according to https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/config/deployment.ts#L65 it seems that all asset will revert upon failed transfer, this listing of new asset can be guarded by owner, downgrading to Low / QA.

[gzeoneth]

Treating as warden's QA report.