NextGen - xAriextz's results

Lines of code

Mint function in minter contract: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196-L254 Mint function in core contract: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L189-L200

Vulnerability details

Bug Description

When minting NFTs, users will using the mint function. This function will mint a NFT using the _safeMint function. The problem is that this mint will be done before crucial variables are updated.

Mint before checking the salesOption == 3 conditions are fulfilled in MinterContract.sol:

        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);
        }
        collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
        // control mechanism for sale option 3
        if (collectionPhases[col].salesOption == 3) {
            uint timeOfLastMint;
            if (lastMintDate[col] == 0) {
                // for public sale set the allowlist the same time as publicsale
                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod;
            } else {
                timeOfLastMint =  lastMintDate[col];
            }
            // uint calculates if period has passed in order to allow minting
            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
            // users are able to mint after a day passes
            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));
        }

Mint before updating tokensMintedAllowlistAddress and tokensMintedPerAddress in NextGenCore.sol mint.

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

The vulnerability arises during the execution of the _safeMint function, which involves the usage of _checkOnERC721Received. Exploiting this, attackers can repeatedly mint NFTs by employing a smart contract that reenters the function upon receiving the call of _checkOnERC721Received. The minting process occurs prior to updating crucial variables such as tokensMintedAllowlistAddress and tokensMintedPerAddress, as well as implementing the control mechanism for sale option 3. This oversight allows attackers to mint the entire collection without proper limitations.

Impact

The reentrancy vulnerability exposes the system to significant risks, allowing attackers to exploit the following critical issues:

• Bypassing the mint amount limit within the allowlist (tokensMintedAllowlistAddress)
• Bypassing the mint amount limit during public sales (tokensMintedPerAddress)
• Bypassing the control mechanism for sale option 3 (salesOption == 3), enabling unlimited NFT minting within a specific timeframe

In some cases, attackers can also drain protocol's funds.

Proof of Concept

Example Illustrating Potential Fund Loss for the Protocol

Scenario

• A collection offers a 0 ETH minting price during the allowlist period.
• The collection incorporates Chainlink's VRF, invoking the calculateTokenHash function in RandomizerVRF.sol during NFT minting. This involves the usage of Link tokens to generate randomness.
• An attacker is permitted to mint only 1 token within the allowlist period.

Attack Path

1. The attacker deploys a malicious smart contract that implements IERC721Receiver, initiating reentry into the mint function upon NFT receipt using the onERC721Received function.
2. The attacker triggers the mint function using the malicious smart contract.
3. The smart contract repeatedly reenters the mint function through onERC721Received.
4. Since tokensMintedAllowlistAddress remains unaltered, the mint amount for the allowlist check is bypassed.
5. The malicious contract successfully mints the entire collection.

This vulnerability results in the attacker being able to mint the whole collection for free and the protocol suffering substantial financial losses, considering the requirement for a significant number of Link tokens to generate the unique hash for all the NFTs. While potential gas limit issues might arise, this scenario effectively demonstrates the concept. Additionally, this vulnerability can be exploited to bypass the mint amount limits during public sales and the control mechanism for sale option 3.

Tools Used

Manual review

Recommended Mitigation Steps

To address this vulnerability, it is imperative to update tokensMintedAllowlistAddress and tokensMintedPerAddress before executing the _mintProcessing function within NextGenCore.sol's mint function. Furthermore, implement the control mechanism for sale option 3 in the MinterContract.sol before invoking the mint function in NextGenCore.sol.

Assessed type

Reentrancy

Lines of code

Function for participating in an auction: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L57-L61 Function for cancelling a bid: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L124-L130

Vulnerability details

Bug Description

• Users participate in auctions via the participateToAuction function, where making the currently highest bid is necessary for participation.

        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);

• Bids can be removed by invoking the cancelBid function.

The problem is that bids can be cancelled until the last second of the auction, with any kind of penalization.

Impact

Users can manipulate auctions by placing big fake bids, subsequently canceling them to secure the auction victory at minimal cost.

Proof of Concept

Attack Path

