Lines of code

Vulnerability details

Impact

• In Comptroller contract : Market.collateralFactorMantissa mltiplier represents the maximum underlying asset amount the depositors can borrow against their collateral in a market,for example:if it is set to 0.9;then 90% of collateral value is allowed to be borrowed.

• This value is also used to calculate the amount of underlying that can be redeemed by the user and to check if the user position is elligible for liquidation or not.

• The collateralFactorMantissa value is initially set to zero when the admin list a market (by _supportMarket function)

  newMarket.collateralFactorMantissa = 0;

• Then the admin can change this value (by _setCollateralFactor function) to any value between 0 and 0.9.

• But changing this value from a higher value to a lower value (for example: from 0.9 to 0.5) will expose some users positions to be directly liquidatable since there’s no grace period or waiting time set in the Comptroller contract to enable the user to increase their collaterals/health before being dirctly exposed to liquidation.

• So some users positions will be exposed to direct liquidation once the Market.collateralFactorMantissa is set to a higher value without having the time to enhance their positions.

Proof of Concept

• Code:
  _setCollateralFactor function

  File: src/core/Comptroller.sol
  Line 707-740:
  function _setCollateralFactor(MToken mToken, uint newCollateralFactorMantissa) external returns (uint) {
  // Check caller is admin
  if (msg.sender != admin) {
  return fail(Error.UNAUTHORIZED, FailureInfo.SET_COLLATERAL_FACTOR_OWNER_CHECK);
  }

      // Verify market is listed
      Market storage market = markets[address(mToken)];
      if (!market.isListed) {
          return fail(Error.MARKET_NOT_LISTED, FailureInfo.SET_COLLATERAL_FACTOR_NO_EXISTS);
      }
  
      Exp memory newCollateralFactorExp = Exp({mantissa: newCollateralFactorMantissa});
  
      // Check collateral factor <= 0.9
      Exp memory highLimit = Exp({mantissa: collateralFactorMaxMantissa});
      if (lessThanExp(highLimit, newCollateralFactorExp)) {
          return fail(Error.INVALID_COLLATERAL_FACTOR, FailureInfo.SET_COLLATERAL_FACTOR_VALIDATION);
      }
  
      // If collateral factor != 0, fail if price == 0
      if (newCollateralFactorMantissa != 0 && oracle.getUnderlyingPrice(mToken) == 0) {
          return fail(Error.PRICE_ERROR, FailureInfo.SET_COLLATERAL_FACTOR_WITHOUT_PRICE);
      }
  
      // Set market's collateral factor to new collateral factor, remember old value
      uint oldCollateralFactorMantissa = market.collateralFactorMantissa;
      market.collateralFactorMantissa = newCollateralFactorMantissa;
  
      // Emit event with asset, old collateral factor, and new collateral factor
      emit NewCollateralFactor(mToken, oldCollateralFactorMantissa, newCollateralFactorMantissa);
  
      return uint(Error.NO_ERROR);
  }
  

• Foundry PoC:

1. The test is copied from testRewards() function in Comptroller.t.sol file & modified to demonstrate the vulnerability in details.

2. The FaucetToken.sol helper test contract is modified by adding mint() function to the StandardToken contract,this token is going to be used as an underlying asset for the market:

contract StandardToken is ERC20 {
    uint8 _decimals;

    constructor(
        uint256 _initialAmount,
        string memory _tokenName,
        uint8 _decimalUnits,
        string memory _tokenSymbol
    ) ERC20(_tokenName, _tokenSymbol) {
        _mint(msg.sender, _initialAmount);
        _decimals = _decimalUnits;
    }

    function decimals() public view virtual override returns (uint8) {
        return _decimals;
    }

+   function mint(address account, uint256 amount) public returns (bool) {
+       _mint(account, amount);
+       return true;
+   }
}

