Last token of maximum supply can be paid, but it isn't minted nor reverted.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L181 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L192 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L217

Vulnerability details

Description

collectionCirculationSupply is incremented in each mint and it's used to check if the mint don't overtakes the collection's max supply. However, it increments before the check, which makes that although last token is in the max supply range, the collectionCirculationSupply becomes a value higher than max supply, not allowing the mint. Even further, the code doesn't revert or refund funds if that happens.

Proof of Concept

1. Let's exemplify:

   • I want to mint the last token of the collection (the token 200)
   • collectionTotalSupply = 200
   • collectionCirculationSupply = 200. The variable is designed to be the next ID to mint, which is 200 in the example case because we will mint the token 200 of the collection. The following line is used everytime the protocol wants to know the next ID to mint:

     uint256 mintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID);

2. I call mint, which calls mint from NextGenCore.sol:

   function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
       require(msg.sender == minterContract, "Caller is not the Minter Contract");
       collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
       if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
           _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
           if (phase == 1) {
               tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
           } else {
               tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
           }
       }
   }

3. First, it increments the collectionCirculationSupply:

   collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;

   • collectionCirculationSupply = 200 + 1 = 201

4. After, it checks if it's higher than the collectionTotalSupply:

   if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply)

   • collectionTotalSupply (200) >= collectionCirculationSupply (201), which is FALSE

5. So, the last token of the supply will not be minted. However, even worse is the fact that the if is the only check, there is no else to revert the transaction or any code to refund the user.

Impact

The collection will have a token less, which is a loss of funds for the collection. Also, the one to mint the last token will lose all his money without refunds.

1. Likelihood: Medium.
2. Impact: High.
   • Risk: High (Medium Likelihood + High Impact)

Tools Used

Manual Review

Recommended Mitigation Steps

1. The collectionTotalSupply increment needs to be after the checks.
2. The check needs to revert, so user doesn't lose money.

Assessed type

ERC721

[c4-pre-sort]

141345 marked the issue as duplicate of #1282

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Insufficient proof