sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

pontifex - QVBaseStrategy: incorrect calculation in voice credits allocation #935

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 12 months ago

pontifex

high

QVBaseStrategy: incorrect calculation in voice credits allocation

The mistake in the calculation leads to faster increasing of votes if _sender several times allocates a small amount of credits than at once the whole amount.

Vulnerability Detail

The QVBaseStrategy._qv_allocate function allocates voice credits to a recipient. There is a mistake at the line L#529 which lets increasing _recipient.totalVotesReceived by spending small amounts of credits.

506    function _qv_allocate(
507        Allocator storage _allocator,
508        Recipient storage _recipient,
509        address _recipientId,
510        uint256 _voiceCreditsToAllocate, // sending small amounts (A) // 1 // 2 // 1 // sending all credits at once (B) // 4
511        address _sender
512    ) internal onlyActiveAllocation {
513        // check the `_voiceCreditsToAllocate` is > 0
514        if (_voiceCreditsToAllocate == 0) revert INVALID();
515
516        // get the previous values
517        uint256 creditsCastToRecipient = _allocator.voiceCreditsCastToRecipient[_recipientId]; // A // 0 // 1 // 4 // B // 0
518        uint256 votesCastToRecipient = _allocator.votesCastToRecipient[_recipientId]; // A // 0 // 10e9 // 1 732 050 807 // B // 0
519
520        // get the total credits and calculate the vote result
521        uint256 totalCredits = _voiceCreditsToAllocate + creditsCastToRecipient; // A // 1 // 1 + 2 = 3 // 4 + 1 = 5 // B // 4
522        uint256 voteResult = _sqrt(totalCredits * 1e18); // A // 10e9 // 1 732 050 807 // 2 236 067 977 // B // 2 000 000 000
523
524        // update the values
525        voteResult -= votesCastToRecipient; // A // 10e9 - 0 // 1 732 050 807 - 10e9 = 732 050 807 // 2 236 067 977 - 1 732 050 807 = 504 017 170 // B // 2 000 000 000 - 0
526        totalRecipientVotes += voteResult; // A // 10e9 // 10e9 + 732 050 807 = 1 732 050 807 // 2 236 067 977 // B // 2 000 000 000
527        _recipient.totalVotesReceived += voteResult; // A // 10e9 // 1 732 050 807 // 2 236 067 977 // B // 2 000 000 000
528
529        _allocator.voiceCreditsCastToRecipient[_recipientId] += totalCredits; // A // 0 + 1 // 1 + 3 = 4 // 4 + 5 = 9 // B // 4
530        _allocator.votesCastToRecipient[_recipientId] += voteResult; // A // 10e9 // 1 732 050 807 // 2 236 067 977 // B // 2 000 000 000
531
532        // emit the event with the vote results
533        emit Allocated(_recipientId, voteResult, _sender);
534    }

In the shown case we receive 2 236 067 977 instead of 2 000 000 000 for 4 credits by allocating small amounts. There should be = instead of += at the line L#529.

Impact

_sender can use less credits for the same amount of votes if several times allocates small amounts of credits.

Code Snippet

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

Tool used

Manual Review

Recommendation

Fix the mistake.

Duplicate of #48