Use `safeTransferFrom()` Instead of `transferFrom()` for ERC721

[code423n4]

Lines of code

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

Vulnerability details

Impact

Use of transferFrom method for ERC721 transferFrom is discouraged and recommended to use safeTransferFrom whenever possible by OpenZeppelin. This is because transferFrom() cannot check whether the receiving address know how to handle ERC721 tokens.

In the function shown at below PoC, ERC721 token is sent to msg.sender with the transferFrom method. If this msg.sender is a contract and is not aware of incoming ERC721 tokens, the sent token could be locked up in the contract forever.

Reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.0/contracts/token/ERC721/IERC721.sol#L84

Proof of Concept

On TokenTransferrer.sol#L22:

            IERC721(collection).transferFrom(address(this), recipient, tokenId);

On LowLevelERC721Transfer.sol#L27:

        (bool status, ) = collection.call(abi.encodeWithSelector(IERC721.transferFrom.selector, from, to, tokenId));

Tools Used

Manual revision

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom, take in consideration that safeTransferFrom will trigger a callback to the receiver, so stick to check - effects - itterarion pattern

[c4-judge]

Picodes marked the issue as duplicate of #254

[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/212