Price is equal to collectionMintCost at the end of sale for salesOption == 2

[c4-submissions]

Lines of code

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

Vulnerability details

Vulnerability Details

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

        } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime){

getPrice checks for < when comparing with block.timestamp

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

       } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {

mint checks for <=

It means that at the last second of the sale the price will be collectionMintCost
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L566

            return collectionPhases[_collectionId].collectionMintCost;

Impact

Impossible to buy at the last second for the collectionEndMintCost
Users who wished to buy at the last second for the lowest price won't be able to do it
Integration problems are possible when 3rd party requests getPrice because the price will suddenly spike at the last second. E.g. bots who try to buy at the last second and request getPrice will massively overpay

Proof of Concept

Put the test file below to hardhat/tests/fileName.test.js and run npx hardhat test test/fileName.test.js

// @ts-check
const TEST_NAME = "Incorrect price formula for salesOption 2";
const {
    loadFixture,
  } = require("@nomicfoundation/hardhat-toolbox/network-helpers")
const { expect } = require("chai");
// @ts-ignore
const { ethers } = require("hardhat");
const fixturesDeployment = require("../scripts/fixturesDeployment.js")
  
let signers
let contracts

const START_MINT_COST = 100_000;
const RATE = 100;
const END_MINT_COST = 10_000;

let PUBLIC_START_TIMESTAMP;
const PUBLIC_SALE_LENGTH = 10_000;

const SALES_OPTION = 2;

let collectionId;
let attacker;

describe("NextGen Tests", function () {
    beforeEach(async function () {
      ;({ signers, contracts } = await loadFixture(fixturesDeployment))
      attacker = signers.addr2;
      await contracts.hhCore.addMinterContract(
        contracts.hhMinter
      );
    })

    context(TEST_NAME, () => {

        beforeEach(async function () {
            collectionId = await contracts.hhCore.newCollectionIndex();
            const collectionAdmin = signers.addr1;

            await contracts.hhCore.createCollection(
                `Test Collection ${collectionId}`,
                `Artist ${collectionId}`,
                "For testing", "www.test.com", "CCO", "https://ipfs.io/ipfs/hash/", "", ["desc"]
            );
            await contracts.hhAdmin.registerCollectionAdmin(
                collectionId, // _collectionID
                collectionAdmin.address,
                true
            );
            await contracts.hhCore.connect(collectionAdmin).setCollectionData(
                collectionId,
                collectionAdmin.address, // _collectionArtistAddress
                2, // _maxCollectionPurchases
                1000, // _collectionTotalSupply
                0, // _setFinalSupplyTimeAfterMint
            );
            await contracts.hhMinter.setCollectionCosts(
                collectionId,
                START_MINT_COST, // _collectionMintCost
                END_MINT_COST, // _collectionEndMintCost
                RATE, // _rate
                2000, // _timePeriod
                SALES_OPTION, // _salesOptions
                '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B', // delAddress
            );
        
            PUBLIC_START_TIMESTAMP = await getLatestBlockTimestamp() + 100;
            await contracts.hhMinter.setCollectionPhases(
                collectionId,
                PUBLIC_START_TIMESTAMP, // _allowlistStartTime
                0, // _allowlistEndTime
                PUBLIC_START_TIMESTAMP, // _publicStartTime
                PUBLIC_START_TIMESTAMP + PUBLIC_SALE_LENGTH, // _publicEndTime
                "0x633522a1353f0e3bf991364afc2d74b59b938bad1726812e25d9f9c09d90b06a"  // _merkleRoot
            );

            await contracts.hhCore.addRandomizer(
                collectionId, contracts.hhRandomizer,
            );

            // Mint several tokens so viewCirSupply is not 0
            await contracts.hhMinter.airDropTokens(
                [signers.owner], //_recipients,
                [""],       //_tokenData,
                [0],        //_saltfun_o,
                collectionId,          //_collectionID,
                [7],         //_numberOfTokens,
            );
            expect(await contracts.hhCore.viewCirSupply(collectionId))
                .to.eq(7);
        })

        it("calculates the price incorrect", async function() {
            // Start price, as expected
            await ethers.provider.send('evm_setNextBlockTimestamp', [PUBLIC_START_TIMESTAMP]);
            await ethers.provider.send("evm_mine");
            expect(await contracts.hhMinter.getPrice(collectionId))
                .to.eq(START_MINT_COST);


            // Skip to the middle time
            const middleOfTheSaleTimestamp = PUBLIC_START_TIMESTAMP + PUBLIC_SALE_LENGTH / 2;
            await ethers.provider.send('evm_setNextBlockTimestamp', [middleOfTheSaleTimestamp]);
            await ethers.provider.send("evm_mine");

            // Some other price, as expected
            expect(await contracts.hhMinter.getPrice(collectionId))
                .not.to.eq(END_MINT_COST);
            expect(await contracts.hhMinter.getPrice(collectionId))
                .not.to.eq(START_MINT_COST);

            // Skip to the end
            const endSaleTimestamp = PUBLIC_START_TIMESTAMP + PUBLIC_SALE_LENGTH;
            await ethers.provider.send('evm_setNextBlockTimestamp', [endSaleTimestamp]);
            await ethers.provider.send("evm_mine");
            // End price is START_MINT_COST, should not be
            expect(await contracts.hhMinter.getPrice(collectionId))
                .to.eq(START_MINT_COST);

        })
        it("disallows to buy with correct price", async function() {
            const endSaleTimestamp = PUBLIC_START_TIMESTAMP + PUBLIC_SALE_LENGTH;
            await ethers.provider.send('evm_setNextBlockTimestamp', [endSaleTimestamp]);

            const mintPromise = contracts.hhMinter.connect(attacker).mint(
                collectionId, // _collectionID
                1, // _numberOfTokens
                1, // _maxAllowance
                '', // _tokenData
                attacker, // _mintTo
                [], // _merkleRoot
                ethers.ZeroAddress, // _delegator
                0, //_varg0
                {value: END_MINT_COST}
            )
            await expect(mintPromise)
                // @ts-ignore
                .to.be.revertedWith("Wrong ETH");

            expect(await contracts.hhMinter.getPrice(collectionId))
                .to.eq(START_MINT_COST);
            expect(await getLatestBlockTimestamp())
                .to.eq(endSaleTimestamp);
        })
        it("allows to buy with incorrect price", async function() {
            const endSaleTimestamp = PUBLIC_START_TIMESTAMP + PUBLIC_SALE_LENGTH;
            await ethers.provider.send('evm_setNextBlockTimestamp', [endSaleTimestamp]);
            expect(await contracts.hhMinter.getPrice(collectionId))
                .to.eq(START_MINT_COST);

            const tx = await contracts.hhMinter.connect(attacker).mint(
                collectionId, // _collectionID
                1, // _numberOfTokens
                1, // _maxAllowance
                '', // _tokenData
                attacker, // _mintTo
                [], // _merkleRoot
                ethers.ZeroAddress, // _delegator
                0, //_varg0
                {value: START_MINT_COST}
            );
            const tokenId = await getLastMintedTokenId(tx);
            expect(await contracts.hhCore.ownerOf(tokenId)).to.eq(attacker.address);

            expect(await getLatestBlockTimestamp())
                .to.eq(endSaleTimestamp);
        })
    });
});


async function getLatestBlockTimestamp() {
    // @ts-ignore
    return (await ethers.provider.getBlock("latest")).timestamp;
}

async function getLastMintedTokenId(tx) {
    return (await tx.wait()).logs
        .map(log => {
            try {
                return new ethers.Interface([
                    "event Transfer(address indexed from, address indexed to, uint256 indexed tokenId)"
                ]).parseLog(log);
            } catch (error) {
                return null;
            }
        })
        .find(parsedLog => parsedLog && parsedLog.name === "Transfer")
        .args.tokenId;
}

Tools Used

Manual review

Recommended Mitigation Steps

Use <, > and <=, >= consistently. Either strict or non-strict in all the relevant places.

Assessed type

Math

[c4-pre-sort]

141345 marked the issue as duplicate of #1391

[c4-judge]

alex-ppg marked the issue as partial-50