dobrevaleri - Incorrect usage of shares will lead to insolvency

[sherlock-admin4]

dobrevaleri

High

Incorrect usage of shares will lead to insolvency

Summary

The shares of previously withdrawn earnings are assumed to have the same value as those at the end of the staking period. This leads to distributing more to users than necessary, potentially causing insolvency.

Root Cause

In vaultEndedWithdraw() when calculating the total earnings, withdrawnStakingEarningsInStakes is used. withdrawnStakingEarningsInStakes represents the shares of already claimed withdrawals, initiated before the vault duration ends. These shares are burned on withdrawal claim, according to the Lido documentation. Hence they are not included in the number of shares at the end of the vault duration (vaultEndingStakesAmount) and also the price of the share at the time of the withdrawal and at the end of the vault duration end is expected to be different. Thus resulting in invalid totalEarned value, which is greater than it must be.

Internal pre-conditions

1. There must be at least one withdrawal of a variable side, so that withdrawnStakingEarningsInStakes is non-zero.

External pre-conditions

No response

Attack Path

1. Fixed side deposit and Variable side deposits are made, so that the vault is started.
2. Rewards are accrued because of the staking.
3. Variable side user calls withdraw(1), which will create withdraw request for a part of these rewards.
4. withdrawnStakingEarningsInStakes is updated to represent the shares that will be burnt, and withdrawnStakingEarnings is updated to represent the withdrawn amount without the protocol fees.
5. When the request is claimed, the shares representing the withdrawn amount are burned from Lido contract
6. When the duration ends, the staking is finalized, all staked amount is unstaked and all withdraw requests are claimed.
7. Any user participating in the variable side call withdraw(1), and according to the calculations he will receive more than he had to.

Impact

The protocol suffers a loss of funds which may vary, because the more variaable side withdrawals are made in the beginning of the vault duration, the greater the error of the total earnings will be, because the ETH price for 1 share is expected to be increasing.

PoC

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import "@openzeppelin/contracts/utils/math/Math.sol";

import {Test, console2} from "forge-std/Test.sol";

import {VaultFactory} from "src/VaultFactory.sol";
import {LidoVault} from "src/LidoVault.sol";
import {ILido} from "src/interfaces/ILido.sol";
import {MockLido} from "src/mocks/MockLido.sol";
import {MockLidoWithdrawalQueue} from "src/mocks/MockLidoWithdrawalQueue.sol";

