Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

emissionToken cannot be reused #321

Open
code423n4 opened this issue Jul 31, 2023 · 6 comments
Open

emissionToken cannot be reused #321

code423n4 opened this issue Jul 31, 2023 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

There can only be one emissionToken per market. If two different users both want to do campaigns with the same token, even if at different times, they share the same "pool". This could either be misused by a user, to intentionally not fund the campaign stealing funds from the other. Or reward claims from one campaign spill over to the next one.

Proof of Concept

Bob requests a campaign where they emit USDC for a market. They then never actually add any USDC but let the rewards accumulate.

Then, after Bobs "campaign" has ended, Alice also wants to do a campaign in USDC. She gets ownership of this configuration and configure their new endTime and speed. The issue is that Bobs previous accumulated rewards are still there and can drain Alice new campaign.

Test in MultiRewardDistributor.t.sol, MultiRewardDistributorCommonUnitTest:

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

        // some setup from `createDistributorWithRoundValuesAndConfig`
        MultiRewardDistributor distributor = new MultiRewardDistributor();
        bytes memory initdata = abi.encodeWithSignature("initialize(address,address)", address(comptroller), address(this));
        TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(address(distributor), proxyAdmin, initdata);
        
        /// wire proxy up
        distributor = MultiRewardDistributor(address(proxy));
        comptroller._setRewardDistributor(distributor);
        
        distributor._addEmissionConfig(
            mToken,
            address(this),
            address(emissionToken),
            0.1e18,
            0,
            block.timestamp + 10
        );

        faucetToken.allocateTo(address(this), 1e18);
        faucetToken.approve(address(mToken), 1e18);

        // user mints but no tokens added in emissions
        mToken.mint(1e18);
        assertEq(MTokenInterface(mToken).totalSupply(), 1e18);

        // time passes
        vm.warp(block.timestamp + 20);

        // initial user withdraws
        mToken.redeem(1e18);

        // another user uses same token for a new campaign
        address alice = address(0x1111);
        distributor._updateOwner(mToken,address(emissionToken),alice);

        vm.prank(alice);
        distributor._updateEndTime(mToken,address(emissionToken),block.timestamp + 10);
        
        emissionToken.allocateTo(address(distributor), 1e18);
        
        vm.warp(block.timestamp + 1 days);

        // original user can claim second campaigns funds without having participated
        comptroller.claimReward();
        assertEq(emissionToken.balanceOf(address(this)), 1e18);
    }

This PoC is a bit handwavey but it shows that the emission "pool" is shared across all users of that token. Hence, in practice, an emissionToken cannot be reused. As soon as someone uses USDC or WETH as a reward token, they can never be used again in the same market.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding an ability to remove an emissionConfig. Or add more bookkeeping which owner adds which funds and can only extract rewards for their own funds.

Assessed type

Other

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jul 31, 2023
code423n4 added a commit that referenced this issue Jul 31, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Aug 3, 2023
@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Aug 3, 2023
@c4-sponsor
Copy link

ElliotFriedman marked the issue as sponsor confirmed

@ElliotFriedman
Copy link

emission owners are trusted actors and there can only be a single type of emission token reward stream per mToken.

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Aug 12, 2023
@c4-judge
Copy link
Contributor

alcueca changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

alcueca marked the issue as grade-a

@alcueca
Copy link

alcueca commented Aug 12, 2023

This is a duplicate of #312, which is QA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

6 participants