0x52 - OngoingBountyV1 is incompatible with NFTs but still accepts NFT deposits

[github-actions]

0x52

medium

OngoingBountyV1 is incompatible with NFTs but still accepts NFT deposits

Summary

OngoingBountyV1 is incompatible with NFTs but still accepts NFT deposits

Vulnerability Detail

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/OngoingBountyV1.sol#L133-L160

OngoingBountyV1 is designed to receive NFTs and NFTs can be deposited to it via DepositManager#fundBountyNFT

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L173-L197

However when ongoing bounties are claimed they have no method to distribute the NFTs that are deposited.

Impact

OngoingBountyV1 has no way to distribute NFTs

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L173-L197

Tool used

Manual Review

Recommendation

Change _claimOngoingBounty to allow it to distribute NFTs

[FlacoJones]

Will remove Ongoing for now

[FlacoJones]

OpenQDev/OpenQ-Contracts#112

[pauliax]

Escalate for 35 USDC. I understand that the lead Watson submitted this and helped the judging, but I believe this issue should be treated similarly as #304, because OngoingBountyV1 inherits from BountyCore and the NFTs can be refunded so no funds are at risk: https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L64

[sherlock-admin]

Escalate for 35 USDC. I understand that the lead Watson submitted this and helped the judging, but I believe this issue should be treated similarly as #304, because OngoingBountyV1 inherits from BountyCore and the NFTs can be refunded so no funds are at risk: https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L64

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

[jkoppel]

This is a duplicate of #403

@hrishibhat Do I need to escalate to make sure my issue gets marked as a duplicate of this one in the event that this is changed to a reward?

[IAm0x52]

Escalate for 25 USDC

What @pauliax said. Should definitely be low since no funds are at risk. While refunds can be broken via number of ways causing the NFTs to get stuck. That is a separate issue and to count this is medium because of that would be double counting the issue that refunds can be broken.

[sherlock-admin]

Escalate for 25 USDC

What @pauliax said. Should definitely be low since no funds are at risk. While refunds can be broken via number of ways causing the NFTs to get stuck. That is a separate issue and to count this is medium because of that would be double counting the issue that refunds can be broken.

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

[jkoppel]

Should definitely be low since no funds are at risk

Seems to assume the NFT's can't have monetary value.

[hrishibhat]

Escalation accepted

Considering this issue as low since the NFT's can be refunded.

[sherlock-admin]

Escalation accepted

Considering this issue as low since the NFT's can be refunded.

This issue's escalations have been accepted!

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