Implementation of sellable NFT

Support Contracts
1

Hi, I have implemented a very basic contract that uses the OpenZeppelin ERC721 token as a base. In the constructor I have set the owner of the contract to be the deployer of the contract.
On purchase of an nft, I run the buy function which is payable. The function ensures that there are no more than an x amount of NFTs minted in total. It then mints a new nft, sets the tokenUri of that nft which is a url on ipfs, with the tokenId appended for the metadata for each nft. The ether is sent directly to the owner. I can see there is a payment contract on OpenZeppelin, but it is too complicated for my use case here which is just to send the ether to the owner of the contract.

I am wondering if there are any immediate and obvious flaws / security risks in my code.

// SPDX-License-Identifier: MIT
pragma solidity >=0.7.0 <0.8.0;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/utils/Counters.sol";
import "@openzeppelin/contracts/utils/Strings.sol";

contract NFTCollectible is ERC721 {
    using Counters for Counters.Counter;
    Counters.Counter private _tokenIds;
    address payable private _owner;
    uint256 private _maxSupply = 420;

    constructor(string memory myBase) ERC721("Collectible", "collect") {
        _setBaseURI(myBase);
        _owner = msg.sender;
    }

    function buyItem(address player)
        public
        payable
        returns (uint256)
    {
        require(totalSupply() < _maxSupply);
        require(msg.value == 0.2 ether, "Need to send exactly 0.2 ether");
        _tokenIds.increment();
        uint256 newItemId = _tokenIds.current();
        string memory newItemIdString = Strings.toString(newItemId);
        string memory metadata = "/metadata.json";
        string memory url = string(abi.encodePacked(newItemIdString, metadata));
        _safeMint(player, newItemId);
        _setTokenURI(newItemId, url);
        _owner.transfer(msg.value);
        return newItemId;
    }   

    function owner() public view returns (address) {
        return _owner;
    }

    function maxSupply() public view returns (uint256){
        return _maxSupply;
    }
}

Appreciate any help or guidance I can receive regarding this. Thank you!

1 Like
2

Hi @Aizea,

Welcome to the community :wave:

:warning: I am a Community Manager and not a Security Researcher. I would recommend appropriate testing and auditing.

Thoughts from my perspective as a Community Manager:

My preference is to only include functionality in a token that is required for the life of the token.
I would separate the purchase functionality from the token, as you are only selling tokens up until a fixed cap.

The price of the token is fixed, which may be an issue depending on the expected life of the sale. The fiat value of 0.2 Ether could drastically change (up or down) over a longer period of time.

You are currently setting a token URI when you mint. This increases the cost of minting, when all you appear to be doing is using a URI combining the baseURI, the token ID and metadata.json, e.g. https://baseURI.com/token/1/metadata.json. I would look at changing the naming scheme on your server to reduce this cost. See the following example:

Also look at the example in: Create an NFT and deploy to a public testnet, using Truffle

1 Like
3

Hi, Thank you very much for the in-depth response. Only after reading your suggestion have I properly registered this line of code in the tokenURI function, which is exactly what I needed so I will be taking that on board, and change the naming scheme on my server!

// If there is a baseURI but no tokenURI, concatenate the tokenID to the baseURI.
return string(abi.encodePacked(_baseURI, tokenId.toString()));

With regards to separating the purchase functionality from the token, I am thinking of making another contract that simply inherits from this one, and wraps the buyItem function with payable and requiring a set amount of ether. Is this what is being recommended?

I have also decided to release these NFTs in waves, so using _cap variable instead of _maxSupply

function buyItem()
    public
    payable
    returns (uint256)
{
    require(totalSupply() < _cap);
    ...
}

function setCap(uint256 newCap) public returns (uint256 changedCap){
    require(_owner==msg.sender);
    require(newCap<=420);
    _cap = newCap;
    return _cap;
}

Does this seem like a sensible approach for this functionality?

4

Hi @Aizea,

Your selling contract could import your NFT but doesn't need to inherit (as it isn't extending). You would have an NFT contract and a separate sale contract.

I would look at the concepts in Crowdsales (which is selling ERC20 tokens).

Alternatively, if you have a preferred market place e.g. OpenSea (depending on your use case), you don't need to create your own selling contract, you could just sell in the marketplace.

If you want to have a cap, you could add this to your NFT minting using hooks (see: https://docs.openzeppelin.com/contracts/3.x/extending-contracts#using-hooks)

Example from ERC20

Rather than implementing your own Owner, you could use Ownable from OpenZeppelin Contracts or use Access Control for finer grained control: https://docs.openzeppelin.com/contracts/3.x/access-control

1 Like
5

Thank you for the responses once again.

With regards to above, are there any services you can recommend that will provide a security audit at an affordable price, considering it's just a single function that I would like checked for security flaws/bugs. Ideally one that can be done within 2 days. Thanks in advance.

1 Like
6

Hi @Aizea,

Other than OpenZeppelin (https://openzeppelin.com/security-audits/) there isn’t any services I can recommend for auditing. You could try looking at what similar projects in the NFT space have used.

I recommend appropriate testing (target 100% unit test coverage) and auditing for anything of value. You should also get everyone in your project to review every line of code.

You can also have a look at: https://blog.openzeppelin.com/follow-this-quality-checklist-before-an-audit-8cc6a0e44845/