code423n4 commented 12 months ago

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

0xSorryNotSorry commented 11 months ago

This logic is a bit off as the malicious user will not be having the ERC777 MToken rewards anyway, so the value extracted from this scenario doesn't match.

only allow ERC20 tokens as emissionTokens

Above mitigation recommendation is also matching the start of the submission. Erc777's are Erc20 tokens as well.

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as low quality report

alcueca commented 11 months ago

The finding shows that rewards are distributed as part of a liquidation.

If an ERC777 token is set as an emissionsToken, which has transfer hooks, a malicious borrower can revert on receiving rewards, and avoid liquidation. Avoiding liquidation in certain situations accrues bad debt for the protocol, or can be compounded with other vulnerabilities. This would be a valid Medium.

@ElliotFriedman, do you have in your documentation anything mentioning that ERC777 tokens should not be used as emission tokens?

c4-judge commented 11 months ago

alcueca changed the severity to 2 (Med Risk)

ElliotFriedman commented 11 months ago

this is a valid finding

c4-sponsor commented 11 months ago

ElliotFriedman marked the issue as sponsor confirmed