code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol#L34 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L220-L222 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol#L86-L93 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L171-L187

Vulnerability details

Impact

For an NFT drop collection after creation, collectors can view the following public maxTokenId state variable to see the maximum tokenId that can be minted. Knowing this, collectors can decide on how many NFTs of this NFT drop collection they want to mint.

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol#L34

  uint32 public maxTokenId;

After creating the NFT drop collection, the creator can call the following updateMaxTokenId function to update maxTokenId. However, this updateMaxTokenId function is still callable by the creator after minting begins. The creator can maliciously update maxTokenId after minting starts to block further minting if there is an incentive for doing so.

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L220-L222

  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
    _updateMaxTokenId(_maxTokenId);
  }

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol#L86-L93

  function _updateMaxTokenId(uint32 _maxTokenId) internal {
    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

    maxTokenId = _maxTokenId;
    emit MaxTokenIdUpdated(_maxTokenId);
  }

After collectors mint some NFTs, the creator blocks further minting by calling the updateMaxTokenId function. Afterwards, calling the following mintCountTo function will always revert. As a result, the collectors are unable to mint their desired number of NFTs and waste gas by sending the relevant transactions.

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L171-L187

  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;
      }
    }
  }

Proof of Concept

Please add the following test in the Can decrease maxTokenId describe block in test\collections\NFTDropCollection\update.ts. This test will pass to demonstrate the described scenario.

  it("Malicious creator can update maxTokenId by calling updateMaxTokenId after minting begins to block further minting for an NFT drop collection", async () => {  
    let nftDropCollection_: NFTDropCollection;
    const tx_ = await contracts.nftCollectionFactoryV2
      .connect(creator)
      .createNFTDropCollection(
        "NAME_",
        "SYM_",
        PREREVEAL_URI,
        ethers.utils.keccak256(ethers.utils.toUtf8Bytes("ipfs://foo/")),
        100,
        contracts.nftDropMarket.address,
        5000,
        {gasLimit: 1e6}
      );
    nftDropCollection_ = await getNFTDropCollection(tx_, creator);

    // minting starts
    await nftDropCollection_.connect(creator).mintCountTo(2, collector.address);

    // maxTokenId is changed from 100 to 2 after minting begins
    await nftDropCollection_.connect(creator).updateMaxTokenId(2);

    // further minting reverts
    await expect(nftDropCollection_.connect(creator).mintCountTo(1, collector.address, {gasLimit: 1e6})).to.be.revertedWith(
      "NFTDropCollection: Exceeds max tokenId",
    );
  });

Tools Used

VSCode

Recommended Mitigation Steps

A boolean state variable can be added in the NFTDropCollection contract to indicate whether minting has started or not. When the mintCountTo function is called for the first time, this state variable can be changed to true. When this state variable is true, calling the updateMaxTokenId function should revert so creators cannot change maxTokenId anymore after minting begins.

ghost commented 2 years ago

I believe this is the intended behaviour. It is the creator's decision whether or not to reduce the number of NFTs available for mint.

HardlyDifficult commented 2 years ago

ACK.

We believe this is low risk. Assets are not compromised, and many users may believe that a more limited supply actually increases the value for holders.

This feature is working as intended / by design. If a collection does not sell out, we wanted to give the creator tools to handle that situation gracefully. Generally in the ecosystem, rarity is considered a value add. If the creator made a collection that's too large for their existing community then they have the option of constricting supply - which allows the floor price to potentially float above the original sale price.

We provide creator tools, it's up to them to gauge their community and only take actions that benefit their holders.

HickupHH3 commented 2 years ago

Agree that it's a feature, not a bug, for creators to tweak the mint supply if needed.

code-423n4 / 2022-08-foundation-findings