bitsurfer - Wrong logic in `_retrieve` function on increasing balances by using loop with sequential index

[sherlock-admin]

bitsurfer

high

Wrong logic in _retrieve function on increasing balances by using loop with sequential index

Summary

Wrong logic in _retrieve function on increasing balances by using loop with sequential index, meanwhile there will be tokenId burned, thus the balances of this is stuck unclaimed, this also reducing other user potential balance.

Vulnerability Detail

balances is an array which is intended to store amount of token can be claimed by tokenId. For example if user own tokenId of #3 then he can claim whatever amount in balances[3].

The _retrieve() function in CouncilMember contains a wrong logic on increasing the balances array.

Looking at line 292 below, the update balance is wrong because the balances is updated using loop through the balances index.

Simply, for example if tokenId #3 is burned, the balances[3] should not get updated but instead current code will update this balances[3] (assuming balances.length is > 3)

File: CouncilMember.sol
267:     function _retrieve() internal {
...
291:         // Add the individual balance to each council member's balance
292:         for (uint i = 0; i < balances.length; i++) {
293:             balances[i] += individualBalance;
294:         }
295:     }

Impact

Burned tokenId balance will be increased decreasing the other user balance, also, (there will be balances of tokenId not getting updated). This burned tokenId which its balance is updated in _retrieve can't be claimed, thus locked in the contract.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L292-L294

Tool used

Manual Review

Recommendation

Consider to use another pointer for updating this balance instead of sequential update using loop, to prevent burned tokenId getting updated and last tokenId not getting this updated balance.

Duplicate of #199

[amshirif]

The issue is not as described with the reward calculation but is actually the burning of tokens. That issue is fixed here. https://github.com/telcoin/telcoin-audit/pull/31

[sherlock-admin2]

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

takarez commented:

valid because { I consider this valid a a dupp of 109 issue due to the same underlying cause of the burn function; the watson explain how burned NFT will still result in it getting amount of tokens when the retieve() function is called and in a situation where the burned NFT is not the last in the array}

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/31.