unforgiven - when issuer set new winner by calling setTierWinner() code should reset invoice and supporting documents for that tier

[github-actions[bot]]

unforgiven

medium

when issuer set new winner by calling setTierWinner() code should reset invoice and supporting documents for that tier

Summary

if invoice or supporting documents are required to receive the winning prize then tier winner should provide them. bounty issuer or oracle would set invoice and supporting document status of a tier by calling setInvoiceComplete() and setSupportingDocumentsComplete(). bounty issuer can set tier winners by calling setTierWinner() but code won't reset the status of the invoice and supporting documents when tier winner changes. a malicious winner can bypass invoice and supporting document check by this issue.

Vulnerability Detail

if bounty issuer set invoice and supporting documents as required for the bounty winners in the tiered bounty, then tier winner should provide those and bounty issuer or off-chain oracle would set the status of the invoice and documents for that tier. but if issuer wants to change a tier winner and calls setTierWinner() code would changes the tier winner but won't reset the status of the invoice and supporting documents for the new winner. This is the setTierWinner() code in OpenQV1 and TieredBountyCore:

    function setTierWinner(
        string calldata _bountyId,
        uint256 _tier,
        string calldata _winner
    ) external {
        IBounty bounty = getBounty(_bountyId);
        require(msg.sender == bounty.issuer(), Errors.CALLER_NOT_ISSUER);
        bounty.setTierWinner(_winner, _tier);

        emit TierWinnerSelected(
            address(bounty),
            bounty.getTierWinners(),
            new bytes(0),
            VERSION_1
        );
    }

    function setTierWinner(string memory _winner, uint256 _tier)
        external
        onlyOpenQ
    {
        tierWinners[_tier] = _winner;
    }

As you can see code only sets the tierWinner[tier] and won't reset invoiceComplete[tier] or supportingDocumentsComplete[tier] to false. This would cause an issue when issuer wants to change the tier winner. these are the steps that makes the issue:

1. UserA creates tiered Bounty1 and set invoice and supporting documents as required for winners to claim their funds.
2. UserA would set User1 as winner of tier 1 and User1 completed the invoice and oracle would set invoiceComplete[1] = true.
3. UserA would change tier winner to User2 because User1 didn't complete supporting documents phase. now User2 is winner of tier 1 and invoiceComplete[1] is true and User2 only required to complete supporting documents and User2 would receive the win prize without completing the invoice phase.

Impact

malicious winner can bypass invoice and supporting document check when they are required if he is replace as another person to be winner of a tier.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/TieredBountyCore.sol#L59-L64

Tool used

Manual Review

Recommendation

set status of the invoiceComplete[tier] or supportingDocumentsComplete[tier] to false in setTierWinner() function.

[0xunforgiven]

Escalate for 31 USDC

my issue is not duplicate of #425.

issue #425 is: "array lists invoiceCompleteClaimIds[] and supportingDocumentsCompleteClaimIds[] can contain claimIds that are incomplete". that is about those array list containing extra values and the issue has no serious impact because code checks mappings to check invoice and docuements status.

my issue explains that when a tier winner gets changed (Bounty issuer change tier1 winner from User1 to User2 for any reason) code doesn't reset the values in the mappings invoiceComplete[tier1] or supportingDocumentsComplete[tier1]. so values in those mappings would shoe wrong values for the new tier1 winner. the POC shows how the issue happens:

1. UserA creates tiered Bounty1 and set invoice and supporting documents as required for winners to claim their funds.
2. UserA would set User1 as winner of tier 1 and User1 completed the invoice and oracle would set invoiceComplete[1] = true.
3. UserA would change tier winner to User2 because User1 didn't complete supporting documents phase. now User2 is winner of tier 1 and invoiceComplete[1] is true and User2 only required to complete supporting documents and User2 would receive the win prize without completing the invoice phase.

[sherlock-admin]

Escalate for 31 USDC

my issue is not duplicate of #425.

issue #425 is: "array lists invoiceCompleteClaimIds[] and supportingDocumentsCompleteClaimIds[] can contain claimIds that are incomplete". that is about those array list containing extra values and the issue has no serious impact because code checks mappings to check invoice and docuements status.

my issue explains that when a tier winner gets changed (Bounty issuer change tier1 winner from User1 to User2 for any reason) code doesn't reset the values in the mappings invoiceComplete[tier1] or supportingDocumentsComplete[tier1]. so values in those mappings would shoe wrong values for the new tier1 winner. the POC shows how the issue happens:

1. UserA creates tiered Bounty1 and set invoice and supporting documents as required for winners to claim their funds.
2. UserA would set User1 as winner of tier 1 and User1 completed the invoice and oracle would set invoiceComplete[1] = true.
3. UserA would change tier winner to User2 because User1 didn't complete supporting documents phase. now User2 is winner of tier 1 and invoiceComplete[1] is true and User2 only required to complete supporting documents and User2 would receive the win prize without completing the invoice phase.

You've created a valid escalation for 31 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Not a valid duplicate of #425 and a valid issue _eligibleToClaimAtomicBounty would end up validating a user without completing the invoice phase as shown.

[sherlock-admin]

Escalation accepted

Not a valid duplicate of #425 and a valid issue _eligibleToClaimAtomicBounty would end up validating a user without completing the invoice phase as shown.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[jacksanford1]

Comment from Protocol Team:

I agree it would be better form to reset associated tiers documents + invoice to false if a tier winner is overwritten, but for now the bounty admin is a trusted party, and can always manually reset the tier afterwards.

I don't think this needs a fix at the moment

Classifying this issue as "Won't Fix."