Open code423n4 opened 2 years ago
I realise there are 2 issues discussed here:
1) The high-risk severity relates to GeneralVault
's tight 1% slippage. Because it is inherited by vaults, it can cause withdrawals to fail and for user funds to be stuck.
2) However, in the context of the LIDO vault specifically, #69's first low-severity issue rightly points out that users can choose to withdraw their funds in stETH, then do conversion to ETH separately afterwards. Hence, funds won't actually be stuck. I would've therefore classified this a medium-severity issue. Nevertheless, it is expected that users will attempt to withdraw to ETH instead of stETH in times of market volatility.
Lines of code
https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L125 https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L130-L137
Vulnerability details
Impact
GeneralVault.sol#L125 GeneralVault set a hardcoded slippage control of 99%. However, the underlying yield tokens price may go down. If Luna/UST things happen again, users' funds may get locked.
LidoVault.sol#L130-L137 Moreover, the withdrawal of the lidoVault takes a swap from the curve pool. 1 stEth worth 0.98 ETH at the time of writing. The vault can not withdraw at the current market.
Given that users' funds would be locked in the lidoVault, I consider this a high-risk issue.
Proof of Concept
1 stEth = 0.98 Eth
LidoVault.sol#L130-L137
Tools Used
Recommended Mitigation Steps
There are different ways to set the slippage.
The first one is to let users determine the maximum slippage they're willing to take. The protocol front-end should set the recommended value for them.
The second one is have a slippage control parameters that's set by the operator.
These are two common ways to deal with this issue. I prefer the first one. The market may corrupt really fast before the operator takes action. It's nothing fun watching the number go down while having no option.