TrungOre - Missing check whether the nft is refunded before transferring the reward to users

[github-actions]

TrungOre

high

Missing check whether the nft is refunded before transferring the reward to users

Summary

Since there is no check about the nft is refunded or not before calling _bounty.claimNft() in function ClaimManagerV1._claimAtomicBounty(), it will make the user unable to claim their rewards.

Vulnerability Detail

Function ClaimManagerV1._claimAtomicBounty() is implemented as follows:

 function _claimAtomicBounty(
        IAtomicBounty _bounty,
        address _closer,
        bytes calldata _closerData
    ) internal {
        /// [#explain] check if _close can claim the bounty 
        _eligibleToClaimAtomicBounty(_bounty, _closer);
        
        /// [#explain] claim ERC20  
        // ... 
        
        /// [#explain] claim ERC721
        for (uint256 i = 0; i < _bounty.getNftDeposits().length; i++) {
            _bounty.claimNft(_closer, _bounty.nftDeposits(i));

            emit NFTClaimed(
                _bounty.bountyId(),
                address(_bounty),
                _bounty.organization(),
                _closer,
                block.timestamp,
                _bounty.tokenAddress(_bounty.nftDeposits(i)),
                _bounty.tokenId(_bounty.nftDeposits(i)),
                _bounty.bountyType(),
                _closerData,
                VERSION_1
            );
        }
    }

Which bounty.claimNft() is

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

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

As we can see, there is no check if the bounty owns the nft or not. Then when a funder call refund his/her nft before the claim happen, the bounty.claimNft() will revert.
Example:

1. Alice deposits nft A into bounty
2. Alice calls refund to get nft A back
3. ClaimManagerV1._claimAtomicBounty() was called --> Revert since the bounty isn't the owner of nft A anymore

Note that this issue happens in atomic, tieredFix, tieredPercentage

Impact

Users is unable to claim the rewards.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L150-L165
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L251-L269
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/ClaimManager/Implementations/ClaimManagerV1.sol#L320-L338

Tool used

Manual review

Recommendation

Check whether the nft is refunded before transferring.

Duplicate of #263