Mimo DeFi contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

According to Aave documentation, when requesting flash-loan, it's possible to specify a receiver, so function executeOperation() of that receiver will be called by lendingPool. https://docs.aave.com/developers/v/2.0/guides/flash-loans In the SuperVault there is no check to prevent this attack so attacker can use this and perform griefing attack and make miner contract lose all its funds. or he can create specifically crafted params so when executeOperation() is called by lendingPool, attacker could steal vault's user funds.

Proof of Concept

To exploit this attacker will do this steps:

1. will call Aave lendingPool to get a flash-loan and specify SuperVault as receiver of flash-loan. and also create a specific params that invoke Operation.REBALANCE action to change user vault's collateral.
2. lendingPool will call executeOperation() of SuperVault with attacker specified data.
3. executeOperation() will check msg.sender and will process the function call which will cause some dummy exchanges that will cost user exchange fee and flash-loan fee.
4. attacker will repeat this attack until user losses all his funds.

  function executeOperation(
    address[] calldata assets,
    uint256[] calldata amounts,
    uint256[] calldata premiums,
    address,
    bytes calldata params
  ) external returns (bool) {
    require(msg.sender == address(lendingPool), "SV002");
    (Operation operation, bytes memory operationParams) = abi.decode(params, (Operation, bytes));
    IERC20 asset = IERC20(assets[0]);
    uint256 flashloanRepayAmount = amounts[0] + premiums[0];
    if (operation == Operation.LEVERAGE) {
      leverageOperation(asset, flashloanRepayAmount, operationParams);
    }
    if (operation == Operation.REBALANCE) {
      rebalanceOperation(asset, amounts[0], flashloanRepayAmount, operationParams);
    }
    if (operation == Operation.EMPTY) {
      emptyVaultOperation(asset, amounts[0], flashloanRepayAmount, operationParams);
    }

    asset.approve(address(lendingPool), flashloanRepayAmount);
    return true;
  }

To steal user fund in SupperVault attacker needs more steps. in all these actions (Operation.REBALANCE, Operation.LEVERAGE, Operation.EMPTY) contract will call aggregatorSwap() with data that are controlled by attacker.

  function aggregatorSwap(
    uint256 dexIndex,
    IERC20 token,
    uint256 amount,
    bytes memory dexTxData
  ) internal {
    (address proxy, address router) = _dexAP.dexMapping(dexIndex);
    require(proxy != address(0) && router != address(0), "SV201"); 
    token.approve(proxy, amount);
    router.call(dexTxData);
  }

Attacker can put special data in dexTxData that make contract to do an exchange with bad price. To do this, attacker will create a smart contract that will do this steps:

1. manipulate price in exchange with flash loan.
2. make a call to executeOperation() by Aave flash-loan with receiver and specific params so that SuperVault will make calls to manipulated exchange for exchanging.
3. do the reverse of #1 and pay the flash-loan and steal the user fund. The details are: Attacker can manipulate swapping pool price with flash-loan, then Attacker will create specific params and perform steps 1 to 4. so contract will try to exchange tokens and because of attacker price manipulation and specific dexTxData, contract will have bad deals. After that, attacker can reverse the process of swap manipulation and get his flash-loan tokens and some of SuperVault funds and. then pay the flash-loan.

Tools Used

VIM

Recommended Mitigation Steps

There should be some state variable which stores the fact that SuperVault imitated flash-loan. When contract tries to start flash-loan, it sets the isFlash to True and executeOperation() only accepts calls if isFlash is True. and after the flash loan code will set isFlash to False.

Lines of code

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

Vulnerability details

Impact

attacker can generate more PAR and MIMO reward for himself and steal other rewards by staking in VotingEscrow then calling updateBoost() (updates user.stakeWithBoost based on user boost multiplier (which is based on user VotingEscrow balance) without updating accAmountPerShare and accParAmountPerShare) then calling releaseRewards() to get more rewards.

Proof of Concept

To exploit this, attacker will do this steps:

