Mimo DeFi contest - hyh's results

Lines of code

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

Vulnerability details

leverageSwap and emptyVaultOperation can be run repeatedly for the same tokens. If these tokens happen to be an ERC20 that do not allow for approval of positive amount when allowance already positive, both functions can become stuck.

https://github.com/d-xo/weird-erc20#approval-race-protections

In both cases logic doesn't seem to guarantee full usage of the allowance given. If it's not used fully, the token will revert each next approve attempt, which will render the functions unavailable for the token.

While emptyVaultOperation can be cured by emptying the balance and rerun, in the leverageSwap case there is no such fix possible.

Setting severity to medium as this clearly impacts leverageSwap and emptyVaultOperation availability to the users.

Proof of Concept

leverageSwap calls target token for maximum approval of core each time:

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

  ///@param token The leveraged asset to swap PAR for
  function leverageSwap(bytes memory params, IERC20 token) internal {
    (uint256 parToSell, bytes memory dexTxData, uint dexIndex) = abi.decode(
      params,
      (uint256, bytes, uint )
    );
    token.approve(address(a.core()), 2**256 - 1);

Some tokens do not have maximum amount as an exception, simply reverting any attempt to approve positive from positive, for example current USDT contract, L205:

https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code

I.e. if leverageSwap be run again with USDT it will revert all the times after the first.

emptyVaultOperation approves core for the whole balance of stablex:

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

    IERC20 par = IERC20(a.stablex());
    par.approve(address(a.core()), par.balanceOf(address(this)));

Recommended Mitigation Steps

Consider adding zero amount approval before actual amount approval, i.e. force zero allowance before current approval.

1. SuperVault uses unchecked collateral transfer and transferFrom with arbitrary ERC20 (low)

Whenever an ERC20 that do not revert on transfer failure is used and transfer / transferFrom fails and return false which will be ignored, there will be a contract malfunction, the end result of which depends on the current state of the contract.

Placing severity to be low for 3 functions that are onlyOwner as in such case the behavior can be controlled by the trusted caller.

Proof of Concept

leverage (onlyOwner):

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

IERC20(asset).transferFrom(msg.sender, address(this), depositAmount);

emptyVault (onlyOwner):

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

collateral.transfer(msg.sender, collateral.balanceOf(address(this)));

depositAndBorrowFromVault (onlyOwner):

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

token.transferFrom(msg.sender, address(this), depositAmount);

Some ERC20 do not revert on transfer failure:

https://github.com/d-xo/weird-erc20#no-revert-on-failure

Recommended Mitigation Steps

Require transfer success as it is done elsewhere in the contract or employ OpenZeppelin's SafeERC20

2. PARMinerV2's liquidate doesn't check the router call success (non-critical)

On router call failure there will be LM104 error issued, which might not be best for failure description.

Proof of Concept

router can be arbitrary and router.call is unchecked:

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L126-L128

    router.call(dexTxData);
    _par.safeTransfer(msg.sender, _liquidateCallerReward);
    require(_par.balanceOf(address(this)) > parBalanceBefore, "LM104");

Recommended Mitigation Steps

Requiring the call success is advised with a tailored error

3. Manual updateBoost is required before GenericMinerV2 become operational (non-critical)

releaseRewards calls _releaseRewards before _updateBoost:

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/GenericMinerV2.sol#L80-L86

  function releaseRewards(address _user) public virtual override {
    UserInfo memory _userInfo = _users[_user];
    _releaseRewards(_user, _userInfo);
    _userInfo.accAmountPerShare = _accMimoAmountPerShare;
    _userInfo.accParAmountPerShare = _accParAmountPerShare;
    _updateBoost(_user, _userInfo);
  }

As only _updateBoost can increase _totalStakeWithBoost, the _releaseRewards() will fail by calling _refresh():

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/GenericMinerV2.sol#L209-L212

  function _releaseRewards(address _user, UserInfo memory _userInfo) internal {
    uint256 pendingMIMO = _pendingMIMO(_userInfo.stakeWithBoost, _userInfo.accAmountPerShare);
    uint256 pendingPAR = _pendingPAR(_userInfo.stakeWithBoost, _userInfo.accParAmountPerShare);
    _refresh();

This is because _refresh() performs division by _totalStakeWithBoost without checking that it is non-zero:

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/GenericMinerV2.sol#L271-L273

    _accMimoAmountPerShare = _accMimoAmountPerShare.add(mimoReward.rayDiv(_totalStakeWithBoost));
    _parBalanceTracker = currentParBalance;
    _accParAmountPerShare = _accParAmountPerShare.add(parReward.rayDiv(_totalStakeWithBoost));

While it will be zero on releaseRewards or _updateStake first run as _totalStakeWithBoost wasn't updated yet.

Recommended Mitigation Steps

Consider checking that _totalStakeWithBoost is not zero before running the logic that use it as a divisor.