Martians - Accepted recipient could get more amount than the `proposalBid` value

[sherlock-admin2]

Martians

medium

Accepted recipient could get more amount than the proposalBid value

In the contract RFPSimpleStrategy, the function setMilestones is intended to set milestones at exactly 100%. However, the function allows multiple calls to be made, which may cause the amountPercentage of milestones to exceed 100%, resulting in extra funds being transferred to the approvedRecipientId beyond the intended amount.

Vulnerability Detail

The setMilestones function permits the poolManager to set milestones using the setMilestones function. This is checked to ensure that milestones are not already set by the following condition:

if (upcomingMilestone != 0) revert MILESTONES_ALREADY_SET();

However, multiple calls can be made until the _distribute function is invoked, which increments the value of upcomingMilestone. This could cause the amountPercentage to exceed 100%.

Before acceptedRecipient submits the milestone, if setMilestones function is called again, milestones would be pushed to array at end and there is no check to ensure the total percentage in the array is less equal to 100. So adding milestones twice would give the recipient 200% of the recipient bid.

Impact

If the proposalBid of acceptedRecipientId is less than the poolAmount, the recipientAddress of acceptedRecipientId receives more tokens, than the issued limit (100%).

Code Snippet

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

POC

/// File: RFPSimpleStrategy.t.sol

    function testMilestones() public {
        __setMilestones();
        __setMilestones();
        uint totalPercentage;
        for (uint x; x < 4;) {
            (uint percentage_, , ) = strategy.milestones(x);
            totalPercentage += percentage_;
            unchecked {
                x++;
            }
        }
        assertEq(totalPercentage, 2e18); // 200%
    }

Tool used

Manual Review, Foundry

Recommendation

function setMilestones(Milestone[] memory _milestones) external onlyPoolManager(msg.sender) {
-        if (upcomingMilestone != 0) revert MILESTONES_ALREADY_SET();
+        if (milestones.length != 0) revert MILESTONES_ALREADY_SET();

Duplicate of https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/176

[neeksec]

Duplicate of #176