`setFinalSupply` can set totalSupply of a non-existent collection

[c4-submissions]

Lines of code

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

Vulnerability details

Description

setFinalSupply allows setting the collectionTotalSupply of a non-existent _collectionId. If that happens, setCollectionData can't set the boolean wereDataAdded[_collectionId] for a collection, which makes several critical functions that checks that boolean to be forever not callable in the collection.

Proof of Concept

1. Admin unconsciously calls setFinalSupply for a non-existent _collectionId:

   function setFinalSupply(uint256 _collectionID) public FunctionAdminRequired(this.setFinalSupply.selector) {
       require (block.timestamp > IMinterContract(minterContract).getEndTime(_collectionID) + collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint, "Time has not passed");
       collectionAdditionalData[_collectionID].collectionTotalSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply;
       collectionAdditionalData[_collectionID].reservedMaxTokensIndex = (_collectionID * 10000000000) + collectionAdditionalData[_collectionID].collectionTotalSupply - 1;
   }

   • The only check made is:

     require (block.timestamp > IMinterContract(minterContract).getEndTime(_collectionID) + collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint, "Time has not passed");

     It requires that block.timestamp > collectionPhases[_collectionID].publicEndTime + collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint . For a non-existent _collectionId, these values are zero as default, so the check is effectively block.timestamp > 0 , which is always true.

2. In the future, when this _collectionId will be used, setCollectionData is called to set collection's importants metadatas. However, the only path to set wereDataAdded[_collectionId] = true is if collectionTotalSupply is zero:

   if (collectionAdditionalData[_collectionID].collectionTotalSupply == 0) {
   // ...
   wereDataAdded[_collectionID] = true;
   }

3. As a consequence, all the functions that checks wereDataAdded[_collectionId], including the one that sets minting price, are forever not callable for that _collectionId. They're critical, so the collection _collectionId is not usable anymore. Here are some of the functions (setCollectionCosts, airDropTokens, mintAndAuction):

   
   function setCollectionCosts(uint256 _collectionID, uint256 _collectionMintCost, uint256 _collectionEndMintCost, uint256 _rate, uint256 _timePeriod, uint8 _salesOption, address _delAddress) public CollectionAdminRequired(_collectionID, this.setCollectionCosts.selector) {
       require(gencore.retrievewereDataAdded(_collectionID) == true, "Add data");
       collectionPhases[_collectionID].collectionMintCost = _collectionMintCost;
       collectionPhases[_collectionID].collectionEndMintCost = _collectionEndMintCost;
       collectionPhases[_collectionID].rate = _rate;
       collectionPhases[_collectionID].timePeriod = _timePeriod;
       collectionPhases[_collectionID].salesOption = _salesOption;
       collectionPhases[_collectionID].delAddress = _delAddress;
       setMintingCosts[_collectionID] = true;
   }
   
   function airDropTokens(address[] memory _recipients, string[] memory _tokenData, uint256[] memory _saltfun_o, uint256 _collectionID, uint256[] memory _numberOfTokens) public FunctionAdminRequired(this.airDropTokens.selector) {
       require(gencore.retrievewereDataAdded(_collectionID) == true, "Add data");
       uint256 collectionTokenMintIndex;
       for (uint256 y=0; y< _recipients.length; y++) {
           collectionTokenMintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID) + _numberOfTokens[y] - 1;
           require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(_collectionID), "No supply");
           for(uint256 i = 0; i < _numberOfTokens[y]; i++) {
               uint256 mintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID);
               gencore.airDropTokens(mintIndex, _recipients[y], _tokenData[y], _saltfun_o[y], _collectionID);
           }
       }
   }
   
   function mintAndAuction(address _recipient, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint _auctionEndTime) public FunctionAdminRequired(this.mintAndAuction.selector) {
       require(gencore.retrievewereDataAdded(_collectionID) == true, "Add data");
       uint256 collectionTokenMintIndex;
       collectionTokenMintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID);
       require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(_collectionID), "No supply");
       uint256 mintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID);
       gencore.airDropTokens(mintIndex, _recipient, _tokenData, _saltfun_o, _collectionID);
       uint timeOfLastMint;
       // check 1 per period
       if (lastMintDate[_collectionID] == 0) {
       // for public sale set the allowlist the same time as publicsale
           timeOfLastMint = collectionPhases[_collectionID].allowlistStartTime - collectionPhases[_collectionID].timePeriod;
       } else {
           timeOfLastMint =  lastMintDate[_collectionID];
       }
       // uint calculates if period has passed in order to allow minting
       uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[_collectionID].timePeriod;
       // users are able to mint after a day passes
       require(tDiff>=1, "1 mint/period");
       lastMintDate[_collectionID] = collectionPhases[_collectionID].allowlistStartTime + (collectionPhases[_collectionID].timePeriod * (gencore.viewCirSupply(_collectionID) - 1));
       mintToAuctionData[mintIndex] = _auctionEndTime;
       mintToAuctionStatus[mintIndex] = true;
   }

- Not being able to call `setCollectionCosts` also makes impossible to call `mint`, since it needs a price for the collection.

## Impact
Makes a `_collectionId` not usable, which takes time from the team to debug the mistake, to recreate the collection if some data was already set and wrongly account the number of `_collectionId`. 
1. Likelihood: Medium. 
2. Impact: Low. 
- Risk: Medium (Medium + Low) 

## Tools Used

Manual Review

## Recommended Mitigation Steps 

`setFinalSupply` needs to check if `wereDataAdded[_collectionId] == true` or if collection exists.

## Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #478

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden has specified valid misbehavior in the system whereby the final supply of a collection can be set by the function's administrator before the collection's data has ever been added by the collection's administrator.

This is a configurational mistake that is highly unlikely to happen, so I will mark this as a valid QA.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)