Moonwell - Udsen's results

Lines of code

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

Vulnerability details

Impact

If a borrower is undercollateralized then he can be liquidated by a liquidator by calling the MErc20.liquidateBorrow function. liquidateBorrow function calls the MToken.liquidateBorrowFresh in its execution process. Inside the liquidateBorrowFresh function the MToken.repayBorrowFresh is called which verifies whether repayment of borrowed amount is allowed by calling the Comptroller.repayBorrowAllowed function. The repayBorrowAllowed function updates the borrower eligible rewards by calling the MultiRewardDistributor.updateMarketBorrowIndexAndDisburseBorrowerRewards.

The MultiRewardDistributor.updateMarketBorrowIndexAndDisburseBorrowerRewards function calls the disburseBorrowerRewardsInternal to ditribute the multi rewards to the borrower. The disburseBorrowerRewardsInternal calls the sendReward function if the _sendTokens flag is set to true.

sendReward function is called for each emissionConfig.config.emissionToken token of the MarketEmissionConfig[] array of the given _mToken market.

sendReward function transafers the rewards to the borrower using the safeTransfer function as shown below:

        token.safeTransfer(_user, _amount);

Problem here is that emissionToken can be ERC777 token (which is backward compatible with ERC20) thus allowing a malicious borrower (the recipient contract of rewards in this case) to implement tokensReceived hook in its contract and revert the transaction inside the hook. This will revert the entire liquidation transaction. Hence the undercollateralized borrower can avoid the liquidation thus putting both depositors and protocol in danger.