contract PoC is Test {
    using Math for uint256;

    address payable constant LIDO = payable(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);
    address payable constant LIDO_WITHDRAWAL_QUEUE = payable(0x889edC2eDab5f40e902b864aD4d7AdE8E412F9B1);

    VaultFactory vf;
    LidoVault lv;

    address fixedUser = makeAddr("fixedUser");
    address variableUser1 = makeAddr("variableUser1");
    address variableUser2 = makeAddr("variableUser2");
    address protocolFeeReceiver = makeAddr("protocolFeeReceiver");

    uint256 fixedSideCapacity = 1 ether;
    uint256 variableSideCapacity = 1 ether;
    uint256 duration = 7 days;

    function setUp() public {
        // Deploy the Lido mocks on the respective addresses
        deployCodeTo("MockLido", LIDO);
        deployCodeTo("MockLidoWithdrawalQueue", LIDO_WITHDRAWAL_QUEUE);

        MockLidoWithdrawalQueue(LIDO_WITHDRAWAL_QUEUE).initialize(LIDO);

        deal(LIDO_WITHDRAWAL_QUEUE, 100 ether);

        // Deploy the vault factory
        vm.prank(protocolFeeReceiver);
        vf = new VaultFactory(100, 100);

        uint256 vaultId = vf.nextVaultId();

        vf.createVault(fixedSideCapacity, duration, variableSideCapacity);
        (, address addr) = vf.vaultInfo(vaultId);

        lv = LidoVault(payable(addr));

        deal(fixedUser, 10 ether);
        deal(variableUser1, 10 ether);
        deal(variableUser2, 10 ether);
    }

    function test3() public {
        uint256 sharePriceBeggining = MockLido(LIDO).getPooledEthByShares(1 ether);

        // Fixed side deposit
        vm.prank(fixedUser);
        lv.deposit{value: 1 ether}(0);

        // Variable side deposit
        vm.prank(variableUser1);
        lv.deposit{value: 0.5 ether}(1);

        vm.prank(variableUser2);
        lv.deposit{value: 0.5 ether}(1);

        // Validate the vault have started
        assert(lv.isStarted() == true);
        assert(lv.isEnded() == false);

        // Claim the fixed premium
        vm.prank(fixedUser);
        lv.claimFixedPremium();

        // Add rewards
        MockLido(LIDO).addStakingEarnings(1 ether);

        // Create withdraw variable request
        vm.prank(variableUser1);
        lv.withdraw(1);

        // Claim the withdraw
        uint256 variableUserBalanceBefore = variableUser1.balance;
        vm.prank(variableUser1);
        lv.finalizeVaultOngoingVariableWithdrawals();
        uint256 variableUserBalanceAfter = variableUser1.balance;

        uint256 variableUserWithdrawals = variableUserBalanceAfter - variableUserBalanceBefore;
        uint256 protocolFee = lv.totalProtocolFee();

        uint256 sharePriceAfterWithdraw = MockLido(LIDO).getPooledEthByShares(1 ether);
        assert(sharePriceAfterWithdraw != sharePriceBeggining);

        // Add a lot of rewards
        MockLido(LIDO).addStakingEarnings(1000 ether);

        // skip time
        skip(duration + 1);

        // Validate the vault have ended
        assert(lv.isEnded() == true);

        // Finalize the vault
        vm.prank(fixedUser);
        lv.withdraw(0);

        vm.prank(fixedUser);
        lv.finalizeVaultEndedWithdrawals(0);

        // Validate that the share price is increased, because rewards have been added
        uint256 sharePriceEnding = MockLido(LIDO).getPooledEthByShares(1 ether);
        assert(sharePriceEnding > sharePriceAfterWithdraw);
        assert(sharePriceEnding > sharePriceBeggining);

        // Check the earnings
        // The total earnings = withdrawnEarnings + vaultEndedStakingEarnings - totalProtocolFees
        // Only the withdrawnEarnings are calculated using shares in the LidoVault.sol::vaultEndedWithdraw() line 775
        // The withdrawnEarnings calculation below includes the totalProtocolFees
        uint256 withdrawnEarningsCalculation =
            lv.vaultEndingETHBalance().mulDiv(lv.withdrawnStakingEarningsInStakes(), lv.vaultEndingStakesAmount());

        // Validate that the previously withdrawn earnings are less than the calculated
        assert(withdrawnEarningsCalculation > variableUserWithdrawals + protocolFee);
    }
}

Mitigation

LidoVault.sol::vaultEndedWithdraw()

...
} else {
            require(variableToVaultOngoingWithdrawalRequestIds[msg.sender].length == 0, "WAR");

            if (msg.sender == protocolFeeReceiver && appliedProtocolFee > 0) {
                return protocolFeeReceiverWithdraw();
            }

            uint256 bearerBalance = variableBearerToken[msg.sender];
            require(bearerBalance > 0, "NBT");

            // Return proportional share of both earnings to caller
            uint256 stakingShareAmount = 0;

-           uint256 totalEarnings = vaultEndingETHBalance.mulDiv(
-                withdrawnStakingEarningsInStakes, vaultEndingStakesAmount
-            ) - totalProtocolFee + vaultEndedStakingEarnings;
+           uint256 totalEarnings = withdrawnStakingEarnings + vaultEndedStakingEarnings;
            if (totalEarnings > 0) {
-                (uint256 currentState, uint256 stakingEarningsShare) = calculateVariableWithdrawState(
-                    totalEarnings,
-                   variableToWithdrawnStakingEarningsInShares[msg.sender].mulDiv(
-                        vaultEndingETHBalance, vaultEndingStakesAmount
-                   ) 
-                );
+              (uint256 currentState, uint256 stakingEarningsShare) = calculateVariableWithdrawState(
+                    totalEarnings,
+                  variableToWithdrawnStakingEarnings[msg.sender]
+                );
                stakingShareAmount = stakingEarningsShare;
                variableToWithdrawnStakingEarningsInShares[msg.sender] =
                    currentState.mulDiv(vaultEndingStakesAmount, vaultEndingETHBalance);
                variableToWithdrawnStakingEarnings[msg.sender] = currentState;
            }
...