Denial of service in mint() after burning an NFT

Summary

The mint() function incorrectly uses the totalSupply() function to set the new tokenId to be minted, which breaks the minting functionality if an NFT is burnt.

Vulnerability Detail

The CouncilMember ’s mint() function allows creating new council member NFTs. In order to do so, the ERC721’s internal _mint() function is called, passing the newMember as the receiver address, and the totalSupply() as the tokenID to mint:

// CouncilMember.sol

function mint(
        address newMember
    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
        if (totalSupply() != 0) {
            _retrieve();
        }

        balances.push(0);
        _mint(newMember, totalSupply());
    }

The problem with using the totalSupply() as the new token ID is that the function will no longer work if an NFT is burnt.

Consider the following scenario, where four NFTs exist (totalSupply() is four), with tokenIds 0, 1, 2 and 3.

Governance decides to burn the NFT with tokenId 2. This will make totalSupply() decrease to 3.
After burning, the current existing NFTs are the ones with tokenId 0, 1 and 3
Some time after burning the NFT, a new mint is required. Because totalSupply() has decreased to 3, mint() will try to mint the NFT with tokenId 3. However, the NFT with tokenId already exists, so this function will always fail, effectively making it completely impossible to mint NFTs.

Impact

ll the calls to the mint() function will fail after any burn() is performed. Because minting is a critical protocol function, the impact of this vulnerability is high.

Proof of Concept

The following proof of concept illustrates the issue. In order to run it, paste the code in the CouncilMember.test.ts file, and execute the following command in your terminal: npx hardhat test test/sablier/CouncilMember.test.ts --grep Vulnerability

it("Vulnerability: Using totalSupply() as tokenId to mint leads to DoS after any token is burnt", async () => {
      // Step 1: Burn NFT with tokenId 1
      await expect(councilMember.burn(1, support.address)).emit(councilMember, "Transfer");
      // Step 2: Confirm that totalSupply() has decreased from 3 to 2
      const totalSupplyAfterBurning = await councilMember.totalSupply()
      await expect(totalSupplyAfterBurning).to.be.eq(2);

      // Step 3: Burning the NFT has decreased totalSupply from 3 to 2. Hence, the next tokenId that will be minted
      // is the NFT with tokenId 2. Because tokenId 2 already exists, minting will always revert
      // with error ERC721InvalidSender()
      await expect(councilMember.mint(holder.address)).to.be.revertedWithCustomError(councilMember, "ERC721InvalidSender")
  });

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L181

Tool used

Manual Review, hardhat

Recommendation

Consider rethinking the approach in which NFTs are minted/tracked. A common decision is to have an internal, increment-only variable that will never be decreased even if new NFTs are minted.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

0xadrii - Denial of service in mint() after burning an NFT #144