An attacker can prevent claimers from claiming

Summary

Vulnerability Detail

In DepositManagerV1, fundBountyNFT and fundBountyToken shares the same whitelist. (DepositManagerV1.sol#L36-L50)

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

(DepositManagerV1.sol#L113-L122)

    function fundBountyNFT(
        address _bountyAddress,
        address _tokenAddress,
        uint256 _tokenId,
        uint256 _expiration,
        bytes calldata _data
    ) external onlyProxy {
        IBounty bounty = IBounty(payable(_bountyAddress));

        require(isWhitelisted(_tokenAddress), Errors.TOKEN_NOT_ACCEPTED);

So an attacker can call DepositManagerV1.fundBountyNFT using a whitelisted ERC20 token. isWhitelisted will pass in fundBountyNFTand and BountyCore._receiveNft will call ERC20.safeTransferFrom. The function signature of ERC20.safeTransferFrom is the same as ERC721.safeTransferFrom. So the deposit will be successful if nftDepositLimit doesn't meet. But when claimers wants to claim the ERC20 deposit as an NFT, ERC20.safeTransferFrom will be called instead of ERC20.safeTransfer. (BountyCore.sol#L257-L264)

    function _transferNft(
        address _tokenAddress,
        address _payoutAddress,
        uint256 _tokenId
    ) internal virtual {
        IERC721Upgradeable nft = IERC721Upgradeable(_tokenAddress);
        nft.safeTransferFrom(address(this), _payoutAddress, _tokenId);
    }

ERC20.safeTransferFrom(address(this), _payoutAddress, _tokenId) will revert because any of bounty contracts doesn't approve from itself to itself. When claimers can't claim the NFT, the whole claim process will fail for atomic and tiered bounties.

Impact

Claimers can't claim for atomic and tiered bounties.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L36-L50 https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L113-L122 https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L257-L264

Tool used

Manual Review

Recommendation

Use different whitelist for ERC20 and NFTs.

Duplicate of #352

FlacoJones commented 1 year ago

Valid. Will Fix.

IAm0x52 commented 1 year ago

Dupe of #352