Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/2467db02cc446374ab9154dde98f7e931d71584d/smart-contracts/MinterContract.sol#L213 https://github.com/code-423n4/2023-10-nextgen/blob/2467db02cc446374ab9154dde98f7e931d71584d/smart-contracts/MinterContract.sol#L217 https://github.com/code-423n4/2023-10-nextgen/blob/2467db02cc446374ab9154dde98f7e931d71584d/smart-contracts/MinterContract.sol#L224

Vulnerability details

Impact

Bypassing _maxAllowance in NextGenMinterContract.mint(): Enables minting more NFTs than permitted.
Exploiting reentrancy in NextGenMinterContract.burnToMint(): Allows acquiring both burnable and mintable NFTs at the same time.

Proof of Concept

The 1st case

The typical flow of a user mint is as follows: User tx -> NextGenMinterContract.mint() external call -> NextGenCore.mint() internal call -> NextGenCore._mintProcessing()

The root of the problem lies in the NextGenCore.mint() and NextGenCore._mintProcessing() functions.

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 _mintProcessing(
    uint256 _mintIndex,
    address _recipient,
    string memory _tokenData,
    uint256 _collectionID,
    uint256 _saltfun_o
) internal {
    tokenData[_mintIndex] = _tokenData;
    collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
    tokenIdsToCollectionIds[_mintIndex] = _collectionID;
    _safeMint(_recipient, _mintIndex);
}

In the mint() function, the _mintProcessing() is called first and then the variables tokensMintedAllowlistAddress or tokensMintedPerAddress are updated. However, this creates a reentrancy issue because the _safeMint() at the end of _mintProcessing() calls _recipient.onERC721Received().

_recipient can be either the user or his chosen delegate, so passing a smart contract is not a problem.

To take advantage of the reentrancy, a user can pass a smart contract that will call the NextGenMinterContract.mint() function again as a delegate of the user. The function's checks will hold as the tokensMintedAllowlistAddress or tokensMintedPerAddress variables are still not updated. This method allows the user to mint as many tokens as the gas limit allows.

If the user is a smart contract, the execution is simplified as delegation is unnecessary.

The 2nd case

A malicious user can call NextGenMinterContract.burnToMint() from a smart contract. The flow is similar to the first case, where _mintProcessing() is called before burning the NFT, enabling the user to utilize both burnable and mintable NFTs together for potential profit extraction.

function burnToMint(
    uint256 mintIndex,
    uint256 _burnCollectionID,
    uint256 _tokenId,
    uint256 _mintCollectionID,
    uint256 _saltfun_o,
    address burner
) external {
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
    collectionAdditionalData[_mintCollectionID].collectionCirculationSupply =
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
    if (
        collectionAdditionalData[_mintCollectionID].collectionTotalSupply
            >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply
    ) {
        _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
        // burn token
        _burn(_tokenId);
        burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
    }
}

As long as the user has the burnable NFT in the end of his actions, the transaction will succeed.

Tools Used

Manual review

Recommended Mitigation Steps

Add reentrancy checks to the NextGenMinterContract.mint() and NextGenMinterContract.burnToMint() functions such as ReentrancyGuard by OZ or change the function to mint only after all the storage changes had been made.

MinterContract.sol

+ import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

...

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

...

    function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o)
        public
        payable
+       nonReentrant
    {

NextGenCore.sol

    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;
            }
+           _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        }
    }

...

    function burnToMint(
        uint256 mintIndex,
        uint256 _burnCollectionID,
        uint256 _tokenId,
        uint256 _mintCollectionID,
        uint256 _saltfun_o,
        address burner
    ) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply =
            collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
        if (
            collectionAdditionalData[_mintCollectionID].collectionTotalSupply
                >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply
        ) {
-           _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
            // burn token
            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
+           _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
        }
    }

Assessed type

Reentrancy

code-423n4 / 2023-10-nextgen-findings

Reentrancy in `NextGenMinterContract.mint()` allows exceeding max allowance and concurrent use of NFTs in `NextGenMinterContract.burnToMint()` #2052