ERC721Holder, IERC721Receiver, and onERC721Received

Support
1

Based on the discussions here, plus some of my experiences, I’m wondering if my understanding of the rationale behind the three functions in the title is correct.

The onERC721Received function in IERC721Receiver or ERC721Holder (as an implementation of IERC721Receiver) is a way of signaling back to the safeTransferFrom function in the calling ERC721 contract. However, it is not able to guarantee the safety of an NFT transferred to the receiving contract.

For example, if only the onERC721Received function is implemented in the receiving contract, in the same way as ERC721Holder is implementing it, an NFT would still stay in the contract for ever, since there is no function transfering it out of it. Is this correct?

That said, the reason this function is still useful is, the sending ERC721 contract has to have a way to trust the receiving contract knows what it is doing and by having this onERC721Received implemented correctly, the sending contract can get a signal that the other side knows what it is doing.

If a higher requirement is desired, the sending ERC721 contract should require that the receiving contract implements a transfer function as well, by checking the supported interfaces? Is this the correct rationale? Thanks.

contract TokenReceiver is IERC721Receiver {

    address private token;

    contructor(address token_) {
        token = token_;
    }

    function transfer(address to, uint256 tokenId) public {
        IERC721(token).safeTransferFrom(address(this), to, tokenId);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) public override returns (bytes4) {
        return this.onERC721Received.selector ^ this.transfer.selector;
    }
}
4 Likes
2

Yes you seem to have understood it well.

1 Like
3

Correct me if I am wrong, but this TokenReciever.transfer method is still problematic. The safeTransferFrom implementation will check for ownership or isApproved before initiating the transfer. As it is using the msg.sender here, it will always fail.

The correct code should be as follow:

contract TokenReceiver is IERC721Receiver {

    address private token;

    contructor(address token_) {
        token = token_;
    }

    function transfer(address to, uint256 tokenId) public {
        IERC721(token)._safeTransfer(address(this), to, tokenId, "");
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) public override returns (bytes4) {
        return this.onERC721Received.selector ^ this.transfer.selector;
    }
}
4

Hm, no, I think @maxareo's code is right, but maybe there is some confusion about how the contract is meant to be used. I think transfer is just there to be able to transfer whatever token is received? But it's missing a reasonable method of access control so as-is it is only viable for testing purposes.

Also, one other thing I noticed is that the return value for onERC721Received says return this.onERC721Received.selector ^ this.transfer.selector. I'm not sure what the ^ this.transfer.selector part is about. It should just return this.onERC721Received.selector.

1 Like