GimelSec - It should store contractId instead of recipient in `withdrawableCollectionTokenId`

[sherlock-admin]

GimelSec

medium

It should store contractId instead of recipient in withdrawableCollectionTokenId

Summary

settleContract() would store the bull address in withdrawableCollectionTokenId, If the bull cannot receive the NFT. However, the bull may not be able to withdraw the NFT through withdrawToken().

Vulnerability Detail

If a bull cannot receive the NFT in settleContract(), it would store the bull address in withdrawableCollectionTokenId[collection][tokenId]. The protocol uses this mechanism to prevent the malicious bull from blocking settleContract().

However, if a bull mistakenly uses an address that cannot receive the NFT, he/she cannot withdraw the NFT through withdrawToken() since the protocol store the broken address in withdrawableCollectionTokenId[collection][tokenId].

Impact

If a bull mistakenly uses an address that cannot receive the NFT, he/she cannot withdraw the NFT after the bear settles the contract.

Code Snippet

https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L450-L462

https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L399

Tool used

Manual Review

Recommendation

To fix the problem, it should store contractId instead of the bull address in withdrawableCollectionTokenId[collection][tokenId]. Then, the bull can use transferPosition() to replace the broken address.

And modify withdrawToken()

    function withdrawToken(bytes32 orderHash, uint tokenId) public {
        address collection = matchedOrders[uint(orderHash)].collection;

        uint contractId = withdrawableCollectionTokenId[collection][tokenId];
        require(contractId != 0);

        address recipient = bulls[contractId] ;

        // Transfer NFT to recipient
        IERC721(collection).safeTransferFrom(address(this), recipient, tokenId);

        // This token is not withdrawable anymore
        withdrawableCollectionTokenId[collection][tokenId] = 0;

        emit WithdrawnToken(orderHash, tokenId, recipient);
    }

Duplicate of #4