The reentrancy vulnerability in NextGenCore can allow an attacker to manipulate minting execution

[thebrittfactor]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/NextGenCore.sol#L194

Vulnerability details

Impact

Function _mintProcessing() has been used in mint() and airDropTokens() and both doesn't follow check-effect-interaction pattern and code updates the values of tokensAirdropPerAddress, tokensMintedAllowlistAddress and tokensMintedPerAddress variables after making external call by using safeMint(). This would give attacker opportunity to reenter the Minter contract logics and perform malicious action while contract storage state is wrong.

Attacker can perform this action:

• call MinterContract::mint() specifying his maxAllowance.
• Function executes and checks attacker isn't exceeding limit, i.e line [224](https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/MinterContract.sol#L224) in the case of public phase and line [217](https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/MinterContract.sol#L217) for the allowList phase - ( Notice this function performs this checks using the values returned from retrieveTokensMintedALPerAddress and retrieveTokensMintedALPerAddress)
• amount is valid and check passes, proceeds minting to attacker on core contract
• _safeMint() triggers a callBack into attacker contract before updating this crucial varaible on line [194](https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/NextGenCore.sol#L194).
• calls MinterContract::mint() again with same valid inputs while transaction has not yet settled reenter the minting ( Note - bypasses limit check because neither tokensMintedAllowlistAddress or tokensMintedPerAddress has been updated ) and mint passed limit.
• perform this multiple times in a single transaction before transactions settle, minting possibly ALL collection's totalSupply

Proof of Concept

This is mint() code in NextGenCore contract:

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); // @audit-issue
            if (phase == 1) {

tokensMintedAllowlistAddress[_collectionID][_mintingAddress] =
tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] =
tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
        }
    }

_mintProcessing:

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); // @note callback hook
    }

As you can see code would make external call to onERC721Received() function of the account address by calling _safeMint() and the code only sets the values for tokensMintedAllowlistAddress and tokensMintedPerAddress after this call. so code don't follow check-effect-interaction pattern and it's possible to perform reentrancy attack. there could be multiple scenarios that attacker can perform the attack and do damage. e.g:

scenario #1 where attacker bypasses limit and mints possibly ALL collection's totalSupply (as shown above) scenario #2 where attacker could execute a read-only reentrancy on the retrieveTokensAirdroppedPerAddress() function if any integrated art company rely on it's returned value (as shown [here](https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/NextGenCore.sol#L183))

Tools Used

Visual Studio Code

Recommended Mitigation Steps

follow the check-effect-interaction pattern or add a reentrancy guard.

Assessed type

Reentrancy

[thebrittfactor]

For transparency, due to submission issues, the warden provided this submission prior to audit close.

[c4-pre-sort]

141345 marked the issue as primary issue

[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