NextGen - 0x3b's results

Lines of code

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

Vulnerability details

Impact

Whitelisted users can mint before the public mint starts, but they are restricted in the number of NFTs they can mint. However, they will be able to re-enter the mint function and mint as many NFTs as they want.

Proof of Concept

When the mint function is called, users need to verify their order with the Merkle tree by inputting their correct _maxAllowance and tokData. After they are verified, gencore.mint is called to mint their tokens. However, before increasing tokensMintedAllowlistAddress, _mintProcessing is called.

            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] += 1;
            }

This, in turn, mints the user their NFT.

    function _mintProcessing(...) internal {
        tokenData[_mintIndex] = _tokenData;
        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
        tokenIdsToCollectionIds[_mintIndex] = _collectionID;
        _safeMint(_recipient, _mintIndex);
    }

Because it uses safeMint, users are called by the 721 Contract with _checkOnERC721Received. This call can be caught in the attacker's contract, and from there, reentrancy begins. After the mint is done, the attacker can call MinterContract.mint again, and the requirement that checks their max mint limit will pass.

    } else {
         node = keccak256(abi.encodePacked(msg.sender, _maxAllowance, tokData));
         //@audit if we re-enter `retrieveTokensMintedALPerAddress` will return the not yet updated value
         require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
         mintingAddress = msg.sender;
    }

This is because we re-entered before tokensMintedAllowlistAddress had the chance to increase, which means that gencore.retrieveTokensMintedALPerAddress still has the old value.

Due to this reentrancy, whitelisted users can mint more NFTs than they should. This will also have an effect on phase 3 sales, enabling users to mint NFTs before the price has had a chance to update. Overall, this breaks the concept of users being restricted in the number of NFTs they can mint.

Tools Used

Manual review.

Recommended Mitigation Steps

Add nonReentrant in the minting functions.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125

Vulnerability details

Impact

Due to claimAuction and cancelBid both using = in their time equations, bidders will be able to double refunds their bids.

function claimAuction(...) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
require(block.timestamp >= minter.getAuctionEndTime(_tokenid)...);

function cancelBid(...) public {...
require(block.timestamp <= minter.getAuctionEndTime(_tokenid)...);

Proof of Concept

The winner is able to claim the auction at it's exact auction end time.

function claimAuction(...) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
require(block.timestamp >= minter.getAuctionEndTime(_tokenid)...);

However bidders are also able to cancel their bids at exact auction end time.

function cancelBid(...) public {...
require(block.timestamp <= minter.getAuctionEndTime(_tokenid)...);

This means that the winner can claim, and him or some bidder to double refunds their bids. This is done by first using the refund in claimAuction and then canceling his bid with cancelBid.

A few example scenarios occur, where in order to do this you must have some failed (not winning) bids:

1. The winner can make multiple bids and either re-enter here after every not winning bid and refund it, or simply schedule in one TX claimAuction and cancelBid on all his not winning bids.

2. Users can re-enter their failed bids and refunds them.

3. Seller of the NFT can re-enter and claim his failed bids.

4. Users can back-run the claimAuction call, and as long as they are with the same timestamp they will be able to cancelBid.

Tools Used

Manual review

Recommended Mitigation Steps

Even if the re-entracy is fixed the winner would still be able to schedule claimAuction and cancelBid in one TX and double refund his other bids. The main issue is <= which needs to be made < in cancelBid.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L252

Vulnerability details

Impact

As explained by the sponsor, some collections might want to conduct multiple mints on different days. However, due to the way salesOption 3 works, these multiple mints might encounter issues.

Proof of Concept

A collection has completed its first mint, where it minted 500 NFTs. However, the collection consists of 1000 NFTs, so the owner plans to schedule another mint, this time using sales option 3.

The first user's mint will proceed smoothly since timeOfLastMint falls within the previous mint period. However, the second user's mint will fail. The same applies to all other whitelisted users. This issue arises due to the following block:

lastMintDate[col] = collectionPhases[col].allowlistStartTime
                + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));

This calculation extends the allowed time significantly, granting the second minter an allowed time of allowlistStartTime + 1 min * (500-1) = allowlistStartTime + 499 min, which is equivalent to 8 hours and 19 minutes after allowlistStartTime. This enables the second user to mint at 12:19 AM, long after the whitelist has ended and in the middle of the public sale. And if anyone tries to mint his call will revert with underflow error, as timeOfLastMint > block.timestamp.

uint256 tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;

It's worth noting that some collections may disrupt the whitelist, while others could brick the entire mint process, especially if there are more minted NFTs or a longer minting period.

POC

Gits - https://gist.github.com/0x3b33/677f86f30603dfa213541cf764bbc0e8 Add to remappings - contracts/=smart-contracts/ Run it with forge test --match-test test_multipleMints --lib-paths ../smart-contracts

Tools Used

Manual review.

Recommended Mitigation Steps

For this fix I am unable to give any suggestion as big parts of the protocol need to be re-done. I can only point out the root cause of the problem, which is (gencore.viewCirSupply(col) - 1) in the snippet below.

lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L104-L120

Vulnerability details

Impact

Winners will be able to DoS auctions and lock both their NFTs and all of the funds of other bidders. This issue arises because AuctionDemo's claimAuction uses push instead of a pull.

The winner does not need to be a user, it can also be a competitive entity. A competitor against the collection or against NextGen.

Proof of Concept

Once the auction has ended, the winner can invoke claimAuction. And to revert on his _checkOnERC721Received.

The core issue is in this if if, within this for loop, where, if the current bidder is not the winner, their funds are refunded.

for (uint256 i=0; i< auctionInfoData[_tokenid].length; i++) {
    ...
    } else if (auctionInfoData[_tokenid][i].status == true) {
        (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
        emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
    } else {}
}

Example:

1. NFT is scheduled for auction with start of 0.01ETH
2. 50 more bidders bid - 0.011, 0.015... 0.29.
3. Attacker bids last - 0.3ETH (1 wei higher than the bid before him) using a contract that reverts when it receives the NFT.
4. Auction ends.
5. claimAuction reverts as the call made to the attacker contract reverts.

Attacker loses some ETH, however 50 bidders lose their bids (the last one is the same as the attaker) and the NFT will remain stuck in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

Utilize the pull method instead of the push method. This can be achieved by creating another function that allows users to manually withdraw their funds after the auction concludes.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L536

Vulnerability details

Impact

As explained by the docs, several steps can occur during the minting process. However, an airdrop before salesOption 3 can lead to price inflation.

Proof of Concept

Under salesOption 3, getPrice returns:

return collectionPhases[_collectionId].collectionMintCost
                    + ((collectionPhases[_collectionId].collectionMintCost / collectionPhases[_collectionId].rate) * gencore.viewCirSupply(_collectionId));

This is the increased rate based on the NFTs already in circulation. If an airdrop occurs before a mint with salesOption 3, the price will be much higher than intended.

Example:

With the current three steps, after the airdrop, salesOption 3 should start at 1 ETH and gradually increase to 2 ETH. Afterward, it should mint at a constant rate of 2 ETH.

However, when sales option 3 starts, getPrice will return 3 ETH instead of 1 ETH. This will cause the initial users to pay an inflated price, which was not intended by the owner and can harm their reputation. It's also unfair to the users, as these so-called special (whitelisted) users will pay increased prices.

POC

Gist - https://gist.github.com/0x3b33/558b919a57101e7a0942e557a464078a Add to remappings - contracts/=smart-contracts/ Run it with forge test --match-test test_airdrop --lib-paths ../smart-contracts

Tools Used

Manual review.

Recommended Mitigation Steps

You can implement a mapping with the airdropped NFTs and deduct this value from gencore.viewCirSupply(_collectionId) to avoid disrupting the minting process.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L553

Vulnerability details

Impact

getPrice in salesOption 2 can round down to the lower barrier, effectively skipping the last time period. In simpler terms, if the price is scheduled to decrease over 4 hours, it will decrease for 2.999... hours and then jump to the price at the end of the 4th hour.

Proof of Concept

getPrice uses this else-if calculation to determine whether it should return the price as an equation or just the final price.

if (((collectionMintCost - collectionEndMintCost) / rate) > tDiff) {
    price = collectionPhases[_collectionId].collectionMintCost - (tDiff * collectionPhases[_collectionId].rate);
} else {
    price = collectionPhases[_collectionId].collectionEndMintCost;
}

The if statement's method of division by rate is incorrect, as it results in rounding down and incorrect price movements. This can cause the price to jump over the last time period, effectively decreasing faster than intended.

Example:

Hour 0 to 1 - 0.49 ETH Hour 1 to 2 - 0.39 ETH - as it crosses the 1st hour, the price becomes 0.39 ETH Hour 2 to 3 - 0.29 ETH - same here It should go from 3 to 4 to 0.19 ETH, but that step is missed, and it goes directly to 0.1 ETH - the final price. Hour 3 to 4 - 0.10 ETH - the final price

Math

The equation for calculating tDiff using the provided variables is:

We want the crossover when hour 2 switches to 3, which is when (block.timestamp - collectionPhases[_collectionId].allowlistStartTime) = 3 h or 10800 sec.

We plug in tDiff and the other variables into the if.

