hals - `RFPSimpleStrategy::rejectMilestone` : pool manager can reject any milestone as there's no check if the milestone status is set to pending by the accepted recipient.

[sherlock-admin2]

hals

medium

RFPSimpleStrategy::rejectMilestone : pool manager can reject any milestone as there's no check if the milestone status is set to pending by the accepted recipient.

RFPSimpleStrategy::rejectMilestone : pool manager can reject any milestone as there's no check if the milestone status is set to pending by the accepted recipient.

Vulnerability Detail

• In RFPSimpleStrategy strategy contract: the pool manager can set a list of milestones of payments to be distributed later to the selected accepted recipient (acceptedRecipientId) , and this operation can be done only once via setMilestones function.

• The acceptedRecipientId can submit a milestone: in order to get the next milestone payment when this milestone is distributed; and this sets the milestone status from None to Pending.

• Then the pool manager can either reject the submitted milestone by calling rejectMilestone function where it sets the status of the milestone from Pending to Rejected; or can leave the status as it is (Pending); so if the milestone is set to Accepted; then it can be distributed to the intended accepted recipient.

• But when the pool manager calls rejectMilestone function to reject a mileStone ; there's no check made to check if the milestone is submitted by the accepted recipient or not; i.e it doesn't check if the status of the milestone is Pending before setting it to Rejected.

• So this function enables the manager from setting the status of any milestone to Rejected as long as it hasn't been distributed (executed).

Impact

This supposed to result in disabling milestone distribution for any milestone payment that were set to Rejected even if they are not submitted by the recipient (but will not due to another vulnerability in the _distribute function where the milestone status is not checked before execution)

Code Snippet

RFPSimpleStrategy::submitUpcomingMilestone function

    function submitUpcomingMilestone(Metadata calldata _metadata) external {
        // Check if the 'msg.sender' is the 'acceptedRecipientId' and is a member of the 'Profile'
        if (acceptedRecipientId != msg.sender && !_isProfileMember(acceptedRecipientId, msg.sender)) {
            revert UNAUTHORIZED();
        }

        // Check if the upcoming milestone is in fact upcoming
        if (upcomingMilestone >= milestones.length) revert INVALID_MILESTONE();

        // Get the milestone and update the metadata and status
        Milestone storage milestone = milestones[upcomingMilestone];
        milestone.metadata = _metadata;

        // Set the milestone status to 'Pending' to indicate that the milestone is submitted
        milestone.milestoneStatus = Status.Pending;

        // Emit event for the milestone
        emit MilstoneSubmitted(upcomingMilestone);
    }

RFPSimpleStrategy::rejectMilestone function

    function rejectMilestone(uint256 _milestoneId) external onlyPoolManager(msg.sender) {
        // Check if the milestone is already accepted
        if (milestones[_milestoneId].milestoneStatus == Status.Accepted) revert MILESTONE_ALREADY_ACCEPTED();

        milestones[_milestoneId].milestoneStatus = Status.Rejected;

        emit MilestoneStatusChanged(_milestoneId, milestones[_milestoneId].milestoneStatus);
    }

Tool used

Manual Review

Recommendation

In rejectMilestone function check the milestone status before setting its status to Rejected:

    function rejectMilestone(uint256 _milestoneId) external onlyPoolManager(msg.sender) {
        // Check if the milestone is already accepted
        if (milestones[_milestoneId].milestoneStatus == Status.Accepted) revert MILESTONE_ALREADY_ACCEPTED();
+       require(milestones[_milestoneId].milestoneStatus == Status.Pending,"milestone is not submitted by the accepted recipient");

        milestones[_milestoneId].milestoneStatus = Status.Rejected;

        emit MilestoneStatusChanged(_milestoneId, milestones[_milestoneId].milestoneStatus);
    }

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

n33k commented:

invalid, intended behavior