tives - When burning a council member NFT in the middle of the array, then the balance moving logic is incorrect.

[sherlock-admin]

tives

high

When burning a council member NFT in the middle of the array, then the balance moving logic is incorrect.

Summary

When burning a council member NFT in the middle of the array, then the balance moving logic is incorrect.

Vulnerability Detail

In the burn function, a random council member NFT can be burned.

function burn(
        uint256 tokenId,
        address recipient
    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
    ...
    uint256 balance = balances[balances.length - 1];
    balances[tokenId] = balance;
    balances.pop();

The tokenIds run in order, according to the mint function

function mint(
    address newMember
) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
    if (totalSupply() != 0) {
        _retrieve();
    }

    balances.push(0);
    _mint(newMember, totalSupply());
}

As you can see, the member balances is set as the id of the NFT token.

Now, if you burn the token at 5/10, then there are 2 problems

1. the balances[5] will be set as balances[10]

   uint256 balance = balances[balances.length - 1];
   balances[tokenId] = balance;

2. The balances[10] will be removed. Therefore, tokens will be deleted from a wrong member.

Impact

Some council members will have their balances changed to wrong balance after burning of a council member.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

This is a starting idea for a fix. But please note that then new issue arises when _retrieve will start refilling the balance for the burned NFT.

1. Set the balance of the correct token
2. Don’t pop from the array. Just keep the 0 balance for the burned token.
   • Please note

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[tokenId] = 0;
-    balances.pop();
    _burn(tokenId);
}

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { valid and a dupp of 109}