Governance council can withdraw and burn the wrong tokenId

Summary

Vulnerability Detail

When burn() or removeFromOffice() is called, _withdrawAll() is also called.

    function removeFromOffice(
        address from,
        address to,
        uint256 tokenId,
        address rewardRecipient
    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
        // Retrieve and distribute any pending TELCOIN for all council members
        _retrieve();
        // Withdraw all the TELCOIN rewards for the specified token to the rewardRecipient
  @     _withdrawAll(rewardRecipient, tokenId);
        // Transfer the token (representing the council membership) from one address to another
        _transfer(from, to, tokenId);
    }

_withdrawAll() will withdraw the balance of the tokenId and transfer it to the address stated in the parameter.

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

Note that the address and the tokenId is not checked to be the same person. If council member A has a token id of 1 and is getting removed from office, instead of withdrawing his tokenId 1, if tokenId 5 from council member B is specified, then the balance of tokenId 5 will be withdrawn instead.

Since it is an irreversible execution, council member B will never get his token back and his token will even get sent to another addrss or burned accidentally.

In the claim() function, there is a check to show that the msg.sender is the owner of the tokenId.

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

Impact

If wrong account is specified, then wrong token will be burned because the functions do not check that the person having their token burned is actually the owner of the tokenID

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin-cryptostaker2/blob/475e5c92315d0b6cc1aa185a856fb8c24d993040/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L122-L134

Tool used

Manual Review

Recommendation

Before burning the token, check that the recipient is the owner of the tokenId.

    function burn(
        uint256 tokenId,
        address recipient
    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
++    require(
            recipient == ownerOf(tokenId),
            "CouncilMember: caller is not council member holding this NFT index"
        );
        require(totalSupply() > 1, "CouncilMember: must maintain council");
        _retrieve();

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

Jaraxxus - Governance council can withdraw and burn the wrong tokenId #197