The last council member cannot claim his allocated TELCOIN after someone's token is burnt.

Summary

When burning a token, the last council member lost his balance, and cannot claim his allocated TELCOIN.

Vulnerability Detail

When burning a target tokenId, the balances of that tokenId will be cleared. It will be set to 0 in _withdrawAll, this is fine. But the tokenId is then poped from the balances array in the burn() function. Actually it is the last tokenId is poped, and the last tokenId's balance is set to the target tokenId. This creates a problem that the last tokenId's owner will lost his balance in balances array, and will not be able to claim his allocated TELCOIN.

210:    function burn(
211:        uint256 tokenId,
212:        address recipient
213:    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
214:        require(totalSupply() > 1, "CouncilMember: must maintain council");
215:        _retrieve();
216:        _withdrawAll(recipient, tokenId);
217:
218:->      uint256 balance = balances[balances.length - 1];
219:->      balances[tokenId] = balance;
220:->      balances.pop();
221:        _burn(tokenId);
222:    }

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

POC: The following test case can show the problem. Add the test case to CouncilMember.test.ts#CouncilMember.tokenomics and run it.

        describe.only("claim_after_burn", () => {
            it("claim after burn revert", async () => {
                // init the addresses of member0, member1, member2 in `beforeEach`

                // mint: token0 for member0, token1 for member1, token2 for member2
                await expect(councilMember.mint(member0.address)).to.not.reverted;
                await expect(councilMember.mint(member1.address)).to.not.reverted;
                await expect(councilMember.mint(member2.address)).to.not.reverted;
                expect(await telcoin.balanceOf(await councilMember.getAddress())).to.equal(200);

                await expect(councilMember.burn(1, holder.address)).to.not.reverted;

                console.log("balance 0: ", await councilMember.balances(0));
                console.log("balance 1: ", await councilMember.balances(1));

                expect(await councilMember.ownerOf(2)).to.be.equal(await member2.address);

                // councilMember.balances.length is 2, claim id 2 will be reverted as out-of-bounds
                await expect(councilMember.connect(member2).claim(2, 1)).to.be.reverted;
            });
        });

The result of the test case is:

  CouncilMember
    tokenomics
      claim_after_burn
balance 0:  183n
balance 1:  33n
        ✔ claim after burn revert (266ms)

Impact

The last council member cannot claim his allocated TELCOIN after some other tokenId is burnt.

Code Snippet

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

Tool used

Manual Review

Recommendation

Do not pop tokenId from balances arrary while burning, i.e. remove the lines of CouncilMember.sol#L218-L220.

210:    function burn(
211:        uint256 tokenId,
212:        address recipient
213:    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
214:        require(totalSupply() > 1, "CouncilMember: must maintain council");
215:        _retrieve();
216:        _withdrawAll(recipient, tokenId);
217:
218:->      uint256 balance = balances[balances.length - 1];
219:->      balances[tokenId] = balance;
220:->      balances.pop();
221:        _burn(tokenId);
222:    }

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

ydlee - The last council member cannot claim his allocated TELCOIN after someone's token is burnt. #235