Risk of Merkle Tree Collision may result in loss of funds

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x1fa8659dd2efe45d37c0008b42ac8093589363ff81837b5dd011af77e6e2347e Severity: high

Description: Description\ The function Metrom._processRewardClaim computes the _leaf node of the merkle tree according to the code below:

bytes32 _leaf = keccak256(bytes.concat(keccak256(abi.encode(_claimOwner, _bundle.token, _bundle.amount))));

However, the leaf node may colide with other nodes, since the same owner, with the same token may have the same amout of reward available in another campain.

Attack Scenario\ It may me able to provide a proof for a node that does not exist.

Attachments

1. Proof of Concept (PoC) File

        _leaf1 = keccak256(bytes.concat(keccak256(abi.encode(_claimOwner, bundle.token, bundle.amount))));
        _leaf2 = keccak256(bytes.concat(keccak256(abi.encode(_claimOwner, bundle.token, bundle.amount))));
        _leaf3 = keccak256(bytes.concat(keccak256(abi.encode(_claimOwner, bundle.token, bundle.amount,bundle.campaignId))));
        _leaf4 = keccak256(bytes.concat(keccak256(abi.encode(_claimOwner, bundle.token, bundle.amount,bundle.campaignId))));

The _leaf1 and _leaf2 are going to colide and have the same node. Including the campainId would avoid that.

2. Revised Code File (Optional)

    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))));
+        bytes32 _leaf = keccak256(bytes.concat(keccak256(abi.encode(_claimOwner, _bundle.token, _bundle.amount, _bundle.campaignId))));
        if (!MerkleProof.verifyCalldata(_bundle.proof, campaign.root, _leaf)) revert InvalidProof();

        Reward storage reward = campaign.reward[_bundle.token];
        uint256 _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;
    }

[mcgrathcoutinho]

@luzzif Can you please point out the issue here? From what I can understand, the leaf does not interfere with other campaigns since we're validating it against a specific campaign.root when verifying the calldata.

[luzzif]

@luzzif Can you please point out the issue here? From what I can understand, the leaf does not interfere with other campaigns since we're validating it against a specific campaign.root when verifying the calldata.

I applied the label at a glance but looking back at this you're right. This issue is most likely invalid.