Moonwell - said's results

Lines of code

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

Vulnerability details

Impact

Users can prevent themselves from being liquidated by entering another market that has low supply/borrow activity or have low/volatile value compared to other markets, the detailed scenario will be explained in PoC.

Proof of Concept

The scenario :

• Alice enter market A, supply and borrow in that market.
• Alice enter market B that have low supply and borrow or low value compared to market A.
• Alice call _addReserves in market B so that totalReserves is bigger than totalCash + totalBorrows.
• After some time Alice has shortfall (his borrow value bigger than supply value).
• Another user try to liquidate Alice and seize her market A collateral by calling liquidateBorrow :

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

    function liquidateBorrow(address borrower, uint repayAmount, MTokenInterface mTokenCollateral) override external returns (uint) {
        (uint err,) = liquidateBorrowInternal(borrower, repayAmount, mTokenCollateral);
        return err;
    }

It will eventually trigger comptroller's liquidateBorrowAllowed hook :

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

    function liquidateBorrowFresh(address liquidator, address borrower, uint repayAmount, MTokenInterface mTokenCollateral) internal returns (uint, uint) {
        /* Fail if liquidate not allowed */
        uint allowed = comptroller.liquidateBorrowAllowed(address(this), address(mTokenCollateral), liquidator, borrower, repayAmount);
        if (allowed != 0) {
            return (failOpaque(Error.COMPTROLLER_REJECTION, FailureInfo.LIQUIDATE_COMPTROLLER_REJECTION, allowed), 0);
        }
...
}

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

Inside liquidateBorrowAllowed hook, it will eventually call getHypotheticalAccountLiquidityInternal that will loop trough Alice entered markets/assets and call getAccountSnapshot to get token balance, borrow, and exchangeRate :

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

    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];

            // Read the balances and exchange rate from the mToken
            (oErr, vars.mTokenBalance, vars.borrowBalance, vars.exchangeRateMantissa) = asset.getAccountSnapshot(account);
            if (oErr != 0) { // semi-opaque error code, we assume NO_ERROR == 0 is invariant between upgrades
                return (Error.SNAPSHOT_ERROR, 0, 0);
            }
...
}

But getAccountSnapshot will result in error when try to calculate exchangeRateStoredInternal because totalReserves bigger than totalCash + totalBorrows.

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

    function exchangeRateStoredInternal() virtual internal view returns (MathError, uint) {
        uint _totalSupply = totalSupply;
        if (_totalSupply == 0) {
            /*
             * If there are no tokens minted:
             *  exchangeRate = initialExchangeRate
             */
            return (MathError.NO_ERROR, initialExchangeRateMantissa);
        } else {
            /*
             * Otherwise:
             *  exchangeRate = (totalCash + totalBorrows - totalReserves) / totalSupply
             */
            uint totalCash = getCashPrior();
            uint cashPlusBorrowsMinusReserves;
            Exp memory exchangeRate;
            MathError mathErr;
            // @audit - this will revert if totalReservers > totalBorrow + totalCash
            (mathErr, cashPlusBorrowsMinusReserves) = addThenSubUInt(totalCash, totalBorrows, totalReserves);
            if (mathErr != MathError.NO_ERROR) {
                return (mathErr, 0);
            }

            (mathErr, exchangeRate) = getExp(cashPlusBorrowsMinusReserves, _totalSupply);
            if (mathErr != MathError.NO_ERROR) {
                return (mathErr, 0);
            }

            return (MathError.NO_ERROR, exchangeRate.mantissa);
        }
    }

Also this scenario will prevent _reduceReserves because when try to accrueInterest, it will also fail due to the same condition (totalReserves bigger than totalCash + totalBorrows) .

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

Now Alice can't be liquidated.

this scenario is profitable when market B has lower token value compared to market A and has low supply/borrow activity, (shortfall of Alice on market A has bigger value than the donated reserves value to market B).

Tools Used

Manual Review

Recommended Mitigation Steps

Consider to restrict add reserves function, or actively monitor and remove market of token that have low/volatile value or low borrow and supply activity.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1239-L1246

Vulnerability details

Impact

If users owned high borrower and supplier accrued rewards, it is potentially hard to claim, due to bad handling when sendReward is called, potentially punishing big rewards holders.

