collection minting can be stopped before the minting is completed

[c4-submissions]

Lines of code

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

Vulnerability details

setFinalSupply() function is used to reduce the supply of the collection to the existing minted supply, it first checks that a time have passed after public minting and than it sets the collection total supply the circulating supply which will stop the minting of the collection .

But in some cases a collection may have the public phase before the allow list phase for any particular reason, which is not forbidden by the protocol .

in thas case an admin can call setFinalSupply() (intentionally or accidentally) before or during the allow list phase and stop the minting of the collection .

That’s why setFinalSupply() should not be related to public minting end time, but it should be related to the whole collection minting end time because public minting can be not the last phase .

Proof of Concept

The PoC is conducted using foundry, to install foundry follow these installation steps .

to reproduce it :

• In the root of the repo run forge init --force
• open new MinterContractTest.t.sol file in test folder
• copy the test in the MinterContractTest.t.sol file and save .
• run forge test --mt test_blockMinting or forge test --mt test_blockMinting -vvvv for more informations .

In the following test the collection has a public phase before an allow list phase and the setFinalSupplyTimeAfterMint is set to zero, so setFinalSupply() can be called just after the public phase end time .
the test :

pragma solidity ^0.8.10;

import "forge-std/Test.sol";

import "../smart-contracts/MinterContract.sol";
import "../smart-contracts/NextGenCore.sol";
import "../smart-contracts/NextGenAdmins.sol";
import "../smart-contracts/NFTdelegation.sol";
import "../smart-contracts/RandomizerNXT.sol";
import "../smart-contracts/XRandoms.sol";

contract NextGenMinterContractTest is Test {
    NextGenAdmins public admins;
    NextGenCore public core;
    DelegationManagementContract public delegation;
    NextGenMinterContract public minter;
    NextGenRandomizerNXT public randomizer;
    randomPool public poolRandom;

    address public globalAdmin;
    address public artist;
    address public user;

    function setUp() public {
        globalAdmin = makeAddr("globalAdmin");
        artist = makeAddr("artist");
        user = makeAddr("user");
        vm.deal(user, 10 ether);

        vm.startPrank(globalAdmin);
        admins = new NextGenAdmins();
        core = new NextGenCore("Next Gen Core", "NEXTGEN", address(admins));
        delegation = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(delegation), address(admins));
        core.addMinterContract(address(minter));
        vm.stopPrank();
    }

    function test_blockMinting() public {
        vm.startPrank(globalAdmin);

        // create colection

        string[] memory collectionScript = new string[](1);
        collectionScript[0] = "desc";

        core.createCollection(
            "Test Collection 1",
            "Artist 1",
            "For testing",
            "www.test.com",
            "CCO",
            "https://ipfs.io/ipfs/hash/",
            "",
            collectionScript
        );

        // set collection data

        core.setCollectionData(1, artist, 2, 100, 0);

        // add randomizer

        randomPool randoms = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(core));
        core.addRandomizer(1, address(randomizer));

        // set collection costs

        minter.setCollectionCosts(1, 1 ether, 1 ether, 0, 0, 1, 0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B);

        // set collection phases

        minter.setCollectionPhases(1, 10, 20, 0, 10, 0x0e096d90003046587967b31edec5bfa647055a492444c9a730cf089a666d91cc); // public phase before allow list phase .

        vm.stopPrank();

        // public phase minting

        bytes32[] memory merkleProof = new bytes32[](2);
        vm.prank(user);
        minter.mint{value: 1 ether}(1, 1, 0, "", user, merkleProof, address(0), 0);

        skip(10); // skip to allow list minting

        vm.prank(globalAdmin);
        core.setFinalSupply(1); // admin calls setFinalSupply()

        merkleProof[1] = 0xad3228b676f7d3cd4284a5443f17f1962b36e491b30a40b2405849e597ba5fb5;
        vm.expectRevert("No supply"); // expect to revert because admin stoped minting by calling setFinalSupply()
        vm.prank(user);
        minter.mint{value: 1 ether}(1, 1, 1, "", user, merkleProof, address(0), 0);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

• Disallow setting the public phase before the allow list phase, by adding a require statement that checks that (_publicEndTime > _allowlistEndTime) in setCollectionPhases(), this will ensure that the public phase is always the last one and publicEndTime means the end of the whole collection minting .

diff --git a/MinterContract.sol.orig b/MinterContract.sol
index b4bd6b6..de953ff 100644
--- a/MinterContract.sol.orig
+++ b/MinterContract.sol
@@ -197,6 +197,7 @@ contract NextGenMinterContract is Ownable {
         bytes32 _merkleRoot
     ) public CollectionAdminRequired(_collectionID, this.setCollectionPhases.selector) {
         require(setMintingCosts[_collectionID] == true, "Set Minting Costs");
+        require(_publicEndTime > _allowlistEndTime, "public phase must be the last one");
         collectionPhases[_collectionID].allowlistStartTime = _allowlistStartTime;
         collectionPhases[_collectionID].allowlistEndTime = _allowlistEndTime;
         collectionPhases[_collectionID].merkleRoot = _merkleRoot;

• Or check that the end time of the whole collection minting is the end of the later phase whether it be public or allow list , by letting MinterContract#getEndTime() return the maximum between public end time and allowlist end time . This will cause setFinalSupply() to revert if called before the end of the collection minting .

diff --git a/MinterContract.sol.orig b/MinterContract.sol
index b4bd6b6..a52aee8 100644
--- a/MinterContract.sol.orig
+++ b/MinterContract.sol
@@ -736,7 +736,11 @@ contract NextGenMinterContract is Ownable {
     // get minting end time
 
     function getEndTime(uint256 _collectionID) external view returns (uint256) {
-        return collectionPhases[_collectionID].publicEndTime;
+        if (collectionPhases[_collectionID].publicEndTime > collectionPhases[_collectionID].allowlistEndTime) {
+            return collectionPhases[_collectionID].publicEndTime;
+        } else {
+            return collectionPhases[_collectionID].allowlistEndTime;
+        }
     }
 
     // get auction end time

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #303

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #2033

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c