ERC721 Patterns Through an Auditor's Lens: Mint Sales and NFT Marketplaces

Non-fungible troken (NFT) smart contracts are used to represent unique and indivisible assets (such as collectibles, digital products, or physical products) in the world of blockchain. Even if an NTF smart contract inherits from Openzeppelin's battle tested ERC721, there is no guarantee it is free of bugs.

In this write-up we'll do a walk through two contracts: a simple paid-mint ERC721, and a peer-to-peer marketplace that escrows NFTs and accepts ETH for them. We will look at things that stood out upon first inspection.

Part 1 — Audit 1: "MyNFT" — A Paid-Mint ERC721

The first engagement was a textbook NFT collection: 10,000 max supply, 0.1 ETH per mint, sequential token IDs starting from 1. Inherits from OpenZeppelin's ERC721.

pragma solidity ^0.8.13;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract MyNTF is ERC721 {

    uint16 immutable public MAX_SUPPLY = 10000;
    uint256 immutable public MINT_PRICE = 0.1 ether;

    uint16 public currentSupply = 0;

    constructor() ERC721("My NFT", "MNFT") {}

    function mint() external payable returns (uint16) {

        require(msg.value == MINT_PRICE, "mint costs 0.1 eth");
        require(currentSupply < MAX_SUPPLY, "max supply reached");

        currentSupply += 1;

        _mint(msg.sender, currentSupply);

        return currentSupply;
    }
}

My First Read

The use of immutable for the constants is correct. The values of MAX_SUPPLY and MINT_PRICE are set in construction and never change, so the compiler can inline them and save gas on every read. The use of uint16 for MAX_SUPPLY is a sensible choice since 10,000 fits in 16 bits with a comfortable margin (max uint16 is 65,535).

Then there is the external mint function. In it, there are two "requires": One that checks the value sent by the user against MINT_PRICE, and one that ensures the number of tokens minted does not exceed MAX_SUPPLY. The state of the smart contract is updated (currentSupply += 1) before the internal _mint, and this is in compliance with the checks-effects-interactions pattern that safeguards against the reentrancy vulnerability (more on that in a future article). The token IDs are incremental, starting from 1 and going all the way up to MAX_SUPPLY.

What could go wrong, right?

As you can see, MyNFT inherits from OpenZeppelin's ERC721, and if you are familiar with that library smart contract, you would know that it provides not only _mint, but also _safeMint. Both _mint and _safeMint accept the same input parameters (address to, uint256 tokenId), however, an overload of _safeMint also accepts data bytes.

The mint function uses _mint, not _safeMint. _mint does a basic mint. In other words, it assigns ownership of the tokenId NFT to address to, without checking whether to is actually a smart contract with logic that can handle ERC721 tokens. _safeMint, on the other hand, does this check after minting. It calls onERC721Received(operator, from, tokenId, data) on the recipient contract signified by the to address, and requires it to return the correct magic selector bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"), which is 0x150b7a02. If to doesn't implement that function, or returns a wrong value, the mint transaction reverts.

Basically, _safeMint does a "Do you know how to handle NFTs?" handshake. EOAs don't get past this check because they are not contracts.

For a paid mint where the contract sees msg.sender and credits them, this is a real concern. A user calling from a smart wallet they didn't realize doesn't implement the receiver hook just lost 0.1 ETH worth of NFT into a black hole. The OZ docs recommend _safeMint for exactly this reason.

There's a counterargument: _safeMint introduces a reentrancy surface, because onERC721Received is a callback into the recipient. If currentSupply had been incremented after the mint, a malicious recipient could reenter mint and grab the same token ID twice (or skip ahead). The current contract increments currentSupply before _mint, which would make _safeMint reentrancy-safe in this specific case. So a switch to _safeMint is recommended with confidence.

There is no withdraw function. ETH paid for mints accumulates in the contract forever. There is no owner, no admin, no way to extract the proceeds. For a real NFT sale this would be a critical finding - the team would not be able to access their own revenue.

There is no per-wallet cap and no allowlist. A single account can mint all 10,000 NFTs by submitting 10,000 transactions. For a fair-launch model this is fine; for a "fairly distributed" art drop, it's not. The fix is a simple mapping(address => uint256) public mintedPerWallet and a require(mintedPerWallet[msg.sender] < MAX_PER_WALLET).

