Calling market contract on ERC721 _beforeTokenTransfer hook to delist NFT when transferred

Smart Contracts
1

I have my own ERC721 contract and marketplace contract to sell the NFTs.

I'm trying to find the best solution to this problem:

When I list an item on the marketplace, and this item is transferred outside the marketplace, or sold on a 3rd party marketplace, my item stays listed on my market, even if it technically can't be bought anymore because the owner changed.

I've seen a few projects that have token and market in 1 contract, so it's easy to delist an item on transfer.
I'm using 2 contracts, and I thought about calling the market contract to delist an item on the _beforeTokenTransfer hook in the token contract.

In my ERC721 token contract (OZ standards):

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId
  ) internal virtual override(ERC721Upgradeable, ERC721PausableUpgradeable) {
    // call market contract to delist
    _marketContractAddress.call(
      abi.encode(bytes4(keccak256("removeItemFromMarket(address,uint256)")),
      address(this),
      tokenId
    )
);
    super._beforeTokenTransfer(from, to, tokenId);
  }

And in my market contract:

  /**
    * @dev Remove a listed item from the market
    *  can only be removed by owner_of or the contract owner
    * @param token_address - nft erc721 address
    * @param token_id - nft token id
    */
  function removeItemFromMarket(
    address token_address,
    uint256 token_id
  )
    public
    whenNotPaused
    CompatibleERC721(token_address)
    ItemExists(token_id,token_address)
    OnlyItemHolder(token_id,token_address)
  {
    _removeItemFromMarket(token_address, token_id);
  }

    function _removeItemFromMarket(
    address token_address, uint256 token_id
  )
    internal
    returns (MarketItem memory)
  {
    address sender = _msgSender();
    MarketItem memory item = marketItemByTokenId[token_address][token_id];

    require(item.item_id != 0, "Item not listed");
    require(item.owner_of == sender || sender == owner(), "Unauthorized");

    bytes32 item_id = item.item_id;
    address owner_of = item.owner_of;
    delete marketItemByTokenId[token_address][token_id];

    emit ItemRemoved(
      item_id,
      token_id,
      token_address,
      owner_of
    );

    return item;
  }

I've debugged in Remix and the removeItemFromMarket is called on the market contract, but it reverts at the step bytes32 item_id = item.item_id; and goes back to the token contract to finish the transaction, which succeeds without deleting the market listing.

I've tried also with all requirement checks removed, but same result. From what I can see in the storage it reads the MarketItem object fine also.

My question: is this something 'logical' to do and could it work like this?
When the token contract calls the market contract, will _msgSender() in the market contract still be the _msgSender() of the token contract transaction?

I've searched many other NFT projects and marketplaces but cannot find the most used answer to this problem.
Could it be that most just leave the item listed forever and just solve it on the web3 side, with additional checks if the listing owner is still the same as the token owner?

Any feedback is very welcome, it's been bugging my brain for a while how to deal with this :slight_smile:

2

The simplest workaround is to make it such that the owner cannot transfer the NFT if it is listed on the marketplace. Logically this makes sense as well since you shouldn't transfer an item that you want to sell until you cancel the sell order.

If this UX is not desirable then we need to figure out why your transaction is failing. Are you sure it is reverting at bytes32 item_id = item.item_id;? What is the error message that it is returning?

3

Thank you for replying. If run it all through Hardhat tests but no problems there.
However, I think I should revise the idea. Making such a specific call in _beforeTransfer of an ERC721 doesn't seem clean or logical.
I've searched the net for examples, but nothing much I could find than a user writing this:

Known Issues / Implementation Details for Markets

  • approve NFT on marketplace A and B
  • it sells on B
  • still listed on A
  • user Alice goes to purchase on marketplace A but this will fail
  • the token has been transferred already and marketplace A has an incorrect approval ID for this NFT

There are 3 potential solutions:

  1. handle in market contract - When it fails because nft_transfer fails, marketplace could make promise call that checks nft_token owner_id is still sale owner_id and remove sale. This means it will still fail for 1 user.
  2. handle with backend - run a cron job to check sales on a regular interval. This potentially avoids failing for any user.
  3. remove from frontend (use frontend or backend) - for every sale, check that sale.owner_id == nft.owner_id and then hide these sale options in the frontend UI. Heavy processing for client side, so still needs a backend.
  4. let it fail client side then alert backend to remove sale. No cron. Still fails for 1 user.

Opinion: Option (2/3) is the best UX and also allows your sale listings to be the most accurate and up to date. If you're implementing a marketplace, you most likely are running backend somewhere with the marketplace owner account. If you go with Option 3 you can simply update a list of "invalid sales" and filter these before you send the sales listings to the client. If you decided to go with 2, modify the marketplace remove_sale to allow your marketplace owner account to remove any sales.

I've decided to also go for Option 2 and allowing the contract owner to clean up the items listed on the market place that are not anymore 'valid'. This is not a necessary thing per se if you solve it on the UI/UX side and my Moralis database. It just feels strange to leave inaccurate data in the contract storage.
I've used a blockchain Transfer event to check the market items listed in my server database and flag them as 'transferred' if the owner doesn't match anymore.
Still wondering how other marketplaces handle this!

2 Likes