hals - [M-09] `QVSimpleStrategy` contract: removing pool allocators will not remove their votes

[sherlock-admin]

hals

medium

[M-09] QVSimpleStrategy contract: removing pool allocators will not remove their votes

QVSimpleStrategy contract: removing pool allocators will not remove their votes.

Vulnerability Detail

• In QVSimpleStrategy strategy contract: funds accepted recipients are receiving funds based on the votes they got from the accepted allocators (pool managers).

• Then when the funds are distributed; each accepted recipient will receive payment proportional to the total number of votes they received:

  QVBaseStrategy::_getPayout function

    function _getPayout(address _recipientId, bytes memory)
        internal
        view
        virtual
        override
        returns (PayoutSummary memory)
    {
        Recipient memory recipient = recipients[_recipientId];
  
        // Calculate the payout amount based on the percentage of total votes
        uint256 amount;
        if (!paidOut[_recipientId] && totalRecipientVotes != 0) {
            amount = poolAmount * recipient.totalVotesReceived / totalRecipientVotes;
        }
        return PayoutSummary(recipient.recipientAddress, amount);
    }

• But allocators can be removed by the pool admin via Allo::removePoolManager; and when doing so, the votes that were given by the removed allocator will still be counted.

Impact

If a recipient got votes from removed allocators; they will be getting more funds upon distribution due to the counting of the invalid votes of the removed allocators.

Code Snippet

QVBaseStrategy::_qv_allocate function

function _qv_allocate(
        Allocator storage _allocator,
        Recipient storage _recipient,
        address _recipientId,
        uint256 _voiceCreditsToAllocate,
        address _sender
    ) internal onlyActiveAllocation {
        // check the `_voiceCreditsToAllocate` is > 0
        if (_voiceCreditsToAllocate == 0) revert INVALID();

        // get the previous values
        uint256 creditsCastToRecipient = _allocator.voiceCreditsCastToRecipient[_recipientId];
        uint256 votesCastToRecipient = _allocator.votesCastToRecipient[_recipientId];

        // get the total credits and calculate the vote result
        uint256 totalCredits = _voiceCreditsToAllocate + creditsCastToRecipient;
        uint256 voteResult = _sqrt(totalCredits * 1e18);

        // update the values
        voteResult -= votesCastToRecipient;
        totalRecipientVotes += voteResult;
        _recipient.totalVotesReceived += voteResult;

        _allocator.voiceCreditsCastToRecipient[_recipientId] += totalCredits;
        _allocator.votesCastToRecipient[_recipientId] += voteResult;

        // emit the event with the vote results
        emit Allocated(_recipientId, voteResult, _sender);
    }

QVBaseStrategy::_distribute function/ L448-L449

 PayoutSummary memory payout = _getPayout(recipientId, "");
 uint256 amount = payout.amount;

QVBaseStrategy::_distribute function/ L456

_transferAmount(pool.token, recipient.recipientAddress, amount);

QVBaseStrategy::_getPayout function

     function _getPayout(address _recipientId, bytes memory)
          internal
          view
          virtual
          override
          returns (PayoutSummary memory)
      {
          Recipient memory recipient = recipients[_recipientId];

          // Calculate the payout amount based on the percentage of total votes
          uint256 amount;
          if (!paidOut[_recipientId] && totalRecipientVotes != 0) {
              amount = poolAmount * recipient.totalVotesReceived / totalRecipientVotes;
          }
          return PayoutSummary(recipient.recipientAddress, amount);
      }

Allo::removePoolManager function

   function removePoolManager(uint256 _poolId, address _manager) external onlyPoolAdmin(_poolId) {
        _revokeRole(pools[_poolId].managerRole, _manager);
    }

Tool used

Manual Review

Recommendation

Add a mechanism to remove/invalidate removed allocator votes so that it would't be counted when calculating the recipient distributed payment.

[sherlock-admin]

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

n33k commented:

invalid, low likelihood of removing allocator and low impact