Attacker can reenter to mint all the collection supply

Here is a very popular NFT problem that was found in the Nextgen protocol over a year ago. Where do you think this bug is hiding?

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

An attacker can reenter the MinterContract::mint function and mint the entire collection supply.

The vulnerability stems from the absence of the Check Effects Interactions pattern. As seen in the code, NextGenCore::mint update the tokensMintedAllowlistAddress and tokensMintedPerAddress after making an external call:

    // Minting logic is here
    if (phase == 1) {
        tokensMintedAllowlistAddress[_collectionID][_mintingAddress]++;
    } else {
        tokensMintedPerAddress[_collectionID][_mintingAddress]++;
    }

Exploitation Steps:
- Attacker calls MinterContract::mint with a malicious contract as the receiver.
- The malicious contract executes a crafted onERC721Received().
- MinterContract::mint invokes NextGenCore::mint, which uses _safeMint() internally.
- _safeMint() calls _recipient.onERC721Received(), leading to the minting of the complete collection supply.

It is recommended to use the Check Effects Interactions pattern to mitigate the problem: do the minting after the checks.

    if (phase == 1) {
        tokensMintedAllowlistAddress[_collectionID][_mintingAddress]++;
    } else {
        tokensMintedPerAddress[_collectionID][_mintingAddress]++;
    }
    // Minting logic should be here