1. Alice initiates the auction process, placing a bid of 1 wei through participateToAuction and becoming the highest bidder.
2. Alice places an exorbitant bid of, for instance, 100 ETH, creating the impression of an unattainable bid for other users.
3. Observing the seemingly insurmountable bid, other users refrain from bidding, assuming they cannot surpass the 100 ETH bid.
4. Just before the auction ends, Alice cancels her bid using cancelBid but still remains being the highest bidder.
5. Alice claims the auction using claimAuction, securing the NFT for a mere 1 wei.

It's worth noting that this tactic can also be employed by collection teams to manipulate auctions, creating a facade of high demand or interest by placing fake bids and retracting them if necessary to ensure a favorable outcome.

Tools Used

Manual review.

Recommended Mitigation Steps

To address this vulnerability, a comprehensive overhaul of the bid and auction system is advisable, which might involve the removal of the ability to cancel a bid entirely. Another option can be just refunding a % of the bid to users who decide to cancel their bids.

Assessed type

Timing

Lines of code

Function for claiming an auction: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104-L120 Function for cancelling a bid: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L124-L130

Vulnerability details

Impact

This vulnerability has the potential for varying impacts, given the numerous ways it can be exploited. The one with the highest impact: a malicious user could exploit the vulnerability to win any auction at no cost and steal all of the funds from the contract.

Proof of Concept

In the claimAuction function, users receive refunds if their bids fail to secure the auction. However, even after receiving their refunds, users can still cancel their bids due to the failure to update the bid status during the refund process.

As we can see, claimAuction can be called when block.timestamp >= minter.getAuctionEndTime, and cancelBid can be called when block.timestamp <= minter.getAuctionEndTime. This means that when block.timestamp == minter.getAuctionEndTime we can call both of them.

This arises multiple different issues with multiple ways of exploiting it. Here I will explain the one with the highest impact that I have found:

Attack Scenario

1. AuctionA progresses with multiple bids, totaling 201 ETH.
2. Simultaneously, AuctionB features a highest bid of 5 ETH.
3. Seizing an opportunity in the last moments of AuctionB, the attacker: 3.1. Places a bid of 100 ETH. 3.2. Places a bid of 101 ETH.
4. The attacker claims the auction just at block.timestamp == minter.getAuctionEndTime, with 101 ETH allocated to the NFT owner and a refund of 100 ETH for the unsuccessful bid. Additionally, all other AuctionB participants receive refunds. 4.1. Notably, the contract now holds 201 ETH from AuctionA bids, and the status of the attacker's bids remains true.
5. The attacker cancels both bids just after, while block.timestamp == minter.getAuctionEndTime condition still holds, leveraging the unaltered status during the claim process to execute the cancellation.

Through this attack, the attacker succeeds in securing the NFT and steals all the funds from the contract, completely draining it. Additionally, similar attacks allow for the theft of various fund amounts, depending on the specific bid configurations. For instance, an attacker could deploy multiple bids of 5 ETH, securing the auction with a smaller bid and stealing even more funds from AuctionA.

Tools Used

Manual review.

Recommended Mitigation Steps

Implement an update mechanism to set the bid status to false during the claim process for all the bids of the corresponding auction. Also, change the check in claimAuction from block.timestamp >= minter.getAuctionEndTime to block.timestamp > minter.getAuctionEndTime.

Assessed type

Invalid Validation

Lines of code

mint function: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196-L254 check in mint function to assure we are in the public sale window: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L221 function for getting the price: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L530-L570 check in the getPrice to assure we are in the public sale window: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L540

Vulnerability details

Impact

Due to an error in the getPrices() function, users might purchase NFTs at the collectionMintCost instead of the intended decreased rate, leading to potential financial losses depending on the specific linear descending sale model configuration.

Proof of Concept

This vulnerability can have a significant impact in instances where a collection utilizes a linear descending sale model with a substantial disparity between the starting and ending prices. Consider the following scenario to understand the potential impact and how users could incur financial losses.

The problem arises because in getPrice we don't take into account the last second of the public sale for the calculation of the price of the NFT when the sale model is of type 2 (descending sale model). This will make the calculation wrong in that specific case, returning a wrong value.

