Existing RecipientId Status can be Overwritten in DonationVoting Strategy, causing loss of allocation funds for accepted recipients

In DonationVotingMerkleDistributionBaseStrategy.sol, a registered recipient status can be easily overwritten by a new recipient due to incorrect logic of current status retrieval and recipient counter update.

As a result, an existing recipientId status can be accidentally rewritten during a new recipient registration process. The existing recipientId will lose the allocation funds if their status has been accepted and then subsequently overwritten. This process can also be maliciously exploited to intentionally cause recipients to lose on allocation funds.

Vulnerability Detail

In DonationVotingMerkleDistributionBaseStrategy.sol, a recipient status is stored in bitmap and the correct fetching of their status depends on getting the correct index of bitmap. However, this is not implemented correctly in _registerRecipient().

_registerRecipient() allows both new registration of recipients and status updates of existing recipients. If the caller is a new recipient, their recipientId will be registered and their status will be Pending. If the caller is an existing recipient, their existing status can be updated from Accepted to Pending, or Rejected to Appealed. The way _registerRecipient() determines whether the caller is a new or existing recipient is to check their currentStatus in the bitmap, if None then it determines the call is a new registration, otherwise it determines the call from an existing register.

There are two problems at work here: (1) currentStatus will not be fetched correctly if there is at least one existing recipient before current registration; (2) When the first-ever recipient registers, their recipientsCounter will be '0', when combined with (1), will allow subsequent recipients to overwrite the first recipient's status.

Specifically, when a new caller registers, whether a RegistryAnchor is used or not, _getUintRecipientStatus() will be called to fetch their status, which under the hood calls _getStatusRowColumn(). This function directly accesses recipientToStatusIndexes mapping with _recipientId and this will return the default '0' as recipientIndex when the caller is new. This tells _getUnitRecipientStatus() to get the status from the bitmap at location '0'. When this is the first-ever registration, location '0' will return Status.None, this will correctly direct _regiterRecipient() to identify the call as a new registration and update recipientToStatusIndexes[recipientId] to '0' - the default value for recipientsCounter. And then set the status at location '0' to Status.Pending.

//DonationVotingMerkleDistributionBaseStrategy.sol
    function _registerRecipient(bytes memory _data, address _sender)
        internal
        override
        onlyActiveRegistration
        returns (address recipientId)
    {
...
      Recipient storage recipient = _recipients[recipientId];
        // update the recipients data
        recipient.recipientAddress = recipientAddress;
        recipient.metadata = metadata;
        recipient.useRegistryAnchor = useRegistryAnchor ? true : isUsingRegistryAnchor;
|>    uint8 currentStatus = _getUintRecipientStatus(recipientId);
...

//DonationVotingMerkleDistributionBaseStrategy.sol
    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.sol
    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]);
    }

However, when the next caller calls to register. Since their recipientIdis new, recipientToStatusIndexes mapping will also return default '0' as their index which directs _getUnitRecipientStatus() will again look at the status at location '0' - now the first recipient's status. This is incorrect. And if the first recipient's status is still Status.Pending. The bitmap will not update. But if the first recipient's status has been upgraded to Status.Accepted by the pool manager. This will redirect _registerRecipient() to mistakenly identify the call as an existing registration and will change Status.Accepted to Status.Pending, effectively overwriting the previous recipient's status.

//DonationVotingMerkleDistributionBaseStrategy.sol
    function _registerRecipient(bytes memory _data, address _sender)
        internal
        override
        onlyActiveRegistration
        returns (address 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));
            }
...

This mistake will disqualify the first recipient's funds allocation. As we see, in _allocate there is a check to ensure that only if a recipient's status is Accepted can they receive allocation funds. In DonationVotingMerkleDistributionDirectTransferStrategy.sol ,the actual funds transfer takes place in _afterAllocate() hook, which means when an accepted recipient will lose on allocation funds.

