popeye - Incorrect Balance Assignment When Burning Non-Sequential Tokens in `CouncilMember::burn`

[sherlock-admin]

popeye

high

Incorrect Balance Assignment When Burning Non-Sequential Tokens in CouncilMember::burn

Summary

The burn() function in the CouncilMember contract contains logic that incorrectly assigns the balance of the last token to any token being burned. This leads to incorrect balances when burning non-sequential tokens.

Vulnerability Detail

The burn() function calls _withdrawAll() to send the balance of the burned token to the target address. It then does:

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

This takes the balance of the last token and assigns it to the token being burned. If the token being burned is not the last in the enumeration order, it will replace the correct balance with the balance of the last token.

Impact

This will directly affect the balances mapping, leading to incorrect balances being reflected for users. When non-sequential tokens are burned, their balances will be overwritten with the balance of the last token.

For example, if there are 3 tokens with balances 100, 200, 300 respectively. Burning token 1 would replace its balance of 100 with the balance of the last token, 300.

Code Snippet

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

Proof of Concept

Let's assume:

• Alice has token 1 with balance 100
• Bob has token 2 with balance 200
• Eve has token 3 with balance 300

If Bob's token 2 is burned:

burn(2, bobAddress);

This will replace token 2's balance with Eve's token balance:

balances[2] = balances[balances.length - 1] = 300;

Now the balances mapping will be: Alice: 100 Bob: 300 Eve: 300

Clearly incorrect since Bob's original balance was 200.

Tool used

Manual Review

Recommendation

The balance assignment logic should be fixed to explicitly zero-out the balance for the burned tokenId:

function burn(uint256 tokenId, address recipient) external {
  // ...
  balances[tokenId] = 0;
  // ...
}

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { this is also a dupp of 109 with a minimal impact}