Context

• A collection adopts a Linear Descending Sale model with the following parameters:
  • Starting minting cost: 100 ETH (collectionMintCost)
  • Ending mint cost: 1 ETH (collectionEndMintCost)
  • The public sale is coming to an end, so the current cost is 1 ETH already

Illustrative Situation Affecting Users

1. Bob attempts to purchase an NFT from the collection during the final second of the public sale. While he should be paying 1 ETH, the reality is different: 1.1. Bob invokes the mint function at block.timestamp == collectionPhases[_collectionId].publicEndTime. 1.2. Due to the error in getPrice function, the function will return the collectionMintCost. This happens because the conditional block.timestamp < collectionPhases[_collectionId].publicEndTime is not met in the else if, which will make the call jump to the else. This shouldn't happen since we are in the public sale time of a Descending Sale model, not a Fixed Price Sale. 1.3. As a result, Bob ends up paying 100 ETH for the NFT, significantly more than the anticipated 1 ETH.

Tools Used

Manual review.

Recommended Mitigation Steps

Rectify the else if condition in the getPrices function from block.timestamp < collectionPhases[_collectionId].publicEndTime to block.timestamp <= collectionPhases[_collectionId].publicEndTime to ensure the accurate calculation of NFT prices during the sale.

Assessed type

Timing

Lines of code

Function for claiming an auction: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104-L120 Line where the highest bid is payed to the wrong address: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L113

Vulnerability details

Impact

The AuctionDemo.sol contract erroneously sends the highest bid to the wrong address during the auction claim process. Consequently, the designated collection owners do not receive the expected ETH upon auction completion.

Proof of Concept

As indicated in the Main invariants section of the contest page: "The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded." Link to the contest page

In the function responsible for auction claiming, the smart contract sends the funds to the contract owner rather than the owner of the token: (bool success, ) = payable(owner()).call{value: highestBid}("");

This results in the owner of the token not receiving the expected ETH from the auction, potentially leading to a loss of funds and possible complications depending on the identity of the contract owner.

Tools Used

Manual review.

Recommended Mitigation Steps

To correct this issue, it is advisable to replace the line: (bool success, ) = payable(owner()).call{value: highestBid}(""); with (bool success, ) = payable(ownerOfToken).call{value: highestBid}(""); to ensure that the funds are accurately directed to the owner of the token.

Assessed type

ETH-Transfer

Lines of code

Function for signing a collection: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L257-L263 Function for setting collection's artist address: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L147-L166

Vulnerability details

Bug Description

The setCollectionData() function allows the owner of a collection to set the data of the collection. As we can see , the collectionArtistAddress is only able to be set in the beginning and won't be able to be changed if the collection is already signed:

else if (artistSigned[_collectionID] == false) {
            collectionAdditionalData[_collectionID].collectionArtistAddress = _collectionArtistAddress;

The artistSignature() function allows an artist of a collection to sign it. This will increase the credibility in the collection since the artist is assuring that he is in fact the artist by signing it with his wallet. Once its signed, the wallet shouldn't be able to be changed:

    function artistSignature(uint256 _collectionID, string memory _signature) public {
        require(msg.sender == collectionAdditionalData[_collectionID].collectionArtistAddress, "Only artist");
        require(artistSigned[_collectionID] == false, "Already Signed");
        artistsSignatures[_collectionID] = _signature;
        artistSigned[_collectionID] = true;
    }

The problem is that a collection owner can trick this checks, and give a false impression of the collection being signed by anyone he wants.

Impact

The vulnerability allows for the bypass of the verification checks in NextGenCore.sol that assure the artist of a collection and its signature. Consequently, malevolent actors can manipulate the appearance of a collection, misleading users into believing that a specific address has signed the collection when, in reality, it has not. Even if collection admins are trusted, there are checks for preventing this scenario, so bypassing them is still an issue.

Proof of Concept

Attack Scenario

1. An administrator of a collection calls setCollectionData, assigning their own address as the collectionArtistAddress and setting collectionTotalSupply to 0.
2. Subsequently, the administrator employs their personal wallet to execute artistSignature.
3. The administrator once again calls setCollectionData, this time specifying a famous and trusted address, such as Vitalik's address, as the collectionArtistAddress. Additionally, they configure the required _collectionTotalSupply. 3.1. This is possible because the collectionTotalSupply == 0, but the collection has already been signed, which is not controlled with the checks.

Consequently, the collection falsely appears to have been signed by Vitalik's address, when in actuality, it has not. This deceptive practice can be utilized to manipulate users into purchasing the NFTs from the misrepresented collection.

Even if collections admins are trusted, existing checks are in place to prevent such occurrences. However, this attack path effectively bypasses these security measures. Hence, I consider the severity level for this vulnerability to be a medium, rather than high.

Tools Used

Manual review

Recommended Mitigation Steps

It is crucial to implement a validation process ensuring that the _collectionTotalSupply is always greater than 0 within the setCollectionData function to mitigate this vulnerability effectively.

Assessed type

Invalid Validation

Lines of code

Timestamp check when bidding: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L58 Timestamp check when claiming auction: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L105

Vulnerability details

Bug Description

• Users participate in auctions through the participateToAuction function, which permits bidding until block.timestamp <= minter.getAuctionEndTime():

        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);

