Warning: Unreachable code on _checkOnERC721Received when overriding the transferFrom function.

[armgit5]

💻 Environment

• Compiler version: 0.8.20
• Target EVM version (as per compiler settings): Paris
• Framework/IDE (e.g. Truffle or Remix): Hardhat
• EVM execution environment / backend / blockchain client: Terminal running npx hardhat clean & npx hardhat compile
• Operating system: Mac OS Sonoma 14.5
• "dependencies": {
  "@openzeppelin/contracts": "^5.0.2",
  "@openzeppelin/contracts-upgradeable": "^5.0.2"
  },

📝 Details
Note: a minor warning has appeared for the first time after compiling.

I’m using @openzeppelin/contracts-upgradeable and trying to override the transferFrom function. When I run npx hardhat clean & npx hardhat compile, I’m encountering a warning:

Warning: Unreachable code.
   --> @openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol:184:9:
    |
184 |         _checkOnERC721Received(from, to, tokenId, data);

It appears that _checkOnERC721Received isn’t being called on line 184, but if I swap the order and place _checkOnERC721Received first like the following code block, the warning disappears. I’m curious if _checkOnERC721Received should be called before transferFrom?

function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual {
    _checkOnERC721Received(from, to, tokenId, data);        
    transferFrom(from, to, tokenId);
}

🔢 Code to reproduce bug

Here’s my overridden transferFrom function:

function transferFrom(
    address,
    address,
    uint256
) public pure override(ERC721Upgradeable, IERC721) {
    revert("Use customTransferFrom function instead");
}

Could you help clarify if _checkOnERC721Received should be called before transferFrom to avoid the warning when overriding the function?

[0xneves]

Hey @armgit5

I could not find any reference for _checkOnERC721Received in the OpenZeppelin/openzeppelin-upgrades, as of openzeppelin-contract it was removed not long ago, you can check here.

As far as I remember, this functions requires the target address to implement this function onERC721Received otherwise it will fail. I really don't think this is very useful especially because almost no contract uses this...

The reason it is reverting when you place _checkOnERC721Received after the transferFrom is because your override is reverting with 100% certain, hence the next line which is the _checkOnERC721Received will never happen.

And you are right, it should be called, as described, after the transfer event happened. But if the target destination doesn't have such implementation or depending on how you coded yours, it should not do anything.

[armgit5]

Hi @0xneves

Thank you so much for your response and clarification. I noticed in the commit above that _checkOnERC721Received was changed from internal to private.

My goal is to override transferFrom to always revert, effectively blocking safeTransferFrom as well. I want to ensure that users can only use the payable customTransferFrom to transfer, which will include an applied fee.

It makes sense that _checkOnERC721Received doesn’t do anything since the override is guaranteed to revert. However, it does trigger a compile warning with “Unreachable code” the first time it compiles. I’ll ignore the warning for now, but I wanted to bring it to your attention, thanks!

[0xneves]

Then you should go for overriding the transferand transferFrom function as simple as:

function transferFrom() public override {
    revert();
}

Then implement your on customTransferFrom and calling _transfer after your custom checks passes.

[armgit5]

I’ve tried the suggested approach, but I encountered the following error: Function has override specified but does not override anything.

TypeError: Function has override specified but does not override anything.
   --> contracts/WSGArt.sol:111:44:
    |
111 |     function transferFrom() public virtual override(ERC721Upgradeable, IERC721) {
    |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


TypeError: Invalid contracts specified in override list: "ERC721Upgradeable" and "IERC721".
   --> contracts/WSGArt.sol:111:44:
    |
111 |     function transferFrom() public virtual override(ERC721Upgradeable, IERC721) {

Here’s my current transfer and transferFrom implementation:

function transferToken(address to, uint256 tokenId) public payable {
    require(ownerOf(tokenId) == _msgSender(), "Caller is not owner");

    TokenDetails memory details = _tokenDetails[tokenId];
    uint256 fee = (details.price * _transferFeePercentage) / MULTIPLIER;

    require(msg.value == fee, "Invalid transfer fee");

    _transfer(_msgSender(), to, tokenId);

    if (!_transferred[tokenId]) {
        _transferred[tokenId] = true;
    }

    emit TokenTransferred(
        _msgSender(),
        to,
        tokenId,
        details.price,
        fee,
        block.timestamp
    );
}

function transferFrom() public override {
      revert();
}

It seems like I have to specify the signature for the transferFrom function, such as function transferFrom(address, address, uint256) public virtual override(ERC721Upgradeable, IERC721) or function transferFrom(address, address, uint256) public pure override(ERC721Upgradeable, IERC721)

[0xneves]

I think by just using this it might work:

function transferFrom(address, address, uint256) public override returns (bool) {
        revert();
    }

You can also send me the repo you are working on and I can take a quick look in the entire contract

[armgit5]

I tried the above suggestion, but I’m still getting an Overriding function return types differ compilation error. I’ve sent you an invitation to my repo—any feedback would be greatly appreciated. Thanks!

[0xneves]

I tried the above suggestion, but I’m still getting an Overriding function return types differ compilation error. I’ve sent you an invitation to my repo—any feedback would be greatly appreciated. Thanks!

Hey @armgit5 sorry for the late, I've looked into your repo and it's compiling normally...

Here is the result

The warning can be removed by changing the transfer function into a require/if statement and making sure you can only transfer to the zeroth address, all other transfers will be unavailable

    function transferFrom(
        address,
        address to,
        uint256
    ) public virtual override(ERC721Upgradeable, IERC721) {
        if (to != address(0)) revert("Use transferToken");
    }

Are you still having the issue? If so, could you describe it better?

[armgit5]

@0xneves No worries at all, and thank you so much for checking into the issue for me. After trying it with require or if statement, the warning disappeared. I really appreciate your help so much 🙏