Security concerns for buying pre-minted NFTs with Ethereum in ERC721 contract

NFT
1

I’m working on an ERC721 contract where I want users to buy pre-minted NFTs using only Ethereum. I’ve seen some code that checks for payment failures but I’m not sure how it applies to Ether transactions.

Here’s what I’ve come up with:

function purchaseNFT(uint256 nftId) external payable {
    uint256 cost = nftPrices[nftId];
    require(cost > 0, 'Not for sale');
    require(msg.value == cost, 'Wrong amount');
    address owner = _ownerOf(nftId);
    
    (bool success, ) = payable(owner).call{value: msg.value}('');
    require(success, 'Payment failed');
    
    _transfer(owner, msg.sender, nftId);
    nftPrices[nftId] = 0;
    emit NFTPurchased(owner, msg.sender, msg.value);
}

Is this safe? Does it handle the gas limit issue?

Also, how do marketplaces usually get info about available NFTs in a collection? Do I need to add a function for that or does OpenZeppelin handle it?

2

hey there! i’m curious about your nft marketplace project. it sounds pretty cool! :blush:

have u thought about using a pull payment pattern instead of pushing payments directly to the seller? that could help with some security concerns.

also, what about adding events for when nfts are listed/delisted? that might make it easier for frontends to keep track of what’s available.

oh and for the gas limit thing - have u considered using tx.origin instead of msg.sender? i’ve heard that can help sometimes but i’m not 100% sure.

what kind of nfts are u planning to sell btw? i’d love to hear more about the project if ur willing to share!

3

Your approach seems sound, but there are a few considerations to enhance security. Implementing a reentrancy guard is crucial when dealing with Ethereum transfers. Consider using OpenZeppelin’s ReentrancyGuard or a custom modifier. Additionally, the check-effects-interactions pattern can mitigate reentrancy risks.

Regarding gas limits, your current implementation doesn’t directly address this. You might want to implement a gas stipend when calling the recipient’s address to prevent potential issues.

For marketplace integration, exposing a view function that returns available NFTs (those with non-zero prices) would be beneficial. This allows easy querying without relying solely on events. Remember to emit events for listing and delisting actions to facilitate off-chain tracking.

Lastly, ensure you’ve thoroughly tested edge cases and considered potential attack vectors specific to NFT marketplaces. Security audits are highly recommended for contracts handling significant value.

4

yo, ur code looks decent but watch out for reentrancy! maybe use transfer() instead of call() for safer eth sending. for available NFTs, u could add a function to return all tokenIds with non-zero prices. marketplaces often use events to track whats for sale. good luck with ur project!