m4ttm - Users balances are incorrectly swapped when a CouncilMember is burned

[sherlock-admin2]

m4ttm

high

Users balances are incorrectly swapped when a CouncilMember is burned

Summary

When a CouncilMember is burned, their balance is set to the balance of the token at the end of the array. This causes users to have the incorrect balance.

Vulnerability Detail

The balance of the tokenId being burned is set to the balance of the last, then it is popped. This causes the balance of the last tokenId to be incorrectly lost if the last NFT is not the one being burned.

Impact

If the CouncilMember at the end of the array is not being burned, their balance will be lost and transferred to the position of a non existant NFT. The other user will lose any unclaimed rewards.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Change the burn function to only allow burning the last ID.

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { dupp of 109 due to same underlying cause of burn function}