NextGen - Fulum's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104-L120 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104-L105 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L124-L125

Vulnerability details

Impact

Because of combination of multiples issues in the AuctionDemo contract, a malicious user can make a sandwich attack on another user to take the auctionned token/NFT and recover his funds.

Proof of Concept

In the AuctionDemo contract, we have multiples problems. Firstly, I think for a more safe auction process, when an auction is ended, we mustn't be able to re-bid. And we can see the functions participateToAuction(), cancelBid() and cancelAllBids() makes a require check for time that block.timestamp <= auctionEndTime:

    function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
        ...
    }

And function claimAuction() make a require check for time that block.timestamp >= auctionEndTime.

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

As the result when block.timestamp == auctionEndTime, users can potentially claim, participate to the auction and cancel a bid.

There's also the following problem. The status of an auctionInfoData (auctionInfoData[_tokenid][index].status) is set to true inside participateToAuction() and set to false in cancelBid() and cancelAllBids(). This permit to verify if for an index of the array, the bid is claimable or not. The problem inside the claimAuction() is the status is not set to false when ETH are send to the user, this problem turn to a vulnerability in the scenario below.

Because of the time checking problem in the contract functions, check this potential scenario of a sandwich attack:

1. Alice and Bob decide to participate in the auction of a tokenID, in using participateToAuction()
2. Sarah try to be the highest bidder by sending her transaction in the last accepted block.timestamp
3. Bob see that in the mempool and decide to frontrun alice by calling claimAuction(). Bob claim the tokens because it's the highest bidder and all the bids are send back to all bidders.
4. The Alice transaction is executed and she becomes the highest bidder and send ETH to the contract.
5. Now Bob backrun the Alice transaction in using cancelBid() for his bid. Because the contract doesn't set the auctionInfoData[_tokenid][index].status to false in claimAuction() and now the contract have the Sarah funds (which is of course more than the Bob amount because Sarah send a transaction to be the highest bidder), Bob retrieve his funds by stealing Alice's deposit.

Bob won the Token/NFT for free and he steal the funds of a user.

Now Alice can't withdraw her deposited ETH because the AuctionDemo balance is less than her bid and all call to claimAuction() or cancelBid() wil revert. And if the same contract is used to auction other tokenId, Alice can withdraw her ETH but impossibility to use claimAuction() for other tokenId because there are an offset between the internal balance of the contract and values saved in all the auctionInfoData[_tokenid][i].bid.

Logs

You have the image with the logs of the PoC

You can check this Gist to setup foundry with the smart contracts for an executable PoC: https://gist.github.com/AxelAramburu/c10597b5ff60616b8a15d091f88de8da

And execute this command: forge test --mt testClaimAuctionForFree -vvv

Tools Used

Manuall Review, Foundry

Recommended Mitigation Steps

You can fix this issue by change the time check in the claimAuction() function, you have also to set the auctionInfoData[_tokenid][i].status to false when the ETH are redeemed:

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        -- require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        ++ require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
                ++ auctionInfoData[_tokenid][i].status == false
                IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                ++ auctionInfoData[_tokenid][i].status == false
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

A user mint a token at an incorrect price and losing funds if he mint on the last authorized block.timestamp during a Linear or Exponential Descending Sale Model.

Proof of Concept

On a Linear or Exponential Descending Sale Model, the admin set the collectionMintCost and the collectionEndMintCost. In context of these sale models, the collectionMintCost is the price for minting a token at the beginning and the collectionEndMintCost the price at the end of the sale.

