NextGen - ZdravkoHr's results

Lines of code

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

Vulnerability details

Impact

The MinterContract has a public mint function which can be called by anyone to mint themself a NextGenCore NFT token against a certain amount of ETH. The mint function in the ERC721 contract does not follow the CEI pattern. It first makes a call to ERC721's _safeMint thorugh the _mintProcessing function.

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

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

This lets a malicious party reenter the execution with the onERC721Received hook. This can cause a variety of problems. Let's look at one of them:

During a public minting phase, each Ethereum account must not be able to mint more NFTs than

collectionAdditionalData[_collectionID].maxCollectionPurchases

This invariant is enforced by the MinterContract by comparing the amount that the user will have if the mint is successful with the maxCollectionPurchases:

require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

As we have already seen, the minted tokens per address are updated after the safeMint which means that this require statement can be bypassed from a contract that mints as long as it has funds to do so.

Proof of Concept

1. Create foundry folder and install Foundry inside of it.
2. Add remappings to map smart-contracts to contracts relative to the needed path.
3. Add the following PoC.
4. Run

forge test --lib-paths ../smart-contracts

Tools Used

Foundry

Recommended Mitigation Steps

1. Follow the CEI pattern.
2. Add a reentrancy protection to the _mintProcessing function since it is the main entrypoint of the minting mechanism.

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#L125 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L135

Vulnerability details

Impact

The AuctionDemo contract has functions for claiming an auction canceling bids. An auction has an end time. Claiming an auction can happen only when the auction has ended and canceling bid can happen only when the auction is active. However, both these functionalities use <= to check the current timestamp value against the end time of the auction.

AuctionDemo.participateToAuction():

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

AuctionDemo.cancelBid():

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

A certain edge case, when the block.timestamp value is equal to the end time allows claiming auction and canceling all bids after that. This will:

1. Mint the NFT to the highest bidder
2. Transfer the bid of the current auction to the owner of the AuctionDemo
3. Return back the bids of the other bidders
4. Transfer the bids of the bidders of other auctions to the highest bidder.
5. The highest bidder will have both the NFT and his funds, while bidders for other tokenIds will not be able to claim their NFT because the funds will have been already drained.

Proof of Concept

1. Create foundry folder and install Foundry inside of it.
2. Add remappings to map smart-contracts to contracts relative to the needed path.
3. Add the following PoC.
4. Run

forge test --lib-paths ../smart-contracts

Tools Used

Foundry

Recommended Mitigation Steps

Decide if the end time of the auction is inclusive or exclusive and remove the = from the needed function.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

When an auction comes to its end, the highest bid is sent to the owner of the NFT selled and all other bids are send back to their owners. A gas griefing attack where a malicious bidder consumes all the gas available is possible. All auctions can be DOS-ed if a malicious bidder sends as little as 1 wei to the contract. Once entered, bidders cannot be removed. The bid call can be initiated by a contract that uses up all the available gas on receive(). The attacker can even bid several times to increase even further their chances of DOS of the system (because of the 63/64 rule) or just gas bomb it returning a huge amount of bytes.

Impact -> all the gas will be consumed and the transaction will revert, the NFT will not be send to the winner and the remainder of the funds will be stuck forever.

Proof of Concept

1. Create foundry folder and install Foundry inside of it.
2. Add remappings to map smart-contracts to contracts relative to the needed path.
3. Add the following PoC.
4. Run

forge test --lib-paths ../smart-contracts

Tools Used

Foundry

Recommended Mitigation Steps

Add a mapping that tracks the balance of each bidder. When an auction ends, increase the corresponding balances. Then create a function that enables withdrawals from the contract by the authorized bidders.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

According to the gitbook, minting of NFTs can happen at different phases. Each phase has configurations for its minting periods and the sale mode. Before a minting phase starts, these configurations have to be set by calling MinterContract.setCollectionCosts() and MinterContract.setCollectionPhases(). This is where the sales mode is chosen.

    function setCollectionCosts(uint256 _collectionID, uint256 _collectionMintCost, uint256 _collectionEndMintCost, uint256 _rate, uint256 _timePeriod, uint8 _salesOption, address _delAddress) public CollectionAdminRequired(_collectionID, this.setCollectionCosts.selector) {
        require(gencore.retrievewereDataAdded(_collectionID) == true, "Add data");
        collectionPhases[_collectionID].collectionMintCost = _collectionMintCost;
        collectionPhases[_collectionID].collectionEndMintCost = _collectionEndMintCost;
        collectionPhases[_collectionID].rate = _rate;
        collectionPhases[_collectionID].timePeriod = _timePeriod;
        collectionPhases[_collectionID].salesOption = _salesOption;
        collectionPhases[_collectionID].delAddress = _delAddress;
        setMintingCosts[_collectionID] = true;
    }

