Using `ERC721::transferFrom` instead of `ERC721::safeTransferFrom` can result in tokens stuck forever

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/lowLevelCallers/LowLevelERC721Transfer.sol#L27 https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/TokenTransferrer.sol#L22

Vulnerability details

Proof of Concept

LooksRareAggregator uses unsafe ERC721 transfer on rescueERC721() and LooksRareProxy uses unsafe ERC721 transfer on _executeSingleOrder(). The difference between ERC721::transferFrom and ERC721::safeTransferFrom is the latter will check that if the recipient of the transfer is a contract then it should support the onERC721Received() interface and return the method selector. If this is not checked then if the recipient is a smart contract that can’t handle ERC721 tokens then the tokens will be stuck there forever. This can be the case for example if a user is using a smart contract wallet that does not support ERC721 tokens or is using just a smart contract to interact with the protocol.

Impact

The impact is ERC721 tokens getting stuck forever, but it requires a non-ERC721 supported smart contract as the transfer recipient, so Medium severity should be appropriate.

Recommendation

Use the safeTransferFrom method of ERC721 instead of transferFrom

[c4-judge]

Picodes marked the issue as duplicate of #174

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-11-looksrare-findings/issues/203