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)