0x52 - Adversary can break NFT distribution by depositing up to max then refunding all of them

[github-actions]

0x52

medium

Adversary can break NFT distribution by depositing up to max then refunding all of them

Summary

Bounties limit the number of NFT deposits to five. An adversary can block adding NFTs by repeatedly depositing and withdrawing an NFT.

Vulnerability Detail

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

All bounties use BountyCore#refundDeposit to process refunds to user. This simply transfers the NFT back to the funder but leaves the nftDeposit. This uses up the deposit limit which is current set to 5. Since the deposit cap is used up by deposits that have been refunded the slots can't be used to distribute legitimate NFTs to the bounty claimant.

Impact

Adversary can block legitimate NFT distribution

Code Snippet

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

Tool used

Manual Review

Recommendation

When an NFT deposit is refunded it should remove the depositID from nftDeposits

[FlacoJones]

Valid. Will fix by removing nft funding and disbaling crowdfunding

[FlacoJones]

OpenQDev/OpenQ-Contracts#113

and

OpenQDev/OpenQ-Contracts#116

and

OpenQDev/OpenQ-Contracts#114

[jacksanford1]

Lead Senior Watson comment on PR #113:


Changes look good.

Token requirement has been simplified to just a whitelist. It makes the rest changes to support this revision. tokenAddressLimitReach method has been removed from DepositManagerV1. _tokenAddressLimit has been removed from TokenWhitelist constructor. TOKEN_ADDRESS_LIMIT has been removed along with it's setter. Tests have been updated to accommodate changes.

Lead Senior Watson comment on PR #114:


Changes look good. Removes all logic from bounties. Seems to have gotten all related code. Will keep my out for any that was missed on when checking the rest of the PRs

Lead Senior Watson comment on PR #116:


Changes look good. Requires that funder is the issuer. This prevents a whole host of potential exploits in exchange for closing the otherwise open funding model.

[jacksanford1]

Lead Senior Watson comment on PR #113:


Changes look good.

Token requirement has been simplified to just a whitelist. It makes the rest changes to support this revision. tokenAddressLimitReach method has been removed from DepositManagerV1. _tokenAddressLimit has been removed from TokenWhitelist constructor. TOKEN_ADDRESS_LIMIT has been removed along with it's setter. Tests have been updated to accommodate changes.

Lead Senior Watson comment on PR #114:


Changes look good. Removes all logic from bounties. Seems to have gotten all related code. Will keep my out for any that was missed on when checking the rest of the PRs

Lead Senior Watson comment on PR #116:


Changes look good. Requires that funder is the issuer. This prevents a whole host of potential exploits in exchange for closing the otherwise open funding model.