Users positions can be directly liquidated when the admin changes the `collateralFactorMantissa` from a higher value to a lower value

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L707-L740

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

[0xSorryNotSorry]

While the submission falls into OOS --> [M‑04] The owner is a single point of failure and a centralization risk category, relaying the submission to the Sponsors due to proposed mitigations and the quality of the report.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

ofc this is an issue, but admin is trusted, so this isn't a finding that should receive a payout. if you say admin is untrusted, they can just rug by upgrading to a malicious smart contract system for all mToken logic contracts and then pull all the funds for themselves

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[c4-sponsor]

ElliotFriedman marked the issue as disagree with severity

[alcueca]

This behaviour is common to all collateralized lending platforms, Maker, Aave, Compound, etc.

Dropping the collateral factor is a governance action, and as such it will usually be delayed by a Timelock. Users should watch independently about the governance changes that are about to impact their assets, but usally governors communicate these changes to avoid reputational damage.

Despite the high quality of the report, I'm sorry to say that there is nothing here.

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid