Liquid Staking

Summary

In simple terms the intended flow of the protocol is that user initiate deposit in PriorityPool, then from PriorityPool after certain checks and conditions tokens should be deposited in StakingPool and after that funds should go to OperatorVCS or CommunityVCS which then deposit tokens into desired vaults and strategies. So flow should look something like this:

User -> PriorityPool -> StakingPool -> OperatorVCS or CommunityVCS -> Operator Vault or Community Vault -> Operator or Community Pool on Chainlink

Throughout this intended flow deposit amount is tracked succesfully and protocol make sure that max deposit limit is not breached. This is enforced also throughout functions:

deposit function on PriorityPool calls deposit function on StakingPool that have onlyPriorityPool modifier which make sure only PriorityPool should be able to deposit in StakingPool as we can see here:

Which eventually deposits funds to desired strategies by calling deposit function in VaultControllerStrategy which is abstract contract that have onlyStakingPool modifier which make sure the function is called only by StakingPool.The deposit function via delegatecall calls deposit function on VaultDepositController as we can see here:

Which intended design is to deposit tokens from StakingPool into Vaults as mentioned in Nat spec above (and also in the first paragraph of this finding).

The deposit function in VaultDepositController looks like this:

The intended design is that function should be only called via delegatecall, this also confirm the flow that is mentioned in the beginning of this finding.

Vulnerability Details

The issue here is that the deposit function does not check if it is called only by delegatecall and if the caller is correct contract.
Anyone could call the deposit function in VaultDepositController and effectively bypass the intended flow of the protocol.
The protocol that way would not be able to track deposited amount correctly.
Malicious user (or multiple users) could bypass the intended flow and deposit directly to strategies, the deposited amount could eventually reach deposit cap, which would make protocol unable to deposit the regular way because maximum amount of tokens that the strategy can hold would be reached. The protocol would try to deposit to different strategies if first one fail to do so but this can be done effectively to to other strategies which eventually could make protocol unable to deposit. This can impact also users they would not be able to deposit to the protocol and they could possibly incur loses.

After passing canDeposit, the malicious user could frontrun and execute their malicious act in mem pool before depositfunction is called, so when it is called it would revert but the PriorityPooldoes not catch that and would think deposit was succesfull and it will deduct toDepositIntoPoolamount from toDepositamount, the remaining amount would be queued or transfered back to user, but the deducted difference would be stuck in PriorityPooland users would incure loss that way. The best case scenario is that user maybe would not incur direct loses because their amount would be returned to them or queued if they want to do that, but they would get denial of service (DoS) when they should not incur that and they should be able to deposit.

Impact

Protocol intended flow can be bypassed and users possibly could incur loss of funds.

Tools Used

Manual Review.

Recommendations

This can be prevented by adding whitelisting addresses and onlyDelegateCall modifier.
Make the following changes to the VaultDepositController :

This will ensure that only the intended VaultControllerStrategy contract can call deposit and withdraw via delegatecall, preventing unauthorized contracts from bypassing the protocol’s intended flow and access control.