sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

pengun - Fee-on-transfer not supported in fundPool #881

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

pengun

medium

Fee-on-transfer not supported in fundPool

According to the contest page (https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/README.md?plain=1#L28), fee-on-transfer can be used when funding a pool. However, the current implementation does not support fee-on-transfer and therefore does not work as intended.

Vulnerability Detail

When Allo.sol's fundPool is performed, Strategy's increasePoolAmount is called. Because _amount represents the quantity to be transferred, not the actual quantity transferred, poolAmount will store a value greater than the actual tokens transferred.

function increasePoolAmount(uint256 _amount) external override onlyAllo {
        _beforeIncreasePoolAmount(_amount);
        poolAmount += _amount;
        _afterIncreasePoolAmount(_amount);
    }

The poolAmount stored in this way is used when distributing from the Strategy, and since it is set to a value larger than the actual token holdings, calculations using it will return incorrect values.

For example, QVBaseStrategy._getPayout below.

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);
    }

The payout is determined by the percentage of votes received in the poolAmount, but because the payout is set to be larger than it should be, people who receive late payouts may not be rewarded.

Impact

If your fundPool doesn't properly support fee-on-transfer, some users may not get rewarded.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/6430c8004017e96ae2f5aac365bdefd0b6eeea72/allo-v2/contracts/core/Allo.sol#L502-L520

Tool used

Manual Review

Recommendation

I recommend passing the actual amount transferred in the increasePoolAmount argument, or calling transfer inside the Strategy to add the increased balance to the poolAmount.

Duplicate of #19