• Users claim auctions using the claimAuction function, which allows claiming until block.timestamp >= minter.getAuctionEndTime().

        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

• This means that bidding and claiming an auction will be possible at the same time for a certain timestamp.

Impact

In auctions where users place last-second bids, there is a potential for these users to lose the auction to the current highest bidder who claims it just before them, and lose the funds they used to bid.

Proof of Concept

In such auctions, it is common practice for bidders to place their bids at the last moment to prevent other bidders from reacting and outbidding them.

Example Illustrating the Issue

• Alice bids 1 ETH in an auction.
• Bob intends to win the auction by bidding 2 ETH in the last second, using the participateToAuction function with a msg.value of 2 ETH, just when block.timestamp == minter.getAuctionEndTime.
• Alice, either intentionally or unintentionally, frontruns Bob's transaction, swiftly calling claimAuction when block.timestamp == minter.getAuctionEndTime and receiving the NFT as the current highest bidder.
• Although Bob's transaction is successful, he fails to secure the auction victory, resulting in an unfair outcome. Also, he will lose his 2 ETH because he won't get refunded, because his bid wasn't added to auctionInfoData array yet. Additionally, note there is no way of withdrawing Bob's funds from the contract even for the admin's, resulting in these funds being stuck in the contract forever.

Alice becomes the winner, while Bob, who should have won, loses the auction, leading to an inequitable auction process. Also, Bob will lose the funds he used to bid.

Another problem that arises from the same check is that if an admin calls claimAuction in block.timestamp == minter.getAuctionEndTime(_tokenid) and a user decides to cancel the bid at the same time, he won't be able to do it.

Tools Used

Manual review.

Recommended Mitigation Steps

Modify the check in the claimAuction function from block.timestamp >= minter.getAuctionEndTime(_tokenid) to block.timestamp > minter.getAuctionEndTime(_tokenid).

Assessed type

Invalid Validation

Auction bids don't need to be a % percentage bigger than the previous one

Link: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L58 In AuctionDemo.sol we can place bids in an auction by calling participateToAuction. This function requires that msg.value > returnHighestBid. This means that any new bids which is bigger than the previous one will be accepted, even if the bid is only 1 wei bigger. This approach doesn't seem to lead to big problems, but in my opinion this is an unfair system for the users. I think that for being able to place a bid you should consider forcing the new bid to be at least a % bigger than the current biggest bid. For example: if the biggest bid right now is 1 ETH, for placing a bid users need to send at least 1.01 ETH, making it 1% bigger than the last bid.

returnHighestBidder doesn't update highBid

Link to returnHighestBidder: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L87-L100 This function is for getting the highest bidder in a certain auction. As we can see, it iterates through the array and checks if the bid done is bigger than highBid. This check will always pass since highBid is not updated and it's value is always 0. This doesn't represent bigger problems since the array is already sorted and the status is checked for getting the index with the biggest bid.

