Periodic Sale (sales option 3) does not work as intended if used during both allowlist phase and public phase

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The Periodic Sale (sales option 3) does not work as intended if used during both allowlist and public phases. This is because when Periodic Sale model is used in the allowlist phase, some tokens are brought into circulation (as expected). Now when the Periodic Sale for the public phase begins, the first user can mint the token in the first period. But when this is done, the lastMintDate for that collection is set to a date far ahead in the future instead of the next time period.

Due to this issue, there are 3 impacts:

1. The property/invariant of periodicity (i.e. 1 mint/period) is broken since token cannot be minted in the next period
2. The users have to wait for a way longer time to mint the next token than the expected period of time (i.e. timePeriod)
3. This waiting can cause loss of user interest and value (ETH) in the collection, thus hurting the creators of the collection.

Note: This issue can occur in multiple combinations of sales models being used with allowlist/public phases. I tried attempting to jot them down but it was not feasible since there can be 1,2,3 or even more allowlist/public phases being held for a collection, which further increases the phase and sales model combinations. For clarity of understanding in this POC, I've used one simple instance of using Periodic Sale in both allowlist and public phases.

Root Cause: Although there are multiple combinations, one thing I could deduce as the root cause of this issue is that basically anytime the first phase (whether it be allowlist or public using any sales model) brings tokens into circulation supply, it causes the above mentioned impact to occur in the second phase (whether it be allowlist or public but using Sales model 3)

Proof of Concept

Here are few things to note in order to understand the example explanation in the POC better:

1. Consider we use Periodic Sale model (sales option 3) during both allowlist phase and public phase.
2. ALST = allowlistStartTime (short form)
3. Since we are using Periodic Sale model (sales option 3), allowlistStartTime (ALST) is set to publicStartTime (as mentioned in comment here) during public sale.
4. ALST during allowlist phase is 0s , ALST during public phase is 864000s (10 days) (for example purposes I will be using the start of Unix time i.e. 0 for ease of understandability and calculations)
5. Time Period used for Periodic Sale will be 10 minutes

Here is the whole process now:

During Allowlist phase:

1. Allowlisted users call mint() function to mint tokens. Let's say 1000 tokens were minted during this phase. These are the changes made:

• Circulating Supply = 1000 tokens
• lastMintDate = ALST + (Time Period * (Circulating Supply - 1) = 0 + (10 mins * 999) = 9990 mins = 166.5 hours = 6.9 days = 7 days (approx)

lastMintDate equation picked up from Line 252 in else-if block below:

File: MinterContract.sol
239:         // control mechanism for sale option 3
240:         if (collectionPhases[col].salesOption == 3) {
241:             uint timeOfLastMint;
242:             if (lastMintDate[col] == 0) {
243:                 // for public sale set the allowlist the same time as publicsale
244:                 timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod;
245:             } else {
246:                 timeOfLastMint =  lastMintDate[col];
247:             }
248:             // uint calculates if period has passed in order to allow minting
249:             uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
250:             // users are able to mint after a day passes
251:             require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
252:             lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));
253:         }

During Public phase (held 10 days after Allowlist phase):

1. Calling the mint() function to mint tokens is now open to all users. Let's say a user calls mint(). The following happens:

• Circulation Supply = 1000 + 1 (minted by user now) = 1001 tokens
• On Line 242, condition is false so we enter the else block on Line 246, which sets timeOfLastMint to lastMintDate (7 days approx - note still talking in unix time)
• On Line 249, tDiff = (10 days - 7 days)/10 mins = (14400 mins - 10080 mins)/10 mins = 432
• On Line 251, we pass the condition since tDiff (432) > 1 i.e. more than 10 minutes have passed since allowlist phase ended (i.e. 3 days back since today is 10 days (864000s) and allowlist phase ended at 7 days(604800s))
• On Line 252, lastMintDate = ALST + (Time Period * (Circulating Supply - 1) = 10 days + (10 mins * (1001 - 1)) = 10 days + (10 mins * 1000) = 10 days + 10000 mins = 10 days + 7 days (approx) = 17 days
• This is where the problem occurs. We can see from the above calculations that although the start time is 10 days, the next period is not 10 minutes later but in fact 7 days (10000 mins) later on the 17th day (note that still talking in unix time). Due to this users need to wait for 7 days more instead of 10 minutes in order to mint the next token. This would mean loss of interest and value in the collection, which can hurt the collection creators.

lastMintDate equation picked up from Line 252 in else-if block below:

File: MinterContract.sol
239:         // control mechanism for sale option 3
240:         if (collectionPhases[col].salesOption == 3) {
241:             uint timeOfLastMint;
242:             if (lastMintDate[col] == 0) {
243:                 // for public sale set the allowlist the same time as publicsale
244:                 timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod;
245:             } else {
246:                 timeOfLastMint =  lastMintDate[col];
247:             }
248:             // uint calculates if period has passed in order to allow minting
249:             uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
250:             // users are able to mint after a day passes
251:             require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
252:             lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));
253:         }

