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