Liquid Staking

Summary

There's vulnerability in the deposit function that allows unused deposits to exceed the unusedDepositLimit when _amount is 0. This issue can lead to reduced efficiency, increased risk, and violation of expected behavior.

Here's how and why it happens:

1. The unusedDepositLimit is set during the initialization of the StakingPool contract. This limit represents the maximum number of tokens that can sit in the pool outside of a strategy.

2. When the deposit function is called with _amount greater than 0, it transfers the tokens from the caller to the pool, calls _depositLiquidity to deposit any unused balance into strategies, mints liquid staking tokens to the _account, and updates totalStaked.

3. However, when deposit is called with _amount equal to 0, it skips the token transfer and minting steps and directly calls _depositLiquidity. This allows depositing the pool's existing unused balance into strategies, even if that balance exceeds the unusedDepositLimit.

4. After calling _depositLiquidity, the function does not check if the remaining unused deposits (i.e., tokens still sitting in the pool) exceed the unusedDepositLimit. This is the core issue.

Specifically in the else block that handles the case when _amount is 0: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L125-L127

This code path connects to the _depositLiquidity function, which is responsible for depositing the pool's unused balance into strategies.

To reproduce the issue:

1. When deployed, the StakingPool contract with a non-zero unusedDepositLimit.

2. Call the donateTokens function to send some tokens to the pool, causing the unused deposits to exceed the limit.

3. Add a strategy to the pool using addStrategy.

4. Call the deposit function with _amount set to 0 and some arbitrary _data.

5. Observe that the transaction succeeds, even though the unused deposits exceed the unusedDepositLimit.

This bug affects users by allowing the pool to hold more unused deposits than intended, potentially impacting the pool's efficiency and risk profile. It bypasses the unusedDepositLimit safeguard, which is meant to ensure that the pool does not accumulate excessive idle funds.

Vulnerability Details

The StakingPool contract has a vulnerability in the deposit function that allows the unused deposits in the pool to exceed the unusedDepositLimit set during initialization. This issue arises when the deposit function is called with _amount equal to 0.

The root cause of the vulnerability lies in the logic of the deposit function: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L120-L127

When _amount is 0, the function directly calls _depositLiquidity without checking if the resulting unused deposits exceed the unusedDepositLimit. This allows the pool to hold more unused deposits than intended, bypassing the limit set to ensure the pool's efficiency and risk management.

The vulnerability is particularly concerning because it can be triggered by any account with the onlyPriorityPool role.

Impact

The vulnerability allows the StakingPool contract to hold more unused deposits than intended, potentially leading to:

1. Reduced efficiency of the pool, as excessive idle funds are not being utilized effectively.

2. Increased risk profile of the pool, as the unused deposits may be subject to security risks or market fluctuations.

3. Violation of the expected behavior and limits set by the contract owner during initialization.

Tools Used

Vs

Recommendations

We recommend updating the deposit function to enforce the limit consistently and implementing access control measures to mitigate the risk of exploitation.