crytic / properties

Pre-built security properties for common Ethereum operations
GNU Affero General Public License v3.0
276 stars 42 forks source link

[Feature-request]: NFT minting properties #25

Open aviggiano opened 1 year ago

aviggiano commented 1 year ago

Describe the desired feature or improvement

I am reviewing a smart contract that contains a very complex logic to mint NFTs with random rarities using Chainlink VRF. Just by reading the code it is hard to understand if it correctly implements the specification. It would be nice to have pre-built echidna properties to test invariants.

In my particular example, the invariants are rather commonplace to other NFT projects, which is why I believe these are generic enough to be applied to other smart contracts:

If you think this is useful I can work on these properties and submit a PR.

montyly commented 1 year ago

Hi @aviggiano , I think that is a great idea.

Do you see them as an extension of the current ERC721 properties, or something separate?

@tuturu-tech might be able to help if you have questions about the repo structure/contribution process

aviggiano commented 1 year ago

I think this could be something separate, since it is a bit independent from the ERC721 properties.

The contract I am reviewing includes the minting logic on the ERC721 contract itself, but I don't think this would be the case for most projects. I think they would most likely have a NFT contract and a NFTSale/NFTMinting contract.

Here's the interface of the contract I want to test:

interface NFTSale is ERC721 {
    function mintNFT(uint256 _amount, bytes calldata _signature) external payable returns (uint256[] memory);
    function setConfig(uint256 _maxSupply, uint256 _maxNFTsPerWallet, uint256 _nftPrice, uint256[] memory _percentages) external;
}

Based on this particular case, I think we could define an interface that these NFTSale/NFTMinting should follow, similar to what we have on CryticIERC4626Internal for ERC4626 vaults.

Here's an initial idea:

/// @title CryticIERC721Minting
/// @notice Interface for NFT minting with support for different prices per quantity, free mints, and payment in either ERC20 tokens or ether.
interface CryticIERC721Minting {
  /// @notice Returns the price for a specific quantity of NFTs when using a specific payment token.
  /// @dev Should pass type(uint256).max if the token specified is not allowed for this mint
  /// @param amount The quantity of NFTs.
  /// @param token The payment token address.
  /// @return The price in the payment token for the specified quantity of NFTs.
  function price(uint256 amount, address token) external view returns(uint256);

  /// @notice Returns the maximum supply of minted NFTs.
  /// @return The maximum supply of minted NFTs.
  function maxTotalMint() external view returns(uint256);

  /// @notice Returns the maximum NFTs that can be minted per wallet.
  /// @param account The wallet address.
  /// @return The maximum NFTs that can be minted per wallet.
  function maxMint(address account) external view returns(uint256);

  /// @notice Checks if an account is allowed to mint NFTs, supporting on-chain mapping, off-chain signed messages, and Merkle tree allowlists.
  /// @param account The account to check for minting permissions.
  /// @param data Additional data for checking minting permissions.
  function isAllowed(address account, bytes memory data) external view returns(bool);

  /// @notice Mints the specified quantity of NFTs, optionally charging users in ERC-20 or ether, and optionally verifying the allowlist.
  /// @dev Should pass an empty data if there is no allowlist
  /// @param amount The quantity of NFTs to mint.
  /// @param token The payment token address.
  /// @param data Additional data for minting.
  function mint(uint256 amount, address token, bytes memory data) external payable;
}

I still need to think how to handle the rarities, though