novaman33 - Lido withdraw limitation will brick the withdraw process in an edge case

[sherlock-admin4]

novaman33

High

Lido withdraw limitation will brick the withdraw process in an edge case

Summary

Lido protocol has limitation regarding the requestWithdraw function. However some of these limitation have not been considered in the _initiateWithdrawImpl leading to users being unable to claim their vault shares even after the cooldown.

Vulnerability Detail

Lido has stated the following withdraw limitation in their docs:

1. withdrawals must not be paused
2. stETH balance of msg.sender must be greater than the sum of all _amounts
3. there must be approval from the msg.sender to this contract address for the overall amount of stETH token transfer
4. each amount in _amounts must be greater than MIN_STETH_WITHDRAWAL_AMOUNT and lower than MAX_STETH_WITHDRAWAL_AMOUNT (values that can be changed by the DAO) Extracted from here( https://docs.lido.fi/contracts/withdrawal-queue-erc721#requestwithdrawals )

Consider the following scenario: 1) A user who has shares representing stETH less than the MIN_STETH_WITHDRAWAL_AMOUNT or more than the MAX_STETH_WITHDRAWAL_AMOUNT calls initiateWithdraw. The withdraw will be initiated successfully and the rsETH to withdraw will be sent to the holder contract which is going to start the cooldown. 2) However after the cooldown has passed the user will call the triggerExtraStep function which will always result in revert because of the Lido requirements regarding the amount to be withdrawn(mentioned in point 4).

Impact

The user will experience a full DOS of the protocol. They will have a pending withdraw that will never finish, which will result in their funds being locked forever. They will not be able to liquidate or deposit because of the pending withdraw. The function triggerExtraStep will always revert and the tokens from Kelp will never be claimed, because of Lido's limitation. - High

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/14d3eaf0445c251c52c86ce88a84a3f5b9dfad94/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L83

Tool used

Manual Review

Recommendation

Consider enforcing withdraw limitations so that if a user has more than the MAX_STETH_WITHDRAWAL_AMOUNT split it on two requests, or create deposit limitations.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Calls made by the Notional proxy (i.e. depositFromNotional and redeemFromNotional) are restricted by deposit sizes and maximum leverage ratios enforced by the main Notional contract

[novaman33]

Escalate, This is a valid issue that will result in permanent lock of funds. The way I understand your comment is that there can be deposit limitations, however I do not consider them to be relevant here as a user could simply deposit more than once or gain funds by liquidating others. Also these min and max values by the Lido protocol are changable by the DAO. The issue is that in the same function funds are pulled from Kelp and sent to Lido and these limitations will cause a revert of the whole function. I did review the code and I did not see any restrictions as you have stated. In case I am missing something could you provide a reference to the check you have considered in order to invalidate the issue?

[sherlock-admin3]

Escalate, This is a valid issue that will result in permanent lock of funds. The way I understand your comment is that there can be deposit limitations, however I do not consider them to be relevant here as a user could simply deposit more than once or gain funds by liquidating others. Also these min and max values by the Lido protocol are changable by the DAO. The issue is that in the same function funds are pulled from Kelp and sent to Lido and these limitations will cause a revert of the whole function. I did review the code and I did not see any restrictions as you have stated. In case I am missing something could you provide a reference to the check you have considered in order to invalidate the issue?

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[mystery0x]

Escalate, This is a valid issue that will result in permanent lock of funds. The way I understand your comment is that there can be deposit limitations, however I do not consider them to be relevant here as a user could simply deposit more than once or gain funds by liquidating others. Also these min and max values by the Lido protocol are changable by the DAO. The issue is that in the same function funds are pulled from Kelp and sent to Lido and these limitations will cause a revert of the whole function. I did review the code and I did not see any restrictions as you have stated. In case I am missing something could you provide a reference to the check you have considered in order to invalidate the issue?

It's found in the contest Details page. These issues are commonly known and will be low at most for the suggested mitigation you provided.

[novaman33]

@mystery0x There are no restriction regarding how much the user can deposit. The restrictions are so that the position is healthy and the leverage ratio is less than the maximum ratio. The issue is that if a user tries to withdraw less than 100 wei they will experience a full DOS since they will have a withdraw request that will be pending but will not be able to finalize it since the triggerExtraStep will always revert because of the lido limitation. The other case however is much more scary. If a whale staker comes, and they have more than 1000ETH(which is the MAX_STETH_WITHDRAWAL_AMOUNT), when trying to withdraw this amount of money, triggerExtraStep will always revert, they will have a pending withdrawRequest meaning they will not be able to deposit or liquidate. While you did mention the contest page statement about the restrictions in deposit sizers I did review them and both cases are possible with these restrictions. I cannot agree that permanently locking funds(which will happen in both cases) and experiencing full DOS is a low severity case.

[WangSecurity]

This is the holder contract, correct? https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/14d3eaf0445c251c52c86ce88a84a3f5b9dfad94/leveraged-vaults-private/contracts/vaults/staking/protocols/ClonedCoolDownHolder.sol

It has a rescue function to retrieve the tokens. Also, can you send a link to initiateWithdraw, there are several functions with similar names, want to understand what you talk about specifically.

[novaman33]

@WangSecurity, So in order for a user to withdraw, they first call initiateWithdraw(BaseStakingVault.sol), which calls _initiateWithdraw(WithdrawRequestBase.sol), which than calls_initiateWithdrawImpl(this is from the Kelp.sol in this case). In _initiateWithdrawImpl(Kelp.sol), a holder contract is made and rsETH is transfered to the new holder address. Than holder.startColldown is called. _startCooldown transfers all the rsETH balance of the holder contract to Kelp and calls WithdrawManager.initiateWithdrawal(stETH, balance); The problem is in the triggerExtraStep function, where firstly the withdraw is completed from Kelp and then LidoWithdraw.requestWithdrawals(amounts, address(this)); is called, which has the limitations mentioned in this report. The main issue is that the holder contract has no other way to take the stETH tokens from Kelp, but only triggerExtraStep which will result in a revert in the forementioned edge cases. The rescueTokens function will not be able to retrieve those tokens because they are not stuck in the holder contract but are stuck in Kelp with no way to be claimed. The restrictions mentioned by the judge are from the readMe but they are not applicable in this case because they do not prevent users from withdrawing less than 100wei or more than 1000ETH. There are only leverage restrictions which are to keep positions healthy. These however are irrelevant in this specific case.

[WangSecurity]

Thank you for that clarifications. For the scenario about minimum withdrawal I would consider low, since the loss if 100 wei. But for the maximum is completely viable. I believe the high severity is appropriate here, since it will affect every whale investor in Notional and causes complete loss of funds.

Planning to accept the escalation and validate with high severity. Are there any duplicates @novaman33 @mystery0x ?

[novaman33]

@WangSecurity believe it is solo.

[WangSecurity]

Result: High Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• novaman33: accepted

[jeffywu]

This issue is resolved by removing the Lido stETH withdraw in favor of an ETH withdraw.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/leveraged-vaults/pull/92

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.