Skip to content

Latest commit

 

History

History
172 lines (110 loc) · 7.21 KB

003-M.md

File metadata and controls

172 lines (110 loc) · 7.21 KB

RFPSimpleStrategy::_distribute() might revert even though it has enough funds to distribute

RFPSimpleStrategy::_distribute() function checks if the contract has enough funds to distribute, but the check is wrong. The contract compares the remaining pool funds and 100% of the proposalBid, but the amount that will be distributed is only a percentage of the proposalBid according to upcoming milestone. The comparison should be done with the milestone percentage of the proposalBid, not the 100% of the bid.

Vulnerability Detail

RFPSimpleStrategy::_distribute() function distributes some portion of the proposalBid in every milestone. This portion is determined by milestones, which are set by the contract manager before the distribution phase.

This function:

  1. Validates the upcoming milestone

  2. Checks if the pool has enough funds to distribute

  3. Updates pool funds by decreasing the poolAmount state variable

  4. Transfers the funds

The issue arises in the second step: while checking the pool funds. This check is done with this line:
if (recipient.proposalBid > poolAmount) revert NOT_ENOUGH_FUNDS();

The function reverts if the proposalBid is greater than the poolAmount. But the amount that will be distributed is not the proposalBid. It is only a portion of the proposalBid.

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L432C9-L432C75

    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 - This amount is not the amount that will be sent. 

        // Calculate the amount to be distributed for the milestone
-->     uint256 amount = (recipient.proposalBid * milestone.amountPercentage) / 1e18; // @audit - This amount is the one that should be checked.

        // Get the pool, subtract the amount and transfer to the recipient
        poolAmount -= amount; // @audit - The poolAmount will be decreased. Last a few milestone might revert because the poolAmount will be decreased below the full proposalBid. 
        _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);
    }

After the first milestone is distributed, future milestones will be at risk of not being paid depending on the milestone percentages.

Example Scenario

T0 - Initial state - The pool is funded with enough amount to pay the full proposalBid

  • proposalBid = 80

  • poolAmount = 120

  • Milestones = 60% and 40%

T1 - The first milestone is distributed.

  • proposalBid is smaller than the poolAmount. Function doesn't revert

  • amount to distribute = 48 (60% of the proposalBid)

  • remaining poolAmount = 72

T2 - The second milestone is going to be distributed.

  • proposalBid = 80

  • poolAmount = 72

  • proposalBid is greater than the poolAmount and the function reverts.

The milestone can not be distributed. But the actual amount that will be paid for this milestone was 32. The pool was very well funded for that amount but the function reverted.

Impact

RFPSimpleStrategy::_distribute() function will revert even if the pool has enough funds to distribute all milestones.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/rfp-simple/RFPSimpleStrategy.sol#L432C9-L432C75

    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 - This amount is not the amount that will be sent. 

        // Calculate the amount to be distributed for the milestone
-->     uint256 amount = (recipient.proposalBid * milestone.amountPercentage) / 1e18; // @audit - This amount is the one that should be checked.

        // Get the pool, subtract the amount and transfer to the recipient
        poolAmount -= amount; // @audit - The poolAmount will be decreased. Last a few milestone might revert because the poolAmount will be decreased below the full proposalBid.. 
        _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);
    }

Tool used

Manual Review

Recommendation

Comparison should be done using the amount that will be distributed in the current milestone, not using the full proposalBid amount

Change this:

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

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

To this:

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

        // make sure has enough funds to distribute based on the proposal bid
        if (amount > poolAmount) revert NOT_ENOUGH_FUNDS();

Note: The original submission can be found here.