Liquid Staking

Summary

The performUpkeep function in the LSTRewardsSplitter contract has a bug that can lead to unexpected decreases in users' principal deposits. When the contract's lst token balance is less than principalDeposits, which can happen due to erroneous transfers or bugs in the lst token contract.

When newRewards is negative, i.e., when the lst token balance of the contract is less than the principalDeposits. In this case, the code decreases principalDeposits by the absolute value of newRewards.

This is problematic because principalDeposits is supposed to represent the total principal deposited by users. By modifying it in performUpkeep, the contract is essentially writing off the difference as a loss, which is incorrect accounting.

This can happen in a few scenarios

1. If tokens are mistakenly or maliciously transferred out of the contract directly (not via withdraw), the balance will be less than principalDeposits.

2. If there's a bug in the lst token contract that allows its balance to decrease without a corresponding transfer (e.g., a fee or burn mechanism), it could lead to this state.

The bug affects users because it means their principal deposits can be decreased without their consent or knowledge. If this happens repeatedly, users' funds could be slowly drained.

To reproduce:

1. Have a user deposit funds via deposit.

2. Artificially decrease the contract's lst balance (e.g., by directly transferring tokens out or invoking a burn/fee mechanism if one exists).

3. Call performUpkeep.

4. Observe that principalDeposits has decreased.

Vulnerability Details

As a proof of concept

In the LSTRewardsSplitter contract, the performUpkeep function contains a bug that can lead to unexpected decreases in the principalDeposits variable, which represents the total principal deposited by users.

The issue occurs when the lst token balance of the contract is less than principalDeposits. In this case, the code decreases principalDeposits by the absolute value of the difference: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L101-L110

The problematic code is: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L103-L104

This code unexpectedly decreases principalDeposits when newRewards is negative, which can happen if the contract's lst token balance is less than principalDeposits. This can lead to loss of user funds.

To demonstrate the issue

1. User deposits funds via deposit, e.g., deposit(100).

2. Artificially decrease the contract's lst balance, e.g., by directly transferring 10 tokens out: lst.transfer(otherAddress, 10).

3. Call performUpkeep(bytes("")).

4. Observe that principalDeposits has decreased to 90, even though the user only deposited 100 and never withdrew.

Impact

This bug allows users' principal deposits to be decreased without their consent or knowledge. If this happens repeatedly, users' funds could be slowly drained, leading to unexpected losses.

Tools Used

Vs Code

Recommendations

The if (newRewards < 0) case should be removed from performUpkeep, and principalDeposits should only be modified in the deposit and withdraw functions.