The minting methods use the function MinterContract::getPrice() to compute the correct price at the actual timing, check the function with the branch for a Linear or Exponential Descending Sale Model:

    function getPrice(uint256 _collectionId) public view returns (uint256) {
        uint tDiff;
        //@audit If Periodic Sale
        if (collectionPhases[_collectionId].salesOption == 3) {
            ...
        } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime){

            tDiff = (block.timestamp - collectionPhases[_collectionId].allowlistStartTime) / collectionPhases[_collectionId].timePeriod;
            uint256 price;
            uint256 decreaserate;
            //@audit If Exponential Descending Sale
            if (collectionPhases[_collectionId].rate == 0) {
                price = collectionPhases[_collectionId].collectionMintCost / (tDiff + 1);
                decreaserate = ((price - (collectionPhases[_collectionId].collectionMintCost / (tDiff + 2))) / collectionPhases[_collectionId].timePeriod) * ((block.timestamp - (tDiff * collectionPhases[_collectionId].timePeriod) - collectionPhases[_collectionId].allowlistStartTime));
            //@audit If Linear Descending Sale
            } else {
                if (((collectionPhases[_collectionId].collectionMintCost - collectionPhases[_collectionId].collectionEndMintCost) / (collectionPhases[_collectionId].rate)) > tDiff) {
                    price = collectionPhases[_collectionId].collectionMintCost - (tDiff * collectionPhases[_collectionId].rate);
                } else {
                    price = collectionPhases[_collectionId].collectionEndMintCost;
                }
            }
            if (price - decreaserate > collectionPhases[_collectionId].collectionEndMintCost) {
                return price - decreaserate; 
            } else {
                return collectionPhases[_collectionId].collectionEndMintCost;
            }
        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }
    }

We can see that if the collectionPhases[_collectionId].salesOption == 2 (it's the number for a descending sale model), and if the block.timestamp is > allowlistStartTime and < publicEndTime. The price is correctly computed. A little check on the mint() function:

    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
        ...
        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
              ...
        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }
    }

But if the publicEndTime is strictly equal to publicEndTime, the returned price is the collectionMintCost instead of collectionEndMintCost because the logic go to the "else" branch. It's an incorrect price because it's the price at the beginning of the collection. And as you can see on mint(), a user can mint a token on the block.timestamp publicEndTime.

User that mint on the last block.timstamp mint at an unexpected price and for all the minting methods which use the getPrice() function.

Logs

You can see the logs of the test with this image:

And a link of a gist if you want to execute the PoC directly. https://gist.github.com/AxelAramburu/c10597b5ff60616b8a15d091f88de8da And you can execute the test with this command:

forge test --mt testgetWrongPriceAtEnd -vvv (or with -vvvvv to see all the transaction logs)

Tools Used

Manual Review

Recommended Mitigation Steps

Change the < & > to <= & => on the else if branch inside the getPrice() function.

    function getPrice(uint256 _collectionId) public view returns (uint256) {
        uint tDiff;
        //@audit-info If Periodic Sale
        if (collectionPhases[_collectionId].salesOption == 3) {
            ...
        -- } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && -- block.timestamp < collectionPhases[_collectionId].publicEndTime){
        ++ } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp => collectionPhases[_collectionId].allowlistStartTime && ++ block.timestamp <= collectionPhases[_collectionId].publicEndTime){

            tDiff = (block.timestamp - collectionPhases[_collectionId].allowlistStartTime) / collectionPhases[_collectionId].timePeriod;
            uint256 price;
            uint256 decreaserate;
            //@audit If Exponential Descending Sale
            if (collectionPhases[_collectionId].rate == 0) {
                price = collectionPhases[_collectionId].collectionMintCost / (tDiff + 1);
                decreaserate = ((price - (collectionPhases[_collectionId].collectionMintCost / (tDiff + 2))) / collectionPhases[_collectionId].timePeriod) * ((block.timestamp - (tDiff * collectionPhases[_collectionId].timePeriod) - collectionPhases[_collectionId].allowlistStartTime));
            //@audit If Linear Descending Sale
            } else {
                if (((collectionPhases[_collectionId].collectionMintCost - collectionPhases[_collectionId].collectionEndMintCost) / (collectionPhases[_collectionId].rate)) > tDiff) {
                    price = collectionPhases[_collectionId].collectionMintCost - (tDiff * collectionPhases[_collectionId].rate);
                } else {
                    price = collectionPhases[_collectionId].collectionEndMintCost;
                }
            }
            if (price - decreaserate > collectionPhases[_collectionId].collectionEndMintCost) {
                return price - decreaserate; 
            } else {
                return collectionPhases[_collectionId].collectionEndMintCost;
            }
        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }
    }

Assessed type

Invalid Validation

[01] - In AuctionDemo contract, a malicious user can carry out a block-stuffing attack to ensure that he is the highest bidder.

