Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

jkoppel - NFTs cannot be claimed from ongoing bounties; will be stuck forever when refunds disabled #403

Closed
github-actions bot opened this issue Feb 21, 2023 · 4 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Excluded Excluded by the judge without consulting the protocol or the senior Non-Reward This issue will not receive a payout

Comments

@github-actions
Copy link

github-actions bot commented Feb 21, 2023

jkoppel

medium

NFTs cannot be claimed from ongoing bounties; will be stuck forever when refunds disabled

Summary

OngoingBountyV1 can accept NFT deposits but will never release them in claims. They can only be retrieved by refunds. However, a separate issue ( https://github.com/sherlock-audit/2023-02-openq-jkoppel/issues/3 ) makes it possible to disable refunds, causing it to be stuck.

Vulnerability Detail

  1. User calls DepositManager.fundBountyNFT for an OngoingBounty, on the mistaken belief that the NFT will be awarded to claimants of the bounty.
  2. Griefer performs the attack in https://github.com/sherlock-audit/2023-02-openq-jkoppel/issues/3
  3. The NFT is stuck in the contract forever.

Impact

Anyone who accidentally sends an NFT to an ongoing bounty may lose it forever.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/OngoingBountyV1.sol#L133 provides an implementation of receiveNFT, same as the other bounty types. However, claimOngoingPayout ( https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/OngoingBountyV1.sol#L96 ) does not release the NFTs.

Tool used

Manual Review

Recommendation

Revert all NFT deposits to an OngoingBounty

Duplicate of #261

@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Feb 21, 2023
@sherlock-admin sherlock-admin added the Non-Reward This issue will not receive a payout label Mar 7, 2023
@jkoppel
Copy link

jkoppel commented Mar 7, 2023

Escalate for 43 USDC

This is a duplicate of #261

My escalation is conditional on #261 remaining a Medium; if it's changed to low, then this is moot.

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Mar 7, 2023

Escalate for 43 USDC

This is a duplicate of #261

You've created a valid escalation for 43 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.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Mar 7, 2023
@hrishibhat
Copy link
Contributor

Escalation accepted

Valid duplicate of #261

@sherlock-admin
Copy link
Contributor

Escalation accepted

Valid duplicate of #261

This issue's escalations have been accepted!

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

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label and removed Escalated This issue contains a pending escalation labels Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Excluded Excluded by the judge without consulting the protocol or the senior Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

3 participants