No reentrancy protection. The current contract is safe because _mint (not _safeMint) doesn't call back. If the developer later swaps to _safeMint without thinking through the state-update ordering, reentrancy could matter. A nonReentrant modifier costs ~2,000 gas and removes the worry.

Verdict

For production, I'd add _safeMint, a withdraw() gated by Ownable, a per-wallet cap, and nonReentrant. The structural skeleton is correct.

Part 2 — Audit 2: "OpenSee" — A Peer-to-Peer NFT Marketplace

The second engagement was more interesting from a security standpoint. OpenSee is a marketplace contract. Users list their NFTs by transferring them into the contract along with an asking price; other users purchase those listed NFTs by sending ETH. The marketplace forwards the ETH to the seller and the NFT to the buyer.

contract OpenSee {

    uint256 constant public MAX_PRICE = 100 ether;

    struct Item {
        uint256 itemId;
        address collection;
        uint256 tokenId;
        uint256 price;
        address payable seller;
        bool isSold;
    }

    uint256 public itemsCounter;
    mapping(uint256 => Item) public listedItems;

    function listItem(address _collection, uint256 _tokenId, uint256 _price) external {
        require(_price != 0 && _price <= MAX_PRICE, "wrong _price");
        itemsCounter += 1;
        IERC721(_collection).transferFrom(msg.sender, address(this), _tokenId);
        listedItems[itemsCounter] = Item(
            itemsCounter, _collection, _tokenId, _price,
            payable(msg.sender), false
        );
    }

    function purchase(uint _itemId) external payable {
        require(_itemId != 0 && _itemId <= itemsCounter, "incorrect _itemId");

        Item storage item = listedItems[_itemId];
        require(!item.isSold, "item is already sold");
        require(msg.value == item.price, "wrong ETH was sent");

        item.isSold = true;

        IERC721(item.collection).transferFrom(address(this), msg.sender, item.tokenId);

        (bool sent, bytes memory data) = item.seller.call{value: msg.value}("");
        require(sent, "Failed to transfer ETH");
    }
}

My First Read

Upon reviewing this code, the architecture is the standard "escrow marketplace" pattern. Sellers list by transferring the NFT into the contract (requires prior approve on the collection). Buyers purchase by sending exact ETH. The contract atomically swaps the NFT for the payment.

The state-update ordering in purchase is the part I read most carefully. Let me reproduce it:

item.isSold = true;                                                    // (1) Effect
IERC721(item.collection).transferFrom(address(this), msg.sender, item.tokenId); // (2) Interaction
(bool sent, ) = item.seller.call{value: msg.value}("");                // (3) Interaction
require(sent, "Failed to transfer ETH");

The isSold flag is flipped to true before either the NFT transfer or the ETH transfer. That's correct CEI. If the buyer were a malicious contract whose onERC721Received reentered purchase (which can happen if the marketplace ever supports safeTransferFrom, or if the NFT collection itself has an unusual transfer hook), the reentrant call would hit require(!item.isSold) and revert. The same applies to the ETH call — if the seller is a contract with malicious receive logic that reenters, isSold is already true.

But upon closer inspection, I have several concerns about this contract that I want to walk through.

Concern 1: transferFrom vs safeTransferFrom

The marketplace uses transferFrom to move the NFT both during listing and during purchase. That works for any standard ERC721, but safeTransferFrom is the safer default because it verifies the recipient can handle ERC721s.

When listing, the recipient is address(this) — the marketplace itself. The marketplace doesn't implement onERC721Received. So if safeTransferFrom were used during listItem, every listing would revert. The developer correctly chose transferFrom here.

When purchasing, the recipient is msg.sender — the buyer's address. If the buyer is a contract that doesn't implement onERC721Received, they'd still receive the NFT (because transferFrom doesn't check), but they'd be unable to do anything with it via standard tooling. Using safeTransferFrom here would protect buyers from accidentally sending their NFT to a non-receiving contract. It would also introduce a reentrancy surface — but as I noted, the isSold = true line is positioned correctly to defend against it.

The audit recommendation: use safeTransferFrom for the purchase-side transfer to the buyer, and keep transferFrom for the list-side transfer to the marketplace. Or — even better — make the marketplace itself implement IERC721Receiver so it can accept safeTransferFrom deposits.

Concern 2: The ETH Forward Uses .call Without Bounded Gas

