NextGen - EricWWFCP's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196-L254 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L189-L200 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L227-L232

Vulnerability details

Impact

Due to a reentrancy in mint()/_mintProcessing() a user can mint more items than allowed potentially not giving other users a chance to mint. This can lead to poor UI and frustrated users and unfairly punishing a user who wanted to mint an item.

Proof of Concept

The minting functionality has a classic reentrancy but it is requires some digging to see. Let's break it down in three places.

1. In the mint() function MinterContract.sol if we are able to mint it happens within one of two phases but in either case we are first checking to make sure the caller has not minted their "max" amount already. If we are in "phase1" we use a call like this

   require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
   
   or
   
   require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");
   

   If we are in "phase2" we use a check like this

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

   So, only if a user has a 'mint allowance' can we ultimately call gencore.mint()

2. Now this will trigger a call to the mint() function in the NextGenCore contract where we can see that the updates to a users 'mint allowance' happen AFTER the call to _mintProcessing()

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

3. This might be fine in some situations but _mintProcessing() ultimately uses a call to _safeMint()

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

   While safeMint() makes sure the item is transferred to an account that can receive it it does not prevent reentrancy if token is sent to a malicious contract.

4. A malicious contract can mint their max amounts of items and call back in to the MinterContract on receive and keep minting their 'max' amount of items until they are gone. This is possibly because it all happens BEFORE their 'allowance' is updated.

Essentially a single user can buy more items than allotted or even all of the items. While no money is lost this is certainly a horrible UX for every other potential buyer.

Tools Used

Manual Inspection

Recommended Mitigation Steps

Follow CEI pattern function and/or use a reentrancy guard in the mint() function in NextGenCore.sol. We can swap the code in step 2 like this

  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