r0ck3tz - Burn functionality breaks accounting

[sherlock-admin2]

r0ck3tz

high

Burn functionality breaks accounting

Summary

The burn function in the CouncilMember contract is implemented incorrectly, causing a disruption in the usage of the protocol. The balance array items are closely tied to the token identifiers of the contract, but when a token is burned, the balance array becomes corrupted.

Vulnerability Detail

The burn function has been implemented incorrectly and is disrupting the accounting. The contract has been designed in a way that ties the identifier of the NFT to the corresponding balance item with the same identifier. The function currently burns the NFT, but instead of appropriately updating the balance, it replaces the balance of that burned NFT with the balance of the last NFT and then removes the last time from the array. Consequently, the last balance is erroneously transferred from the user and assigned to an identifier that cannot be claimed because the corresponding NFT was already burned and the holder of token with the highest identifier looses balance.

Scenario:

1. There are three users holding tokens with following balances:

   NFTs Ids: 0, 1, 2
   balances: 100, 200, 300

2. The burn function is triggered by targeting a token id 1 - burn(1, address(0x1234). Because of the implementation the balance with id 1 is swapped with the balance of the last element, and the last element is popped out from the array:

   NFTs Ids: 0, burned(1), 2 
   balances: 100, 200

3. The user that holds NFT with id 2 lost his balance.

Impact

Since balances are out of sync with the NFTs identifiers it leads to series of issues:

• The balance of last user is lost with the execution of burn function.
• Depending on the number of burn function execution one or more users does not accrue balance.
• Minting is broken since totalSupply returns the value of the token id that already exists.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is recommended to redesign burn function so it does not break accounting and token identifiers are in sync with the balances.

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because {Also a dupp of 014 with 2 impact but of the same function; so 014 still the best}