ak1 - Number of token limit check is not same for ERC20 and ERC721 contracts

[github-actions]

ak1

medium

Number of token limit check is not same for ERC20 and ERC721 contracts

Summary

Protocol limits the total number of tokens that are used to bounty funding. This is due to avoid the potential DOS when the number of tokens are increased

It has logic to check for white listed tokens and number of tokens.

For ERC20 tokens, this number of token limit is done only for non-whitelisted tokens.

But, for ERC721 tokens, there is only whitelisted tokens are allowed.

Vulnerability Detail

fundBountyToken function checks the limit only on the non whitelisted tokens.

function fundBountyToken(
    address _bountyAddress,
    address _tokenAddress,
    uint256 _volume,
    uint256 _expiration,
    string memory funderUuid
) external payable onlyProxy {
    IBounty bounty = IBounty(payable(_bountyAddress));


    if (!isWhitelisted(_tokenAddress)) {
        require(
            !tokenAddressLimitReached(_bountyAddress),
            Errors.TOO_MANY_TOKEN_ADDRESSES
        );
    }

receiveNft strictly checking the number of tokens

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

Impact

When number of whitelisted tokens are increased, again this could lead to DOS.

Code Snippet

NFT limit check

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

ERC20 limit check

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

Tool used

Manual Review

Recommendation

Update the below codes such that it would validate number of tokens also.

function fundBountyToken(
    address _bountyAddress,
    address _tokenAddress,
    uint256 _volume,
    uint256 _expiration,
    string memory funderUuid
) external payable onlyProxy {
    IBounty bounty = IBounty(payable(_bountyAddress));


    if (!isWhitelisted(_tokenAddress)) {
        require(
            !tokenAddressLimitReached(_bountyAddress),
            Errors.TOO_MANY_TOKEN_ADDRESSES
        );
    }

[FlacoJones]

We are going to remove the ability to fund with an arbitrary ERC20 - removing the TOKEN_ADDRESS_LIMIT and simply reverting if a token, erc721 or erc20, is not whitelisted



This will effectively cap the number of ERC20 or ERC721 tokens to the total number of whitelisted tokens (which the protocol controls)

[IAm0x52]

Invalid. Protocol controls the number of tokens in the whitelist and would have to add enough tokens to make this an issue.

[FlacoJones]

OpenQDev/OpenQ-Contracts#113

[hrishibhat]

Considering this issue as low