MALICIOUS USER CAN PREVENT A BORROWER FROM ADDING LIQUIDITY TO THIER POSITION TO AVOID LIQUIDATION

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L228-L237 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L500 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MErc20.sol#L171-L174

Vulnerability details

Impact

The Comptroller.mintAllowed function is used by the MToken.mintFresh function to verify whether the mint is allowed. The omptroller.mintAllowed function uses the supplyCaps mapping to check whether the supply cap is reached for the specific mToken market. If the supply cap is reached then the transaction will revert as follows:

        uint nextTotalSupplies = add_(totalSupplies, mintAmount);
        require(nextTotalSupplies < supplyCap, "market supply cap reached");

Now let's conisder the following scenario:

1. A borrower has borrowed Token B by depositing Token A as collateral to the moonwell protocol.

2. The liquidation threshold for the Token A deposited collateral is 70%.

3. The price of Token A is currently having a sharp price decline and the borrower decides to call the MToken.mint function to deposit more collateral (Token A) to keep his position healthy.

4. But in the Token A market the supply cap is 10,000 (Using this number for ease of understanding) and currently 9,000 has been supplied.

5. Borrower needs to deposit 500 of Token A as collateral to moonwell to increase his liquidity position so he can avoid liquidation if the Token A price further declines.

6. Borrower calls the MToken.mint function and attempts to deposit 500 of Token A.

7. A malicious user front runs the Borrower and deposits 501 of Token A to the MToken contract direclty.

8. Now when the Borrower's mint transaction starts executing it will perform the following check in the Comptroller.mintAllowed function:

       uint totalCash = MToken(mToken).getCash();
       uint totalBorrows = MToken(mToken).totalBorrows();
       uint totalReserves = MToken(mToken).totalReserves();
       // totalSupplies = totalCash + totalBorrows - totalReserves
       uint totalSupplies = sub_(add_(totalCash, totalBorrows), totalReserves);
   
       uint nextTotalSupplies = add_(totalSupplies, mintAmount);
       require(nextTotalSupplies < supplyCap, "market supply cap reached");

9. Since the attacker deposited 501 tokens to the mToken contract directly. The MToken(mToken).getCash() will retrieve the increased Token A balance of the contract.

10. Now the Borrower's mint transaction will revert since the nextTotalSupplies < supplyCap (10,001 < 10,000) is false.

11. As a result Borrower liquidation threshold will be breached and the attacker will be able to liquidate the Borrower thus profiting the extra liquidated collateral.

Proof of Concept

        if (supplyCap != 0) {
            uint totalCash = MToken(mToken).getCash();
            uint totalBorrows = MToken(mToken).totalBorrows();
            uint totalReserves = MToken(mToken).totalReserves();
            // totalSupplies = totalCash + totalBorrows - totalReserves
            uint totalSupplies = sub_(add_(totalCash, totalBorrows), totalReserves);

            uint nextTotalSupplies = add_(totalSupplies, mintAmount);
            require(nextTotalSupplies < supplyCap, "market supply cap reached");
        }

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L228-L237

        uint allowed = comptroller.mintAllowed(address(this), minter, mintAmount);

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L500

    function getCashPrior() virtual override internal view returns (uint) {
        EIP20Interface token = EIP20Interface(underlying);
        return token.balanceOf(address(this));
    }

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MErc20.sol#L171-L174

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

It is recommended to further analyze the requirement of the supplyCap functionality of the Comptroller.mintAllowed function. If there is no limitation of the supplyCap then the above attack vector is non-existent.

If the supplyCap functinality is a must, then the borrowers should be informed on a timely manner via front-end or other notifications of the supplyCap limit being reached. So the borrowers will know that only way left for them to avoid liquidation would be to repay the full amount or portion of the borrowed amount. Hence he will not attempt to deposit collateral (to avoid liquidation) and get front run by an attacker which would make him vulnerable to liquidation.

Assessed type

Other

[0xSorryNotSorry]

Seems like it's an edge case narrowing the room for the end user unless the supplyCap is not a big value.

Relaying to the Sponsors for the perusal.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[ElliotFriedman]

not a valid issue, there are other ways to avoid this situation as a borrower such as posting collateral of a different type or repaying existing debt, also this report assumes that a position will go under water atomically, which is incredibly dubious that in a single block the following steps would happen:

1. a chainlink oracle price update would come in that puts the borrower underwater
2. the user submits the tx to supply more collateral of the same type in the same block
3. the attacker sees the user submitting the tx, maliciously front runs them, thus blocking the mint, then liquidates them when the cl update confirms

[alcueca]

The attacker doesn't need to be watching the mempool (which doesn't exist in Base) to execute this. If user A is dangerously close to liquidation, and the market in question is close to the supplyCap, the liquidator might decide to add liquidity so that the supplyCap is reached and an avenue for A to avoid liquidation is closed. It is a jungle out there and this looks like fair play to me.

[alcueca]

I'll accept it as grade-b QA, and let the sponsor include this in the documentation if they wish to.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-b

[alcueca]

Actually, reporting this as High is egregious. Invalid.

[c4-judge]

alcueca marked the issue as grade-c