Due to lack of expiration of the merkle proofs users can claim after campaign has concluded

[hats-bug-reporter[bot]]

Github username: @https://github.com/SB-Security Twitter username: https://x.com/SBSecurity_ Submission hash (on-chain): 0x7572df1458aa3f722de2f64344c5792a8b3fcad7e81a698a728606f03db279f3 Severity: medium

Description: Description\ Users can call Metrom::claimRewards even after the campaign has ended, due to lack of expiration included in the leaf.

Attack Scenario\ When a campaign is created, creators must specify to parameter of the struct that indicates when the rewards distribution should stop. But it is not enforced when rewards are being claimed in Metrom::claimRewards and especially in the leaf.

As a result campaign owner can be griefed when calling Metrom::recoverRewards as well as breaking the overall idea of the protocol to allow time-limited rewards distribution.

Attachments

    function _processRewardClaim(Campaign storage campaign, ClaimRewardBundle calldata _bundle, address _claimOwner)
        internal
        returns (uint256)
    {
        bytes32 _leaf = keccak256(bytes.concat(keccak256(abi.encode(_claimOwner, _bundle.token, _bundle.amount))));
        if (!MerkleProof.verifyCalldata(_bundle.proof, campaign.root, _leaf)) revert InvalidProof(); //@audit missing time validation

    }

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) Add expiration time to the Merkle proof validation.

[luzzif]

This is actually intended behavior to not force users to claim rewards in a certain period of time. Could you better explain how campaign owners would be griefed?

[blckhv]

By frontrunning the recoverRewards transaction, since the campaign owner doesn't know whether the claimer will claim his tokens or not.

[kakarottosama]

The original submission is about "user can claim after campaign has concluded" which is intended behavior.

For the:

By frontrunning the recoverRewards transaction, since the campaign owner doesn't know whether the claimer will claim his tokens or not.

IMO, same root cause vector to #14 but different angle / wording. Lack of claim expiration and recover rewards issue. The #14 issue tends more reasonable tho, even though the updater need to add new leaf for owner to recover some amount, but the unclaimed part (which distinguish between assigned reward and real unclaimed amount) need a fix.

[luzzif]

By frontrunning the recoverRewards transaction, since the campaign owner doesn't know whether the claimer will claim his tokens or not.

Could you explain this better. I don't see how frontrunning can happen on a function that is exclusively scoped to the owner (so whoever else calls it just incurs a revert). Besides, the backend assigns rewards in such a way that the owner has its own portion of rewards assigned, if any, and the LPs have their own, so there's no "overlapping" risk of sorts. Again, would it be possible to have a PoC?

[blckhv]

I'm not sure about - the user has its portion of rewards assigned?

There is a textual PoC: Let's say there are 100 tokens unclaimed after the campaign has concluded, user A has an unclaimed proof for 30 tokens but hasn't claimed them yet. The owner wants to recover these 100 tokens, but the user decides to frontrun him and claim the 30 tokens that he was initially granted. Now the admin has to provide another proof and initiate another tx for these 70 tokens.

[luzzif]

I'm not sure about - the user has its portion of rewards assigned?

There is a textual PoC: Let's say there are 100 tokens unclaimed after the campaign has concluded, user A has an unclaimed proof for 30 tokens but hasn't claimed them yet. The owner wants to recover these 100 tokens, but the user decides to frontrun him and claim the 30 tokens that he was initially granted. Now the admin has to provide another proof and initiate another tx for these 70 tokens.

In our case the precondition never really happens. The backend generates non-overlapping claims so we'll never be in a situation where a campaign has 100 unclaimed tokens and user A has a claim for 100 and user B has a claim for 30. That would be a huge backend issue.

It's also worth discerning between unclaimed and unassigned rewards. Unclaimed rewards are rewards that have either yet to be claimed or that are yet to be assigned because the campaign is still running. Unassigned rewards are rewards assigned to the campaign's owner in the extremely rare case where no LP was active in a targeted pool. In that case there is no one to reward so rewards would indeed get lost. Instead of doing that, we assign those to the campaign's owner, even though we're calling them unassigned.