unforgiven - Attacker can perform griefing and DOS by deposit and refunding NFTs into the bounty

[github-actions]

unforgiven

medium

Attacker can perform griefing and DOS by deposit and refunding NFTs into the bounty

Summary

Function fundBountyNFT() is used for depositing NFTs into the Bounty contract to distribute NFT as prize for winners and it can be called by anyone. each Bounty accept maximum 5 NFT and attacker can deposit 5 cheap NFT and refund them and prevent real bounty issuer to deposit real prizes and by doing so Bounty contract would be useless and already deposited funds would be locked for deposit time.

Vulnerability Detail

Function fundBountyNFT() is for depositing funds into the bounty and it is callable by anyone. when deposits accrue for a bounty, Bounty contract add it to deposits and add token address to nftDeposits in the receiveNft() function. function receiveNft() only allow deposits if number of deposits are lower than nftDepositLimit which the default value is 5. attacker can call fundBountyNFT() for a bounty a couple of time with different white listed NFT token addresses and make that bounty's NFT deposits to reach the limit and then issuer can't fund the bounty. it's a DOS and griefing attack as some deposit funds would stuck in the contract for deposit time and Bounty contract won't be useful.
This is receiveNft() code in AtomicBountyV1 contract (receiveNft() in other contracts are similar):

    function receiveNft(
        address _sender,
        address _tokenAddress,
        uint256 _tokenId,
        uint256 _expiration,
        bytes calldata
    ) external onlyDepositManager nonReentrant returns (bytes32) {
        require(
            nftDeposits.length < nftDepositLimit,
            Errors.NFT_DEPOSIT_LIMIT_REACHED
        );
        require(_expiration > 0, Errors.EXPIRATION_NOT_GREATER_THAN_ZERO);
        _receiveNft(_tokenAddress, _sender, _tokenId);

        bytes32 depositId = _generateDepositId();

        funder[depositId] = _sender;
        tokenAddress[depositId] = _tokenAddress;
        depositTime[depositId] = block.timestamp;
        tokenId[depositId] = _tokenId;
        expiration[depositId] = _expiration;
        isNFT[depositId] = true;

        deposits.push(depositId);
        nftDeposits.push(depositId);

        return depositId;
    }

As you can see code checks that nftDeposits.length < nftDepositLimit and only allow deposits when this condition is true. and code adds deposited NFT to nftDeposits. to exploit this attacker can perform this steps:

1. white listed NFT tokens are 10 which are registered in OpenQWhitelist contract.
2. the value of nftDepositLimit is 5.
3. issuer creates Bounty1 and he wants to deposit (id=10 NFT1) token and (id=20 NFT2) token.
4. issuer deposits (id=10 NFT1) for 3 month which is the duration of the bounty.
5. attacker deposits 4 other NFT3 into the Bounty1 for 1 day and make nftDeposits.length to reach nftDepositLimit.
6. now issuer can't deposit (id=20 NFT2) because Bounty1 has 5 NFT deposits in nftDeposits and code won't allow deposits.

Impact

Attacker can cause griefing and DOS.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L113-L147

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L132-L135

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

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

Tool used

Manual Review

Recommendation

create two limit for NFT deposit tokens. one for issuer and one for others. and keep track of issuer deposit NFT tokens and others separately. in this way issuer can always deposit tokens even if others deposits small amounts before him.

Duplicate of #262