if (((collectionPhases[_collectionId].collectionMintCost - collectionPhases[_collectionId].collectionEndMintCost) 
                / (collectionPhases[_collectionId].rate)) > tDiff) {

We obtain 3 as the answer, which is due to rounding down, as we divide 0.39 by 0.1. Solidity only works with whole numbers, causing the if to fail as 3 > 3 is false. This leads to entering the else statement where the last price of 0.1 ETH is returned, effectively missing one step of the 4-hour decrease.

PoC

Gist - https://gist.github.com/0x3b33/ab8a384f9979c4a9b7c4777be00a78de. Add contracts/=smart-contracts/ to mappings and run it with forge test --match-test test_changeProof --lib-paths ../smart-contracts -vv.

Logs:

[PASS] test_changeProof() (gas: 745699)
Logs:
  4900000000000000000
  3900000000000000000
  2900000000000000000
  1000000000000000000

Tools Used

Manual review.

Recommended Mitigation Steps

A simple yet effective suggestion is to add = in the if.

-  if (...) > tDiff){ 
+  if (...) >= tDiff){ 

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L58

Vulnerability details

Impact

claimAuction and participateToAuction both use <=, which can result in bidders accidentally bidding immediately after the auction ends. This can lead to their bids becoming stuck in the contract.

function participateToAuction(...) public {...
require(block.timestamp <= minter.getAuctionEndTime(_tokenid)...);

function claimAuction(...) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
require(block.timestamp >= minter.getAuctionEndTime(_tokenid)...);

Proof of Concept

Shortly after the winner of an auction ends it by calling claimAuction, a bidder's transaction can execute and bid for the just-ended auction. This is possible because bidders can bid at the exact closing time, which should not be possible.

function participateToAuction(uint256 _tokenid) public payable {
    require(.. && block.timestamp <= minter.getAuctionEndTime(_tokenid) && ...);

Example scenario:

1. Bob sends a bid a few seconds before the closing time.
2. The bid transaction remains in the mempool.
3. The winner closes the auction.
4. Right after claimAuction, Bob's participateToAuction gets executed.

The winner (which should have been the last bidder - Bob, but it's the guy before him) receives his NFT, and all of the users get refunded on their bids, except for Bob. His bid remains stuck in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

Change from <= to < in participateToAuction to make it impossible for bidders to bid after an auction has ended.

function participateToAuction(uint256 _tokenid) public payable {
-   require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
+   require(msg.value > returnHighestBid(_tokenid) && block.timestamp < minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
    auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
    auctionInfoData[_tokenid].push(newBid);
}

Assessed type

Error

[L-01] Changing gencore will effect the mints

Admins are able to change the gencore used by the minter with the help of updateCoreContract. However such change will effect any minting that are held, as the information inside gencore (whitelists, mint counts, cirSupply and so on) is used in the minting process.

Example:

1. Mint on sales option 3 happens. Start price of 1 ETH, 100 minted with 0.01 ETN increase => 2ETH current price.
2. Admin changes gencore
3. The same NFT is scheduled for another sales option 3.
4. It's price will start again from 1ETH , as gencore.viewCirSupply(_collectionId) will return 0.

[L-02] getPrice uses different time checks from both mint and burnToMint

While mint and burnToMint use >= and <= phase 2 under getPrice uses only > and <.

        } else if (
            collectionPhases[_collectionId].salesOption == 2
                && block.timestamp > collectionPhases[_collectionId].allowlistStartTime 
                && block.timestamp < collectionPhases[_collectionId].publicEndTime
        ) {

This will lead to a few inconsistency. Example is when users try and mint at block.timestamp == publicEndTime Which is possible in both mint and burnToMint, however instead of giving the decreased price,it will go into the else and return the normal collectionMintCost

[L-03] People can set their hash to 0x00

When minting tokens _mintProcessing which calls randomizer.calculateTokenHash(...), afterwards the randomizer calls setTokenHash to set the hash for this ID . When ChainLink is used as the randomizer the call is returned after a few block. This call can be made to revert on the return call, by simple providing enough gas for the request to reach ChainLink, but not enough for the return call to be successful. If the return call runs out of gas setTokenHash will never be called, thus leaving tokenToHash[tokenID] == 0. This will not cause any damage to the protocol, but may render some NFTs useless, or the opposite - extremely rare, due to their nature.

[L-04] If randomizer is changed while a mint is happening token hash will be 0

addRandomizer is used to change the randomizer for its collection. This function can be called at any time by an admin of this collection. When it is called, a new randomizer will be put in place. However, if a call is made to the old randomizer, its return function setTokenHash will take some time to execute. This means that the NFTs that were minted just before the change will have a hash of 0, rendering them useless.

Example:

1. A user mints an NFT - _mintProcessing calls randomizer.calculateTokenHash, and VRF will take 3 blocks to respond.
2. An admin changes the randomizer.
3. Chainlink responds; however, setTokenHash reverts, as Chainlink is not the current randomizer.

The NFTs will have a hash of 0.

[L-05] Emergency functions should not perform external calls

emergencyWithdraw as the name suggest does a emergent withdraw. Emergent means that most likely a hack has happened and the protocol is looking to rescue the funds. However the function calls adminsContract.owner() which in direct contradiction with the emergency state. This may cause an issue if adminsContract is compromised. I suggest to have this address in storage or to be an input to the function.

[L-06] Users can mint a few NFTs at once with sales option 3

When using sales option 3, mint sets lastMintDate and timeOfLastMint. However it does not set them to the current mint time, but to the expected unlock time, unlike the docs periodic Sale Example scheme.

Example: Mint with 10 min time between mints.

1. Mint starts at 24/07/2023 14:00
2. First user mints at 24/07/2023 14:03
3. The second user is able to mint at 24/07/2023 14:10