NextGen - ubl4nk's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L224 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L236 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L193-L198

Vulnerability details

Impact

Attacker is able to bypass the check for tokensMintedPerAddress (number of tokens minted per address) during public-sale (phase 2) and due to that he is able to mint all the available NFTs (number of tokens remaining until totalSupply) in 1 transaction.

Proof of Concept

• A malicious contract calls MinterContract#mint function and it wants to mint 1 NFT (assume maxCollectionPurchases is 1).
• The transaction comes to this line:

gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);

• It goes to NextGenCore#mint function, let's look at it:

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;
            }
        }
}

• This checks if circulationSupply of collection is less than totalSupply, it goes to _mintProcessing and then updates number of tokens minted per address (the problem is exactly here -> this function is increasing number of tokens minted per address just after minting token, but it should be before calling _mintingProcess):

function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
        tokenData[_mintIndex] = _tokenData;
        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
        tokenIdsToCollectionIds[_mintIndex] = _collectionID;
        _safeMint(_recipient, _mintIndex);
    }

• _mintProcessing calls safeMint which triggers the function onERC721Received of transaction-caller (if caller is a contract)
• So transaction goes to onERC721Received function of caller-contract, and it re-enters to MinterContract#mint function.

Tools Used

Manual Review

Recommended Mitigation Steps

Change NexGenCore#mint function like this:

--- a/smart-contracts/NextGenCore.sol
+++ b/smart-contracts/NextGenCore.sol
@@ -190,12 +190,12 @@ contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {
         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;
             }
+                       _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#L57-L61 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104-L120

Vulnerability details

Impact

Participant calls participateToAuction and winner calls claimAuction, if both transactions are executed at auctionEndTime (in same block where block.timestamp = auctionEndTime), if the transaction of winner is executed earlier, the participant will lost their funds and the funds will be locked in contract for ever.

Proof of Concept

Let's take a look at participateToAuction:

// @audit `participateToAuction` doesn't check if the auction is already claimed or not
function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
        // ...
}

1. The above condition lets Participant's transaction to be executed at auctionEndTime (because it checks block.timestamp <= auctionEndTime and not < ) And look at claimAuction:

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

2. The above condition lets Winner's claim transaction to be executed at auctionEndTime (because it checks block.timestamp >= auctionEndTime and not > )

So what we get from 1 and 2 ? if the transaction of a participant and the transaction of winner are executed in same block (for example block N where timestamp of block N is equal to auctionEndTime) -> then if transaction of winner executes earlier, the participant has lost their funds forever. (In other words, Winner transaction can be executed at x and Participate transaction can be executed at x, so if Winner claim transaction is executed earlier, it means auction is claimed, so which auction does the participant participate in? While the auction has been claimed by the winner)

Scenario:

• There is a block called N and timestamp for block N is 123456
• Assume the auction ends at 123456 in block N (it means, auctionEndTime = 123456 = block.timestamp)
• Winner calls claimAuction and Alice (a participant) calls participateToAuction.
• If both transactions are executed in same block (block N) and transaction of Winner executes earlier, Alice funds will be lost forever (because the both transactions are allowed to be executed at exactly auctionEndTime) and there is no way for Alice for get their funds back (because the auction is already claimed and there is no any function to cancel the bid after auction ends).

Tools Used

Manual Review

Recommended Mitigation Steps

Add the following line in participateToAuction:

diff --git a/smart-contracts/AuctionDemo.sol b/smart-contracts/AuctionDemo.sol
index 95533fb..435c0cd 100644
--- a/smart-contracts/AuctionDemo.sol
+++ b/smart-contracts/AuctionDemo.sol
@@ -55,6 +55,7 @@ contract auctionDemo is Ownable {
     // participate to auction

     function participateToAuction(uint256 _tokenid) public payable {
+               require(auctionClaim[_tokenid] == false, "It's too late");
         require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
         auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
         auctionInfoData[_tokenid].push(newBid);

Assessed type

Invalid Validation