Truncating division in getPrice results in under-quoting the minting price in the last period of a linear descending sale

[c4-submissions]

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

[c4-pre-sort]

141345 marked the issue as duplicate of #469

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Out of scope

[c4-judge]

alex-ppg marked the issue as duplicate of #271

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg removed the grade

[c4-judge]

alex-ppg marked the issue as satisfactory