1. First deposit fund to miner (each of the miners that inherit GenericMinerV2 has its own deposit) so miner start tracking attacker's rewards.
2. Then attacker will wait for some time until _accMimoAmountPerShare and _accParAmountPerShare values gets high enough in contract and attack become more profitable(higher change in those values will make attacker profit more) (every time someone deposits or withdraws those values get updated in refresh() function).

  function _refresh() internal {
    if (_totalStake == 0) {
      return;
    }
    uint256 currentMimoBalance = _a.mimo().balanceOf(address(this));
    uint256 currentParBalance = _par.balanceOf(address(this));
    uint256 mimoReward = currentMimoBalance.sub(_mimoBalanceTracker);
    uint256 parReward = currentParBalance.sub(_parBalanceTracker);

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

3. Then attacker will deposit high amount of token to VotingEscrow so his balance in that contract become very high.(maybe even use flash loan)
4. In next step attacker will call updateBoost() of miner, and this function will update attacker stakeWithBoost based on attacker's VotingEscrow balance(because of higher boost multipler, so userInfo.stakeWithBoost will be more higher) but userInfo.accAmountPerShare and userInfo.accParAmountPerShare will not be updated by this call. This is updateBoost() and _updateBoost() and _getBoostMultiplier code which shows how this happens:

  function updateBoost(address _user) public {
    UserInfo memory userInfo = _users[_user];
    _updateBoost(_user, userInfo);
  }

  function _updateBoost(address _user, UserInfo memory _userInfo) internal {
    // if user had a boost already, first remove it from the totalStakeWithBoost
    if (_userInfo.stakeWithBoost > 0) {
      _totalStakeWithBoost = _totalStakeWithBoost.sub(_userInfo.stakeWithBoost);
    }
    uint256 multiplier = _getBoostMultiplier(_user);
    _userInfo.stakeWithBoost = _userInfo.stake.wadMul(multiplier);
    _totalStakeWithBoost = _totalStakeWithBoost.add(_userInfo.stakeWithBoost);
    _users[_user] = _userInfo;
  }

function _getBoostMultiplier(address _user) internal view returns (uint256) {
    uint256 veMIMO = _a.votingEscrow().balanceOf(_user);

    if (veMIMO == 0) return 1e18;

    // Convert boostConfig variables to signed 64.64-bit fixed point numbers
    int128 a = ABDKMath64x64.fromUInt(_boostConfig.a);
    int128 b = ABDKMath64x64.fromUInt(_boostConfig.b);
    int128 c = ABDKMath64x64.fromUInt(_boostConfig.c);
    int128 e = ABDKMath64x64.fromUInt(_boostConfig.e);
    int128 DECIMALS = ABDKMath64x64.fromUInt(1e18);

    int128 e1 = veMIMO.divu(_boostConfig.d); // x/25000
    int128 e2 = e1.sub(e); // x/25000 - 6
    int128 e3 = e2.neg(); // -(x/25000 - 6)
    int128 e4 = e3.exp(); // e^-(x/25000 - 6)
    int128 e5 = e4.add(c); // 1 + e^-(x/25000 - 6)
    int128 e6 = b.div(e5).add(a); // 1 + 3/(1 + e^-(x/25000 - 6))
    uint64 e7 = e6.mul(DECIMALS).toUInt(); // convert back to uint64
    uint256 multiplier = uint256(e7); // convert to uint256

    require(multiplier >= 1e18 && multiplier <= _boostConfig.maxBoost, "LM103");

    return multiplier;
  }

5. Now attacker has higher amount for userInfo.stakeWithBoost (without updating userInfo.accAmountPerShare and userInfo.accParAmountPerShare ) so he will call releaseRewards() and this call will calculate attacker's _pendingMIMO and _pendingPAR with unsync userInfo.stakeWithBoost and userInfo.accAmountPerShare values (becasue in step 4 contract didn't update all of them and only increased userInfo.stakeWithBoost ) which will result in more reward for attacker, and contract will transfer those rewards to attacker(attacker will steal others rewards that are currently in contract address). These codes shows step 5 process:

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

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

    if (_userInfo.stakeWithBoost > 0) {
      _mimoBalanceTracker = _mimoBalanceTracker.sub(pendingMIMO);
      _parBalanceTracker = _parBalanceTracker.sub(pendingPAR);
    }

    if (pendingMIMO > 0) {
      require(_a.mimo().transfer(_user, pendingMIMO), "LM100");
    }
    if (pendingPAR > 0) {
      require(_par.transfer(_user, pendingPAR), "LM100");
    }
  }

  function _pendingMIMO(uint256 _userStakeWithBoost, uint256 _userAccAmountPerShare) internal view returns (uint256) {
    if (_totalStakeWithBoost == 0) {
      return 0;
    }
    uint256 currentBalance = _a.mimo().balanceOf(address(this));
    uint256 reward = currentBalance.sub(_mimoBalanceTracker);
    uint256 accMimoAmountPerShare = _accMimoAmountPerShare.add(reward.rayDiv(_totalStakeWithBoost));
    return _userStakeWithBoost.rayMul(accMimoAmountPerShare.sub(_userAccAmountPerShare));
  }

  function _pendingPAR(uint256 _userStakeWithBoost, uint256 _userAccParAmountPerShare) internal view returns (uint256) {
    if (_totalStakeWithBoost == 0) {
      return 0;
    }
    uint256 currentBalance = _par.balanceOf(address(this));
    uint256 reward = currentBalance.sub(_parBalanceTracker);
    uint256 accParAmountPerShare = _accParAmountPerShare.add(reward.rayDiv(_totalStakeWithBoost));

    return _userStakeWithBoost.rayMul(accParAmountPerShare.sub(_userAccParAmountPerShare));
  }

Attacker can do step 3,4,5 with a smart contract.

Tools Used

VIM

Recommended Mitigation Steps

before user's stake amounts get updated, contract should first calculated rewards for that user. so updateBoost() most be like this:

  function updateBoost(address _user) public {
    releaseRewards(_user);
    UserInfo memory userInfo = _users[_user];
    _updateBoost(_user, userInfo);
  }

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/DemandMinerV2.sol#L46-L49 https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/DemandMinerV2.sol#L69-L74

Vulnerability details

Impact

If by mistake admin set _feeCollector to ZERO address Fund will be lost.

Proof of Concept

function setFeeCollector() code in DemandMinerV2:

  function setFeeCollector(address feeCollector) external override onlyManager {
    _feeCollector = feeCollector;
    emit FeeCollectorSet(feeCollector);
  }

As you can see it allows to set zero address for _feeCollector. and address is not checked when fee collection happens too:

 function deposit(uint256 amount) public override {
    _token.safeTransferFrom(msg.sender, address(this), amount);
    uint256 depositAmount = amount;
    if (_feeConfig.depositFee > 0) {
      uint256 fee = amount.wadMul(_feeConfig.depositFee);
      depositAmount = depositAmount.sub(fee);
      _token.safeTransfer(_feeCollector, fee);
      emit DepositFeeReleased(fee);
    }
    _increaseStake(msg.sender, depositAmount);
  }

Tools Used

VIM

Recommended Mitigation Steps

when setting _feeCollector make sure it's not ZERO address.