clems4ever - NFT that are blacklisted after deposit should be refundable with no expiration and they should not be distributed

[github-actions]

clems4ever

medium

NFT that are blacklisted after deposit should be refundable with no expiration and they should not be distributed

Summary

Currently, there is no mechanism to quickly return a blacklisted NFT that has been deposited when it was whitelisted. Indeed, the owner cannot get the NFT back before the the expiration period because blacklisting is not checked during refund. Even worse, the NFT might be distributed as part of the payouts if its not refunded before the claim.

Vulnerability Detail

Let say I'm a funder and I deposit a token with fundBountyNFT. It's properly deposited because it is actually whitelisted. After some time but before the payout and before the expiration of the deposit is hit, the NFT gets blacklisted. There should be a way to the depositor to retrieve it without waiting for the expiration period. And above all, it should not be distributed to the claimants.

There is no filter to avoid the claiming of blacklisted NFT in

function claimNft(address _payoutAddress, bytes32 _depositId)
        external
        virtual
        onlyClaimManager
        nonReentrant
    {
        _transferNft(
            tokenAddress[_depositId],
            _payoutAddress,
            tokenId[_depositId]
        );
    }

or higher in the stack.

Also there is no bypass of the expiration check in the refundDeposit function

function refundDeposit(
        bytes32 _depositId,
        address _funder,
        uint256 _volume
    ) external virtual onlyDepositManager nonReentrant {
        require(!refunded[_depositId], Errors.DEPOSIT_ALREADY_REFUNDED);
        require(funder[_depositId] == _funder, Errors.CALLER_NOT_FUNDER);
        require(
            block.timestamp >= depositTime[_depositId] + expiration[_depositId],
            Errors.PREMATURE_REFUND_REQUEST
        );

        refunded[_depositId] = true;

        if (tokenAddress[_depositId] == address(0)) {
            _transferProtocolToken(funder[_depositId], _volume);
        } else if (isNFT[_depositId]) {
            _transferNft(
                tokenAddress[_depositId],
                funder[_depositId],
                tokenId[_depositId]
            );
        } else {
            _transferERC20(
                tokenAddress[_depositId],
                funder[_depositId],
                _volume
            );
        }
    }

Impact

A depositor of a blacklisted NFT might get mad not being able to get it back quickly to invest it somewhere else. The concept of blacklisting is subjective and some other projects might not consider this token as blacklisted and so the user could invest it somewhere else in the meantime if he or she could get it back before the expiration.
In the worst case scenario, this token might still be paid out to the developer(s).

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L72

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

Tool used

Manual Review

Recommendation

function claimNft(address _payoutAddress, bytes32 _depositId)
        external
        virtual
        onlyClaimManager
        nonReentrant
    {
        if (isWhitelisted(tokenAddress[_depositId])) {
            _transferNft(
                tokenAddress[_depositId],
                _payoutAddress,
                tokenId[_depositId]
            );
        }
    }

function refundDeposit(
        bytes32 _depositId,
        address _funder,
        uint256 _volume
    ) external virtual onlyDepositManager nonReentrant {
        require(!refunded[_depositId], Errors.DEPOSIT_ALREADY_REFUNDED);
        require(funder[_depositId] == _funder, Errors.CALLER_NOT_FUNDER);
        if (isWhitelisted(tokenAddress[_depositId])) {
            require(
                block.timestamp >= depositTime[_depositId] + expiration[_depositId],
                Errors.PREMATURE_REFUND_REQUEST
            );
        }

        refunded[_depositId] = true;

        if (tokenAddress[_depositId] == address(0)) {
            _transferProtocolToken(funder[_depositId], _volume);
        } else if (isNFT[_depositId]) {
            _transferNft(
                tokenAddress[_depositId],
                funder[_depositId],
                tokenId[_depositId]
            );
        } else {
            _transferERC20(
                tokenAddress[_depositId],
                funder[_depositId],
                _volume
            );
        }
    }

Duplicate of #62