0xmuxyz - Lack of check whether the caller of `withdrawToken()` function is the Bull or not

[sherlock-admin]

0xmuxyz

medium

Lack of check whether the caller of withdrawToken() function is the Bull or not

Summary

• Lack of check whether the caller of withdrawToken() function is the Bull or not

Vulnerability Detail

• On the assumption is that, when withdrawToken() function is used is written in the document like below:

  • In case the Bull cannot accept NFTs (eg a malicious smart contract), the NFT is kept in our smart contract and can be later retrieved by the Bull through a call to function withdrawToken https://bullvbear.gitbook.io/home/understanding-the-protocol/smart-contracts#settlement-by-the-option-buyer-bear

• Based on the assumption (context) of withdrawToken() function above, only the Bull should can call the withdrawToken() function.

  • However, there is no check whether the caller of the withdrawToken() is the Bull or not.
  • Therefore, the Bear and any external user who is neither the Bull nor the Bear can call withdrawToken() function.

Impact

• For example, this vulnerability can lead to negative impact like below:
  • If the caller besides the Bull can call withdrawToken() function, the Bull may receive a NFT when they doesn't hope and intend.
  • If the caller besides the Bull can call withdrawToken() function, the Bull may receive a NFT that they doesn't want to receive.

Code Snippet

• Below is the code snippet that is lack of check whether the caller of withdrawToken() function is the Bull or not. https://github.com/masaun/2022-11-bullvbear-masaun/blob/main/bvb-protocol/src/BvbProtocol.sol#L450-L462

  function withdrawToken(bytes32 orderHash, uint tokenId) public {
      address collection = matchedOrders[uint(orderHash)].collection;
  
      address recipient = withdrawableCollectionTokenId[collection][tokenId];
  
      // Transfer NFT to recipient
      IERC721(collection).safeTransferFrom(address(this), recipient, tokenId);
  
      // This token is not withdrawable anymore
      withdrawableCollectionTokenId[collection][tokenId] = address(0);
  
      emit WithdrawnToken(orderHash, tokenId, recipient);
  }

Tool used

• Manual Review

Recommendation

• Should add a condition for checking whether a caller of withdrawToken() is the Bull (=Option seller) or not by using require() statement to the withdrawToken() function.

  • Below is an example code.

  • NOTE: In the example code below, there is also the check whether the Bull is recipient or not (just in case).

    function withdrawToken(bytes32 orderHash, uint tokenId) public {
    address collection = matchedOrders[uint(orderHash)].collection;
    
    // ContractId
    uint contractId = uint(orderHash);
    
    // Retrieve the bull address
    address bull = bulls[contractId];
    require(bull == msg.sender, "Only bull can call withdrawToken function");
    
    address recipient = withdrawableCollectionTokenId[collection][tokenId]; //@dev - Original code
    
    // Check that only the bull can call the withdrawToken() function
    require(bull == recipient, "Recipient must be bull");
    
    // Transfer NFT to recipient
    IERC721(collection).safeTransferFrom(address(this), recipient, tokenId);
    
    // This token is not withdrawable anymore
    withdrawableCollectionTokenId[collection][tokenId] = address(0);
    
    emit WithdrawnToken(orderHash, tokenId, recipient);
    }

Duplicate of #4