`recipientsCounter` should start from 1 in `DonationVotingMerkleDistributionBaseStrategy`

When doing DonationVotingMerkleDistributionBaseStrategy._registerRecipient, it checks the current status of the recipient. If the recipient is new to the pool, the status should be Status.None. However, recipientsCounter starts from 0. The new recipient actually gets the status of first recipient of the pool.

Vulnerability Detail

DonationVotingMerkleDistributionBaseStrategy._registerRecipient calls _getUintRecipientStatus to get the current status of the application. The status of the new application should be Status.None. Then, the recipientToStatusIndexes[recipientId] to recipientsCounter and recipientsCounter. https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L580

    function _registerRecipient(bytes memory _data, address _sender)
        internal
        override
        onlyActiveRegistration
        returns (address recipientId)
    {
        …

        uint8 currentStatus = _getUintRecipientStatus(recipientId);

        if (currentStatus == uint8(Status.None)) {
            // recipient registering new application
            recipientToStatusIndexes[recipientId] = recipientsCounter;
            _setRecipientStatus(recipientId, uint8(Status.Pending));

            bytes memory extendedData = abi.encode(_data, recipientsCounter);
            emit Registered(recipientId, extendedData, _sender);

            recipientsCounter++;
        } else {
            if (currentStatus == uint8(Status.Accepted)) {
                // recipient updating accepted application
                _setRecipientStatus(recipientId, uint8(Status.Pending));
            } else if (currentStatus == uint8(Status.Rejected)) {
                // recipient updating rejected application
                _setRecipientStatus(recipientId, uint8(Status.Appealed));
            }
            emit UpdatedRegistration(recipientId, _data, _sender, _getUintRecipientStatus(recipientId));
        }
    }

DonationVotingMerkleDistributionBaseStrategy._getUintRecipientStatus calls _getStatusRowColumn to get the column index and current row. https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L819

    function _getUintRecipientStatus(address _recipientId) internal view returns (uint8 status) {
        // Get the column index and current row
        (, uint256 colIndex, uint256 currentRow) = _getStatusRowColumn(_recipientId);

        // Get the status from the 'currentRow' shifting by the 'colIndex'
        status = uint8((currentRow >> colIndex) & 15);

        // Return the status
        return status;
    }

DonationVotingMerkleDistributionBaseStrategy._getStatusRowColumn computes indexes from recipientToStatusIndexes[_recipientId]. For the new recipient. Those indexes should be zero. https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L833

    function _getStatusRowColumn(address _recipientId) internal view returns (uint256, uint256, uint256) {
        uint256 recipientIndex = recipientToStatusIndexes[_recipientId];

        uint256 rowIndex = recipientIndex / 64; // 256 / 4
        uint256 colIndex = (recipientIndex % 64) * 4;

        return (rowIndex, colIndex, statusesBitMap[rowIndex]);
    }

The problem is that recipientCounter starts from zero. https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L166

    /// @notice The total number of recipients.
    uint256 public recipientsCounter;

Consider the following situation:

Alice is the first recipient calls registerRecipient

// in _registerRecipient
recipientToStatusIndexes[Alice] = recipientsCounter = 0;
_setRecipientStatus(Alice, uint8(Status.Pending));
recipientCounter++

Bob calls registerRecipient.

// in _getStatusRowColumn
recipientToStatusIndexes[Bob] = 0 // It would access the status of Alice
// in _registerRecipient
currentStatus = _getUintRecipientStatus(recipientId) = Status.Pending
currentStatus != uint8(Status.None) -> no new application is recorded in the pool.

This implementation error makes the pool can only record the first application.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L580 https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L819 https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L833 https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L166

Tool used

Manual Review

Recommendation

Make the counter start from 1. There are two methods to fix the issue.

1.

    /// @notice The total number of recipients.
+   uint256 public recipientsCounter;
-   uint256 public recipientsCounter;

2.

    function _registerRecipient(bytes memory _data, address _sender)
        internal
        override
        onlyActiveRegistration
        returns (address recipientId)
    {
        …

        uint8 currentStatus = _getUintRecipientStatus(recipientId);

        if (currentStatus == uint8(Status.None)) {
            // recipient registering new application
+           recipientToStatusIndexes[recipientId] = recipientsCounter + 1;
-           recipientToStatusIndexes[recipientId] = recipientsCounter;
            _setRecipientStatus(recipientId, uint8(Status.Pending));

            bytes memory extendedData = abi.encode(_data, recipientsCounter);
            emit Registered(recipientId, extendedData, _sender);

            recipientsCounter++;
        …
    }

0xnirlin commented 10 months ago

Escalate

This should be upgraded to high, breaks the core functionality of protocol, setting wrong status.

sherlock-admin2 commented 10 months ago

Escalate

This should be upgraded to high, breaks the core functionality of protocol, setting wrong status.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

kadenzipfel commented 10 months ago

To clarify impact, adding to @AhmadDecoded's escalation, since every new recipient after the initial registered recipient will have a currentStatus != Status.None, we will continually update the status of the initial recipient, never actually correctly marking any recipient statuses. Since the registration process is used to determine distribution, in the worst case, following the on-chain registration statuses as expected to determine distribution, the pool manager may incorrectly distribute the full poolAmount solely to the initial registered recipient. In the best case, the pool will not be usable and a new one will have to be redeployed, causing the pool manager and any registered recipients to lose a material amount of funds due to gas costs every time a pool is created with this strategy.

To summarize, since the impact is either (perhaps unlikely) significant fund loss by believing contract state to be true or (100% likely) less significant gas loss due to the system completely failing every time, this should be classified as high severity.

neeksec commented 10 months ago

Agree with Escalation that this is a high becasue of this impact described by @kadenzipfel,

in the worst case, following the on-chain registration statuses as expected to determine distribution, the pool manager may incorrectly distribute the full poolAmount solely to the initial registered recipient.