Liquid Staking

Summary

The deposit function in StakingPool.sol allows tokens to be deposited into the contract even when there is insufficient room in the strategies to handle the deposit. This can lead to a situation where the unused deposits in the contract exceed the unusedDepositLimit.

Here's how it happens:

1. The deposit function first checks if there is at least one strategy available using require(strategies.length > 0, "Must be > 0 strategies to stake"). However, this check only ensures the existence of a strategy and does not guarantee that the strategy has enough room to accept the deposit.

2. If the _amount is greater than zero, the tokens are transferred from the caller to the contract using token.safeTransferFrom(msg.sender, address(this), _amount).

3. The _depositLiquidity function is then called to deposit the tokens into the available strategies. However, if the strategies do not have enough room to accept all the deposited tokens, some tokens will remain in the contract as unused deposits.

4. The _mint function is called to mint liquid staking tokens to the _account, and the totalStaked variable is incremented by the _amount.

5. Finally, the function checks if the ending balance of the contract exceeds both the starting balance and the unusedDepositLimit. If true, it reverts with an InvalidDeposit error. However, this check is performed after the tokens have already been deposited and minted.

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L120-L127

The issue is connected to the _depositLiquidity function, which is responsible for depositing tokens into the strategies. If the strategies cannot accept all the tokens, the remaining tokens will stay in the contract, potentially exceeding the unusedDepositLimit.

Vulnerability Details

The issue arises because the deposit function transfers the tokens from the user to the contract before attempting to deposit them into the strategies. If the strategies do not have enough room to accept all the deposited tokens, the remaining tokens will stay in the contract as unused deposits. The function then mints liquid staking tokens to the user and increments the totalStaked variable by the full deposit amount, regardless of whether all the tokens were successfully deposited into the strategies.

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L111-L132

Stems from the following issues

1. token.safeTransferFrom(msg.sender, address(this), _amount);: The tokens are transferred from the user to the contract before ensuring that they can be successfully deposited into the strategies. If the strategies cannot accept the full deposit amount, the remaining tokens will stay in the contract as unused deposits.

2. _mint(_account, _amount);: The liquid staking tokens are minted to the user's account regardless of whether the full deposit amount was successfully deposited into the strategies. This can lead to a mismatch between the minted tokens and the actual deposited amount.

3. totalStaked += _amount;: The totalStaked variable is incremented by the full deposit amount, even if some of the tokens remain as unused deposits in the contract. This can cause inconsistencies in the accounting of staked tokens.

The _depositLiquidity function is responsible for depositing tokens into the strategies, but it does not ensure that all the tokens are successfully deposited. As a result, the contract can end up holding more unused deposits than the specified unusedDepositLimit.

Consider the following scenario

1. The StakingPool contract is initialized with unusedDepositLimit set to a low value, e.g., 100 tokens.

2. A strategy is added to the pool with a deposit limit of 500 tokens.

3. A user calls the deposit function with an amount of 1000 tokens.

4. The deposit function transfers 1000 tokens from the user to the contract.

5. The _depositLiquidity function attempts to deposit the tokens into the strategy but can only deposit 500 tokens due to the strategy's deposit limit.

6. The remaining 500 tokens stay in the contract as unused deposits.

7. The deposit function mints 1000 liquid staking tokens to the user and increments totalStaked by 1000.

8. After the deposit, the unused deposits in the contract exceed the unusedDepositLimit of 100 tokens.

Impact

• When unused deposits exceed the limit, it means that a portion of the users' deposited tokens are not being efficiently utilized by the strategies to generate yields.

• The unusedDepositLimit is intended to limit the number of tokens that can sit idle in the contract. Exceeding this limit violates the expected behavior and may undermine the trust of users in the system.

• If a significant amount of tokens remain unused in the contract, it can impact the overall liquidity of the staking pool and potentially affect the ability of users to withdraw their funds when needed.

Tools Used

Vs

Recommendations

1. Modify the deposit function to ensure that all the deposited tokens are successfully deposited into the strategies before minting liquid staking tokens to the user. This can be achieved by:

   • Checking the available deposit room in the strategies before transferring tokens from the user.

   • Reverting the transaction if there is insufficient room in the strategies to accept the full deposit amount.

2. Adjust the unusedDepositLimit logic to account for edge cases and ensure that it accurately reflects the maximum allowed unused deposits in the contract.