The owner of the last NFT is ineligible to claim his TELCOIN if other NFTs have been burned

Summary

The owner of the last NFT is ineligible to claim his TELCOIN if other NFTs have been burned, because that the burn function will pop the last element in balances, the access to balances[lastNFTId] will result in an out of range.

Vulnerability Detail

The burn function will swap the burned NFT and the last NFT in balances, and then pop the last one. However, the NFT id is still equal to the origin length of balances, so any access to the balances[lastNFTId] will revert.

    function burn(
        uint256 tokenId,
        address recipient
    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
        require(totalSupply() > 1, "CouncilMember: must maintain council");
        _retrieve();
        _withdrawAll(recipient, tokenId);

        uint256 balance = balances[balances.length - 1];
        balances[tokenId] = balance;
        balances.pop();
        _burn(tokenId);
    }

For example, if there are three NFTs [0,1,2] in the contract, and the NFT1 is burned, the NFT2 can never get his unclaimed TELCOIN because the length of balances is 2, there are no more balances[2]. Add the test to telcoin-audit/test/sablier/CouncilMember.test.ts and run it with npx hardhat test.

diff --git a/telcoin-audit/test/sablier/CouncilMember.test.ts b/telcoin-audit/test/sablier/CouncilMember.test.ts
index 675b89d..9e23d4a 100644
--- a/telcoin-audit/test/sablier/CouncilMember.test.ts
+++ b/telcoin-audit/test/sablier/CouncilMember.test.ts
@@ -308,6 +308,23 @@ describe("CouncilMember", () => {
                 expect(await telcoin.balanceOf(support.address)).to.equal(0);
                 expect(await telcoin.balanceOf(await councilMember.getAddress())).to.equal(200);
             });
+            it.only("claiming rewards1", async () => {
+                // mint 0,1,2
+                await expect(councilMember.mint(member.address));
+                await expect(councilMember.mint(support.address));
+                await expect(councilMember.mint(member.address));
+                // retrieve
+                await councilMember.retrieve();
+                const x = await councilMember.balances(0);
+                const y = await councilMember.balances(1);
+                const z = await councilMember.balances(2);
+                console.log("bal ",x,y,z);
+                //burn 1
+                await councilMember.burn(1, support.address);
+                //there are only two elements in balances, so NFT2 can never get his rewards because there are no balances[2]
+                await (councilMember.connect(member).claim(2, 10));
+
+            });
         });

         describe("retrieve", () => {

Impact

The owner of last NFT can't claim any TELCOIN if there are other NFTs are burned.

Code Snippet

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

Tool used

hardhat

Recommendation

It's recommended to add a map to record the new index of balances for NFT.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

Krace - The owner of the last NFT is ineligible to claim his TELCOIN if other NFTs have been burned #130