Liquid Staking

Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Submission Details
Severity: medium
Invalid

Inconsistent `totalStaked` Accounting in `withdraw()` Function

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

uint256 toWithdraw = _amount;
if (_amount == type(uint256).max) {
toWithdraw = balanceOf(_account);
}

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

_burn(_account, toWithdraw);
totalStaked -= toWithdraw;

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

function withdraw(
address _account,
address _receiver,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool {
uint256 toWithdraw = _amount;
if (_amount == type(uint256).max) {
toWithdraw = balanceOf(_account); // <-- @audit toWithdraw is set to user's balance
}
uint256 balance = token.balanceOf(address(this));
if (toWithdraw > balance) {
_withdrawLiquidity(toWithdraw - balance, _data);
}
require(
token.balanceOf(address(this)) >= toWithdraw,
"Not enough liquidity available to withdraw"
);
_burn(_account, toWithdraw);
totalStaked -= toWithdraw; // <-- @audit totalStaked is decreased by toWithdraw instead of _amount
token.safeTransfer(_receiver, toWithdraw);
}

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

function withdraw(
address _account,
address _receiver,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool {
uint256 toWithdraw = _amount;
if (_amount == type(uint256).max) {
toWithdraw = balanceOf(_account);
}
// ...
_burn(_account, toWithdraw);
- totalStaked -= toWithdraw;
+ totalStaked -= _amount;
// ...
}
  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

function withdraw(
address _account,
address _receiver,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool {
+ require(_amount != type(uint256).max, "Invalid withdrawal amount");
// ...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.