The periodic scale mode allows minting 1 NFT per time period. The problem lies in the way the calculation for when the last mint has happened is done. It uses all the tokens in circulation, including ones that have been minted in different phases. If a minting with different sale mode has happened before the periodic scale, this calculation will become broken and users will not be able to mint.

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

Proof of Concept

1. Create foundry folder and install Foundry inside of it.
2. Add remappings to map smart-contracts to contracts relative to the needed path.
3. Add the following PoC.
4. Run

forge test --lib-paths ../smart-contracts

Tools Used

Foundry

Recommended Mitigation Steps

Add internal counter of all the NFTs that have been minted during the periodic scale phase. Use it instead of the total circulation. Reset this counter when changes are made to the costs or phases.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

When an auction comes to an end, the AuctionDemo.claimAuction() can be called to transfer the NFT to the winner and returns the bids of the other bidders. If the winner of the auction is a contract that either reverts on received ERC721 token or does not implement the onERC721Received function (may be a simple mistake, no malicious intent behind it), the funds of the other bidders will be stuck forever in the contract.

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        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) {
                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) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

Proof of Concept

1. A protocol XYZ implements an immutable smart contract to participate in the NextGen auction for an expensive NFT, but the developers have forgotten to add an onERC721Receive function.
2. The auction becomes popular and a lot of bidders participate.
3. The XYZ contract wins the auction.
4. When time comes for claiming the reward, the whole transaction reverts.
5. The NextGen team cannot receive the winning bid.
6. Other bidders do not receive back the funds they have put in the auction. This will bring bad reputation to the NextGen protocol

Tools Used

Manual Review

Recommended Mitigation Steps

Add another admin protected function which distributes all the funds to the bidders that have not won the auction after it has ended. I suggest adding another mapping which tracks if a given auction has failed to transfer an NFT. The default will be false. The transfer of the NFT will be wrapped in a try/catch where the catch sets the flag to true. Then our function will check if the flag is true and only then execute its logic. This will not add any new centralization risks.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

During a linear descending [sales mode], the mint price decreases on each mint period until it reaches a given endPrice. This is achieved by the MinterContract.getPrice() function. It divides the endMintCost by the rate, which results in over how many periods will the mint cost reach the endMintCost.

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

The above if uses ** > ** instead of ** >= ** to check if the totalPeriods are greater than the passed periods. This will skip the last period before the final cost. If the selling is set to have 1 decreasing phase, the endCost / rate will be 1. tDiff will also be 1. 1 > 1 => false. In result, minters will be able to mint paying end price earlier.

Proof of Concept

1. Create foundry folder and install Foundry inside of it.
2. Add remappings to map smart-contracts to contracts relative to the needed path.
3. Add the following PoC.
4. Run

forge test --lib-paths ../smart-contracts

Tools Used

Foundry

Recommended Mitigation Steps

Use ** >= ** instead of ** > **:

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

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

Assessed type

Math

Report

[L-1] Malicious receiver can stop airdropping

The MinterContract allows airdrops. The airdrop function loops through every recipient and mints him an amount of NFTs. If one of the receivers does not implement the ERC721 onERC721Received or implements it to always revert, the whole airdrop will fail. Consider using Pull over Push

[L-2] NFTs minted in the same block will have the same hash preimage excluding the mintIndex

Since an NFTs hash in XRandoms.sol is generated by a mintIndex, block.prevrandao, blockhash and block.number, all NFTs generated in the same block will have the same preimage except for mintIndex. Because mintIndex cannot be the same for 2 tokens, this is a finding of LOW severity.

[L-3] The last word of the wordList cannot be accessed

The XRandoms.getWord function stores 100 words as strings in an array. It takes an id parameter and returns words[id] if the id is 0 and words[id - 1] otherwise. The function is private, so it can be called only from the same contract. The only place where it is called is the XRandoms.randomWord with a value from 0-99. The last word is at index 99. It can never be accessed because the id is subtracted by 1.

[L-4] There is a possibility of a function selectors collision

There are functions that are protected by a modifier that checks their signatures. The caller is able to execute the function only if he is whitelisted for its signature. With adding more and more functions, the possibility of function selector clash increases. Consider mixing the signature with an additional element.

[NC-1] Organize the files in the main folder

Currently, all files are stored in the root folders. Consider organizing them into different folders. For example, libs, randoms, core

[NC-2] Format the code

The code is not formatted properly. Consider using a tool that formats it.