A malicious user can carry out a block-stuffing attack to ensure that he is the highest bidder. For example a very rare/expensive Token/NFT is auctionned. The highest bid is more than 50ETH and our attacker Bob absolutetly want this token. He check is nearly the end of the auction time, he place a higher bid of 55ETH and become the new highest bidder. Now he can check the mempool and make a blockstuffing attack by sending dummy transactions with a lot of gas to prevent the potential participateToAuction() transactions to be integrated into the chain before the end of the auction. I consider it at low because a low likelihood and because gas cost is expensive on the Ethereum blockchain.

More infos here: https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service/#gas-limit-dos-on-the-network-via-block-stuffing

[02] - In MinterContract::setCollectionPhases(), lack of verifications can lead to potential issues on the minting process

Lacks of verification inside the MinterContract::setCollectionPhases() functions can lead to potentials time issue on the minting process. Examples if times parameters are misconfigured are impossibility for user to mint, computation problems inside the getPrice() function, etc.

Recommendations

Add verification like _allowlistStartTime < _allowlistEndTime and _publicStartTime < _publicEndTime. You can verify also the periods are greater than block.timestamp.

[03] - Function MinterContract::airDropTokens() doesn't check for inputs arrays have the sames sizes

The function MinterContract::airDropTokens() is a function that allows an authorized admin to execute multiples minting in a airdrop for multiples users. It requires multiples inputs and some of them are dynamic sized arrays, and because size of some arrays are not checked to be at the same length, it can lead to minting with _tokenData[] or _saltfun_o[] with zero values, or no minting if the _numberOfTokens[] is less than others arrays (and equal 0).

Recommendations

Add verification to check if the _recipients[], _tokenData[], _saltfun_o[] and _numberOfTokens[] have the same length.

[04] - Rounding error lead to bad distribution of royalties and losing ETH

MinterContract::payArtist() is the function called by admin to pay the artist and the team percentage in sending the deposited amount for a collection. We look that the function makes the following operations to divided the total royalties to the artist and the team.

    function payArtist(uint256 _collectionID, address _team1, address _team2, uint256 _teamperc1, uint256 _teamperc2) public FunctionAdminRequired(this.payArtist.selector) {
        require(collectionArtistPrimaryAddresses[_collectionID].status == true, "Accept Royalties");
        require(collectionTotalAmount[_collectionID] > 0, "Collection Balance must be grater than 0");
        require(collectionRoyaltiesPrimarySplits[_collectionID].artistPercentage + _teamperc1 + _teamperc2 == 100, "Change percentages");
        uint256 royalties = collectionTotalAmount[_collectionID];
        ...
        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;
        (bool success1, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd1).call{value: artistRoyalties1}("");
        (bool success2, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd2).call{value: artistRoyalties2}("");
        (bool success3, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd3).call{value: artistRoyalties3}("");
        (bool success4, ) = payable(tm1).call{value: teamRoyalties1}("");
        (bool success5, ) = payable(tm2).call{value: teamRoyalties2}("");
        ...
    }

Take this scenario with this percentage separation and this little amount:

Artist: 60% 18% 2% Team: 10% 10% royalties = 100495

artistRoyalties1 = 100495 * 61 / 100
artistRoyalties2 = 100495 * 17 / 100
artistRoyalties3 = 100495 * 2 / 100
teamRoyalties1 = 100495 * 11 / 100
teamRoyalties2 = 100495 * 9 / 100

artistRoyalties1 = 61301
artistRoyalties2 = 17084
artistRoyalties3 = 2009
teamRoyalties1 = 11054
teamRoyalties2 = 9044

Total) 61301 + 17084 + 2009 + 11054 + 9044 = 100492

We can see a difference of 3 wei, it not a big amount and it's for this reason I think this issue is low. But with a lot of call for multiples payment during times and calls for all the collections on the contract. Artist are impacted and lose amount of tokens while the dust ETH are losed in the contract.

Recommendations

Change the methods of computations and/or verify is the royalties amount is equal to all the computed amount and add the difference to one amount to prevent dust ETH in the contract.

[05] - In RandomizerNXT contract, the updateAdminsContract() don't verify if the contract is an admin contract

To follow the logic to check that a contract is an admin contract/minter contract/etc. In the updateAdminsContract(), when an admin change the admin contract, there is no check if the contract is an admin contract. By mistake, admin can change the admin contract to another address and the check with the function is not made.

Recommendations

