Can "onERC721Received" be safely relied upon to reject unwanted ERC721 tokens?

Question

I have a master contract (Contract A) which interacts with another contract (Contract B). Contract A has functions that use 'safeTransferFrom' to move tokens into Contract B, and there are some require statements which need to be present to ensure that only certain tokens may be moved from Contract A to Contract B.

My question is - is it secure enough to include these require statements only within Contract B's "onERC721Received" function, and not within Contract A's transfer function? Is it good practice to be redundant and include the require statements in both (transfer and receiving)?

My understanding is that onERC721Received will be called by Contract B every time "safeTransferFrom" is called from Contract A, therefore I feel this should be adequate. Just wondering if there's some edge cases that i'm unaware of where onERC721Received could get bypassed, or if this would be considered reasonably secure. I'm aware that unsafe transfers etc won't trigger it, but that's fine - all valid transfers will be via safeTransferFrom. Thanks.

Answer 1

I will use the OpenZeppelin implementation of the ERC721 to explain the scenario here.

_checkOnERC721Received()

    function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert("ERC721: transfer to non ERC721Receiver implementer");
                } else {
                    /// @solidity memory-safe-assembly
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        } else {
            return true;
        }
    }

when contract A is called to perform an ERC721 transfer to the address of contract B onERC721Received function implemented in contract B will be executed.

Having required statements for selected token Ids in Contract A is not a good practice and

function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data) public override returns (bytes4) {
        require(tokenId==1, "Unauthorized TokenId");
        return this.onERC721Received.selector;
    }

If the minting and transfer options are available only to specific known addresses which you are sure are safe, you can avoid using _safeTransfer and _safeMint and use _transfer and _mint functions.

Answer 2

It is generally considered good practice to include a require statement in the function that is performing the transfer as well as in the onERC721Received function. This is because the onERC721Received function can only be called if the transfer was successful, so if the require statement is only included in the onERC721Received function and the transfer function does not have a require statement, an attacker could potentially bypass the requirement by making an unsafe transfer.

It's also a good idea to include a check for the return value of the safeTransferFrom function to ensure that the transfer was successful. If the transfer was not successful, it could indicate that the require statement in the transfer function was not met and the transfer should not proceed.

In general, it is a good idea to be redundant with checks like this to help ensure the security of your contract.