Insecure example in ERC721 documentation

[Alexander-Herranz]

Hi,

I noticed an insecure practice in the following example (GameItem.sol), from the documentation: https://docs.openzeppelin.com/contracts/4.x/erc721#constructing_an_erc721_token_contract

// contracts/GameItem.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

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

contract GameItem is ERC721URIStorage {
    using Counters for Counters.Counter;
    Counters.Counter private _tokenIds;

    constructor() ERC721("GameItem", "ITM") {}

    function awardItem(address player, string memory tokenURI)
        public
        returns (uint256)
    {
        uint256 newItemId = _tokenIds.current();
        _mint(player, newItemId);
        _setTokenURI(newItemId, tokenURI);

        _tokenIds.increment();
        return newItemId;
    }
}

💻 Environment

Any

📝 Details

In the provided example you use the _mint function, that is said to be discouraged in the documentation:
https://docs.openzeppelin.com/contracts/4.x/api/token/erc721#ERC721-_mint-address-uint256-

Usage of this method is discouraged, use _safeMint whenever possible

I think that _mint function should be changed to _safeMint to encourage best practices. This practical example is the entry point for lots of new Solidity developers.

Thanks

🔢 Code to reproduce bug

You can find the code in the documentation.

[frangio]

I think I agree we should use _safeMint, but the .increment() line should be executed before minting.

[Rassska]

It seems that the function awardItem(address, string memory) is supposed to be called by player, however the following is mentioned:

To restrict what accounts can mint items we can add [Access Control]

Hence there is a risk of unbounded gas consumption. It also opens some doors to re-entrancy. So, if you're planning to replace it, be sure to include all details.

[Alexander-Herranz]

I think we can also replace the Counter with a regular uint++/uint--

Counters library was designed to avoid overflows when managing numbers, but I think that we do not need it anymore from Solidity 8.

[Medlab-web3]

You are correct in noticing that the _mint function is discouraged in the OpenZeppelin documentation in favor of using _safeMint. Here's why:

Why Use _safeMint Instead of _mint?

Reentrancy Protection:

The _safeMint function includes checks to ensure that the minting process is secure, especially when transferring tokens to addresses that are smart contracts. It prevents reentrancy attacks by ensuring that any contract receiving the minted token implements the onERC721Received function properly.

Compatibility with Other Contracts:

The _safeMint function ensures compatibility with contracts that may implement onERC721Received. This is important for ensuring that your token can be safely received by other contracts without causing unexpected behavior.

Suggested Changes

Here's how you can modify your GameItem contract to use _safeMint instead of _mint:

// contracts/GameItem.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

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

contract GameItem is ERC721URIStorage {
    using Counters for Counters.Counter;
    Counters.Counter private _tokenIds;

    constructor() ERC721("GameItem", "ITM") {}

    function awardItem(address player, string memory tokenURI)
        public
        returns (uint256)
    {
        uint256 newItemId = _tokenIds.current();
        _safeMint(player, newItemId); // Use _safeMint instead of _mint
        _setTokenURI(newItemId, tokenURI);

        _tokenIds.increment();
        return newItemId;
    }
}

Benefits of Using _safeMint

Safety: By using _safeMint, you automatically incorporate additional checks that protect against potential vulnerabilities, such as reentrancy attacks.
Better Developer Practices: Encourages Solidity developers to adopt best security practices from the beginning.
Improved Interoperability: _safeMint ensures that your contract is compatible with others that may require onERC721Received for proper handling of tokens.
*****Using _safeMint is recommended over _mint because it provides extra security, ensures compatibility with other contracts, and helps prevent potential vulnerabilities.

[TUPM96]

Opened #6549 to switch the ERC721 GameItem docs example to _safeMint while keeping the token id increment before minting.