Reentrancy in mint function leads to various problems

[captainmangoC4]

Lines of code

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

Vulnerability details

Bug Description

When minting NFTs, users will using the mint function. This function will mint a NFT using the _safeMint function. The problem is that this mint will be done before crucial variables are updated.

Mint before checking the salesOption == 3 conditions are fulfilled in MinterContract.sol:

        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) {
            uint timeOfLastMint;
            if (lastMintDate[col] == 0) {
                // for public sale set the allowlist the same time as publicsale
                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod;
            } else {
                timeOfLastMint =  lastMintDate[col];
            }
            // uint calculates if period has passed in order to allow minting
            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
            // users are able to mint after a day passes
            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));
        }

Mint before updating tokensMintedAllowlistAddress and tokensMintedPerAddress in NextGenCore.sol mint.

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

The vulnerability arises during the execution of the _safeMint function, which involves the usage of _checkOnERC721Received. Exploiting this, attackers can repeatedly mint NFTs by employing a smart contract that reenters the function upon receiving the call of _checkOnERC721Received. The minting process occurs prior to updating crucial variables such as tokensMintedAllowlistAddress and tokensMintedPerAddress, as well as implementing the control mechanism for sale option 3. This oversight allows attackers to mint the entire collection without proper limitations.

Impact

The reentrancy vulnerability exposes the system to significant risks, allowing attackers to exploit the following critical issues:

• Bypassing the mint amount limit within the allowlist (tokensMintedAllowlistAddress)
• Bypassing the mint amount limit during public sales (tokensMintedPerAddress)
• Bypassing the control mechanism for sale option 3 (salesOption == 3), enabling unlimited NFT minting within a specific timeframe

In some cases, attackers can also drain protocol's funds.

Proof of Concept

Example Illustrating Potential Fund Loss for the Protocol

Scenario

• A collection offers a 0 ETH minting price during the allowlist period.
• The collection incorporates Chainlink's VRF, invoking the calculateTokenHash function in RandomizerVRF.sol during NFT minting. This involves the usage of Link tokens to generate randomness.
• An attacker is permitted to mint only 1 token within the allowlist period.

Attack Path

1. The attacker deploys a malicious smart contract that implements IERC721Receiver, initiating reentry into the mint function upon NFT receipt using the onERC721Received function.
2. The attacker triggers the mint function using the malicious smart contract.
3. The smart contract repeatedly reenters the mint function through onERC721Received.
4. Since tokensMintedAllowlistAddress remains unaltered, the mint amount for the allowlist check is bypassed.
5. The malicious contract successfully mints the entire collection.

This vulnerability results in the attacker being able to mint the whole collection for free and the protocol suffering substantial financial losses, considering the requirement for a significant number of Link tokens to generate the unique hash for all the NFTs. While potential gas limit issues might arise, this scenario effectively demonstrates the concept. Additionally, this vulnerability can be exploited to bypass the mint amount limits during public sales and the control mechanism for sale option 3.

Tools Used

Manual review

Recommended Mitigation Steps

To address this vulnerability, it is imperative to update tokensMintedAllowlistAddress and tokensMintedPerAddress before executing the _mintProcessing function within NextGenCore.sol's mint function. Furthermore, implement the control mechanism for sale option 3 in the MinterContract.sol before invoking the mint function in NextGenCore.sol.

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 selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

This submission was created as part of a divergence of #1517 per the relevant comment.

Identically to the original exhibit, this submission has been adequately graded as high due to breaking a main invariant of the protocol and with a sensitive sale model at that (periodic sales which permit one NFT per period).

[alex-ppg]

After further consideration, the submission's rationale concerning bypassing the minting limit is incorrect. The code adheres to an Interactions-Checks-Effects pattern, rendering it secure against a re-entrancy that attempts to bypass the limit itself.

A real vulnerability that arises from this is re-using the same getPrice for a valid multi-token period mint in case multiple periods have elapsed with no mint. As such, I will invalidate this and all relevant submissions as none identified this particular exploitation vector.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[alex-ppg]

Upon further consideration, the getPrice attack vector is not feasible either. The NextGenCore::mint function will increment the circulating supply prior to minting, causing price measurements to be correct. As such, no attack vector presently exists from a re-entrancy arising from periodic sales.