Collection Total Supply invariant can be broken

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196-L253 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L307-L311

Vulnerability details

Impact

It is possible to push the collection total supply past what is set by the owner on creating the collection. Collection total supply accounting is compromised.

Proof of Concept

In MinterContract.sol mint() has the following checks to ensure the amount of minted NFTs remain in the bound set by the creator.

// Minter Contract mint() function
collectionTokenMintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col) + _numberOfTokens - 1;
        require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(col), "No supply");
        require(msg.value >= (getPrice(col) * _numberOfTokens), "Wrong ETH");
        for(uint256 i = 0; i < _numberOfTokens; i++) {
            uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col);
            gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);
        }

The problem is that the NFTs are minted in a for-loop, therefore a malicious user could reenter mint(...) through the onERC721Received hook before the for-loop has iterated over all tokens. In a situation where the minted NFTs in the first for loop will be the last of a collection a malicious user can reenter & artificially push collectionAdditionalData[_collectionID].collectionCirculationSupply since the checks before the for loop will have stale balances & mint(...) inside the NextGenContract doesn't revert. It's an artificial inflation since no new NFTs are minted only the circulation supply is updated - see mint(...) inside NextGenCore. Later when setFinalSupply(...) is called the collection total supply is set to the Circulating Total Supply which is over inflated & collectionTotalSupply will hold a wrong value. However, whoever extends the count beyond the total supply set by the owner will still have to pay the price per NFT (without receiving one). Whether there is incentive to disrupt the total supply accounting for a given price depends on external circumstances but it is achievable.

// Next Gen Contract mint function 
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;
            }
        }
    }

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

Tools Used

Manual Inspection

Recommended Mitigation Steps

Perform updates in batches depending on the number of tokens minted. Alternatively, Have the mint(...) function in the NextGenContract revert when total supply is surpassed.

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

there are additional checks on the mint function on the Minter Contract.

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #1282

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as duplicate of #1282

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1201

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c