users who add liquidity twice in the campaign range time may not be able to claim the second fair reward

[hats-bug-reporter[bot]]

Github username: @0Ksecurity Twitter username: -- Submission hash (on-chain): 0x50c28d593860b9d729e05c9006588e1a459ac4fc4590e9d554dc742e12386d6c Severity: high

Description: Description\ the function _processRewardClaim which is internal function is an internal function that get called when the claimRewards function called, this function allows user who added liquidty to the pool to claim their fair amount of rewards and this is possible by setting specific proof for each LPs that it get checked in this line however, users who add liquidity and then remove it and then add smaller amount again may face permanent freez of their claimed reward, more details in Attack Scenario.

Attack Scenario\

• assume Bob added liquidity to a pool and this pool have campaign that expired/finished after 6 months, bob adds 1k USDC to this pool.

• time passed and the metrom backend snapshot the liquidity provider and adds a valid proof for bob to claim his reward ( 100 usdc for example) \n- bob claim his reward and this lead to update reward.claimed[_claimOwner]; in this line now the claimed reward for bob is 100 USDC

• 3 months passed and bob decided to remove his 1k USDC liquidity from the pool and in the 4th month bob adds 500 USDC for the second time, the metrom backed again take snapshot for liquidity providers and create a proof for bob to claim 20 USDC this time.

  • now when bob try to claim his fair reward again the _processRewardClaim reverts in this line because of the underflow and prevent bob to claim his reward amount.

  Attachments.

  1. Proof of Concept (PoC) File will added if the team saw the attack scenario hard to understand.

2. Revised Code File (Optional)

we recommend to add warning on the front-end for users who want to participate in pool that inncentivized(recommended) by the campaign or add checks below:

NOTE : please read the comment in the code below

    function _processRewardClaim(
        Campaign storage campaign,
        ClaimRewardBundle calldata _bundle,
        address _claimOwner
    ) internal returns (uint256) {
        if (_bundle.receiver == address(0)) revert InvalidReceiver();
        if (_bundle.token == address(0)) revert InvalidToken();
        if (_bundle.amount == 0) revert InvalidAmount();

        bytes32 _leaf = keccak256(
            bytes.concat(
                keccak256(
                    abi.encode(_claimOwner, _bundle.token, _bundle.amount)
                )
            )
        );
        if (!MerkleProof.verifyCalldata(_bundle.proof, campaign.root, _leaf))
            revert InvalidProof();\

        uint256 _claimAmount;

        Reward storage reward = campaign.reward[_bundle.token];
        //WARNING : check should be added for double bundle use to prevent using same bundle to claim twice 
        if(reward.claimed[_claimOwner] >= _bundle.amount) {
             _claimAmount = _bundle.amount; // if the claimed is more or equal to bundle then this is the second time or above for user to claim
        } else {
            _claimAmount = _bundle.amount - reward.claimed[_claimOwner];

        }

        if (_claimAmount == 0) revert ZeroAmount();
        if (_claimAmount > reward.unclaimed) revert InvalidAmount();

        reward.claimed[_claimOwner] += _claimAmount;
        reward.unclaimed -= _claimAmount;

        IERC20(_bundle.token).safeTransfer(_bundle.receiver, _claimAmount);

        return _claimAmount;
    }

Files:

• Metrom.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmQ4azCajMfYMWsdnpNRN265gbxyyXUxr722KKvJweonSh)

[luzzif]

The way the backend works is that claims to the rewards are ever increasing. So in your example Bob's claim to the rewards would be 100 USDC for the first tranche and 120 USDC for the second one (the second reward amount is added to the second one). That's why reward.claimed[_claimOwner] is updated that way: the first time Bob claims 100 USDC, and the second time he's only allowed to claim 20 USDC as expected.

[0Ksecurity]

While it's unknown how the backend work and depend on the in scope contract we have, I believe that this is an issue, will leave the discussion to the protocol team