ravikiran.web3 - CouncilMember::burn() function is incorrectly implemented

[sherlock-admin2]

ravikiran.web3

high

CouncilMember::burn() function is incorrectly implemented

Summary

CouncilMember::burn() function is incorrectly implemented as it updates the balance for a token that is being currently burnt.

Vulnerability Detail

In the burn function, the Telcoin balances for all the councilMembers are updated wit retrieve function call. Immediately after that, the TelCoin for the tokenId is withdrawn to the recipient address.

This clears the balances of the tokenId that is being burnt.
So, what is left is to update the balances array by removing the tokenId from the array and then burning the token itself.

Refer to the below code snippet, where the above intention is implemented. Instead of remove the tokenId from the array, the tokenId balance is updated with the balance of the last element in the array.

       _retrieve();
        _withdrawAll(recipient, tokenId);

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

Impact

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

using Array is not a good approach here. It is recommended to use map with tokenId -> balance.
This way, deletion will be much more easier and simpler.

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because {its a dupp of 109 but without the impact beign mentioned}