Liquid Staking

Summary

The withdraw function in the StakingPool contract contains a severe race condition vulnerability that allows users to over-withdraw their staked tokens by executing multiple withdrawals in quick succession.

The vulnerable logic is connected to:

• The _withdrawLiquidity function which withdraws from strategies to top up the pool balance

• The _burn function which burns the user's staked balance (but uses the potentially stale toWithdraw amount)

• The totalStaked state variable which tracks total staked amounts (and is decremented by toWithdraw)

Vulnerability Details

Occurs because the function does not recheck the user's staked balance after potentially topping up the pool's balance by withdrawing from strategies. This can lead to a situation where a user can execute multiple withdrawals in quick succession, each one using stale balance information, and thereby drain more tokens than they are entitled to.

Here's how it happens:

1. The withdraw function first checks if the requested withdrawal amount exceeds the pool's current balance.

2. If so, it attempts to withdraw the deficit from the pool's strategies using _withdrawLiquidity.

3. After that, it only checks that the pool's updated balance is sufficient for the withdrawal.

4. However, it does not recheck the user's actual staked balance (their share of the total pool) after potentially updating the pool balance.

5. This allows a second withdrawal to be executed using stale balance information, potentially exceeding the user's real staked amount.

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

The issue stems from the fact that toWithdraw is not updated after calling _withdrawLiquidity, so the subsequent _burn, totalStaked update, and token.safeTransfer all use a potentially stale value.

The fact that toWithdraw is not updated after potentially topping up the pool balance by calling _withdrawLiquidity.

As a result, the subsequent _burn, totalStaked update, and token.safeTransfer all use a toWithdraw amount that may exceed the user's actual current staked balance, allowing them to over-withdraw tokens.

Consider the following scenario:

1. User stakes 100 tokens, so balanceOf(user) = 100 and totalStaked = 100

2. All 100 tokens are deposited into strategies, so token.balanceOf(pool) = 0

3. User calls withdraw(user, user, 80, data)

4. Pool balance is 0 < 80, so 80 tokens are withdrawn from strategies

5. Pool balance is now 80 >= 80, so the withdraw succeeds

6. _burn(user, 80), so balanceOf(user) = 20, and totalStaked = 20

7. 80 tokens are transferred to user, so token.balanceOf(pool) = 0 again

8. User calls withdraw(user, user, 30, data)

9. Pool balance is 0 < 30, so 30 tokens are withdrawn from strategies

10. Pool balance is now 30 >= 30, so the second withdraw also succeeds

11. _burn(user, 30), so balanceOf(user) = -10, and totalStaked = -10

12. 30 tokens are transferred to user, so in total they withdrew 110 tokens

The user was able to withdraw a total of 110 tokens despite only having staked 100 initially. This is made possible by the race condition vulnerability in the withdraw function.

Impact

• Users can exploit this to withdraw more tokens than they actually staked

• This will drain the pool's strategies and deficit the totalStaked accounting

Tools Used

Vs

Recommendations

The withdraw function must recheck the user's current staked balance after topping up the pool from strategies, and use the minimum of that updated balance and the requested withdrawal amount for the final transfer and burn. By rechecking balanceOf(_account) after potentially updating the pool balance, and taking the minimum of that and the original toWithdraw amount, we ensure that users can never withdraw more than their actual current staked balance.