NFTfi - GenArt - NFTfi

The Collection.sol contracts rely on a pseudo-random number generation technique that introduces predictability. The generation of tokenIdRandWordHash in the _mintInternal function is based on elements that are inherently predictable, such as the global variables blockhash(block.number - 1) and msg.sender, and also the incrementing startTokenId.

- contracts/Collection.sol [Lines: 108-117]

        for (; i < _quantity; ) {
            bytes32 tokenIdRandWordHash = keccak256(
                abi.encodePacked(startTokenId + i, blockhash(block.number - 1), msg.sender)
            );
            tokenGenerationParams[startTokenId + i] = tokenIdRandWordHash;
            tokenGenerationParamsEvent[i] = tokenIdRandWordHash;
            unchecked {
                ++i;
            }
        }

The tokenIdRandWordHash is constructed by hashing a combination of the previous blockhash, an incremental token ID and the msg.sender. This approach presents two primary concerns:

• Predictability of blockhash : Utilizing the hash of the preceding block as part of the randomness source is inherently insecure. Given that the blockhash is publicly available and can be predicted by the time a transaction is mined, it does not provide a robust foundation for randomness, allowing the manipulation of tokenIdRandWordHash.
  

• The other components of the hash function, namely the incremented startTokenId + i and msg.sender, are predictable within the context of a transaction: msg.sender remains unchanged, and startTokenId + i follows a sequential order.

  
  

This could potentially allow users to gain undue advantage in the NFT minting process, such as preferential access to rare or valuable attributes or traits, by anticipating the associated hashes because they are relying on a predictable source of randomness.

Adopting a more robust mechanism for randomness is advisable. Employing a Verifiable Random Function (VRF) from established third-party providers, like Chainlink VRF, is recommended. This would guarantee the unpredictability and integrity of the randomness in tokenIdRandWordHash creation.

Remediation Plan

RISK ACCEPTED: The NFTfi team accepted the risk related to this issue.

During the audit of the Collection.sol contract, it was identified that the contract inherits from ERC721A.sol version 4.2.3. However, the most recent and stable version of ERC721A.sol, according to the official GitHub repository chiru-labs/ERC721A, is version 4.3.0.

- ERC721A.sol (v.4.2.3) [Lines: 1-7]

// SPDX-License-Identifier: MIT
// ERC721A Contracts v4.2.3
// Creator: Chiru Labs

pragma solidity ^0.8.4;

import './IERC721A.sol';

Using an outdated version can expose the contract to previously addressed vulnerabilities and deprive it of the enhanced features and optimizations introduced in the newer version.

An upgrade of ERC721A.sol to version 4.3.0 is recommended to ensure access to the latest improvements and security updates.

The latest version is available in the following GitHub repository:

• chiru-labs/ERC721A

Remediation Plan

SOLVED: The NFTfi team solved this issue by updating the ERC721A dependency to its latest version.

Within the Collection.sol contract, there are two similar functions: setGeneraingScript with a typo, and setGeneratingScript, which aim to update the generatingScript variable, but they handle this task differently.

The former function takes its parameter as calldata and includes a conditional security check that prevents updates if a isGeneratingScriptLocked flag is set. On the other hand, the latter function accepts a parameter in memory and lacks this conditional security check, allowing for a broader range of updates.

- contracts/Collection.sol [Lines: 54-58]

    function setGeneraingScript(string calldata _generatingScript) external onlyOwner {
        if (isGeneratingScriptLocked) revert Locked();

        generatingScript = _generatingScript;
    }

- contracts/Collection.sol [Lines: 122-124]

    function setGeneratingScript(string memory _generatingScript) external onlyOwner {
        generatingScript = _generatingScript;
    }

The subtle differences between the two functions can make the contract harder to navigate and could lead to errors in use.

Merging these functions into one streamlined version is recommended. Use the name setGeneratingScript, use calldata for the string parameter to keep gas costs lower.

Incorporating the necessary security check within this single function will ensure that script updates are safely managed. This approach simplifies the interface, enhances security, and maintains efficiency, providing a clear, unified method for script updates.

Example:

    function setGeneratingScript(string calldata _generatingScript) external onlyOwner {
        if (isGeneratingScriptLocked) revert Locked();

        generatingScript = _generatingScript;
    }

Remediation Plan

SOLVED: The NFTfi team solved the issue by implementing a suggested fix.

Users can write arbitrary _generatingScript to contracts state (`string public generatingScript`) when calling createCollection on CollectionFactory.sol contract.