3. Add this test to the test/unit/Comptroller.t.sol file; where the following scenario is set:
   * first, the admin sets the market configurations,collateralRatioMantissa to 0.9e18,closeFactor and liquidation incentive.
   * then the borrower deposits in the market,then waits sometime,then borrows the maximum allowed underlying amount.
   * the liquidator tries to liquidate the borrower position; but he will not be able as the position is healthy (no shortfall,complies with the market collateralRatioMantissa).
   * then the admin sets the collateralRatioMantissa to a lower value (0.5e18).
   * the liquidator tries again to liquidate the borrower position, and he succeeds as the position health drops because of the newly lower collateralRatioMantissa.

   function testBorrowWhenChangingCollateralFactorMantissa() public {
   //----setting market configuration (copied from testRewards test)----//
   uint time = 1678430000;
   uint err = comptroller._setCollateralFactor(mToken, 0.9e18);
   assertEq(err, 0); //successful operation
   err = comptroller._setCloseFactor(0.9e18); //a multiplier used to calculate the maximum repayAmount when liquidating a borrow: it’s value between 0.05e18- 0.9e18
   assertEq(err, 0); //successful operation
   err = comptroller._setLiquidationIncentive(1.1e18);
   assertEq(err, 0); //successful operation

      distributor._addEmissionConfig(
          mToken,
          address(this), //_owner
          address(faucetToken), //_emissionToken (reward token)
          0.5e18, //_supplyEmissionPerSec
          0, // _borrowEmissionsPerSec
          time + 86400 //_endTime
      );
      faucetToken.allocateTo(address(distributor), 100000e18);
   
      //----setting actors: borrower & liquidator----//
      address borrower = address(0x2);
      address liquidator = address(0x3);
      uint256 depositedAmount = 1e18;
      uint256 borrowedAmount = 9e17; // which is collateralFactor*deposited amount=0.9*1e18
   
      //0. faucetToken is the underlying token
      faucetToken.mint(borrower, depositedAmount);
      faucetToken.mint(liquidator, depositedAmount);
      assertEq(faucetToken.balanceOf(borrower), depositedAmount);
      assertEq(faucetToken.balanceOf(liquidator), depositedAmount);
   
      //1. the borrower will first deposit in the market:
      vm.startPrank(borrower);
      faucetToken.approve(address(mToken), type(uint256).max);
      err = mToken.mint(depositedAmount);
      assertEq(err, 0); //successful operation
      assertEq(mToken.balanceOf(borrower), depositedAmount);
      assertEq(faucetToken.balanceOf(borrower), 0);
      vm.warp(time + 10);
   
      //2. the borrower will then borow from the market the maximum allowable amount against his collateral:
      err = mToken.borrow(borrowedAmount);
      assertEq(err, 0); //successful operation
      vm.stopPrank();
      assertEq(faucetToken.balanceOf(borrower), borrowedAmount);
   
      //3. the liquidator will not be able to liquidate the borrower since the borrower position is healthy:
      vm.warp(time + 20);
      vm.startPrank(liquidator);
      faucetToken.approve(address(mToken), type(uint256).max);
      err = mToken.liquidateBorrow(borrower, borrowedAmount, mToken);
      //the returned error code is 3 which is:COMPTROLLER_REJECTION, emitted in  Line 972 of MToken contract when liquidate user borrow is not allowed
      //https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L972-L972
      assertEq(err, 3); //COMPTROLLER_REJECTION error code
      assertEq(faucetToken.balanceOf(liquidator), depositedAmount); // the liquidator hasn't lost any faucetToken
      assertEq(mToken.balanceOf(liquidator), 0);
      vm.stopPrank();
   
      //4. now the admin changes the collateralFactorMantissa to a lower value (from 0.9e18 to 0.5e18):
      vm.warp(time + 30);
      vm.startPrank(address(this)); //the admin of the comptroller
      uint newCollateralFactorMantissa = 0.5e18;
      err = comptroller._setCollateralFactor(
          mToken,
          newCollateralFactorMantissa
      );
      assertEq(err, 0); //successful operation
      vm.stopPrank();
   
      //5. since the collateralFactorMantissa is now set to a lower value; now the liquidator will be able to liquidate the borrower:
      vm.warp(time + 40);
      vm.startPrank(liquidator);
      uint256 maximumAllowableLiquidationAmount = (borrowedAmount * 9) / 10; //borrowed*closeFactor
      err = mToken.liquidateBorrow(
          borrower,
          maximumAllowableLiquidationAmount,
          mToken
      );
      assertEq(err, 0); //successful operation
      vm.stopPrank();
   
      //6. borrower balances checks:
      assertEq(faucetToken.balanceOf(borrower), borrowedAmount);
      assertLt(mToken.balanceOf(borrower), depositedAmount); //depositedAmount-calculated seizedTokens in the comptroller contract
   
      //7. liquidator balances checks:
      assertEq(
          faucetToken.balanceOf(liquidator),
          depositedAmount - maximumAllowableLiquidationAmount
      );
   
      assertEq(
          mToken.balanceOf(liquidator),
          depositedAmount - mToken.balanceOf(borrower)
      );// the liquidator will gain the seized borrower's mTokens as incentive
   

   }

4. Test result:

$ forge test --match-test testBorrowWhenChangingCollateralFactorMantissa
Running 1 test for test/unit/Comptroller.t.sol:ComptrollerUnitTest
[PASS] testBorrowWhenChangingCollateralFactorMantissa() (gas: 1083441)
Test result: ok. 1 passed; 0 failed; finished in 6.83ms

Tools Used

Manual Testing & Foundry.

Recommended Mitigation Steps

This can be mitigated by either one of the two options:

• Option#1: Set a gracePeriod for each market, and this value is updated whenever the admin changes the Market.collateralFactorMantissa (gracePeriod=block.timestamp + 7 days), so that when a keeper tries to liquidate a position; it will check if the gracePeriod is passed or not.

• Option#2: record the Market.collateralFactorMantissa with each BorrowSnapshot, so that users can be liquidated based on the collateralFactorMantissa they have borrowed against,not the new one.

Assessed type

Context

---

The text was updated successfully, but these errors were encountered:

All reactions