Proof of Concept

inside sendReward, when _amount is higher than currentTokenHoldings, it will not send the reward to user, just emit event and return the _amount back :

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1214-L1247

    function sendReward(
        address payable _user,
        uint256 _amount,
        address _rewardToken
    ) internal nonReentrant returns (uint256) {
        // Short circuit if we don't have anything to send out
        if (_amount == 0) {
            return _amount;
        }

        // If pause guardian is active, bypass all token transfers, but still accrue to local tally
        if (paused()) {
            return _amount;
        }

        IERC20 token = IERC20(_rewardToken);

        // Get the distributor's current balance
        uint256 currentTokenHoldings = token.balanceOf(address(this));

        // Only transfer out if we have enough of a balance to cover it (otherwise just accrue without sending)
        if (_amount > 0 && _amount <= currentTokenHoldings) {
            // Ensure we use SafeERC20 to revert even if the reward token isn't ERC20 compliant
            token.safeTransfer(_user, _amount);
            return 0;
        } else {
            // @audit - this will make big reward hard to claim
            // If we've hit here it means we weren't able to emit the reward and we should emit an event
            // instead of failing.
            emit InsufficientTokensToEmit(_user, _rewardToken, _amount);

            // By default, return the same amount as what's left over to send, we accrue reward but don't send them out
            return _amount;
        }
    }

This design has a problem and will punish big rewards holders, because sendReward is called only when disburseSupplierRewardsInternal or disburseBorrowerRewardsInternal is called, which will always process emissionConfig.borrowerRewardsAccrued[_borrower] and emissionConfig.supplierRewardsAccrued[_supplier], not allowing users to put the amount that wants to be claimed.

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1082-L1093

...
            if (_sendTokens) {
                // Emit rewards for this token/pair
                uint256 unsendableRewards = sendReward(
                    payable(_supplier),
                    emissionConfig.supplierRewardsAccrued[_supplier],
                    emissionConfig.config.emissionToken
                );

                emissionConfig.supplierRewardsAccrued[
                    _supplier
                ] = unsendableRewards;
            }
...

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MultiRewardDistributor/MultiRewardDistributor.sol#L1192-L1204

...
            if (_sendTokens) {
                // Emit rewards for this token/pair
                uint256 pendingRewards = sendReward(
                    payable(_borrower),
                    emissionConfig.borrowerRewardsAccrued[_borrower],
                    emissionConfig.config.emissionToken
                );

                emissionConfig.borrowerRewardsAccrued[
                    _borrower
                ] = pendingRewards;
            }
        }
...

Also, when disburseSupplierRewardsInternal or disburseBorrowerRewardsInternal is triggered, potentially update rewards accrued to bigger value and make the claim reward even harder if it is bigger than currentTokenHoldings.

This will make high accrued rewards harder to claim and punishing users to have high rewards unclaimed.

Tools Used

Manual review

Recommended Mitigation Steps

Consider to update sendReward to :

    function sendReward(
        address payable _user,
        uint256 _amount,
        address _rewardToken
    ) internal nonReentrant returns (uint256) {
        // Short circuit if we don't have anything to send out
        if (_amount == 0) {
            return _amount;
        }

        // If pause guardian is active, bypass all token transfers, but still accrue to local tally
        if (paused()) {
            return _amount;
        }

        IERC20 token = IERC20(_rewardToken);

        // Get the distributor's current balance
        uint256 currentTokenHoldings = token.balanceOf(address(this));

        // Only transfer out if we have enough of a balance to cover it (otherwise just accrue without sending)
        if (_amount > 0 && _amount <= currentTokenHoldings) {
            // Ensure we use SafeERC20 to revert even if the reward token isn't ERC20 compliant
            token.safeTransfer(_user, _amount);
            return 0;
        } else {
+           token.safeTransfer(_user, currentTokenHoldings);
-          emit InsufficientTokensToEmit(_user, _rewardToken, _amount);

            // By default, return the same amount as what's left over to send, we accrue reward but don't send them out
-           return _amount;
+           return _amount - currentTokenHoldings;
        }
    }

Or have separate functions to allow user claim rewards with the specified amount.

Assessed type

Other