//DonationVotingMerkleDistributionBaseStrategy.sol
    function _allocate(bytes memory _data, address _sender) internal virtual override onlyActiveAllocation {
...
       // If the recipient status is not 'Accepted' this will revert, the recipient must be accepted through registration
        if (Status(_getUintRecipientStatus(recipientId)) != Status.Accepted) {
            revert RECIPIENT_ERROR(recipientId);
        }
...

Here are (2) POC tests: (1) Shows a previously accepted recipient status gets overwritten by a new registration.

//test/foundry/strategies/DonationVotingMerkleDistributionBase.t.sol
    function test_register_second_time_overwrite_first_recipient_status() public {
        address recipientId = __register_accept_recipient();
        IStrategy.Status recipientStatus = strategy.getRecipientStatus(recipientId);
        assertEq(uint8(recipientStatus), uint8(IStrategy.Status.Accepted));
        address recipientId2 = __register_recipient2();
        recipientStatus = strategy.getRecipientStatus(recipientId);
        assertEq(uint8(recipientStatus), uint8(IStrategy.Status.Pending));
        IStrategy.Status recipientStatus2 = strategy.getRecipientStatus(recipientId2);
        assertEq(uint8(recipientStatus), uint8(IStrategy.Status.Pending));
    }

Running 1 test for test/foundry/strategies/DonationVotingMerkleDistributionBase.t.sol:DonationVotingMerkleDistributionBaseMockTest
[PASS] test_register_second_time_overwrite_first_recipient_status() (gas: 314716)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 26.42ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

(2) Shows in the context of DonationVotingMerkleDistributionDirectTransferStrategy.sol, a previously accepted recipient's status is overwritten and loss of allocation funds. There are two tests - a success and a failure case for comparison.

//test/foundry/strategies/DonationVotingMerkleDistributionDirectTransferStrategy.t.sol
//Success case
   function test_allocate_1st_recipient_allocation_succeed() public {
        uint256 balanceBefore = recipientAddress().balance;
        //1st recipient register and gets accepted
        address recipientId = __register_accept_recipient();
        IStrategy.Status recipientStatus = strategy.getRecipientStatus(recipientId);
        assertEq(uint8(recipientStatus), uint8(IStrategy.Status.Accepted));
        //randomAddress allocate 1e18 to 1st recipient
        DonationVotingMerkleDistributionBaseStrategy.Permit2Data memory permit2Data =
        DonationVotingMerkleDistributionBaseStrategy.Permit2Data({
            permit: ISignatureTransfer.PermitTransferFrom({
                permitted: ISignatureTransfer.TokenPermissions({token: NATIVE, amount: 1e18}),
                nonce: 0,
                deadline: allocationStartTime + 10000
            }),
            signature: ""
        });

        vm.warp(allocationStartTime + 1);
        vm.deal(randomAddress(), 1e18);
        vm.prank(randomAddress());

        vm.expectEmit(false, false, false, true);
        emit Allocated(recipientId, 1e18, NATIVE, randomAddress());

        allo().allocate{value: 1e18}(poolId, abi.encode(recipientId, permit2Data));
        uint256 balanceAfter = recipientAddress().balance;
        //1st recipient received 1e18 allocation
        assertEq(balanceAfter - balanceBefore, 1e18);
    }
//Fail case
   function test_allocate_1st_recipient_accepted_2nd_recipient_overwrrite_allocation_failed() public {
        uint256 balanceBefore = recipientAddress().balance;
        //1st recipient register and gets accepted
        address recipientId = __register_accept_recipient();
        IStrategy.Status recipientStatus = strategy.getRecipientStatus(recipientId);
        assertEq(uint8(recipientStatus), uint8(IStrategy.Status.Accepted));
        //2nd recipient register
        address recipientId2 = __register_recipient2();
        recipientStatus = strategy.getRecipientStatus(recipientId);
        //1st recipient status was overwritten by 2nd recipient. status: accepted->pending.
        assertEq(uint8(recipientStatus), uint8(IStrategy.Status.Pending));
        //2nd recipient status was in fact the 1st recipient status- also pending.
        IStrategy.Status recipientStatus2 = strategy.getRecipientStatus(recipientId2);
        assertEq(uint8(recipientStatus), uint8(IStrategy.Status.Pending));
        //randomAddress allocate 1e18 to 1st recipient but Reverted due to 1st recipient status was overwritten
        DonationVotingMerkleDistributionBaseStrategy.Permit2Data memory permit2Data =
        DonationVotingMerkleDistributionBaseStrategy.Permit2Data({
            permit: ISignatureTransfer.PermitTransferFrom({
                permitted: ISignatureTransfer.TokenPermissions({token: NATIVE, amount: 1e18}),
                nonce: 0,
                deadline: allocationStartTime + 10000
            }),
            signature: ""
        });
        vm.warp(allocationStartTime + 1);
        vm.deal(randomAddress(), 1e18);
        vm.prank(randomAddress());
        //allocation reverted
        vm.expectRevert(abi.encodePacked(RECIPIENT_ERROR.selector, uint256(uint160(recipientId))));
        allo().allocate{value: 1e18}(poolId, abi.encode(recipientId, permit2Data));
    }

Running 2 tests for test/foundry/strategies/DonationVotingMerkleDistributionDirectTransferStrategy.t.sol:DonationVotingMerkleDistributionDirectTransferStrategyTest
[PASS] test_allocate_1st_recipient_accepted_2nd_recipient_overwrrite_allocation_failed() (gas: 376623)
[PASS] test_allocate_1st_recipient_allocation_succeed() (gas: 303662)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 26.35ms

Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

Impact

As detailed above, existing accepted recipients eligible for rewards will lose their allocation funds due to status overwritten by new registration. In addition, a malicious user can repeatedly register new recipients to intentionally overwrite the existing eligible recipient causing a loss of funds. I think this is high severity, due to the easy and cheap attack, and the damage can be done also accidentally causing loss of funds and unfair review and allocation process.

Code Snippet

https://github.com/allo-protocol/allo-v2/blob/0b881ef4a0013d2809374c9ea69f4cf1288dfe62/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L580-L597

Tool used

Manual Review

Recommendation

In _registerRecipient, consider increment recipientsCounter before assigning it to recipientToStatusIndexes such that the first valid index would be 1 instead of 0. This way '0' location in the bitmap will only be accessed for identifying new registrants.

Duplicate of #199

sherlock-audit / 2023-09-Gitcoin-judging