Rejected milestones can be distributed

Rejected milestones can be distributed and acceptedRecipientId will receive each milestone amount if the pool has enough amount.

Vulnerability Detail

The function is intended to distribute the upcomingMilestone amount, but there is no check if the upcomingMilestone was rejected.

The function starts with this check, but if the upcomingMilestone was previously rejected, it will pass.

// check to make sure there is a pending milestone
if (upcomingMilestone >= milestones.length) revert INVALID_MILESTONE();

And from there:

the whole rejectMilestone() defeats its purpose because acceptedRecipientId can submit an invalid upcomingMilestone and it can still be used in _distribute.
A milestone that was set by the pool manager and not submitted by acceptedRecipientId will continue to be valid in _distribute.

Impact

Funds from the pool will be distributed incorrectly and the recipient may receive a rejected milestone.amountPercentage of the pool.

Code Snippet

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

Tool used

Manual Review

Recommendation

/// @notice Distribute the upcoming milestone to acceptedRecipientId.
/// @dev '_sender' must be a pool manager to distribute.
/// @param _sender The sender of the distribution
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();

+   if(milestones[upcomingMilestone].milestoneStatus == Status.Rejected) 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();

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

sherlock-audit / 2023-09-Gitcoin-judging