search

sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

sobieski - Minting new CouncilMemeber NFTs is no longer possible after burning any but last of the NFTs #31

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

sobieski

high

Minting new CouncilMemeber NFTs is no longer possible after burning any but last of the NFTs

Summary

The CouncilMember::mint() method utilizes totalSupply() call to determine the new NFT index. Because in the process of burning NFT totalSupply is decreased, if any of the NFTs (excluding the last one) gets burned, no subsequent mints will be possible, as the contract will try to mint the already existing token ID.

Vulnerability Detail

When new CouncilMember token gets minted, total supply is used to determine which index should the new token get.

_mint(newMember, totalSupply());

The issue is that the totalSupply can be decreased by the CouncilMember::burn() method call. As such, if a token gets burned, subsequent CouncilMember::mint() will revert.

Please consider the following scenario:

  1. Alice mints NFT ID 0. Total supply gets updated to 1.
  2. Bob mints NFT ID 1. Total supply gets updated to 2.
  3. Alice burns her NFT. Total supply gets updated to 1.
  4. Charlie wants to mint a NFT. The CouncilMember::mint() will try to mint NFT ID 1 for him. This will revert, as this ID is already owned by Bob.

The POC for the issue is provided below. Please paste it into CouncilMember.test.ts test suite.

POC

describe("POC", () => {
            it.only("Burning any NFT locks the last NFT owner from their balance", async () => {
                /* 1. Mint 3 NFTs */
                await expect(councilMember.mint(member.address)).to.not.reverted;
                await expect(councilMember.mint(support.address)).to.not.reverted;
                await expect(councilMember.mint(holder.address)).to.not.reverted;
                /* 2. Burn one NFT */
                await councilMember.burn(1, support.address);
                //* 3. No new NFTs can be minted, as the contract attempts to mint ID 1 again */
                await expect(councilMember.mint(member.address)).to.be.revertedWithCustomError(councilMember, "ERC721InvalidSender");
            });            
})

Please note that burning the NFT with the highest ID so far will not have the aforementioned consequences, as this ID will no longer be owned. The issue applies to burning any but last of the NFTs.

Impact

High, as the core functionality of the protocol will be DOSed forever.

Code Snippet

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

Tool used

Manual Review

Recommendation

Keep track of the number of NFTs minted so far and use that number to determine the new index instead of totalSupply.

Duplicate of #199

sherlock-admin2 commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { It is also a dupp of (014) but with minimal impact and min recommendation thus (014) remians the best}