Smart contract security for ETH-based NFT marketplace purchase function

I’m building a marketplace where users can purchase NFTs that I’ve already minted. I want to accept only ETH as payment, not other tokens.

While researching, I found some code that handles edge cases where buyers might get NFTs for free if gas runs out during execution. Here’s what I’m working with:

function purchaseToken(uint256 _id) external payable {
    uint256 cost = itemPrices[_id];
    require(cost > 0, 'Item not available');
    require(msg.value == cost, 'Wrong amount sent');
    address currentOwner = ownerOf(_id);
    payable(currentOwner).transfer(msg.value);
    _transfer(currentOwner, msg.sender, _id);
    itemPrices[_id] = 0;
    emit TokenSold(currentOwner, msg.sender, msg.value);
}

My main concern is about gas limit edge cases when dealing with ETH payments. Do I need additional checks to prevent buyers from getting NFTs without paying if the transaction runs out of gas?

Also wondering about displaying NFTs on marketplaces. Do I need to create a function that returns all token IDs and their metadata URIs, or does OpenZeppelin handle this automatically?

Hmm, curious about something others mentioned - you’re using _transfer() but what happens if the marketplace contract doesn’t have proper ownership management?

Are you inheriting from ERC721 and handling ownership transfers correctly? If your contract calls _transfer(currentOwner, msg.sender, _id) but currentOwner isn’t actually the contract itself, that’ll cause issues.

Also wondering - how do you handle initial minting and setting itemPrices? Got admin functions for that? Are they secured with access controls?

About the gas thing - transactions are atomic, but have you tested with different gas limits? What if someone deliberately sends a transaction with juuust enough gas to execute most of the function but not all?

For marketplace display - planning to support filtering by price ranges or categories? That might affect how you structure getter functions. Consider events for off-chain indexing if you haven’t already.

What testnet are you deploying on? Would be interesting to see how this performs in practice!

gas issues aren’t that big of a concern - ethereum txs are atomic, so if gas runs out, everything reverts, incl. the nft transfer. real issue is using .transfer() since it breaks with smart contract wallets. better switch to call() with proper checks or use a withdrawal pattern. for marketplace display, custom functions are needed since erc721 lacks bulk getters.

Your concern about gas limits is understandable, but rest assured, it’s not a significant issue. If a transaction runs out of gas, both the NFT transfer and the payment revert, ensuring no one receives an NFT without proper payment. However, a more pressing issue is the potential failure of your transfer() method if the current owner is a contract that cannot accept ETH. It’s advisable to adopt a pull payment model where you keep the funds in the contract, allowing owners to withdraw them at their convenience.

As for marketplace displays, OpenZeppelin’s ERC721 does not provide bulk retrieval functions by default. You will need to implement a function that retrieves available tokens alongside their metadata URIs to facilitate front-end rendering efficiently.