Using ERC165 supportsInterface() causes MetaMask and Etherscan Warnings

Support Contracts
1

I have a contract that uses ERC165Checker. My goal is to have the user enter an address as a function parameter, and I want the contract code to determine if that address is ERC-721 or not. The issue (although this may be expected behaviour) is that MetaMask and Etherscan are showing spurious warnings for this use case.

So I have code that works, shown below. I use ERC165Checker to see if the address implements ERC-721 or not. Technically, this all works as intended, and a boolean isNFT is derived correctly in every case.

However there are significant side-effects:

  1. When the user sends the transaction with an address that is not an NFT, MetaMask warns saying "this transaction may fail". I believe this warning is triggered by ERC165Checker, because the address it is testing doesn't implement ERC165 at all. Even though this doesn't fail the transaction as a whole, it is enough for MetaMask to warn.
  2. The transaction is posted successfully, but once completed, Etherscan shows a warning saying "SUCCESS" but lower down it says "with Revert". Here is an example Sepolia Etherscan transaction showing how this looks. It says "SUCCESS... with revert". If you run this transaction in Tenderly you'll see the revert in the ERC165 code.

Interestingly, Blockscout shows "SUCCESS" for the same transaction and doesn't mention the warning at all.

I kind of think this is expected behaviour, and outside the control of OpenZeppelin, but I'd like confirmation of that. Is my goal possible, without showing spurious warnings in MetaMask and Etherscan?

:1234: Code to reproduce

 function _isERC721(address token) internal view returns (bool) {
    return token.supportsInterface(type(IERC721).interfaceId);
  }
}

struct FlowCreate {
  address token;
  address from;
  address to;
  uint256 amountOrId;
}
  
function createSettlement(
    FlowCreate[] calldata flows
  ) external returns (uint256) {
    // ...other code...
    for (uint256 i = 0; i < flows.length; i++) {
      FlowCreate calldata flow = flows[i];
      settlement.flows.push(        // store in a mapping
        Flow({
          token: flow.token,
          from: flow.from,
          to: flow.to,
          amountOrId: flow.amountOrId,
          isNFT: _isERC721(flow.token)  // <-- this generates warning if token doesnt support ERC165
        })
      );
    }
    emit SettlementCreated(settlementIdCounter, msg.sender);
    return settlementIdCounter;
}

:computer: Environment

Sepolia, Hardhat.

1 Like
2

I would check externally if it is an nft or not, since I don't see that you reject the transaction in any case and it is one more piece of data in the registry that you can pass as a parameter.

Mostly to avoid checking inside the push.

On the other hand, I wouldn't do the for path inside the contract either.
I would generate a script, an action in hardhat for example, that executes the for in it and only do individual writes in each route as you can easily run out of gas and thus take advantage of the computational power of node.

1 Like
3

my refactoring:

    function createSettlement(FlowCreate[] calldata flows) external returns (uint256) {
        uint256 settlementId = // ...generate or get the settlement ID...
        Flow[] storage settlementFlows = settlements[settlementId];

        for (uint256 i = 0; i < flows.length; i++) {
            FlowCreate calldata flow = flows[i];
            bool isNFT = _isERC721(flow.token);

            settlementFlows.push(
                Flow({
                    token: flow.token,
                    from: flow.from,
                    to: flow.to,
                    amountOrId: flow.amountOrId,
                    isNFT: isNFT
                })
            );
        }

        return settlementId;
    }
1 Like