Jeiwan - Non-whitelisted tokens cannot be added if the limit of token addresses is filled with whitelisted ones

[github-actions]

Jeiwan

medium

Non-whitelisted tokens cannot be added if the limit of token addresses is filled with whitelisted ones

Summary

Non-whitelisted tokens cannot be deposited to a bounty contract if too many whitelisted contracts were deposited.

Vulnerability Detail

The DepositManagerV1.fundBountyToken function allows depositing both whitelisted and non-whitelisted tokens by implementing the following check:

1. if a token is whitelisted, it can be deposited without restrictions;
2. if a token is not whitelisted, it cannot be deposited if openQTokenWhitelist.TOKEN_ADDRESS_LIMIT tokens have already been deposited.

However, while the token addresses limit requirement is only applied to non-whitelisted tokens, whitelisted tokens also increase the counter of token addresses: both non-whitelisted and whitelisted token addresses are added to the tokenAddresses set.

Impact

Bounty minters may not be able to deposit non-whitelisted tokens after they have deposited multiple whitelisted ones.

Code Snippet

DepositManagerV1.sol#L45-L50
BountyCore.sol#L326-L328
BountyCore.sol#L55

Tool used

Manual Review

Recommendation

Consider excluding whitelisted token addresses when checking the number of deposited tokens against the limit.

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

[FlacoJones]

OpenQDev/OpenQ-Contracts#113

[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.