Incorrect Balance Is Popped While Burn

Summary

The logic for burn is incorrect where the incorrect entry in the balances array would be popped.

Vulnerability Detail

1.) The logic for burn is as follows

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

2.) Here tokenId is burnt and the rewards for that tokenId is sent to some receiver. Let's say we want to remove tokenId 3 (and total are 5 in the balances array)

3.) At L216 the balance for the removed tokenId is sent to the receiver.

4.) At L 218 the balance of the last id (5 in our case) is fetched and assigned i.e

uint256 balance = balances[balances.length - 1]; so balance holds the balance of tokenId 5

5.) This balance is assigned to the removed tokenId ->

balances[tokenId] = balance;

Meaning balance for tokenId 3 is assigned the balance of tokenId 5

Finally we pop therefore tokenId 5 is popped.

The final state is -> a.) tokenId 3 still remains (which was meant to be popped) , b.) balances[3] holds the balance for tokenId 5 (which is incorrect , tokenId 5 should hold the balance for 5) c.) TokenId 5 is popped (incorrect , 3 was meant to be popped)

Impact

Flawed burn logic would result in incorrect balances being popped

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

The logic should be corrected as demonstrated.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

sakshamguruji - Incorrect Balance Is Popped While Burn #213