Add this line on the function :

    function updateAdminsContract(address _admin) public FunctionAdminRequired(this.updateAdminsContract.selector) {
        ++ require(INextGenAdmins(_newadminsContract).isAdminContract() == true, "Contract is not Admin");
        adminsContract = INextGenAdmins(_admin);
    }

[06] - An attempt to mint with a RandomizerRNG contract can always revert

The RandomizerRNG contract not set in the constructor the minimum ETH required to make a call to the ArrngController (see below), you can set it directly in the constructor with the minimum value defined by the ArrngController controller in the minimumNativeToken (see the concerned parts of the Arrng controller below). If an admin forgot to set the ethRequired, all the users call will revert.

https://github.com/arrng/arrng-contracts/blob/117d2eadc55eaabb5bea58396dbc8f421d735fd5/contracts/controller/ArrngController.sol#L427 https://github.com/arrng/arrng-contracts/blob/117d2eadc55eaabb5bea58396dbc8f421d735fd5/contracts/controller/ArrngController.sol#L31-L35

Recommendations

Set the ethRequired in the constructor of the contract to the minimal value to avoid the calls revert when a user try to mint. At the time of writing minimumNativeToken = 1000000000000000.

General

I audit the NextGen project during 1 week. Previously not audited and I expected to finds some interesting vulnerabilities.

Approach

Most the focus was on a deep understanding of the sales models and the differents ways of minting a token. Verify the correctness of the functions, computations, transfer. Also focused on the generation of the randomness with the differents methods.

Architecture recommendations

Architecture is classic and simple, it's easy to understand how the protocol work and it's a good thing. I recommend the following architectural improvements:

• Recommend to add a pause feature in the important contract/functions to prevent from worst scenarios.

• Reinforce your architecture in creating special numbers for each sales models, choose one and implement the good logic in the setup of the collections and phases of minting. It helps you to not make mistake on collections/minting phases creations, have a more clear vision of the sales models and an esaye way to add new sales models in th future.

• Implement properly the randomness generation with the ChainLink VRF, the Chainlink documentation recommend for example to minting the token on randomness fulfillment to avoid potential revert of the request or multiples requests for the same token. Chekc this documentation: https://docs.chain.link/vrf/v2/security https://docs.chain.link/vrf/v2/subscription#request-and-receive-data https://docs.chain.link/vrf/v2/subscription#subscription-limits

• You can improve and simplify the AuctionDemo contract by deleting the power to cancelBid() for a user. This allows you to register the highest bidder when a new one coming, and to remove the for loops on the contract which present Denial-of-Service risks. Provide way to user to withdraw bids when auction is ended, help the user to withdrawn even if the claimAuction() function is DoS I recommend to look at this auction contract architecture: https://github.com/stader-labs/ethx/blob/mainnet_V0/contracts/Auction.sol

Qualitative analysis

Documentation is writed in simple and good way. You can add a category for security. Some functions explanations are missing from the documentation. The code is well writed but some mistakes and inadvertent error were made on the contracts lead to calculation errors, logic errors, problems of timing in the differents phases of the minting process and errors in randomness creation. Recommend you to add NatSpec in your contracts.

Centralization risks

The Admin contract (NextGenAdmins) is a good idea to manage all the actors of the ecosystem. The team use a multisig wallet and will be the owner of the admin contract to call the admin functions and add/remove privilege of differents actors. The problem is the power of the admin is too high and lead to potential rug pull scenarios like stealing funds or not paying artist. Example, admin have a lot of power for changing parameters of a collection in each time and broke important process. You can thinking about a way to have less power in sensitive time to guarantee the security of the users. It reinforces user confidence in the protocol. Also need to give artists the power to claim their wages.

Systemic risks

The primary systemic is the contracts unupgradability, in case of a security problem (epecially in the NextGenCore), it will be challenging to change logic. Thinking about potentials worst scenarios and implement functionnality (like upgradability, pause functions) and scenario to react quickly in case of issue.

Another risk is calling external contract which can be untrusted with a low level call, add risk of re-entrancy. Also a safeTransferFrom method to an external ERC721 collection which add risk of re-entrancy and/or no transferring token.

MEV is a systemic risk, especially on the auction system with block stuffing attack and front-running attack (example from preventing a user to give a higher bid).

Time Spent

5 days

Time spent:

50 hours