Moonwell - Nyx's results

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/IRModels/JumpRateModel.sol#L61-L75

Vulnerability details

Impact

The utilizationRate() function can return a value above 1 and not between [0, BASE].

Proof of Concept

function utilizationRate(uint cash, uint borrows, uint reserves) public pure returns (uint) {
        // Utilization rate is 0 when there are no borrows
        if (borrows == 0) {
            return 0;
        }

        return borrows.mul(1e18).div(cash.add(borrows).sub(reserves)); 
    }

In the utilizationRate() function, cash and borrows and reserves values gets used to calculate utilization rate between between [0, 1e18].

If Borrow value is 0, then function will return 0. but in this function the scenario where the value of reserves exceeds cash is not handled. the system does not guarantee that reserves never exceeds cash. the reserves grow automatically over time, so it might be difficult to avoid this entirely.

If reserves > cash (and borrows + cash - reserves > 0), the formula for utilizationRate above gives a utilization rate above 1.

Tools Used

Manual Review

Recommended Mitigation Steps

Make the utilization rate computation return 1 if reserves > cash.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L380-L463 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L1385-L1399

Vulnerability details

Impact

Borrow rates are calculated dynamically and accrueInterest() reverts if the calculated rate is greater than a hard-coded maximum. As accrueInterest() is called by most functions, the entire contract may halt and be unrecoverable.

Proof of Concept

The hard-codes the maximum borrow rate and accrueInterest() reverts if the dynamically calculated rate is greater than the hard-coded value.

/* Calculate the current borrow interest rate */
        uint borrowRateMantissa = interestRateModel.getBorrowRate(cashPrior, borrowsPrior, reservesPrior);
        require(borrowRateMantissa <= borrowRateMaxMantissa, "borrow rate is absurdly high"); 

The actual calculation is dynamic and takes no notice of the hard-coded cap, so it is possible that this state will manifest, causing a major DoS due to most functions calling accrueInterest() and accrueInterest() reverting.

The admin role may update the interest rate calculation contract via the setInterestRateModel() function.

function _setInterestRateModel(InterestRateModel newInterestRateModel) override public returns (uint) {
        uint error = accrueInterest(); //@audit here !
        if (error != uint(Error.NO_ERROR)) {
            // accrueInterest emits logs on errors, but on top of that we want to log the fact that an attempted change of interest rate model failed
            return fail(Error(error), FailureInfo.SET_INTEREST_RATE_MODEL_ACCRUE_INTEREST_FAILED);
        }
        // _setInterestRateModelFresh emits interest-rate-model-update-specific logs on errors, so we don't need to.
        return _setInterestRateModelFresh(newInterestRateModel);
    }

However, this method also calls accrueInterest before completing the upgrade, so it would fail as well.

Tools Used

Manual Review

Recommended Mitigation Steps

Prevent utilization rate calculations from returning anything above borrowRateMaxMantissa or Remove the accrueInterest from setInterestRateModel() function.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L563-L619 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L283-L313

Vulnerability details

Impact

Interest are not accrued for each asset inside getHypotheticalAccountLiquidityInternal. This can cause wrong calculations of borrowed amount and can allow user to redeem and leave his account in bad state.

Proof of Concept

The getHypotheticalAccountLiquidityInternal function is called to calculate if this user has shortfall or no when he will do specific action like redeem or borrow.

function getHypotheticalAccountLiquidityInternal(
        address account,
        MToken mTokenModify,
        uint redeemTokens,
        uint borrowAmount) internal view returns (Error, uint, uint) {

        AccountLiquidityLocalVars memory vars; // Holds all our calculation results
        uint oErr;

        // For each asset the account is in
        MToken[] memory assets = accountAssets[account];
        for (uint i = 0; i < assets.length; i++) {
            MToken asset = assets[i];

This function is going to loop through all assets of user in order to calculate how much collateral he has in that market and how much debt he has.

function borrowBalanceStoredInternal(address account) internal view returns (MathError, uint) {
        /* Note: we do not assert that the market is up to date */
        MathError mathErr;
        uint principalTimesIndex;
        uint result;

        /* Get borrowBalance and borrowIndex */
        BorrowSnapshot storage borrowSnapshot = accountBorrows[account];

        /* If borrowBalance = 0 then borrowIndex is likely also 0.
         * Rather than failing the calculation with a division by 0, we immediately return 0 in this case.
         */
        if (borrowSnapshot.principal == 0) {
            return (MathError.NO_ERROR, 0);
        }

        /* Calculate new borrow balance using the interest index:
         *  recentBorrowBalance = borrower.borrowBalance * market.borrowIndex / borrower.borrowIndex
         */
        (mathErr, principalTimesIndex) = mulUInt(borrowSnapshot.principal, borrowIndex); //@audit here 
        if (mathErr != MathError.NO_ERROR) {
            return (mathErr, 0);
        }

        (mathErr, result) = divUInt(principalTimesIndex, borrowSnapshot.interestIndex);
        if (mathErr != MathError.NO_ERROR) {
            return (mathErr, 0);
        }

        return (MathError.NO_ERROR, result);
    }

To calculate balance they use borrow index and this index is increasing with time. In order to receive correct borrow amount, accrueInterests function should be called for market, which will update borrowIndex. But protocol doesn't accrue interests for each market of borrower and as result, getHypotheticalAccountLiquidityInternal can allow user to redeem, when he should not be allowed.

Tools Used

Manual Review

Recommended Mitigation Steps

The protocol needs to accrue interest for each market of the user.

Assessed type

Other