From the above example, based on the calculations, we can clearly see how the property/invariant of periodicity is broken (i.e. 1 mint/period) in the Periodic Sale model used during both allowlist and public phases. The example also showed how the users and creators of the collections are affected due to the wait time of 7 days instead of the expected 10 minutes.

(Note: In the example, I've only used 1000 tokens minted as the circulating supply during the allowlist phase but as the circulating supply increases during the allowlist phase, the wait time during the public phase increases as well. For example, if 10000 tokens were minted during allowlist phase, the wait time for the next period during the public phase is now 70 days instead of the expected 10 minutes)

Coded POC

There are few things to note in this POC:

• Instead of allowlist phase followed by public phase, I have used public phase 1 followed by public phase 2. The root cause still remains the same (which I mentioned in the beginning).
• Run the test using forge test --match-test testPeriodicSaleDoesNotWork -vvvvv
• Lines 24 to 42 represent the setup.
• In the testSetUpCollection method, max allowance per user to mint during public phase 1 and 2 is set to 2000 and total supply to 100000
• Price to mint token = 4ETH, Time Period = 600s or 10 mins, Sales model 3 in both public phases.
• Allowlist start time is set to public sale start time to use periodic sale model 3
• Public phase 1 start time = 1000s, Public phase 1 end time = 700000 (In foundry, time starts from 0)
• Public phase 2 start time = 1000000s, Public phase 2 end time = 1700000s
• Using such a long span of time since 1000 tokens need to be minted every 10 mins in public phase 1. Although not necessary in public phase 2, since only 2 tokens are being minted.
• In public phase 2, only 2 tokens are minted to show that after first token is minted, the second token is not mintable 10 mins (or 600s) later but at 1600000s (almost close to end time of public phase 2).
• This value of 1600000s is console logged for proof to show lastMintDate is now 1600000, so the next token can only be minted when block.timestamp catches upto it.
• Additionally, the assertEq on Line 87 evaluates to true, which means alice cannot mint even though we skipped to the next period (Line 83)
• In the stack traces we can observe the arithmetic underflow when Alice tries to mint in next period. Note the arithmetic underflow is expected since when calculating tDiff, block.timestamp - timeOfLastMint underflows due to timeOfLastMint (or lastMintDate) being 1600000s (basically instead of reverting with "1mint/period", it reverts with the underflow).
• Make sure to add the POC provided below to the hardhat/test folder before running the forge test --match-test testPeriodicSaleDoesNotWork -vvvvv command.
• I've provided helpful comments to assist while reading the POC and tried to make the function names as self-explanatory as possible as well.

File: TestContract2.sol
01: // SPDX-License-Identifier: MIT
02: 
03: pragma solidity ^0.8.19;
04: 
05: import {Test, console} from "forge-std/Test.sol";
06: 
07: import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
08: import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
09: import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
10: import {randomPool} from "../smart-contracts/XRandoms.sol";
11: import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
12: 
13: contract TestContract2 is Test {
14: 
15:     NextGenMinterContract minter;
16:     NextGenCore gencore;
17:     NextGenAdmins adminsContract;
18:     randomPool xrandoms;
19:     NextGenRandomizerNXT randomizer;
20: 
21:     address alice = makeAddr("Alice");
22:     
23: 
24:     function setUp() public {
25:         //Deployment process - Followed from docs
26:         adminsContract = new NextGenAdmins();
27:         gencore = new NextGenCore("", "", address(adminsContract));
28:         minter = new NextGenMinterContract(address(gencore),address(0) , address(adminsContract));
29:         xrandoms = new randomPool();
30:         randomizer = new NextGenRandomizerNXT(address(xrandoms), address(this), address(gencore));
31:     }
32: 
33:     function testSetUpCollection() public {
34:         string[] memory emptyScript;
35:         gencore.createCollection("Azuki", "6529", "Empty", "Empty", "Empty", "Empty", "Empty", emptyScript);
36:         gencore.setCollectionData(1, address(0), 2000, 100000, 0);
37:         minter.setCollectionCosts(1, 4000000000000000000, 0, 0, 600, 3,
38:         address(0));
39:         minter.setCollectionPhases(1, 1000, 0, 1000, 700000, 0);//Set allowliststarttime to public sale start time to pass check
40:         gencore.addMinterContract(address(minter));
41:         gencore.addRandomizer(1, address(randomizer));
42:     }
43: 
44:     function testPeriodicSaleDoesNotWork() public payable{
45:         testSetUpCollection();
46:         testPublicPhasePeriodicSale1();
47:         testPublicPhasePeriodicSale2();
48:     }
49: 
50:     function testPublicPhasePeriodicSale1() public {
51:         bytes32[] memory emptyArray;
52:         bytes memory selector = abi.encodeWithSignature("mint(uint256,uint256,uint256,string,address,bytes32[],address,uint256)", 1, 1, 0, "", alice, emptyArray, address(0), 0);
53:         //uint price = minter.getPrice(1);
54:         vm.warp(1000); //Enter public phase 1
55:             for(uint i = 0; i < 1000; i++){
56:                 if(i==0){
57:                     vm.deal(alice, 4000000000000000000);//4ETH for public phase 1
58:                     vm.prank(alice);
59:                     (bool success,) = address(minter).call{value: 4000000000000000000}(selector);
60:                 } else {
61:                     skip(600); //skipping periods to mint 1000 tokens faster
62:                     vm.deal(alice, 4000000000000000000);//4ETH for public phase 1
63:                     vm.prank(alice);
64:                     (bool success,) = address(minter).call{value: 4000000000000000000}(selector);
65:                 }
66: 
67:         }
68:     }
69: 
70:     function testPublicPhasePeriodicSale2() public {
71:         bytes32[] memory emptyArray;
72:         bytes memory selector = abi.encodeWithSignature("mint(uint256,uint256,uint256,string,address,bytes32[],address,uint256)", 1, 1, 0, "", alice, emptyArray, address(0), 0);
73:         minter.setCollectionPhases(1, 1000000, 0, 1000000, 1700000, 0); //Set allowliststarttime to public sale start time to pass check// Updating times for public phase 2 as the admin would
74:         vm.warp(1000000); // To enter public phase 2
75:         uint time = block.timestamp;
76:         console.log(time);
77:         for(uint i = 0 ; i < 2 ; i++){
78:             if(i==0){
79:                 vm.deal(alice, 4000000000000000000);//4ETH for public phase 2
80:                 vm.prank(alice);
81:                 (bool success,) = address(minter).call{value: 4000000000000000000}(selector);
82:             } else {
83:                 skip(600); //Skipping to next period 2
84:                 vm.deal(alice, 4000000000000000000);//4ETH for public phase 2
85:                 vm.prank(alice);
86:                 (bool success,) = address(minter).call{value: 4000000000000000000}(selector);
87:                 assertEq(success, false);  // This here and the two lines below are the proof 
88:                 uint256 lastMintDate = minter.lastMintDate(1);// that users now have to wait for longer time
89:                 console.log("LastMintDate:", lastMintDate);// Note the arithmetic overflow is expected since when calculating tDiff, block.timestamp - timeOfLastMint underflows due to timeOfLastMint being way ahead in future i.e. 1600000
90:             }
91:         } 
92:     }
93: 
94: }

Tools Used

Manual Review

Recommended Mitigation Steps

We can see the issue arises due to tokens being brought into the circulation supply during allowlist phase, which then during the public phase increases the lastMintDate due to timePeriod * circulationSupply being added to the start time of the public sale.

It is hard to boil down to a specific solution (such as only limiting sales model 3 to allowlist phase) since there can be two allowlist phases as well, which still causes the same issue. Basically anytime the first phase (whether it be allowlist or public) brings tokens into circulation supply, it causes this issue.

The only solution I can think of is to limit the Periodic Sale model (sales model 3) to the first allowlist or public phase that is ever conducted for the collection. Thereafter, using the periodic sale model will cause the problem mentioned in this POC.

Assessed type

Math

[c4-pre-sort]

141345 marked the issue as duplicate of #89

[c4-judge]

alex-ppg marked the issue as duplicate of #2012

[c4-judge]

alex-ppg marked the issue as satisfactory

[mcgrathcoutinho]

Hi @alex-ppg , here is why I believe this report should be chosen for the report:

1. The primary issue Multiple mints can brick any form of salesOption 3 mintings #380 has underestimated the severity of the issue and failed to demonstrate the maximal impact. Such an issue should receive partial-grading as mentioned in the C4 final SC verdict in the docs.
2. The primary issue Multiple mints can brick any form of salesOption 3 mintings #380 does not provide any form of mitigation at all and just mentions the root cause. In this report, there is atleast a minimal mitigation provided to the sponsor.
3. Due to considering all these factors, I believe this issue should be selected for report.

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for contributing to PJQA! The Warden's submission title is that the sales of a sale model 3 can be bricked which is in line with the maximal impact of this submission. Furthermore, they detail that:

It's worth noting that some collections may disrupt the whitelist, while others could brick the entire mint process, especially if there are more minted NFTs or a longer minting period.

I believe the above encompasses the full impact of the submission. The HM rating on this one can be debated, and it was awarded a rating of H not due to impact but rather due to impact combined with invalidating a core invariant. A collection can be reconfigured to mint under a different model, thereby not leading to any permanent loss of minting capabilities.

The mitigation this submission contains is identical to #380 (It is hard to boil down to a specific solution) and recommends a mitigation that contradicts the project's specification. A proper solution is not to restrict sale model 3 types to only being the first configured sale of a collection as sale models are meant to be dynamically combined.

For a proposed solution that aligns with the project's specification, you can consult the relevant response I left on the primary exhibit.