Adversary can reenter mint to bypass max allowance.

[c4-submissions]

Lines of code

github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L189-L200

Vulnerability details

Description

MinterContract.mint calls NextGenCore.mint, which variables that accounts the amount of tokens each user minted is changed only after _mintProcessing, that has a callback in _safeMint. Because of that, attackers can reenter MinterContract.mint before tokensMintedAllowlistAddress and tokensMintedPerAddress are incremented, allowing them to bypass the max allowance check that limits the amount of tokens an user can mint for public or allowlist phase of a collection.

Proof of Concept

1. Let's supose the following values:

• tokensMintedPerAddress[_collectionID][attacker] = 2 (retrieved via retrieveTokensMintedPublicPerAddress)
• collectionAdditionalData[_collectionID].maxCollectionPurchases = 3 (retrieved via viewMaxAllowance)

2. Adversary calls MinterContract.mint to mint one token. Logic that executes for public phase (and no delegation, sales mode not 3):

        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
            phase = 2;
            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
            require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
            mintingAddress = msg.sender;
            tokData = '"public"';
        } else {
            revert("No minting");
        }
        uint256 collectionTokenMintIndex;
        collectionTokenMintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col) + _numberOfTokens - 1;
        require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(col), "No supply");
        require(msg.value >= (getPrice(col) * _numberOfTokens), "Wrong ETH");
        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 following line checks if user didn't overpass the limit of mints per collection, comparing tokensMintedPerAddress[_collectionID][attacker] plus amount of tokens to mint and collectionAdditionalData[_collectionID].maxCollectionPurchases.

require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

• This is 3 <= 3, which is true, so no reverts.
• Then, subcall gencore.mint is called:

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

3. The function has the subcall _mintProcessing, which has _safeMint, a function with ERC721.onERC721Received callback:

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

• However, as seen in his caller, the checks effects interactions pattern isn't followed, so the callback happens before the state change:

    function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
	    collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
		 // ...
            _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;
            }
        }
    }

4. In the middle of ERC721.onERC721Received callback, MinterContract.mint is called again:

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
		    uint256 counter;
		    uint256 reenter_x_times;
		    
		    if (counter != reenter_x_times){
			    target.mint{value: NFT_PRICE}(); 
			    counter++;
			}
			// ...
	    }

5. Let's see how the reenter will interact with key lines of the MinterContract.mint:

• The check of max mints per address will still return true, because tokensMintedAllowlistAddress[_collectionID][attacker] isn't updated during the reentrancy, making the check 3 <= 3 again:

require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

• The code will not mint an already used mintIndex, because it relies on collectionCirculationSupply, which is updated before the callback.

6. Only after the tokens are minted, the following lines of code finally are finally reach:

            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
        }
    }

7. So, considering the attacker reentered only one time, the following unexpected outcome happened:

• tokensMintedPerAddress[_collectionID][attacker] = 4
• collectionAdditionalData[_collectionID].maxCollectionPurchases = 3
  Attacker successfully minted more tokens than he could. The same exploit could be executed for allowlist, since tokensMintedAllowlistAddress is also updated only after the callback.

Impact

Attacker can bypass the maximum purchases for allowlist or public phases, effectively being able to use mint more times than a normal user, which is specially dangerous for the phase 1.

1. Likelihood: High. This attack doesn't have any special conditions to be possible and is easily spotted.
2. Impact: Medium. It's unintended and an invariance breaking. Although it's low impact for public phase (user can just use differents EOA for max allowance bypassing), the fact that can be also done for allowlist phase (which can't be bypassed via different EOA's and has potentially higher price speculation for NFT's) rises it's impact to medium.

• Risk: Medium (High Likelihood + Medium Impact)

Tools Used

Manual Review

Recommended Mitigation Steps

Use nonReentrant modifier from ReentrancyGuard.sol and follow the Checks-Effects-Interactions pattern.

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #2039

[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

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)