[WP-H2] Improper control over the versions of distributions' metadata may lead to repeated claims of rewards

Lines of code

Vulnerability details

function updateRewardsMetadata(Common.Distribution[] calldata distributions)
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
{
    require(distributions.length > 0, "Invalid distributions");
    IRewardDistributor(distributor).updateRewardsMetadata(distributions);
}

In the current implementation, DEFAULT_ADMIN_ROLE of BribeVault can call updateRewardsMetadata() to update the rewards metadata for the specified identifiers.

When a distribution’s metadata is updated, it will also increase the updateCount and reset the claimed tracker.

<https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/RewardDistributor.sol#L97-L119&gt;

function updateRewardsMetadata(
        Common.Distribution[] calldata _distributions
    ) external {
        require(msg.sender == bribeVault, "Invalid access");
        require(_distributions.length > 0, "Invalid _distributions");

        for (uint256 i = 0; i < _distributions.length; i++) {
            // Update the metadata and also increment the update to reset the claimed tracker
            Reward storage reward = rewards[_distributions[i].rewardIdentifier];
            reward.token = _distributions[i].token;
            reward.merkleRoot = _distributions[i].merkleRoot;
            reward.proof = _distributions[i].proof;
            reward.updateCount += 1;

            emit RewardMetadataUpdated(
                _distributions[i].rewardIdentifier,
                _distributions[i].token,
                _distributions[i].merkleRoot,
                _distributions[i].proof,
                reward.updateCount
            );
        }
    }

However, when the network is congested, DEFAULT_ADMIN_ROLE of BribeVault may mistakenly send 2 updateRewardsMetadata() txs, and the transactions can be packaged into different blocks.

Let’s say there 2 updateRewardsMetadata() tx with the same calldata, if someone claims rewards in between the two txs, then they can claim again after the second transaction.

PoC

Given:

• distributionA’s proof is set wrong in transferBribes()
• Alice is eligible for rewards in distributionA
• the network is congested
• current block number = 10000

1. DEFAULT_ADMIN_ROLE of BribeVault tries to call updateRewardsMetadata() and update distributionA’s proof;
2. After a while, since the prev tx is stucked, DEFAULT_ADMIN_ROLE of BribeVault calls updateRewardsMetadata() again with same calldata;
3. The first tx got packed into block 10010;
4. Alice calls claim() and got the reward;
5. The 2nd tx got packed into block 10020;
6. Alice calls claim() again and get the reward again.

Recommendation

Change to:

struct UpdateDistribution {
    bytes32 rewardIdentifier;
    address token;
    bytes32 merkleRoot;
    bytes32 proof;
    uint256 prevUpdateCount;
}

function updateRewardsMetadata(
    Common.UpdateDistribution[] calldata _distributions
) external {
    require(msg.sender == bribeVault, "Invalid access");
    require(_distributions.length > 0, "Invalid _distributions");

    for (uint256 i = 0; i < _distributions.length; i++) {
        require(reward.updateCount == _distributions[i].prevUpdateCount, "Invalid updateCount");
        // Update the metadata and also increment the update to reset the claimed tracker
        Reward storage reward = rewards[_distributions[i].rewardIdentifier];
        reward.token = _distributions[i].token;
        reward.merkleRoot = _distributions[i].merkleRoot;
        reward.proof = _distributions[i].proof;
        reward.updateCount += 1;

        emit RewardMetadataUpdated(
            _distributions[i].rewardIdentifier,
            _distributions[i].token,
            _distributions[i].merkleRoot,
            _distributions[i].proof,
            reward.updateCount
        );
    }
}  

---

The text was updated successfully, but these errors were encountered:

All reactions