Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

`_deposit` can end up wrongly allowing users queuing in withdrawal queue to withdraw and cause balance to fall below `getMinDeposits()`

rscodes

Vulnerability Details

In PriorityPool.sol, lets refer to the _deposit function which is called by deposit.

function _deposit(
address _account,
uint256 _amount,
bool _shouldQueue,
bytes[] memory _data
) internal {
if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
​
uint256 toDeposit = _amount;
​
if (totalQueued == 0) {
uint256 queuedWithdrawals = withdrawalPool.getTotalQueuedWithdrawals();
if (queuedWithdrawals != 0) {
uint256 toDepositIntoQueue = toDeposit <= queuedWithdrawals
? toDeposit
: queuedWithdrawals;
withdrawalPool.deposit(toDepositIntoQueue);
toDeposit -= toDepositIntoQueue;
IERC20Upgradeable(address(stakingPool)).safeTransfer(_account, toDepositIntoQueue);
}
​
if (toDeposit != 0) {
uint256 canDeposit = stakingPool.canDeposit();
if (canDeposit != 0) {
uint256 toDepositIntoPool = toDeposit <= canDeposit ? toDeposit : canDeposit;
stakingPool.deposit(_account, toDepositIntoPool, _data);
toDeposit -= toDepositIntoPool;
}
}
}
....
....
}

As we can see, the order of the code when there is no queue in the PriorityPool is to

  1. Check whether it can "swap tokens" with withdraw orders sitting in the WithdrawalPool's queue.

  2. Check if user can directly deposit into StakingPool through checking against limit stakingPool.canDeposit()

However, functions like addStrategy could have been called slightly before. That increase may cause StakingPool's totalStaked to fall below the new getMinDeposits(), because the new strategies added will increase getMinDeposits().

Proof Of Concept

  1. Currently StakingPool has 500 tokens. (And getMinDeposits() = 500)

  2. The deposit queue in PriorityPool is currently empty.

  3. Alice who is a staker in StakingPool, wants to withdraw 100 of her tokens.

  4. Since canWithdraw() of StakingPool.sol is 0, Alice's order to withdraw 100 tokens will be sent to queue in WithdrawalPool

  5. A new strategy is added to StakingPool, raising getMinDeposits() to 600.

  6. Bob is a fan of the new strategy and decides to deposit 100 tokens to the pool. (He has to go through PriorityPool which is the only way to deposit)

  7. PriorityPool's deposit calls internal function _deposit which "swaps" Bob's 100 tokens with Alice's withdrawal order, wrongly allowing Alice to withdraw.

  8. After accepting Bob as a new staker and mistakenly allowing Alice to withdraw, StakingPool's tokens remain at 500, which is below the new getMinDeposits() when it should not have been.

Impact

The purpose of the WithdrawalPool in this protocol is such that users who's withdrawal order will cause the balance to dip below getMinDeposits() will have to wait for new depositors to come and balance out their order such that they are only able to withdraw while not causing balance to drop below getMinDeposits().

The purpose of such a feature (which can be found in the documentation) is to ensure the pool enjoy continously and uninterrupted yield.

Hence, when such a senario occurs, it breaks the protocol's intended invariant.

Recommended Steps

Limit the amount to swap with the withdrawal queue to stakingPool.totalStaked - stakingPool.getMinDeposits().

function _deposit(
address _account,
uint256 _amount,
bool _shouldQueue,
bytes[] memory _data
) internal {
if (poolStatus != PoolStatus.OPEN) revert DepositsDisabled();
​
uint256 toDeposit = _amount;
​
if (totalQueued == 0) {
uint256 queuedWithdrawals = withdrawalPool.getTotalQueuedWithdrawals();
if (queuedWithdrawals != 0) {
uint256 toDepositIntoQueue = toDeposit <= queuedWithdrawals
? toDeposit
: queuedWithdrawals;
+ uint256 limitAmount = stakingPool.totalStaked() > stakingPool.getMinDeposits() ? stakingPool.totalStaked() - stakingPool.getMinDeposits() : 0;
+ toDepositIntoQueue = toDepositIntoQueue > limitAmount ? limitAmount : toDepositIntoQueue;
withdrawalPool.deposit(toDepositIntoQueue);
toDeposit -= toDepositIntoQueue;
IERC20Upgradeable(address(stakingPool)).safeTransfer(_account, toDepositIntoQueue);
}
​
if (toDeposit != 0) {
....
}
}
​
....
....
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Appeal created

rscodes Submitter
about 1 year ago
rscodes Submitter
about 1 year ago
rscodes Submitter
about 1 year ago
inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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