Wrong burn logic

Summary

Burning the counselor token does not work as intended and leads to a corrupted state.

Vulnerability Detail

The governance can burn any token on behalf of the owner:

    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);
    }

First, it updates and sends all the accumulated balance to the recipient:

    function _withdrawAll(address from, uint256 tokenId) internal {
        TELCOIN.safeTransfer(from, balances[tokenId]);
        balances[tokenId] = 0;
    }

Then, it assigns the last balance to this same tokenId and pops an array. In the end, it burns the token. However, burning does not swap the indexes, it just sends this token to the zero (0x0) address. Thus the counselor owning the last token ID will lose their balance, and will not be able to claim it while the burned token will have a dead balance.

Impact

The balance shift when burning the token is incorrect and produces the wrong state. If the burned token is not the last, it gets assigned the wrong balance, and the last token will get an invalid claim. A simple test case showcasing this situation is provided in the code snippet section.

Code Snippet

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

A test case when burning the token with a middle index:

  it("the correct removal of middle index is made", async () => {
      await expect(councilMember.mint(member.address)).to.not.reverted;
      await expect(councilMember.mint(support.address)).to.not.reverted;
      await expect(councilMember.mint(await councilMember.getAddress())).to.not.reverted;

      expect(await telcoin.balanceOf(await support.getAddress())).to.equal(0);
      await expect(councilMember.burn(1, holder.address)).to.not.reverted;
      expect(await telcoin.balanceOf(await support.getAddress())).to.equal(0);

      expect(await councilMember.balances(0)).to.equal(183);
      expect(await councilMember.balances(1)).to.equal(0); // 33
      await expect(councilMember.balances(2)).to.not.reverted;
      expect(await telcoin.balanceOf(holder.address)).to.equal(83);
  });

Tool used

Manual Review

Recommendation

Do not pop from the balances array, just leave it with 0 amount and move on.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

HonorLt - Wrong burn logic #215