sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

sonny2k - Removing the item in array without preserving the array index making other functions unusable in CouncilMember.sol: burn() and StakingRewardsManager.sol: removeStakingRewardsContract() #244

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

sonny2k

high

Removing the item in array without preserving the array index making other functions unusable in CouncilMember.sol: burn() and StakingRewardsManager.sol: removeStakingRewardsContract()

Summary

In burn() and removeStakingRewardsContract(), the protocol only delete the item in the array without preserving the order of the elements.

Vulnerability Detail

Without preserving the order of the array, the protocol removes the last item value which is not the index we want to delete, leaving other functions when calling the item with that index rendered unusable

Impact

The last item is removed, and the intentional deleting item remains with the last item value. Which is wrong.

Code Snippet

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

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L166

Tool used

Manual Review

Recommendation

Implement a correct logic of deleting item while preserving the sequence:

function deleteItemPreserve(uint _index) public {
        for (uint i = _index; i < arr.length - 1; i++) {
            arr[i] = arr[i + 1];
        }
        arr.pop();
 }

Duplicate of #199

sherlock-admin2 commented 5 months ago

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

takarez commented:

valid because {valid and same with issue 109 due to same underlying cause}