Minting can be reentered because mint allowance is updated after the mint itself

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L213
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L217
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L224
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L193-L198

Vulnerability details

Impact

Minting is re-entrable because internal accounting is updated after the actual minting is done. If the address that mints is of a contract, it can re-enter in its onERC721Received callback and mint as much tokens as it'd like, given it has a sufficient ETH balance to buy the NFTs and that there's also enough sufficient supply of that collection left.

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

After the initial mint transaction when the attacker reenters into MinterContract#mint() their tokens minted per public address will not be updated and will return the old balance:

    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
        require(setMintingCosts[_collectionID] == true, "Set Minting Costs");
        uint256 col = _collectionID;
        address mintingAddress;
        uint256 phase;
        string memory tokData = _tokenData;
        if (block.timestamp >= collectionPhases[col].allowlistStartTime && block.timestamp <= collectionPhases[col].allowlistEndTime) {
	    ...
        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
            phase = 2;
            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
→           require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
            mintingAddress = msg.sender;
            tokData = '"public"';
        } else {
            revert("No minting");
        }
        uint256 collectionTokenMintIndex;
        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);
        }
	...
    }

Only NFT collections with a Periodic Sale model cannot be taken advantage of because the time difference calculation since last mint underflows if more than 1 NFT is attempted to be minted within 1 transaction/block.

Proof of Concept

In order to run the provided PoC, please first initialize Forge: forge init --no-git --force
Then in the root directory's test folder paste the content of the gist in a new file POC.t.sol in the ./test directory and run it using forge test --match-path ./test/POC.t.sol.

Link to PoC: https://gist.github.com/flackoon/39e2f08a6e9b30a86de607585b6cb1e4

The PoC demonstrates how an honest minter BOB is only allowed to mint up to maxCollectionPurchases (set to 1 in the test) and on the other side a GreedyNFTReceiver contract designed to take advantage of the vulnerability can mint as much NFTs as they wish, given they have enough ETH to pay for them & that there's sufficient supply left.

Tools Used

Foundry Forge

Recommended Mitigation Steps

Just process mint after internal allowances accounting for minted tokens per address has been done.

diff --git a/smart-contracts/NextGenCore.sol b/smart-contracts/NextGenCore.sol
index 6d294ed..e099763 100644
--- a/smart-contracts/NextGenCore.sol
+++ b/smart-contracts/NextGenCore.sol
@@ -190,12 +190,12 @@ contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {
         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;
             }
+            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
         }
     }

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #51

[c4-pre-sort]

141345 marked the issue as duplicate of #1742

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg marked the issue as satisfactory