sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

pengun - RFPSimpleStrategy's _distribute implementation is incorrect #879

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 12 months ago

pengun

high

RFPSimpleStrategy's _distribute implementation is incorrect

RFPSimpleStrategy cannot distribute a portion of the fund because the proposalBid comparison syntax in _distribute is incorrect.

Vulnerability Detail

User can receive a portion of the fund when the recipient achieves a milestone.

function _distribute(address[] memory, bytes memory, address _sender)
        internal
        virtual
        override
        onlyInactivePool
        onlyPoolManager(_sender)
    {
        // check to make sure there is a pending milestone
        if (upcomingMilestone >= milestones.length) revert INVALID_MILESTONE();

        IAllo.Pool memory pool = allo.getPool(poolId);
        Milestone storage milestone = milestones[upcomingMilestone];
        Recipient memory recipient = _recipients[acceptedRecipientId];

        // make sure has enough funds to distribute based on the proposal bid
        if (recipient.proposalBid > poolAmount) revert NOT_ENOUGH_FUNDS(); //@audit <==

        // Calculate the amount to be distributed for the milestone
        uint256 amount = (recipient.proposalBid * milestone.amountPercentage) / 1e18;

        // Get the pool, subtract the amount and transfer to the recipient
        poolAmount -= amount;
        _transferAmount(pool.token, recipient.recipientAddress, amount);

        // Set the milestone status to 'Accepted'
        milestone.milestoneStatus = Status.Accepted;

        // Increment the upcoming milestone
        upcomingMilestone++;

        // Emit events for the milestone and the distribution
        emit MilestoneStatusChanged(upcomingMilestone, Status.Accepted);
        emit Distributed(acceptedRecipientId, recipient.recipientAddress, amount, _sender);
    }

The if (recipient.proposalBid > poolAmount) revert NOT_ENOUGH_FUNDS(); syntax in _distribute compares proposalBid, the total reward to be received, to poolAmount, and requires that poolAmount be greater to pass.

As the recipient achieves milestones, the poolAmount is reduced by poolAmount -= amount;.

Therefore, you shouldn't compare the gradually decreasing poolAmount to the fixed value of proposalBid.

When the latter milestone is achieved, NOT_ENOUGH_FUNDS will occur because the poolAmount is reduced by the rewarded amount, but the proposalBid remains at its initial value.

Impact

The recipient does not receive the full amount promised.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/6430c8004017e96ae2f5aac365bdefd0b6eeea72/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L432

Tool used

Manual Review

Recommendation

Decrease recipient.proposalBid by the amount of the reward paid.

Duplicate of #492