Incorrect removal of a council member

Summary

Incorrect removal of a council member in "burn" function. github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L210-L222

Vulnerability Detail

Burn function allows to remove a council member by burning his NFT. His position in the array is replaced by the position of the last member of the array, and the last position is removed.

        uint256 balance = balances[balances.length - 1];
        balances[tokenId] = balance;
        balances.pop();
        _burn(tokenId);;

The issue is that the balance of the last council member is transferred to the position of the removed one, his tokenId does not change. Thus, when the "claim" function is called, the last tokenId member will not be able to receive funds, since the position according to the index of his NFT is deleted, and the tokenId, which represents the new position of the last tokenId member in the array, is burned.

Impact

Lock of telcoin balance of the last council member in "balances[tokenId]" array.

Code Snippet

First, add the new roles to the CouncilMember.test.ts, then modify the test as follows. This test shows that the third user cannot withdraw his funds. Even if he specifies a deleted tokenId that represents his new position, the function will also revert since he is not the owner of this NFT.

let member1: SignerWithAddress;
let member2: SignerWithAddress;

[admin, support, member, member1, member2, holder, target] = await ethers.getSigners();

describe("burn", () => {
            it("PoC", async () => {
                //creating 3 council members
                await expect(councilMember.mint(member.address)).to.not.reverted;
                await expect(councilMember.mint(member1.address)).to.not.reverted;
                await expect(councilMember.mint(member2.address)).to.not.reverted;

                expect(await councilMember.balances(0)).to.equal(150);
                expect(await councilMember.balances(1)).to.equal(50);
                expect(await councilMember.balances(2)).to.equal(0);

                //deleting 1st council Member
                await expect(councilMember.burn(0, holder.address)).to.not.reverted;

                expect(await councilMember.balances(0)).to.equal(33);
                expect(await councilMember.balances(1)).to.equal(83);

                //3rd council Member trying to claim telcoin
                await expect(councilMember.connect(member2).claim(2, 10)).to.be.reverted;
            });
        });

Tool used

Manual review.

Recommendation

Recommended not only to replace the last position with the deleted one in the array, but also to change the tokenId. It is necessary to first forward the NFT of the deleted member to the last one, and burn the NFT of the last one.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

DenTonylifer - Incorrect removal of a council member #193