Summary

principalDeposits is not properly updated if the token transfer in deposit fails. It's happening because principalDeposits is updated after the transfer.

• It can be reproduced by calling deposit with an amount exceeding the allowance.

Vulnerability Details

The deposit function in the LSTRewardsSplitter contract incorrectly updates the principalDeposits state variable after the token transfer. If the token transfer fails for any reason (e.g., insufficient allowance or a revert in the lst token contract), the principalDeposits variable will still be incremented, leading to an inconsistency between the actual deposited tokens and the recorded principal deposits.

Steps

1. The deposit function in LSTRewardsSplitter.sol updates the principalDeposits state variable after the token transfer: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L68-L72

1. If the lst.safeTransferFrom call fails due to insufficient allowance or any other reason, the execution will revert. However, the principalDeposits variable will still be incremented by _amount, even though no tokens were actually deposited.

Consider the following scenario:

1. The LSTRewardsSplitter contract is deployed with a valid lst token address and an initial principalDeposits value of 0.

2. A user attempts to deposit 100 tokens by calling the deposit function through the controller, but they have not approved the LSTRewardsSplitter contract to transfer tokens on their behalf.

3. The lst.safeTransferFrom call fails due to insufficient allowance, causing the transaction to revert.

4. Despite the failed deposit, the principalDeposits variable is still incremented by 100.

5. The principalDeposits variable now incorrectly reflects a deposit of 100 tokens, even though no tokens were actually transferred to the contract.

Impact

If users rely on the principalDeposits variable to track their deposited funds and make decisions based on its value, they may incorrectly believe they have more funds in the contract than they actually do. This could lead to incorrect assumptions and potential loss of funds if users attempt to withdraw or manage their deposits based on the inflated principalDeposits value.

Tools Used

Manual Review

Recommendations

Iincrement the principalDeposits variable before the token transfer