Burning the CouncilMember token will break the contract

Summary

When burn() is called the last balances array element is copied into the place to remove. This will create problems because the id of the last token will be different than its index in the balances array.

Vulnerability Detail

When a token is being burned the id is being removed from the balances array by copying the last array element into the place of the burned token. This will break the contract because the balances index of the token will not correspond to the id of the token that a council member owns.

For example: we have 5 NFTs and the NFT with id 0 is being burned. The council member that owns the NFT with id 4 will now have his accumulated balance in balances[0] but he still owns a NFT with the id 4. Then when he wants to claim he will call claim() with the id 4:

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

function claim(uint256 tokenId, uint256 amount) external {
        // Ensure the function caller is the owner of the token (council member) they're trying to claim for
        require(
            _msgSender() == ownerOf(tokenId),
            "CouncilMember: caller is not council member holding this NFT index"
        );
        // Retrieve and distribute any pending TELCOIN for all council members
        _retrieve();

        // Ensure the requested amount doesn't exceed the balance of the council member
        require(
            amount <= balances[tokenId],
            "CouncilMember: withdrawal amount is higher than balance"
        );

        // Deduct the claimed amount from the token's balance
        balances[tokenId] -= amount;
        // Safely transfer the claimed amount of TELCOIN to the function caller
        TELCOIN.safeTransfer(_msgSender(), amount);
    }

The problem is that the council member owns an NFT with id 4 but balances[4] does not exist and this will cause the tx to revert. Because the id doesnt correspond to the index in the balances array, other functions like removeFromOffice() and burn() will fail too.

Impact

The council member will fail to claim his accumulated amount because when calling claim() the function will always revert. The admin will also fail to burn or remove this token from office.

Code Snippet

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

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

As you can see when burning this will make the last token id to be different from its balances index.

Tool used

Manual Review

Recommendation

One way to fix this would be to burn the last token id, and transfer the token that is being burned to the owner of the last token id.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

Arz - Burning the CouncilMember token will break the contract #106