CouncilMember:burn renders the contract inoperable after the first execution

Summary

The CouncilMember contract suffers from a critical vulnerability that misaligns the balances array after a successful burn, rendering the contract inoperable.

Vulnerability Detail

The root cause of the vulnerability is that the burn function incorrectly manages the balances array, shortening it by one each time an ERC721 token is burned while the latest minted NFT still withholds its unique tokenId which maps to the previous value of balances.length.

// File: telcoin-audit/contracts/sablier/core/CouncilMember.sol
210:    function burn(
        ...
220:        balances.pop(); // <= FOUND: balances.length decreases, while latest minted nft withold its unique tokenId
221:        _burn(tokenId);
222:    }

This misalignment between existing tokenIds and the balances array results in several critical impacts:

Holders with tokenId greater than the length of balances cannot claim.
Subsequent burns of tokenId greater than balances length will revert.

Subsequent mint operations will revert due to tokenId collision. As totalSupply now collides with the existing tokenId.

// File: telcoin-audit/contracts/sablier/core/CouncilMember.sol
173:    function mint(
    ...
179:
180:        balances.push(0);
181:        _mint(newMember, totalSupply());// <= FOUND
182:    }

This mismanagement creates a cascading effect, collectively rendering the contract inoperable. Following POC will demonstrate the issue more clearly in codes.

POC

Run git apply on the following patch then run npx hardhat test to run the POC.

diff --git a/telcoin-audit/test/sablier/CouncilMember.test.ts b/telcoin-audit/test/sablier/CouncilMember.test.ts
index 675b89d..ab96b08 100644
--- a/telcoin-audit/test/sablier/CouncilMember.test.ts
+++ b/telcoin-audit/test/sablier/CouncilMember.test.ts
@@ -1,13 +1,14 @@
 import { expect } from "chai";
 import { ethers } from "hardhat";
 import { SignerWithAddress } from "@nomicfoundation/hardhat-ethers/signers";
-import { CouncilMember, TestTelcoin, TestStream } from "../../typechain-types";
+import { CouncilMember, TestTelcoin, TestStream, ERC721Upgradeable__factory } from "../../typechain-types";

 describe("CouncilMember", () => {
     let admin: SignerWithAddress;
     let support: SignerWithAddress;
     let member: SignerWithAddress;
     let holder: SignerWithAddress;
+    let lastCouncilMember: SignerWithAddress;
     let councilMember: CouncilMember;
     let telcoin: TestTelcoin;
     let stream: TestStream;
@@ -18,7 +19,7 @@ describe("CouncilMember", () => {
     let supportRole: string = ethers.keccak256(ethers.toUtf8Bytes("SUPPORT_ROLE"));

     beforeEach(async () => {
-        [admin, support, member, holder, target] = await ethers.getSigners();
+        [admin, support, member, holder, target, lastCouncilMember] = await ethers.getSigners();

         const TestTelcoinFactory = await ethers.getContractFactory("TestTelcoin", admin);
         telcoin = await TestTelcoinFactory.deploy(admin.address);
@@ -182,6 +183,22 @@ describe("CouncilMember", () => {
                 it("the correct removal is made", async () => {
                     await expect(councilMember.burn(1, support.address)).emit(councilMember, "Transfer");
                 });
+                it.only("inoperable contract after burn", async () => {
+                    await expect(councilMember.mint(lastCouncilMember.address)).to.not.reverted;
+
+                    // This 1st burn will cause contract inoperable due to tokenId & balances misalignment
+                    await expect(councilMember.burn(1, support.address)).emit(councilMember, "Transfer");
+
+                    // Impact 1. holder with tokenId > balances length cannot claim
+                    await expect(councilMember.connect(lastCouncilMember).claim(3, 1)).to.revertedWithPanic("0x32"); // @audit-info 0x32: Array accessed at an out-of-bounds or negative index
+
+                    // Impact 2. subsequent burns of tokenId > balances length will revert
+                    await expect(councilMember.burn(3, lastCouncilMember.address)).to.revertedWithPanic("0x32"); 
+
+                    // Impact 3. subsequent mint will revert due to tokenId collision
+                    await expect(councilMember.mint(lastCouncilMember.address)).to.revertedWithCustomError(councilMember, "ERC721InvalidSender");
+
+                });
             });
         });

Result

CouncilMember mutative burn Success ✔ inoperable contract after burn (90ms) 1 passing (888ms)

The Passing execution of the POC confirmed that operations such as claim, burn & mint were all reverted which make the contract inoperable.

Impact

The severity of the vulnerability is high due to the high likelihood of occurence and the critical impacts on the contract's operability and token holders' ability to interact with their assets.

Code Snippet

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

Tool used

VsCode

Recommendation

It is recommended to avoid popping out balances to keep alignment with uniquely minted tokenId. Alternatively, consider migrating to ERC1155, which inherently manages a built-in balance for each NFT.

sherlock-admin2 commented 9 months ago

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

takarez commented:

valid because { this is a valid findings because the watson explain how again the burn function will break a functionality just like the previous issue thus making it a dupp of 109}