Use _safeTransfer when transferring NFTs

[code423n4]

Handle

shw

Vulnerability details

Impact

The transferNft function of RCNftHubL2 is called when transferring the card to the final winner. However, this function does not check whether the recipient is aware of the ERC721 protocol and calls _transfer directly. If the recipient is a contract not aware of incoming NFTs, then the transferred NFT would be locked in the recipient forever.

Proof of Concept

Referenced code:
RCNftHubL2.sol#L135

Recommended Mitigation Steps

Use the _safeTransfer function instead, which checks if the recipient contract implements the onERC721Received interface to avoid loss of NFTs.

[Splidge]

This transfer function is just for the market to move the NFTs while the market is operational, during this phase it doesn't matter if the NFT is given to a non-compliant contract as the market is in control of moving the NFT and will simply give it to the next user that makes a rental.
Using _safeTransfer would be dangerous as it would give an attacker the opportunity to take part in a market via a contract that is not ERC721 compliant, when the market attempts to pass ownership to this contract the transfer would revert and the market would become locked.

[0xean]

claimCard in RCMarket.sol also calls the transfer method to claim the card, which could result in the situation outlined by the warden.

[Splidge]

Ahh sorry, I missed the key part in the wardens impact "when transferring the card to the final winner".

I still think this isn't an issue as if the destination is a non-compliant contract, when using _safeTransfer the claimCard call would revert. The NFT is permanently stuck on either the market or on the non-compliant contract.
At least the person who wrote the contract, funded it, used it to place bids, win the NFT and make the claimCard call would still get it transferred to their contract to use as a trophy cabinet of sorts. Maybe that person didn't implement onERC721Received?
This check would be too little too late.

Maybe we could check when the first rental is made that the sender is eligible? Although that would deviate slightly from the standard as onERC721Received is supposed to be called after the transfer.