User can mint more tokens than he should during the allowlist phase

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L193-L198

Vulnerability details

Impact

The identified vulnerability allows a user to exceed the prescribed token minting limit during the allowlist phase. If this user is the first to initiate minting during this phase and possesses sufficient capital, they can exploit this flaw to mint the entire available token supply from the collection.

Proof of Concept

A user gains allowlist access for minting a limited number of tokens from a new collection, utilizing a smart contract address for the allowlist. He invokes the MinterContract#mint method to start minting.

Here we jump into the first condition block because we are in the first phase. One important require block is here to check whether _maxAllowance is higher or equal to retrieveTokensMintedALPerAddress (Number of tokens minted by address during the first phase) + _numberOfTokens (Number of tokens user wants to mint). The user is allowed to mint limited number of tokens so this condition should fail when he tries to mint more. The retrieveTokensMintedALPerAddress method returns value from NextGenCore tokensMintedAllowlistAddress state variable that keeps track of how many tokens have been minted to an address.

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

When this condition is passed, it gets to a for loop that calls NextGenCore#mint to process a token mint.

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

The root cause of this vulnerability lies here in the NextGenCore mint method. As you can see, it mints the token before it updates the count of tokens minted by address. This violates Checks-Effects-Interactions pattern and opens paths for reentrancy attacks.

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");
    // @note Update circulating supply
    collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
    if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
        // @note Mint token to user using safeMint.
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        // @audit-issue Checks-Effects-Interactions pattern broken. The count of minted tokens should be updated before the token is minted.
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

When we check the _mintProcessing function we find out that it uses _safeMint method. This opens the attack path because the user wants the token to be minted to a smart contract address. Safe mint calls IERC721Receiver.onERC721Received in the recipient contract.

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

The user can use this callback to reenter the MinterContract#mint method and start the process again. There is the require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit"); condition that should revert the transaction as we describe at the beginning. But he reentered the method before the number of minted tokens was updated. Because of it, the user is able to mint as many tokens as he wants to up to the maximum supply if he has enough ether. He just need to reenter the mint method from the onERC721Received callback.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend fixing the NextGenCore#mint method by applying the C-E-I pattern or you may consider adding a reentrancy guard.

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

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #1597

[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