Borrower can cause a DoS by frontrunning a liquidation and repaying as low as 1 wei of the current debt

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L858-L932 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L968-L1037

Vulnerability details

Impact

Borrowers can cause DoS when liquidator attempts to liquidate a 100% of the borrower's position. The borrower just need to frontrun the liquidation tx and repay a slightly portion of the debt, paying as low as 1 wei will make the borrowBalance to be less than what it was when the liquidator sent the tx to liquidate the position.

Proof of Concept

If a liquidator intends to liquidate the entire position, but the borrower frontruns the liquidator's transaction and repays an insignificant amount of the total debt, it will cause the borrowBalance to be less than it was when the liquidator sent its transaction and these lines will revert.

uint borrowBalance = MToken(mTokenBorrowed).borrowBalanceStored(borrower);
        uint maxClose = mul_ScalarTruncate(Exp({mantissa: closeFactorMantissa}), borrowBalance);
        if (repayAmount > maxClose) {
            return uint(Error.TOO_MUCH_REPAY); //@audit 
        }

(vars.mathErr, vars.accountBorrowsNew) = subUInt(vars.accountBorrows, vars.actualRepayAmount);

Normally, the protocol has a check that can be a solution for this problem.

/* If repayAmount == uint.max, repayAmount = accountBorrows */
        if (repayAmount == type(uint).max) {
            vars.repayAmount = vars.accountBorrows;
        } else {
            vars.repayAmount = repayAmount; 
        }

If uint max is used for repay amount, repay amount will be equal accountBorrows, but when liquidating, the liquidator cant use uint max because this line will revert.

/* Fail if repayAmount = uint.max */
        if (repayAmount == type(uint).max) {
            return (fail(Error.INVALID_CLOSE_AMOUNT_REQUESTED, FailureInfo.LIQUIDATE_CLOSE_AMOUNT_IS_UINT_MAX), 0);
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Allow using uint max or

if (repayAmount > vars.accountBorrows) {
            vars.repayAmount = vars.accountBorrows;
        } else {
            vars.repayAmount = repayAmount;
        }

Assessed type

DoS

[0xSorryNotSorry]

whilte mathematically true, it becomes an on-chain implementation feature without the gas cost tradeoff for sending 1 wei.

Relaying to the sponsors for their perusal.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

not an issue

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[alcueca]

This is a valid DoS attack, easily mitigated by capping the repay amount at the existing debt, instead of reverting.

An attacker might be willing to compete against liquidators in certain conditions, so that the liquidations fail until the price drops enough to make further liquidations unprofitable, forcing the protocol to take bad debt.

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[ElliotFriedman]

it's like an erc20 approve "bug", yeah you can frontrun, but we're not going to change code based on this finding.

[alcueca]

I'll admit the finding as Medium in this case, but the defense of the sponsor might be valid. If this bug hasn't been exploited after a period of time, it shouldn't be accepted in future contests.

[lyoungblood]

This is a valid DoS attack, easily mitigated by capping the repay amount at the existing debt, instead of reverting.

This finding is invalid because liquidators can never repay 100% of borrow. They can only repay closeFactor which is 50%: https://github.com/moonwell-fi/moonwell-contracts-v2/blob/main/src/Comptroller.sol#L62

Working as designed. If a borrower frontruns a liquidation by supplying more collateral or repaying debt that liquidation is no longer valid and will revert. I don't understand how this results in any loss.

[lyoungblood]

An attacker might be willing to compete against liquidators in certain conditions, so that the liquidations fail until the price drops enough to make further liquidations unprofitable, forcing the protocol to take bad debt.

If the borrower continues doing "just in time" repayments then they aren't technically liquidatable and the protocol is not taking on bad debt. There is a 10% liquidation "bonus" so the price would have to move 10.01% or more in a single price update and the borrower would have to be right at the liquidation limit for there to be any bad debt incurred, and the use of MEV is irrelevant.

[alcueca]

@lyoungblood, you are not understanding the issue. It is not about bringing the borrowing position back to health. Liquidating 100% of the position is not necessary either.

The issue, as presented, is that when the liquidator sends a transaction the borrower can front-run the liquidation tx and repay a tiny amount of debt, not enough to bring the position back to health, but enough to reduce maxClose by one wei.

This will make the liquidation revert because repayAmount is now more than maxClose, and the underwater position, still underwater, remains unliquidated, sliding more and more towards unprofitability for liquidators and therefore bad protocol debt, for as long as the borrower can front-run liquidators.

However, liquidators rarely work that way. A liquidator would call a smart contract of its own specifying the position to borrow, and that smart contract would calculate repayAmount and liquidate the position. That behaviour can't be front-run.

I'm downgrading this issue to QA on those grounds, but I would encourage you to understand the issue presented and apply the mitigation for safety.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[liveactionllama]

Removing the selected for report label here, as I believe it was not meant to move forward with the downgrade to QA. cc: @alcueca

[lyoungblood]

not enough to bring the position back to health

I see what you mean, that the liquidation would revert because it is attempting to liquidate more than maxClose, however if the position is still unhealthy it would get liquidated next block, and it would be an exceedingly unprofitable strategy to keep bribing MEV to be at top of block and try to keep this up forever. Even if you could maintain this strategy for a few blocks, you'd eventually be liquidated by a smart contract that bypasses this entirely, as you mentioned.

Ultimately we have to decide whether its worth fixing bugs or issues that aren't likely to occur in the real world, and this is one of those. I agree with the QA label here.

[Nyksxx]

Hi, As you said, this is a valid DOS attack and was always considered medium before. I can show more examples, but I am giving from the Venus contest because both protocols fork of the compound. Link: https://github.com/code-423n4/2023-05-venus-findings/issues/255

[alcueca]

Quoting myself:

However, liquidators rarely work that way. A liquidator would call a smart contract of its own specifying the position to borrow, and that smart contract would calculate repayAmount and liquidate the position. That behaviour can't be front-run.

If a liquidator would call liquidateBorrow from an EOA, then repayAmount would be in the calldata. In that case if front-run repayAmount could be higher than maxClose, and the liquidation could revert. If done repeatedly and with high enough gas values, this would be a DoS on liquidations.

The reason this will stay as QA is because liquidators don't need to call liquidateBorrow from an EOA, and most likely they won't. A liquidator usually needs to take a flash loan to repay the debt, and to convert the swap obtained to repay the flash loan. This can't be done from an EOA. Since the liquidator will be using a smart contract, calculating repayAmount within the transaction is trivial, and will usually be the path taken.

[alcueca]

My bad for not seeing this was wrong from the start, and leading wardens and sponsor to confusion here.

[c4-judge]

alcueca marked the issue as grade-b