distributeRewards function allows duplicate campaigns to be submitted by the updater

[hats-bug-reporter[bot]]

Github username: @sonny2k Twitter username: -- Submission hash (on-chain): 0xfbcbe4889614551e63f2f91b05ab1ad63262d8341abe84ef1103ad7e1c2b1ae6 Severity: medium

Description: Description\ In distributeRewards, there is no check if _bundles array argument has duplicate campaignIds. Therefore, if the updater provides duplicate entries for campaignIds, users from other campaigns may have the chance to claim rewards on the duplicate campaignId which was mistakenly put in by the updater.

Attack Scenario

1. The updater decides to distributeRewards for 2 campaigns, for example: campaign 1 and campaign 2.
2. For somehow the updater, or the system off-chain submitted 2 different root and data but with the same campaignId of 1.
3. users and campaignOwner from campaign 2 is able to claim the rewards which was distributed for campaign 1.

Attachments

1. Proof of Concept (PoC) File copy this test case to test/DistributeRewards.sol to see how duplicate campaign can be submitted

 function test_successDuplicateCampaigns() public {
        MintableERC20 _mintableErc20 = new MintableERC20("Test", "TST");
        _mintableErc20.mint(address(this), 10.1 ether);
        _mintableErc20.approve(address(metrom), 10.1 ether);
        vm.assertEq(_mintableErc20.balanceOf(address(this)), 10.1 ether);

        address[] memory _rewardTokens = new address[](1);
        _rewardTokens[0] = address(_mintableErc20);

        uint256[] memory _rewardAmounts = new uint256[](1);
        _rewardAmounts[0] = 10 ether;

        CreateBundle memory _createBundle = CreateBundle({
            chainId: 1,
            pool: address(1),
            from: uint32(block.timestamp + 10),
            to: uint32(block.timestamp + 20),
            specification: bytes32(0),
            rewardTokens: _rewardTokens,
            rewardAmounts: _rewardAmounts
        });

        CreateBundle[] memory _createBundles = new CreateBundle[](1);
        _createBundles[0] = _createBundle;

        metrom.createCampaigns(_createBundles);

        DistributeRewardsBundle memory _bundle = DistributeRewardsBundle({
            campaignId: metrom.campaignId(_createBundle),
            root: bytes32("test"),
            data: bytes32("test")
        });
        // Another bundle with the same campaignId
        DistributeRewardsBundle memory _bundle1 = DistributeRewardsBundle({
            campaignId: metrom.campaignId(_createBundle),
            root: bytes32("test1"),
            data: bytes32("test1")
        });
        DistributeRewardsBundle[] memory _bundles = new DistributeRewardsBundle[](2);
        _bundles[0] = _bundle;
        _bundles[1] = _bundle1;

        vm.expectEmit();
        emit IMetrom.DistributeReward(_bundle.campaignId, _bundle.root, _bundle.data);

        vm.prank(updater);
        metrom.distributeRewards(_bundles);
    }

2. Revised Code File (Optional) distributeRewards function should include the validation that check for duplicate campaignId

[mcgrathcoutinho]

@luzzif The issue being described here is not valid since if two bundles are provided for the same campaign id, the second bundle would just overwrite the first one in the for loop (see here). This means two roots for a campaign id cannot exist at the same time, thus invalidating what this issue is describing.

Additionally, the underlying assumption this issue makes is the updater incorrectly updating the bundles, which is a centralization risk and out of scope as per the out-of-scope section on the Hats competition page here.

[luzzif]

I actually decided to put the medium label here because this issue made me want to add a check regarding the double Merkle root update. While it's not an issue per-se, the backend should never create a function call with duplicate same-campaign updates, so it might make sense to add a check in that sense in the smart contract and that's why I wanted to reward the issue. Maybe it could be downgraded to a low in case.

[sonny2k]

Although the likelihood of this issue is low, the impact is high so it makes sense to rate it as Medium.

[mcgrathcoutinho]

@luzzif I believe this should be a low since it is just a sanity check in place.

@sonny2k The impact cannot be high at all since not only does it rely on the updater making a mistake (which is a centralization risk as per the rules) but the issue you have described is not possible (read my first comment above).

[0xmahdirostami]

@luzzif

i think the issue is related to centralization risk and at best should be considered low.

[luzzif]

Yeah you guys are right, lowering this to a low

[0xmahdirostami]

Yeah you guys are right, lowering this to a low

@luzzif SER, low, not invalid.

[luzzif]

Oh sorry, my bad. Put the low tag back. I also addressed this here: https://github.com/metrom-xyz/contracts/commit/06da107479d3de27a9c9baab405826929af9d8fa

[0xmahdirostami]

Oh sorry, my bad. Put the low tag back. I also addressed this here: metrom-xyz@06da107

@luzzif

i think this isn't a great way to prevent a double root update. The better way is as follows; this way, the past root will not be updated as well.

            DistributeRewardsBundle calldata _bundle = _bundles[_i];
            if (_bundle.root == bytes32(0)) revert InvalidRoot();
            if (_bundle.data == bytes32(0)) revert InvalidData();

  -          for (uint256 _j = _i + 1; _j < _bundles.length; _j++) {
  -              if (_bundles[_i].campaignId == _bundles[_j].campaignId) revert DuplicatedDistribution();
  -         }

            Campaign storage campaign = _getExistingCampaign(_bundle.campaignId);
 +          if (campaign.root != bytes32(0)) revert InvalidData();  
            campaign.root = _bundle.root;
            campaign.data = _bundle.data;

[luzzif]

Oh sorry, my bad. Put the low tag back. I also addressed this here: metrom-xyz@06da107

@luzzif

i think this isn't a great way to prevent a double root update. The better way is as follows; this way, the past root will not be updated as well.

            DistributeRewardsBundle calldata _bundle = _bundles[_i];
            if (_bundle.root == bytes32(0)) revert InvalidRoot();
            if (_bundle.data == bytes32(0)) revert InvalidData();

  -          for (uint256 _j = _i + 1; _j < _bundles.length; _j++) {
  -              if (_bundles[_i].campaignId == _bundles[_j].campaignId) revert DuplicatedDistribution();
  -         }

            Campaign storage campaign = _getExistingCampaign(_bundle.campaignId);
 +          if (campaign.root != bytes32(0)) revert InvalidData();  
            campaign.root = _bundle.root;
            campaign.data = _bundle.data;

This is wrong as a campaign will have multiple root updates in its lifetime as rewards are assigned by the backend, claims updated and in turn also the derived Merkle tree/root. What this fixes is multiple updates to the same campaign in the same transaction.

[0xmahdirostami]

@luzzif sorry, my bad.