Auction won't be claimable if the owner of the NFT doesn't approve the AuctionDemo.sol contract

Link: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L112 When an auction is claimed using claimAuction the NFT is transferred from the owner of the NFT directly to the highest bidder. Even if the address that owns that NFT is trusted, I think a better approach would be sending the NFT directly to the AuctionDemo.sol contract when using mintAndAuction in MinterContract.sol, instead of sending it to a trusted address.

No check of call success in ETH transfers

Some examples:

• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L128
• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L139
• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L113
• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L116

Even if this issue was already in the automated findings, I think it's worth explaining the impact this could have in this specific context.

Users get their ETH refunded when not winning an auction. Also, they can cancel their bids before an auction is finished and get their ETH refunded. Not checking that the ETH was correctly transferred from AuctionDemo.sol to the users will result in this ETH getting stuck in the contract, since there is no possibility of withdrawing it in any other way. This happens because even if the ETH transfer doesn't work the transaction will still succeed. In the case of cancelling a bid, the status of the bid is still updated and users won't be able to call the function again. In the case of claiming an auction, we have a similar case, since auctionClaim will still be updated.

I want to remark that there is no way of getting the stuck ETH back even for the admins, because there are no functions like an emergencyWithdraw or similar. This will result in a loss of funds for the users, having their ETH stuck in the contract.

For fixing this issue we should check that success is true after making the ETH transfer using the call.

The maxCollectionPurchases for the collections can be bypassed by minting with another wallet

Link: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L224 This check isn't really doing anything, since the same user could just use another wallet for minting more NFTs. There is no possible way of checking how many NFTs each user buys. In my opinion, this check will hurt the users and the protocol. The reason is that "normal" and unexperienced users wouldn't think about changing wallets for buying more NFTs, and more experienced users would take advantage of this. If there is a maxCollectionPurchases == 1 in a collection with 1000 NFTs, users would think that there are 1000 holders. This is in fact not true, since an experiences user could use a script or manually buy 500 NFTs by simply using different wallets. In my opinion it is an unnecessary check, because you will only limit the unexperienced users.

No check if too much msg.value is sent when minting NFTs

Example:

• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L233

Even if this issue was already in the automated findings, I think it's worth explaining the impact this could have in this specific context.

As there is no function for withdrawing excess funds in MinterContract.sol, all these funds will get stuck forever in the contract. I would advise to change the check from msg.value >= getPrice() to msg.value == getPrice() to prevent any lose of funds.

Use Basis Points for percentages

Links:

• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L370-L371
• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L429-L433

In order to have better precision when calculating the funds to be sent to the artists, I would suggest using Basis Points for assigning the percentages. For instance, 100% would be represented as 10_000. This will prevent possible rounding errors and less funds being distributed. This won't be a problem when there is a lot of ETH to be sent, but for example if there is only 1 ETH to be distributed across the 5 wallets, with some of them having very low percentages, this could be a bigger issue.

A collection's randomizer can be changed even if the collections is frozen

Link: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L170-L174 Even if a collection has been frozen, it's randomizer can still be changed. Even if this function would be called by a trusted role, I think it shouldn't be possible. Freezing a collection gives users the certainty that collection's importan info, etc. won't be changed. Nonetheless, this will entail users' getting different token hashes than they thought.

A collection's randomizer can be changed even if the collections is frozen

Link: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L170-L174 Even if a collection has been frozen, it's randomizer can still be changed. Even if this function would be called by a trusted role, I think it shouldn't be possible. Freezing a collection gives users the certainty that collection's importan info, etc. won't be changed. Nonetheless, this will entail users' getting different token hashes than they thought.

Request confirmations within Chainlink's VRFv2 should be bigger

Link: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L28 Even if the value returned from Chainlink's VRFv2 won't have a big impact in the particular context of this project, it is advised to use a value which is bigger than the common length of the block reorgs that can happen in the particular chain. As this project is going to be deployed only in Ethereum Mainnet, changing it to requestConfirmations = 10 would be more than enough. Reorgs have already happened in Ethereum. One of the biggest ones was 7 blocks, not so long ago.