NextGen - Kose's results

Lines of code

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

Vulnerability details

Impact

Users can mint more than they are allowed hence one of the main invariant of the protocol is broken.

Proof of Concept

Although this is also true for allowlist minting, let's focus on public minting and show the way that this attack happens: 0- Let's assume users are allowed to mint 1 token for simplicity. 1- User calls mint function in MinterContract that is in phase 2 and tries to mint an NFT:

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

As we can see this part will check : a- User is not trying to mint more than he is allowed in one mint call b- User is not trying to mint more than he is allowed considering his old mints Because this is users first mint, it will pass these statements and then mint function in gencore will be 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;
            }
        }
    }

then _mintProcessing will be called after adding 1 to circulation supply and checking if there is enough supply:

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

Which then mint token to the recipient via _safeMint. 2- The problem rises from _safeMint, which hands over the control to the recipient contract in order to check if it can receive NFT via _checkOnERC721Received function as shown below:

    function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }

In this function call, the control is handed to receiving contract as shown below:

    function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;

Attacker can implement "onERC721Received" function such that it returns the expected selector (hence don't revert) but also calls the mint() function from MinterContract (Re-entrancy) 3 - mint() function in the MİnterContract will again check: a- User is not trying to mint more than he is allowed in one mint call b- User is not trying to mint more than he is allowed considering his old mints a will pass since the attack will try to mint one token. b also will pass because we didn't increased the tokensMintedPerAddress yet, it is increased after _mintProcessing is finished in mint() function from NextGenCore as we can see below:

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

Hence user will pass both checks and be able to mint again and again. Only after the last mint "tokensMintedPerAddress" will start to increase and because of stacking multiple mints, it will increase more than 1. But since only check for this variable has already passed in MinterContract and there is no further check in the current stack. User will leave with more NFT's than he is allowed.

Tools Used

Manual Review

Recommended Mitigation Steps

Follow Checks Effects Interactions pattern and increase the tokensMintedPerAddress(Effect) before _mintProcessing(Interaction), and also think about using re-entrancy guard since there are so many external calls in the protocol.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

It is possible to either re-enter via receive function or back-run the claimAuction function in order to steal funds from auction contract.

Proof of Concept

Both claimAuction and cancelBid function is callable when block.timestamp is equal to AuctionEndTime as shown below:

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){ 
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
...
function cancelBid(uint256 _tokenid, uint256 index) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

Hence it is possible to steal funds from contract with 2 different attack vectors: 1- Participant(Smart Contract) implement a receive function such that when he gets the refund amount he also re-enters to cancelBid function and cancel his bid hence get the same amount again. 2- Participant(EOA) saw the claimAuction call in mempool and back-runs it in the same block via cancelBid function and again get the same amount.

It has two impact: 1- User doubled their money via stealing fund from contract. 2- In the future when there is only one token in auction because of lack of funds, claimAuction will fail and the last NFT in the contract will be locked with its corresponding participant funds.

Tools Used

Manual Review

Recommended Mitigation Steps

There are multiple problems such as not changing auctionstatus to false in claimAuction or not having re-entrant modifier. But the one thing that can solve all is changing require statement of cancelBid to:

require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

This way it won't be callable in the AuctionEndTime.

Assessed type

Reentrancy

1. General Overview

The Nextgen Protocol allows artists to generate and publish NFTs using generative scripts on the blockchain. It offers various minting features like airdropping and mintpassing, alongside diverse sales models and auctions. This protocol isn't just beneficial for Web3 artists; it can also aid organizations, such as universities and companies, in creating blockchain-based certificates. While the protocol's primary use might not be for creating certificates, it is one of them, and bringing these organizations onto the blockchain is a significant achievement. However, the protocol's codebase is highly centralized and susceptible to various admin-related issues, including trust and admin errors. This response will address several issues related to admin functions, as it's essential to consider the protocol's various uses. Even if Nextgen admins are expected to be experienced in Web3 and less likely to make mistakes, other protocol deployers may not have the same level of expertise.

2. Architecture Feedback / Improvements

2.1 Consider Changing Time Related Variable Declaration System

Time-related variables like _allowlistStartTime and endTime are based on block.timestamp. Therefore, declaring these variables requires adding block.timestamp and the expected time period for allowlisting. Instead, passing a time period for these variables as an argument and creating variables using block.timestamp and the time period within a function could simplify the process and make it more understandable.

2.2 Consider Decreasing The Number of Mappings

The NextGenCore contract currently includes 21 mappings, all linking collectionID's to other variables. Creating this many redundancy-prone storage variables isn't optimal. It's more efficient to create one variable connecting collectionID's to all other variables. Having this many redundant variables could lead to the creation of parallel data structures (tracking the same thing), which is considered risky. It's better to improve the contract's storage layout in this manner.

2.3 Consider Adding Checks for Edge Cases

Currently, nearly all admin-related functions can be called with arbitrary arguments. Given that some protocol admins may not be familiar with the space (use case where universities and companies deploy the codebase), it's advisable to limit expected inputs and check for edge cases. While not critical, it's possible to change some state variables to the same state, which can also be prevented with this approach.

2.4 Consider Following CEI Pattern and Usage of non-Reentrant Modifier

The protocol does not currently use the non-Reentrant modifier, and many functions do not follow the CEI pattern. During the codebase review, several reentrancy attacks were identified and reported. It's highly likely that the protocol has more reentrancy attack vectors that weren't detected in the limited review time, and more could emerge after the audit and mitigation processes are completed. It's crucial to understand that fewer attack vectors mean more secure code. With so many potential reentrancy attacks, it's risky to assume that all attacks have been identified in this contest. ###2.5 Consider Refunding Excess Ether Sent During Mint

During the minting process, the check verifies if the msg.value is greater than or equal to the NFT's price. However, any excess value is not refunded to the user. For improved user experience and satisfaction, it's recommended to implement a functionality that refunds any excess value sent.

3. Risks of Centralization

The codebase of the Nextgen Protocol is unfortunately highly centralized, possibly one of the most centralized in the space. All power is vested in the hands of admins, with various types of admins adding complexity to the execution process. Here are some of the risks associated with this:

• There's an emergencyWithdraw function that allows for a potential rug-pull scenario.
• Global admins hold extensive power. They can alter anything in the protocol, including Collection Data, mint tokens for anyone (via airdrop), and even freeze collections.
• Function admins, who are privileged in their respective functions, possess more power than the artists themselves. They can do everything a global admin can, but only within their specific functions.
• Artists have more control over their collections than necessary. If the collection is not frozen, they can alter token metadata, images, attributes, etc. This undermines one of the main benefits of a blockchain, which is immutability.

This level of centralization presents a significant risk and can compromise the integrity and security of the protocol.

Time spent:

20 Hours

Time spent:

20 hours