Minato7namikazi - Inaccurate Variable Withdraw Calculation in Slashing Scenarios

[sherlock-admin3]

Minato7namikazi

Medium

Inaccurate Variable Withdraw Calculation in Slashing Scenarios

https://github.com/sherlock-audit/2024-08-saffron-finance/blob/38dd9c8436db341c331f1b14545770c1766fc0ee/lido-fiv/contracts/LidoVault.sol#L880

in function: getCalculateVariableWithdrawStateWithStakingBalance

The function doesn't account for the scenario where the Lido stETH balance is less than the fixed ETH deposits. This can happen due to slashing incidents or other unexpected losses in the Lido Liquid Staking protocol.

function getCalculateVariableWithdrawStateWithStakingBalance(address user) public view returns (uint256) {
    uint256 lidoStETHBalance = stakingBalance();
    uint256 fixedETHDeposits = fixedETHDepositTokenTotalSupply;
    require(lidoStETHBalance > fixedETHDeposits, "LBL");
    // ... rest of the function
}

The function requires that the Lido stETH balance is greater than the fixed ETH deposits. If this condition is not met, the function will revert with the error "LBL" (Lido Balance Low).

However, this approach doesn't align with the stated acceptable risk in the documentation:

"The Lido Liquid Staking protocol can experience slashing incidents (such as this https://blog.lido.fi/post-mortem-launchnodes-slashing-incident/). These incidents will decrease income from deposits to the Lido Liquid Staking protocol and could decrease the stETH balance. The contract must be operational after it, but it is acceptable for users to lose part of their income/deposit (for example, a fixed user receives less at the end of the Vault than he deposited at the start)."

The current implementation prevents the contract from being operational in such scenarios, contradicting the stated design goal.

To address this vulnerability and align with the project's risk acceptance, the function should handle the case where lidoStETHBalance <= fixedETHDeposits. Instead of reverting, it should calculate the variable withdraw state based on the available balance, even if it results in a loss for the users.

This missed case could have a significant impact on the project, as it prevents variable side users from withdrawing their funds in scenarios where losses have occurred, which goes against the stated design principles of the contract.

1. Trigger Condition:

The bug can be triggered when the Lido stETH balance becomes less than the fixed ETH deposits. This can happen due to slashing events in the Lido protocol, as mentioned in the documentation.

2. Why it can be triggered:

The contract doesn't have any mechanism to prevent or handle a situation where the stETH balance drops below the fixed deposits. The stakingBalance() function directly returns the stETH balance from Lido, which can fluctuate.

3. Impact and Flow:

The impact of this bug is less severe because:

a) It only affects a view function, not a state-changing function.
b) The main withdrawal functions (withdraw, finalizeVaultOngoingVariableWithdrawals, vaultEndedWithdraw) do not directly use this function.

However, it still has some impact:

1. It prevents variable side users from accurately estimating their withdrawable amount when the stETH balance is low.
2. It could cause issues in any external contracts or UI components that rely on this function for calculations.

PoC Example:

1. Deploy the LidoVault contract
2. Users deposit into both fixed and variable sides, starting the vault
3. A slashing event occurs in Lido, reducing the stETH balance
4. Try to call getCalculateVariableWithdrawStateWithStakingBalance:

function testSlashingScenario() public {
    // Setup vault and deposits...
    
    // Simulate a slashing event
    uint256 initialBalance = vault.stakingBalance();
    uint256 slashedBalance = initialBalance * 90 / 100; // 10% slash
    // (You'd need to mock the Lido contract to actually reduce the balance)

    // This call will revert
    try vault.getCalculateVariableWithdrawStateWithStakingBalance(variableUser) {
        fail("This should have reverted");
    } catch Error(string memory reason) {
        assertEq(reason, "LBL");
    }

    // However, actual withdrawals would still work
    vault.withdraw(VARIABLE);
}

4. Actual Severity:

While this is a real bug, its severity is MEDIUM rather than HIGH because:

• It doesn't directly affect the core withdrawal functionality.