bitsurfer - burn will remove last `tokenId` balance, resulting user who own last `tokenId` can't claim their balance

[sherlock-admin]

bitsurfer

high

burn will remove last tokenId balance, resulting user who own last tokenId can't claim their balance

Summary

Burn logic contains flaw, it will remove (or clear out) last balance (which is not tokenId being burned). User who own this last tokenId will failed to claim due to their balance is popped out.

Vulnerability Detail

In CouncilMember burn function, there is a swap position of balance amount, from the last tokenId balance to the burned tokenId which is wrong.

On line 219, balances[tokenId] = balance; is a clear example this logic is wrong, setting balance of a tokenId which is being burned. While in fact in claim() function a user will claim balance of tokenId if they are the owner of that tokenid.

Swapping balance from last balance array to the removed (burned) tokenId is completely wrong.

Another result of this logic is, the last balance in balances array (which is owned by last tokenId) will be pop-ed, and its value is cleared. Thus when user of last tokenId want to claim, they are unable to do because their balance is popped out resulting they can't claim what they should have.

File: CouncilMember.sol
210:     function burn(
211:         uint256 tokenId,
212:         address recipient
213:     ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
...
218:         uint256 balance = balances[balances.length - 1];
219:         balances[tokenId] = balance;
220:         balances.pop();
221:         _burn(tokenId);
222:     }

Impact

Last tokenId's balance which is not being burned is cleared out, thus owner of that tokenId can't claim what they should have.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider to clear up the balances of burned tokenId instead of overwriting it with last balance of tokenId, and remove the balances pop

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}