(bool sent, bytes memory data) = item.seller.call{value: msg.value}("");
require(sent, "Failed to transfer ETH");

The .call pattern forwards all remaining gas to the seller. If the seller is a contract with an expensive receive() function, that gas gets consumed. If the seller is a malicious contract that intentionally reverts in receive(), the entire purchase reverts — but the isSold flag has already been set and the NFT has already been transferred.

Wait — let me trace that again. In Solidity, when a require fails after state changes in the same transaction, all state changes are reverted. So if the seller's receive reverts, the entire purchase tx reverts: isSold is rolled back, the NFT goes back to the marketplace, the buyer's ETH stays with the buyer. The listing is intact and re-purchasable.

That's actually fine — the marketplace's invariants are preserved. But it does mean a malicious seller can grief buyers by blocking purchases of their own listing. They list at 5 ETH, never pay gas to receive payments, and when a buyer tries to purchase, the seller's revert kills the tx. The buyer wastes gas on a failed transaction. The seller has no obvious motive to do this — they want to be paid — but if they want to manipulate the appearance of an available listing, they can.

A more robust pattern is a pull payment: instead of pushing ETH to the seller during purchase, credit it to a mapping(address => uint256) pendingWithdrawals and let the seller pull it. That eliminates the griefing vector and the reentrancy surface in one stroke.

Concern 3: No Way to Cancel a Listing

Once an item is listed, it's in the marketplace until someone buys it. There's no cancelListing(itemId) that lets the seller change their mind and reclaim their NFT. For an educational contract this is fine; for production, sellers will absolutely want this, and adding it later requires careful access control — the seller must be the only one who can cancel their own item.

Concern 4: No Fee Mechanism, No Owner, No Pause

Real marketplaces take a fee (OpenSea historically took 2.5%). This contract takes nothing. There's no Ownable, no admin who can pause the contract if something goes wrong, no royalty support (EIP-2981). For a from-scratch design, this is a list of features rather than a list of bugs — but I'd note them.

Concern 5: The Counter Increments Before the Transfer

itemsCounter += 1;
IERC721(_collection).transferFrom(msg.sender, address(this), _tokenId);
listedItems[itemsCounter] = Item(...);

If the transferFrom reverts (e.g., the seller never approved the marketplace, or doesn't own the token), the whole listing transaction reverts and itemsCounter is rolled back. That's fine semantically. But the order is slightly unusual — most marketplaces do the transfer first, then bump the counter, then write the storage slot. The current ordering doesn't cause a bug, just feels backward.

The deeper question worth asking: what happens if _collection is a non-ERC721 contract or a malicious contract whose transferFrom does something weird? If transferFrom returns success but doesn't actually transfer the NFT, the marketplace records a listing for an NFT it doesn't own, and the eventual buyer's purchase fails when the marketplace tries to forward the NFT it doesn't have. The buyer's ETH gets refunded by the revert, so no funds are lost — but it pollutes the listing space.

The mitigation is a post-transfer ownership check:

IERC721(_collection).transferFrom(msg.sender, address(this), _tokenId);
require(IERC721(_collection).ownerOf(_tokenId) == address(this), "transfer didn't land");

For a real marketplace serving a hostile market of arbitrary user-deployed NFT collections, that check is worth its gas.

Concern 6: bytes memory data Is Captured But Never Used

(bool sent, bytes memory data) = item.seller.call{value: msg.value}("");

The returned data is allocated in memory but immediately discarded. This is a small gas waste — better written as:

(bool sent, ) = item.seller.call{value: msg.value}("");

Audit comment, not a bug.

Part 3 — Closing Notes from the Auditor's Desk

If I zoom out across these two contracts, the patterns that earn my attention on ERC721 audits are:

I check whether the developer used _mint or _safeMint, and whether their choice is justified by the state-update ordering. I trace every external call (ETH transfer, NFT transfer, callback hook) and ask which state I've already updated and which I haven't, because reentrancy bugs live in that gap. I look for missing functions — withdraw, cancel, pause — as much as I look at the functions that are present. I ask whether the contract assumes anything about the NFT collections it interacts with that could be falsified by a hostile collection. I check whether ETH is pushed or pulled, and whether push-payment patterns leave the contract open to grief or DoS by uncooperative recipients.

NFT contracts are simple in the same way a chess opening is simple. The pieces are few, the moves are constrained, and you can lose the game by missing one detail.