_qv_allocate function will not work as expected

Vulnerability Detail

The _qv_allocate function will not work as expected during a specific time period. The onlyActiveAllocation modifier allows calling of the _qv_allocate function between allocationStartTime and allocationEndTime. Notably, the allocationStartTime is earlier than the registrationStartTime

        if (
            block.timestamp > _registrationStartTime || _registrationStartTime > _registrationEndTime
                || _registrationStartTime > _allocationStartTime || _allocationStartTime > _allocationEndTime
                || _registrationEndTime > _allocationEndTime
        ) {
            revert INVALID();
        }

Before block.timestamp >= registrationStartTime, newly recipients can not be registered. Consequently, between the times of allocationStartTime and registrationStartTime, the _qv_allocate function can not be called because no recipients are registered to bypass this check:

        // check that the recipient is accepted
        if (!_isAcceptedRecipient(recipientId)) revert RECIPIENT_ERROR(recipientId);

in _allocate function of QVSimpleStrategy contract.

Impact

_qv_allocate function is blocked for specific time period.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/qv-base/QVBaseStrategy.sol#L512

    function _qv_allocate(
        Allocator storage _allocator,
        Recipient storage _recipient,
        address _recipientId,
        uint256 _voiceCreditsToAllocate,
        address _sender
    ) internal onlyActiveAllocation {

Tool used

Manual Review

Recommendation

Replace allocationStartTime with registrationStartTime in _checkOnlyActiveAllocation modifier:

if (registrationStartTime > block.timestamp || block.timestamp > allocationEndTime) {
            revert ALLOCATION_NOT_ACTIVE();
        }

sherlock-admin commented 1 year ago

1 comment(s) were left on this issue during the judging contest.

n33k commented:

invalid, low impact

jkoppel commented 1 year ago

Escalate.

This is invalid and not a duplicate of #839. #839 concerns the 1-second overlap between allocation and distribution time caused by the buggy check in _checkOnlyActiveAllocation. This issue does not address that code or that cause at all.

sherlock-admin2 commented 1 year ago

Escalate.

This is invalid and not a duplicate of #839. #839 concerns the 1-second overlap between allocation and distribution time caused by the buggy check in _checkOnlyActiveAllocation. This issue does not address that code or that cause at all.

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.

neeksec commented 1 year ago

Agree with Escalation.

Was mistakenly put valid by me.

Evert0x commented 1 year ago

Planning to accept escalation and remove duplication state.

Evert0x commented 1 year ago

Result: Invalid Unique

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

jkoppel: accepted

sherlock-audit / 2023-09-Gitcoin-judging