Implementation of an IERC721Receiver Contract - Preventing bad actor scenarios?

I'm working on an NFT auction contract, which is an implementation of IERC721Receiver and utilizes the onERC721Received function to take action when a token is sent to the contract:

    function onERC721Received(
        address,
        address _from,
        uint256 _tokenId,
        bytes calldata _data
    ) 
        external 
        returns (bytes4) 
    {
        _createAuction(msg.sender, _from, _tokenId, _data);
        return this.onERC721Received.selector;
    }

My contract's onERC721Received function is called by an NFT contract when the NFT contract's own safeTransferFrom function is called by a token owner (or approved address) to transfer a token to my contract. My onERC721Received function returns the expected response to the caller, and the NFT contract completes the transfer of the token to my contract's address.

This should all work as intended by the ERC721 specification.

However, as part of developing a robust contract, I'm dwelling ways that the onERC721Received function could be used misused. I'm wondering how realistic these scenarios are, and what level of effort is appropriate to mitigate them.

Some of the scenarios I'm imagining are:

  1. The onERC721Received function can be spammed by non-contract addresses in order to create non-working auctions.

Since the function is external, it can be called by anyone. There is currently nothing preventing any user-owned address from calling it and passing in any data they want. The result would be an auction created in the contract (handled in my _createAuction function) with erroneous data passed by a user.

Of course, a user would waste money spending gas fees to do this, and it would result in a non-working auction, with no benefit to them, but I'd still like to prevent the cleanup of any non-working auctions created this way.

The only way I can think to prevent this is to add a require(msg.sender.isContract(), 'Sender must be a contract address') type of check to the function.

  1. The onERC721Received function can be spammed by other contracts in order to create non-workign auctions.

Similar ot the problem above, but in this scenario the call to onERC721Received is called by a contract that is not compliant to the ERC721 standard. Erroneous data could be passed in the function call resulting in non-working auctions being created.

Again, this would not benefit the person trying to do this, and would only cost them gas fees, but its still a possibility.

I'm not sure what the solution to this one would be. Having an additional check to see if the msg.sender address is ERC721 compliant? (although I believe this can be spoofed). Or, making sure a token transfer has actually taken place by calling ownerOf on the NFT contract address?

Both of these scenarios seem like they aren't worth the effort of a bad actor, so I'm trying to gauge how concerned I should be about their possibility of happening. Any thoughts?

It's true that it's unlikely a spammer would waste gas on this, but your concerns are very reasonable. It's always a good idea to try hard to restrict smart contracts to behave within the expected parameters.

In this case, it sounds like a simple check would help. onERC721Received can confirm that it did indeed receive the token that the function call claims, by checking IERC721(msg.sender).ownerOf(tokenId) === address(this). This can still be worked around by an attacker, unless you whitelist the NFTs that you accept, so you can't blindly rely on it, but I think it does the job to prevent spamming.

1 Like