BAICE - TokenId collision in `CouncilMember`, user A can claim user B's Telcoin balance

[sherlock-admin]

BAICE

high

TokenId collision in CouncilMember, user A can claim user B's Telcoin balance

Summary

A critical error of token Id collision in CouncilMember, the arrangement of tokenId is out of order .

Vulnerability Detail

When GOVERNANCE_COUNCIL_ROLE burn a specfied tokenId's NFT in CouncilMember, update the balance's to a new balance .

  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

TokenId

0, 1, 2, 3, 4

balances

[0, 10, 20, 30, 40]

After burn tokenId of 2, the tokenId 4 will take place of tokenId 2 .

TokenId

0,1,3,4

balances

[0, 10, 40, 30]

but at now , the owner of tokenId is not changed, and claim method still use array's id -> balance , so now tokenId's pre owner can claim current tokenId 4's 40 telcoin balance .

Impact

Logic error, user A can claim user B's Telcoin balance .

Code Snippet

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

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

Claim Token https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L92C2-L105C11

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

Tool used

Manual Review, Vscode

Recommendation

Use mapping to store user's balance, array is not safe .

 // current uncliamed members balances
  @-  uint256[] public balances;
  @+  mapping(uint256 => uint256) public balances;

Duplicate of #199

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { this is valid and a dupp of 109 due to same underlying cause}