code-423n4 / 2023-07-moonwell-findings

1 stars 0 forks source link

Liquidator can seize more tokens than the borrower has as collateral, leading to an arithmetic underflow and locking collateral. #352

Closed code423n4 closed 11 months ago

code423n4 commented 12 months ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L1023 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L1012

Vulnerability details

Impact

Lack of validation on seizeTokens could allow collateral locking by underflow.

Proof of Concept

The liquidateBorrowFresh function does not explicitly validate that seizeTokens is less than or equal to accountTokens[borrower] before transferring tokens from the borrower to the liquidator. This could allow the liquidator to seize more tokens than the borrower has as collateral, leading to an arithmetic underflow and locking collateral. There is no check between https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L1023 and https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L1012 to ensure seizeTokens is <= the borrower's balance.

This could be exploited by:

-Borrower borrows close to type(uint).max tokens -Liquidator tries to seize more tokens than the borrower has -This underflows the borrower's balance, locking their collateral

Tools Used

Manual

Recommended Mitigation Steps

Cap seizeTokens to the borrower's balance,

Assessed type

Other

0xSorryNotSorry commented 12 months ago

The validation is carrier out in seizeInternal internal function as below;


    function seizeInternal(address seizerToken, address liquidator, address borrower, uint seizeTokens) internal returns (uint) {
        /* Fail if seize not allowed */
        uint allowed = comptroller.seizeAllowed(address(this), seizerToken, liquidator, borrower, seizeTokens);
        if (allowed != 0) {
            return failOpaque(Error.COMPTROLLER_REJECTION, FailureInfo.LIQUIDATE_SEIZE_COMPTROLLER_REJECTION, allowed);
        }

        /* Fail if borrower = liquidator */
        if (borrower == liquidator) {
            return fail(Error.INVALID_ACCOUNT_PAIR, FailureInfo.LIQUIDATE_SEIZE_LIQUIDATOR_IS_BORROWER);
        }

        SeizeInternalLocalVars memory vars;

     We calculate the new borrower and liquidator token balances, failing on underflow/overflow:
 @>   borrowerTokensNew = accountTokens[borrower] - seizeTokens
 @>   liquidatorTokensNew = accountTokens[liquidator] + seizeTokens

        (vars.mathErr, vars.borrowerTokensNew) = subUInt(accountTokens[borrower], seizeTokens);
        if (vars.mathErr != MathError.NO_ERROR) {
            return failOpaque(Error.MATH_ERROR, FailureInfo.LIQUIDATE_SEIZE_BALANCE_DECREMENT_FAILED, uint(vars.mathErr));
        }

Invalid assumption

c4-pre-sort commented 12 months ago

0xSorryNotSorry marked the issue as low quality report

alcueca commented 11 months ago

-Borrower borrows close to type(uint).max tokens

Very unlikely to happen.

c4-judge commented 11 months ago

alcueca marked the issue as unsatisfactory: Invalid