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.
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:
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
.
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 allowstotalStaked
to be manipulated and potentially enables users to withdraw more tokens than they originally staked.
PoC
A scenario demonstrating the issue.
User A stakes 100 tokens by calling deposit()
, increasing totalStaked
by 100.
User B stakes 50 tokens, increasing totalStaked
by 50. Now totalStaked
is 150.
User A calls withdraw()
with _amount = type(uint256).max
.
Inside withdraw()
, toWithdraw
is set to User A's balance of 100 tokens.
totalStaked
is decreased by toWithdraw
(100), becoming 50.
User B now calls withdraw()
with their balance of 50 tokens.
totalStaked
is decreased by 50, becoming 0, even though only 150 tokens were ever staked.
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.
Vs
Make the withdraw()
function to consistently decrease totalStaked
by the original _amount
value, regardless of the special case for type(uint256).max
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
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.