Proof of Concept

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

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

        (uint repayBorrowError, uint actualRepayAmount) = repayBorrowFresh(liquidator, borrower, repayAmount);

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

        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 {

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to disallow any ERC777 tokens being configured as emissionToken in any MToken market and only allow ERC20 tokens as emissionTokens.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The Mtoken markets configured for the respective collateral asset types can get deprecated due to various reasons associated with those assets. There should be functionality in the Comptroller.liquidateBorrowAllowed function to liquidate all the borrows in the accounts for deprecated markets.

This functionality is present in the Comptroller.liquidateBorrowAllowed of the compound protocol, but is missing in the Comptroller.liquidateBorrowAllowed of the moonwell protocol.

Since this isDeprecated functionality is missing the borrowed assets of the deprecated MToken will not able to be repaid after the deprecation.

Following code snippet shows how the isDeprecated functionality is implemented in Compound.Comptroller.sol.

function isDeprecated(CToken cToken) public view returns (bool) {
    return
        markets[address(cToken)].collateralFactorMantissa == 0 &&
        borrowGuardianPaused[address(cToken)] == true &&
        cToken.reserveFactorMantissa() == 1e18
    ;
}

Proof of Concept

    function liquidateBorrowAllowed(
        address mTokenBorrowed,
        address mTokenCollateral,
        address liquidator,
        address borrower,
        uint repayAmount) override external view returns (uint) {
        // Shh - currently unused
        liquidator;

        if (!markets[mTokenBorrowed].isListed || !markets[mTokenCollateral].isListed) {
            return uint(Error.MARKET_NOT_LISTED);
        }

        /* The borrower must have shortfall in order to be liquidatable */
        (Error err, , uint shortfall) = getAccountLiquidityInternal(borrower);
        if (err != Error.NO_ERROR) {
            return uint(err);
        }
        if (shortfall == 0) {
            return uint(Error.INSUFFICIENT_SHORTFALL);
        }

        /* The liquidator may not repay more than what is allowed by the closeFactor */
        uint borrowBalance = MToken(mTokenBorrowed).borrowBalanceStored(borrower);
        uint maxClose = mul_ScalarTruncate(Exp({mantissa: closeFactorMantissa}), borrowBalance);
        if (repayAmount > maxClose) {
            return uint(Error.TOO_MUCH_REPAY);
        }

        return uint(Error.NO_ERROR);
    }

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to add this functionality and set the deprecated flag to true for the tokens which are going to be deprecated thus allowing the borrowed positions of the deprecated token to be immediately liquidated thus keeping the protocol stable and safe. The isDeprecated should be called inside the Comptroller.liquidateBorrowAllowed function to enable liquidation.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The MultiRewardDistributor.sendReward has declared the _user as a payable address as shown below:

function sendReward(
    address payable _user,
    uint256 _amount,
    address _rewardToken
) internal nonReentrant returns (uint256) {

This denotes the _user can be paid in native ETH if the emission token is native token (denoted in address(0) or otherwise). But in the sendReward function there is no functionlity handle ETH transfer via send, transfer or low level call functions.

Proof of Concept

    function sendReward(
        address payable _user,
        uint256 _amount,
        address _rewardToken
    ) internal nonReentrant returns (uint256) {

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

            token.safeTransfer(_user, _amount);

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

    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 {
            // 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;
        }
    }

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

If the ETH is not used as a reward payment currency then there is no usecase of declaring _user as payable. If the ETH is used to pay the rewards to _user, it is recommended to add the functionality in the sendReward function to handle ETH balance calculation of the contract and transfer of the specified _amount.

Assessed type

Payable

Any comments for the judge to contextualize your findings

My findings are based on following areas:

How the supplyCaps can be used by malicious users to prevent borrower adding liquidity to thier existing position to avoid liquidations How a malicioius borrower can avoid liquidations by using the tokenRecieved hook calls in the ERC777 tokens How certain critical functinalities are missing in the Moonwell.Comptroller.sol compared to the Compound.Comptroller.sol contract which it is forked from. How block.timestamp overflow can break critical functionality of the protocol. How functions containing payable address do not have functionality to handle native ETH transfers. How the latestRoundData() function of chainlink oracles can provide stale prices due to not using the updatedAt timestamp

Approach taken in evaluating the codebase

I mainly focussed on the MultiRewardDistributor, ChainlinkCompositeOracle and TemporalGovernor contracts since the sponsor had mentioned they were the contracts with specific areas of concern. Further I analyzed the Comptroller.sol to find any discrepencies between the Compound.Comptroller.sol that could lead to vulnerabilities.
Further I audited the MToken contract and MErc20 contracts since they had main functinality related to asset management and liquidation process.

Architecture recommendations

The MultiRewardDistributor contract was implementing the novel idea of multiple emission tokens which behave as multiple reward tokens that should be provided to users of the protocol. More attention should be given to how the rewards are distributed to users during critical functions such as liquidations and Borrow Repayements. Since the rewards are distributed by iterating through each of the emission tokens for a particular market, a single revert in an iteration could completely revert the entire transaction. Futher How passing the control to an external contract during reward distribution could impact the transaction execution should also considered. For example if ERC777 which is backward compatible with ERC20 is used as emissionToken then a malicious user can use the tokenRecieved hook call of the ERC777 token to revert the entire liquidation transaction. These vulnearbilities could be extremely dangerous for proper execution of the protocol.

Codebase quality analysis

Codebase was nicely coded and well structured. Most of the functionalities were imported from the Compound protocol. Few customization were added to core function like Comptroller.mintAllowed by introducing supplyCap mapping. The Comptroller, MToken and MErc20 showed close resemblance to core contracts of the Compound protocol. Few typos in the Natspec comments were observed and it is recommended to pay more attention on clarity of the Natspec comments. For example some of the functions did not have the Natspec comments on the return value. It will make it easier for the audtiors and developers to better understand the codebase.

Centralization risks

Centrelization risk was observed in the MultiRewardDistributor contract where the admin was give all the authority within the contract to control key functions such as _addEmissionConfig etc ... Further admin acts like an owner of the config to set parameters, and act like the comptroller to kick the reward index updates, and act like a pause guardian to pause things.
Hence in this contract admin plays few major roles.

Systemic risks

More focus should be paid on how the multiple token reward distribution is executed. Since this could open more avenues for various vulnerabilities which are common among ERC20 tokens. These vulnerabilities occur since multiple ERC20 tokens can be configured as emissionTokens.

Time spent:

15 hours