QA Report

[code423n4]

QA Report

Table of Contents

• Low Risk
  • [L-01] selfdestruct will be a noop in the future
  • [L-02] NFT buyer and NFT mint receiver can be a contract with no onERC721Received method, freezing NFT

Low Risk

[L-01] selfdestruct will be a noop in the future

Description

selfdestruct is a native Solidity function used to delete contracts on the blockchain. When a contract executes a self-destruct operation, the remaining ether on the contract account will be sent to a specified target, and its storage and code are erased.

After EIP-4758, the SELFDESTRUCT op code will no longer be available. According to the EIP, "The only use that breaks is where a contract is re-created at the same address using CREATE2 (after a SELFDESTRUCT)".

As the protocol does not necessarily depend on re-deploying contracts to the same address (however, a user an still deploy NFT contracts to the same address via NFTCollectionFactory calls), this will not break the protocol. It will simply render the selfDestruct function useless.

Findings

mixins/collections/SequentialMintCollection.sol#L77

function _selfDestruct() internal {
  require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

  emit SelfDestruct(msg.sender);
  selfdestruct(payable(msg.sender));
}

Called by NFTCollection.sol#L230

/**
  * @notice Allows the collection creator to destroy this contract only if
  * no NFTs have been minted yet or the minted NFTs have been burned.
  * @dev Once destructed, a new collection could be deployed to this address (although that's discouraged).
  */
function selfDestruct() external onlyCreator {
  _selfDestruct();
}

and NFTDropCollection.sol#L210

/**
  * @notice Allows a collection admin to destroy this contract only if
  * no NFTs have been minted yet or the minted NFTs have been burned.
  * @dev Once destructed, a new collection could be deployed to this address (although that's discouraged).
  */
function selfDestruct() external onlyAdmin {
  _selfDestruct();
}

Recommended mitigation steps

Consider removing the SequentialMintCollection._selfDestruct function and the other caller functions.

[L-02] NFT buyer and NFT mint receiver can be a contract with no onERC721Received method, freezing NFT

Description

If the NFT drop collection buyer or the receiver of a NFT mint is a smart-contract that does not implement the onERC721Received method, in the current implementation, by using _mint(), a minted NFT can get stuck in a recipients contract.

Findings

NFTCollection.sol#L271

function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {
    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
    require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
    unchecked {
        // Number of tokens cannot overflow 256 bits.
        tokenId = ++latestTokenId;
        require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
        cidToMinted[tokenCID] = true;
        _tokenCIDs[tokenId] = tokenCID;
        _mint(msg.sender, tokenId);
        emit Minted(msg.sender, tokenId, tokenCID, tokenCID);
    }
}

NFTDropCollection.sol#L182

function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {
    require(count != 0, "NFTDropCollection: `count` must be greater than 0");

    unchecked {
      // If +1 overflows then +count would also overflow, unless count==0 in which case the loop would exceed gas limits
      firstTokenId = latestTokenId + 1;
    }
    latestTokenId = latestTokenId + count;
    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

    for (uint256 i = firstTokenId; i <= latestTokenId; ) {
      _mint(to, i);
      unchecked {
        ++i;
      }
    }
}

Recommended mitigation steps

Its is recommended to use _safeMint whenever possible.

_safeMint(to, tokenId);

[HardlyDifficult]

[L-01] selfdestruct will be a noop in the future

Understood. selfdestruct is never required, it's an optional feature and it's fine if it's not available in the future.

[L-02] NFT buyer and NFT mint receiver can be a contract with no onERC721Received method, freezing NFT

Agree will fix - for context see our response here.