Liquid Staking

Summary

withdraw() function that allows totalStaked can become inconsistent with the actual total staked amount when withdrawing with _amount = type(uint256).max. The inconsistency can be exploited to manipulate totalStaked and withdraw more tokens than originally staked.

There is a special case handling when the _amount parameter equals type(uint256).max: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L148-L151

If _amount is type(uint256).max, toWithdraw is set to the user's balance (balanceOf(_account)) instead of the actual _amount value.

However, later in the function, totalStaked is decreased by toWithdraw: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L162-L163

This means when withdrawing with _amount = type(uint256).max, totalStaked will decrease by the user's actual staked balance rather than the specified max amount.

The issue arises because totalStaked is not always decrease by the same amount that was originally deposited.

Vulnerability Details

The totalStaked variable does not consistently decrease by the original withdrawal amount. When a user calls withdraw() with _amount set to type(uint256).max, the totalStaked is decreased by the user's actual staked balance (balanceOf(_account)) instead of the specified max amount. https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L142-L165

This inconsistency can lead to totalStaked becoming out of sync with the sum of individual user balances. As a result, totalStaked can be manipulated by strategically withdrawing with _amount = type(uint256).max, potentially allowing users to withdraw more tokens than they originally staked.

The vulnerability lies in two specific lines:

1. Line 151: toWithdraw = balanceOf(_account);

   • When _amount is equal to type(uint256).max, toWithdraw is set to the user's balance instead of the original _amount.

2. Line 163: totalStaked -= toWithdraw;

   • totalStaked is decreased by toWithdraw, which may not be equal to the original _amount if the special case _amount == type(uint256).max was triggered.

These lines are responsible for the inconsistency in totalStaked accounting when withdrawing with _amount = type(uint256).max. The vulnerability allows totalStaked to be manipulated and potentially enables users to withdraw more tokens than they originally staked.

PoC

A scenario demonstrating the issue.

1. User A stakes 100 tokens by calling deposit(), increasing totalStaked by 100.

2. User B stakes 50 tokens, increasing totalStaked by 50. Now totalStaked is 150.

3. User A calls withdraw() with _amount = type(uint256).max.

4. Inside withdraw(), toWithdraw is set to User A's balance of 100 tokens.

5. totalStaked is decreased by toWithdraw (100), becoming 50.

6. User B now calls withdraw() with their balance of 50 tokens.

7. totalStaked is decreased by 50, becoming 0, even though only 150 tokens were ever staked.

Impact

This vulnerability allows totalStaked to be manipulated, enabling users to withdraw more tokens than they should be allowed to. It can lead to an inconsistent state where totalStaked does not accurately reflect the total amount of staked tokens in the pool.

Tools Used

Vs

Recommendations

1. Make the withdraw() function to consistently decrease totalStaked by the original _amount value, regardless of the special case for type(uint256).max

1. Alternatively, disallow using type(uint256).max as a withdrawal amount altogether to prevent this inconsistency, can be achieved by adding a require statement at the beginning of the withdraw() function