- contracts/CollectionFactory.sol [Lines: 9-29]

    function createCollection(
        bytes32 _merkleRoot,
        address _admin,
        string memory _generatingScript,
        string memory _baseUri,
        uint256 _mintTimeout,
        uint256 _maxSupply
    ) external {
        Collection collectionAddress = new Collection(
            "Nftfi GenArt Collection",
            "NGAC",
            _merkleRoot,
            _admin,
            _generatingScript,
            _baseUri,
            _mintTimeout,
            _maxSupply
        );

        emit CollectionCreated(address(collectionAddress));
    }

This could, potentially, allow users to save "payloads" on-chain, through the contract's state, that could be further executed by the Front-end application and/or the web server, accordingly to the business rules and overall system infrastructure.

If those inputs aren't thoroughly validated in the off-chain premises, on-chain data (crafted payloads stored in generatingScript) can be used as an attack vector for exploring vulnerabilities in the web application, such as Stored Cross-site Scripting (Stored XSS) or HTML injection.

This finding is not directly related to a security issue. However, it's important to bear in mind that any future and unaudited releases of the Front-end application should be thoroughly tested, specially when reading user-provided, non sanitized on-chain data.

Because of inherent design limitations within EVM and Solidity, it is not economically neither technically possible to sanitize the user inputs - in this case, possible malicious payloads - that are written on-chain when calling the createCollection function in the CollectionFactory.sol contract.

It is recommended to test thoroughly any unaudited updates in the User Interface and Front-end application, to make sure that user-provided inputs, specifically those which are read on-chain, are correctly sanitized before interpreted by the browser.

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai. The generated bytecode will include PUSH0 opcodes. The recently released Solc compiler version 0.8.25 switches the default target EVM version to Cancun, so it is also important to note that it also adds-up new opcodes such as TSTORE, TLOAD and MCOPY.

Be sure to select the appropriate EVM version in case you intend to deploy on a chain apart from mainnet like L2 chains that may not support PUSH0, TSTORE, TLOAD and/or MCOPY, otherwise deployment of your contracts will fail.

It is important to consider the targeted deployment chains before writing immutable contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

During the analysis, both contracts Collection.sol and CollectionFactory.sol were identified using a floating pragma, as follows:

pragma solidity ^0.8.20;

Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of pragma solidity ^0.8.20;, use pragma solidity 0.8.25;.

Remediation Plan

SOLVED: The NFTfi team solved the issue by implementing a suggested fix.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and add them to a special data structure known as “topics” instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.

Each event can use up to three indexed fields. If there are fewer than three fields, all the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).

This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

- contracts/Collection.sol [Line: 13]

	    event Mint(uint256 indexed _startTokenId, uint256 _quantity, bytes32[] _hashes, address indexed _owner);

- contracts/CollectionFactory.sol [Line: 7]

	    event CollectionCreated(address collectionAddress);

- contracts/MerkleDistributor.sol [Line: 22]

	    event Claimed(uint256 _index, uint256 _amount, bytes32[] _merkleProof, address indexed _account);

It is recommended to add the indexed keyword when declaring events, considering the following example:

    event Indexed(
        address indexed from,
        bytes32 indexed id,
        uint indexed value
    );

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

In the MerkleDistributor.sol contract, the NatSpec specifications are slightly wrong, considering they are referring to the contract TokenLock.sol, which belongs to another NFTIfi's contract set.

- contracts/MerkleDistributor.sol [Lines: 9-16]

/**
 * @title MerkleDistributor
 * @author NFTfi
 * @dev Modified version of Uniswap's MerkleDistributor
 * https://github.com/Uniswap/merkle-distributor/blob/master/contracts/MerkleDistributor.sol
 * Main difference: in claim instead of transferring the tokens to the user,
 * we transfer it to the tokenLock contract
 */

- contracts/MerkleDistributor.sol [Lines: 24-28]

    /**
     * @dev Constructor initializes references for NFTfi token and TokenLock contract.
     * It also sets the owner of the contract.
     * @param _admin The initial owner of the contract, usually able to set Merkle roots.
     */

Keeping consistency across NatSpec documentation enhances contract readability and makes it easier both for developers and non-developers to understand the contract's source code.

It is recommended to review the NatSpec specifications for any typos or misalignment.

Remediation Plan

SOLVED: The NFTfi team solved the issue by implementing a suggested fix.