transferFrom is used for ERC721 transfers instead of safeTransferFrom.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/TokenTransferrer.sol#L22

Vulnerability details

Impact

The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Proof of Concept

In the TokenTransferer.sol::TokenTransferer::_transferTokenToReceipient function the transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. There are reasons not use transferFrom function:

• OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible.
• There are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Tools Used

Manual review

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom() for ERC721 transfers.

        if (collectionType == CollectionType.ERC721) {
            IERC721(collection).safeTransferFrom(address(this), recipient, tokenId);
        } else if (collectionType == CollectionType.ERC1155) {
            IERC1155(collection).safeTransferFrom(address(this), recipient, tokenId, amount, "");
        }

[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]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b