safeTransfer is recommended instead of transfer (1)

[code423n4]

Lines of code

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

Vulnerability details

Impact

ERC20 standard allows transfer function of some contracts to return bool or return nothing. Using safeTransfer of SafeERC20.sol is recommended instead.

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4a9cc8b4918ef3736229a5cc5a310bdc17bf759f/contracts/token/ERC20/utils/SafeERC20.sol

Tools Used

Recommended Mitigation Steps

Recommended code:
require(IERC20(a.stablex()).safeTransfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));

[m19]

Duplicate of #124

[gzeoneth]

Duplicate of #147

[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]

Consider with warden's' QA report #51