GimelSec - Refunding NFT doesn't decrease the length of nftDeposits. A malicious user can block other users from depositing any NFT.

[github-actions]

GimelSec

high

Refunding NFT doesn't decrease the length of nftDeposits. A malicious user can block other users from depositing any NFT.

Summary

nftDepositLimit is hardcoded in all four kinds of bounty. And receiveNft() checks nftDeposits.length < nftDepositLimit. The problem is that refunding an NFT won’t decrease nftDeposits.length. A malicious user can deposit a NFT, then refund it. The user can do it repeatedly to fill up nftDeposits. After that, no one is able to deposit any NFT.

Vulnerability Detail

nftDepositLimit is hardcoded in all four kinds of bounty.
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L49

    function initialize(
        string memory _bountyId,
        address _issuer,
        string memory _organization,
        address _openQ,
        address _claimManager,
        address _depositManager,
        OpenQDefinitions.InitOperation memory _operation
    ) external initializer {
        require(bytes(_bountyId).length != 0, Errors.NO_EMPTY_BOUNTY_ID);
        require(bytes(_organization).length != 0, Errors.NO_EMPTY_ORGANIZATION);

        __ReentrancyGuard_init();

        __OnlyOpenQ_init(_openQ);
        __ClaimManagerOwnable_init(_claimManager);
        __DepositManagerOwnable_init(_depositManager);

        bountyId = _bountyId;
        issuer = _issuer;
        organization = _organization;
        bountyCreatedTime = block.timestamp;
        nftDepositLimit = 5;

        ...
    }

And receiveNft would check nftDeposits.length < nftDepositLimit
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L133

    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
        );
        …
    }

However, refunding NFT won’t decrease the length of nftDeposits[]. So a malicious user can do the following things to block other users from depositing any NFT.

1. deposit a NFT and refund it (nftDeposits.length = 1)
2. deposit a NFT and refund it (nftDeposits.length = 2)
3. deposit a NFT and refund it (nftDeposits.length = 3)
4. deposit a NFT and refund it (nftDeposits.length = 4)
5. deposit a NFT and refund it (nftDeposits.length = 5, receiveNft always reverts)

Impact

A malicious user can block other users from depositing any NFT.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L133
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L49
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/OngoingBountyV1.sol#L49
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/TieredFixedBountyV1.sol#L49
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/TieredPercentageBountyV1.sol#L48

Tool used

Manual Review

Recommendation

There are two things that need to be fixed. First, nftDepositLimit should not be hardcoded, it should be decided by the issuers. And don’t use nftDeposits.length in the check when nftDeposits.length can only increase. Use a new variable like nftDepositCount to record the actual amount of nftDeposit.

Duplicate of #262

[FlacoJones]

Going to remove the ability to fund with ERC721 all together

[IAm0x52]

Dupe of #262

[FlacoJones]

OpenQDev/OpenQ-Contracts#114