The incorrect accounting of protocol fee will cause double charging fee and wrong distribution of earnings for variable users

Crit/HighSaffron Lido Vaults·Sherlock · Staking · 16th September 2024CodebaseH-2

Summary

The incorrect accounting of protocol fee will cause double charging fee and wrong distribution of earnings for variable users.

Root Cause

The calculation for a variable user's earnings, when they withdraw where isStarted() and !isEnded()

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

solidity

          uint256 lidoStETHBalance = stakingBalance();
          uint256 fixedETHDeposits = fixedSidestETHOnStartCapacity;

        // staking earnings have accumulated on Lido
        if (lidoStETHBalance > fixedETHDeposits + minStETHWithdrawalAmount()) {
          uint256 currentStakes = stakingShares();
1>        (uint256 currentState, uint256 ethAmountOwed) = calculateVariableWithdrawState(
            (lidoStETHBalance.mulDiv(currentStakes + withdrawnStakingEarningsInStakes, currentStakes) - fixedETHDeposits),
            variableToWithdrawnStakingEarningsInShares[msg.sender].mulDiv(lidoStETHBalance, currentStakes)
          );
          if (ethAmountOwed >= minStETHWithdrawalAmount()) {
            // estimate protocol fee and update total - will actually be applied on withdraw finalization
2>          uint256 protocolFee = ethAmountOwed.mulDiv(protocolFeeBps, 10000);
            totalProtocolFee += protocolFee;
3>          uint256 stakesAmountOwed = lido.getSharesByPooledEth(ethAmountOwed);

            withdrawnStakingEarnings += ethAmountOwed - protocolFee;
4>          withdrawnStakingEarningsInStakes += stakesAmountOwed;

            variableToWithdrawnStakingEarnings[msg.sender] += ethAmountOwed - protocolFee;
5>          variableToWithdrawnStakingEarningsInShares[msg.sender] += stakesAmountOwed;
            variableToWithdrawnProtocolFee[msg.sender] += protocolFee;
            variableToVaultOngoingWithdrawalRequestIds[msg.sender] = requestWithdrawViaETH(
              msg.sender,
              ethAmountOwed
            );
        ...

The variable user's earnings is the variable ethAmountOwed at 1>. Note that, the earnings also includes the protocol fee (2>). Then ethAmountOwed is converted to shares stakesAmountOwed at 3>. Then the shares is added to withdrawnStakingEarningsInStakes and variableToWithdrawnStakingEarningsInShares[msg.sender]

When the vault ends, the variable user's earnings is calculated at

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

solidity

      uint256 stakingShareAmount = 0;

1>    uint256 totalEarnings = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes,vaultEndingStakesAmount) - totalProtocolFee  + vaultEndedStakingEarnings;

      if (totalEarnings > 0) {
2>      (uint256 currentState, uint256 stakingEarningsShare) = calculateVariableWithdrawState(
          totalEarnings,
3>        variableToWithdrawnStakingEarningsInShares[msg.sender].mulDiv(vaultEndingETHBalance, vaultEndingStakesAmount)
        );
        stakingShareAmount = stakingEarningsShare;
        variableToWithdrawnStakingEarningsInShares[msg.sender] = currentState.mulDiv(vaultEndingStakesAmount,vaultEndingETHBalance);
        variableToWithdrawnStakingEarnings[msg.sender] = currentState;
      }

The variable user's earnings is the variable stakingEarningsShare at 2>. stakingEarningsShare is calculated basing on withdrawnStakingEarningsInStakes, variableToWithdrawnStakingEarningsInShares[msg.sender] (1>, 3>).

We believe by including protocol fee shares in withdrawnStakingEarningsInStakes, variableToWithdrawnStakingEarningsInShares[msg.sender] will cause stakingEarningsShare to be wrongly calculated.

Refer to the attack path and the PoC for a concrete example.

Internal pre-conditions

A variable user withdraws when isStarted() and !isEnded()

Attack Path

Let's have a vault with:

fixedSideCapacity = 100 ether
variableSideCapacity = 10 ether
protocolFeeBps = 2_000 (20%)
stETHRate = 1 (1 shares equals to 1 stETH)

First vulnerability path

On the variable side:

Alice deposits 10 ether

The expected behavior

stETHRate = 1.1. Vault's balance (stETH): 110 ether (increased 10% since the beginning).

Alice withdraws 8 ether. totalProtocolFee = 2 ether
New vault's balance: 100 ether

Vault ends. stETHRate = 1.21. Vault's balance (stETH): 110 ether (increased 10% since the 1.).

Alice withdraws 8 ether. Protocol fee: 2 ether
New vault's balance: 100 ether

This is the expected behavior of the vault.

The actual behavior

stETHRate = 1.1. Vault's balance (stETH): 110 ether (increased 10% since the beginning).

Alice withdraws 8 ether. totalProtocolFee = 2 ether
New vault's balance: 100 ether

Vault ends. stETHRate = 1.21. Vault's balance (stETH): 110 ether (increased 10% since the 1.).

Alice withdraws.

solidity

      uint256 stakingShareAmount = 0;

      uint256 totalEarnings = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes,vaultEndingStakesAmount) - totalProtocolFee  + vaultEndedStakingEarnings;

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

In this code

$$totalEarnings = 110 \times \frac{10/1.1}{100 - 10/1.1} - 2 + 10 \times 0.8 = 17 $$

$$stakingEarningsShare = totalEarnings - \frac{10}{1.1} \times \frac{110}{100 - 10/1.1} = 6$$

She can only withdraw back 6 ether, which is less than 2 ether comparing to the expected behavior. Moreover, this 2 ether is not credited to the protocolFeeReceiver, no one can claim it and it will stuck in the contract.

Second vulnerability path

On the variable side:

Alice deposits 5 ether
Bob deposits 5 ether

The expected behavior

stETHRate = 1.1. Vault's balance (stETH): 110 ether (increased 10% since the beginning).

Alice withdraws 4 ether. totalProtocolFee = 1 ether
New vault's balance: 105 ether

Vault ends. stETHRate = 1.21. Vault's balance (stETH): 115.5 ether (increased 10% since the 1.).

Alice withdraws 4 ether. Protocol fee: 1 ether
Bob withdraws 10.5 0.8 = 8.4 ether. Protocol fee: 10.5 0.2 = 2.1 ether

This is the expected behavior of the vault.

The actual behavior

stETHRate = 1.1. Vault's balance (stETH): 110 ether (increased 10% since the beginning).

Alice withdraws 4 ether. totalProtocolFee = 1 ether
New vault's balance: 105 ether

Vault ends. stETHRate = 1.21. Vault's balance (stETH): 115.5 ether (increased 10% since the 1.).

Alice withdraws

$$totalEarnings = 115.5 \times \frac{5/1.1}{100-5/1.1} - 1 + 15.5 \times 0.8 = 16.9$$

$$stakingEarningsShare = totalEarnings/2 - \frac{5}{1.1} \times \frac{115.5}{100 - 5/1.1} = 2.95$$

Bob withdraws

$$stakingEarningsShare = totalEarnings/2 - 0 = 8.45$$

In the current logic comparing to the expected behavior, Alice is charged with 1 ether more protocol fee. The protocolFeeReceiver can not claim this 1 ether more fee, and it will stuck in the contract. Moreover, 0.05 ether earnings of Alice is credited to Bob.

Impact

The variable user, who withdraws when isStarted() and !isEnded(), will be charged more protocol fee when they withdraw when the vault ends
The exceed protocol fee will be stuck in the contract
Wrong distribution of the variable earnings

PoC

Add a setter in LidoVault.sol to set lido and lidoWithdrawalQueue to the mock version for easier debugging

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

diff

  /// @notice Lido contract
- ILido public constant lido = ILido(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);
+ ILido public lido = ILido(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);

  /// @notice Lido withdrawal queue contract
- ILidoWithdrawalQueueERC721 public constant lidoWithdrawalQueue =
    ILidoWithdrawalQueueERC721(0x889edC2eDab5f40e902b864aD4d7AdE8E412F9B1);
+ ILidoWithdrawalQueueERC721 public lidoWithdrawalQueue =
    ILidoWithdrawalQueueERC721(0x889edC2eDab5f40e902b864aD4d7AdE8E412F9B1);

+ function setLidoInfo(address _lido, address _withdrawalQueue) public {
+   lido = ILido(_lido);
+   lidoWithdrawalQueue = ILidoWithdrawalQueueERC721(_withdrawalQueue);
+ }

Run command: forge test --match-path test/PoC.t.sol -vv

solidity

// SPDX-License-Identifier: MIT

pragma solidity 0.8.18;

import { VaultFactory } from "contracts/VaultFactory.sol";
import { LidoVault } from "contracts/LidoVault.sol";

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

contract MockLido {
    uint256 public rate;
    mapping(address => uint256) private _sharesOf;

    constructor() {
        rate = 1e27;
    }

    function submit(address _referral) external payable returns (uint256) {
        uint256 shares = msg.value * 1e27 / rate;
        _sharesOf[msg.sender] += shares;
        return shares;
    }

    function approve(address spender, uint256 amount) external returns (bool) {
        return true;
    }

    function getPooledEthByShares(uint256 _sharesAmount) external view returns (uint256) {
        return _sharesAmount * rate / 1e27;
    }

    function getSharesByPooledEth(uint256 _ethAmount) external view returns (uint256) {
        return _ethAmount * 1e27 / rate;
    }

    function balanceOf(address _account) external view returns (uint256) {
        return _sharesOf[_account] * rate / 1e27;
    }

    function sharesOf(address _account) external view returns (uint256) {
        return _sharesOf[_account];
    }

    function setRate(uint256 _rate) external {
        rate = _rate;
    }

    function burnShares(uint256 _amount, address _owner) external {
        uint256 shares = _amount * 1e27 / rate;
        _sharesOf[_owner] -= shares;
    }
}

contract MockLidoWithdrawalQueueERC721 {
    MockLido public lido;

    mapping(uint256 requestId => uint256 amount) public requestIdToAmount;
    uint256 public currentRequestId;
    
    constructor(address _lido) {
        lido = MockLido(_lido);
    }
    function claimWithdrawal(uint256 _requestId) external {
        payable(msg.sender).call{value: requestIdToAmount[_requestId]}("");
    }

    function requestWithdrawals(uint256[] calldata _amounts, address _owner) external returns (uint256[] memory requestIds) {
        lido.burnShares(_amounts[0], _owner);
        requestIdToAmount[currentRequestId] = _amounts[0];

        requestIds = new uint256[](1);
        requestIds[0] = currentRequestId++;
    }
    receive() external payable {
    }
}


contract PoC is Test {
    MockLido lido;
    MockLidoWithdrawalQueueERC721 lidoWithdrawalQueueERC721;

    VaultFactory factory;
    LidoVault vault;

    address fixedDepositor = makeAddr('fixedDepositor');
    address alice = makeAddr('alice');
    address bob = makeAddr('bob');
    address feeReceiver = makeAddr('feeReceiver');

    uint256 protocolFeeBps = 2_000;
    uint256 fixedCap = 100 ether;
    uint256 variableCap = 10 ether;

    function setUp() public {
        lido = new MockLido();
        lidoWithdrawalQueueERC721 = new MockLidoWithdrawalQueueERC721(address(lido));

        factory = new VaultFactory(protocolFeeBps, 0);
        factory.setProtocolFeeReceiver(feeReceiver);

        factory.createVault(fixedCap, 1 days, variableCap);
        (,address addr) = factory.vaultInfo(1);
        vault = LidoVault(payable(addr));

        vault.setLidoInfo(address(lido), address(lidoWithdrawalQueueERC721));

        vm.deal(address(lidoWithdrawalQueueERC721), 1000 ether);

        vm.deal(fixedDepositor, fixedCap);

        vm.prank(fixedDepositor);
        vault.deposit{value: fixedCap}(0);
    }

    function testFirstVulnerabilityPath() public {
        vm.deal(alice, variableCap);

        vm.prank(alice);
        vault.deposit{value: alice.balance}(1);

        lido.setRate(1.1e27);

        vm.startPrank(alice);
        vault.withdraw(1);
        vault.finalizeVaultOngoingVariableWithdrawals();
        vm.stopPrank();

        console.log("Alice's balance after first claim: %e", alice.balance);

        skip(1 days + 1);

        lido.setRate(1.21e27);

        vm.startPrank(alice);
        vault.withdraw(1);
        vault.finalizeVaultEndedWithdrawals(1);
        vm.stopPrank();

        console.log("Alice's balance at the end: %e", alice.balance);        

        vm.prank(feeReceiver);
        vault.withdraw(1);

        vm.startPrank(fixedDepositor);
        vault.claimFixedPremium();
        vault.withdraw(0);
        vm.stopPrank();

        console.log("Vault's balance at the end: %e", address(vault).balance);
    }

    function testSecondVulnerabilityPath() public {
        vm.deal(alice, variableCap / 2);
        vm.deal(bob, variableCap / 2);

        vm.prank(alice);
        vault.deposit{value: alice.balance}(1);

        vm.prank(bob);
        vault.deposit{value: bob.balance}(1);

        lido.setRate(1.1e27);

        vm.startPrank(alice);
        vault.withdraw(1);
        vault.finalizeVaultOngoingVariableWithdrawals();
        vm.stopPrank();

        console.log("Alice's balance after first claim: %e", alice.balance);

        skip(1 days + 1);

        lido.setRate(1.21e27);

        vm.startPrank(alice);
        vault.withdraw(1);
        vault.finalizeVaultEndedWithdrawals(1);
        vm.stopPrank();

        vm.prank(bob);
        vault.withdraw(1);

        console.log("Alice's balance at the end: %e", alice.balance);
        console.log("Bob's balance at the end: %e", bob.balance);

        vm.prank(feeReceiver);
        vault.withdraw(1);

        vm.startPrank(fixedDepositor);
        vault.claimFixedPremium();
        vault.withdraw(0);
        vm.stopPrank();

        console.log("Vault's balance at the end: %e", address(vault).balance);
    }
}

bash

testFirstVulnerabilityPath()
Logs:
  Alice's balance after first claim: 8e18
  Alice's balance at the end: 1.4e19
  Vault's balance at the end: 2e18

Alice's final balance is only 8 ether + 6 ether = 14 ether
The feeReceiver and fixedDepositor have already withdrawn, but there is still ETH left in the contract.

bash

testSecondVulnerabilityPath()
Logs:
  Alice's balance after first claim: 4e18
  Alice's balance at the end: 6.95e18
  Bob's balance at the end: 8.449999999999999999e18
  Vault's balance at the end: 1.000000000000000001e18

Alice's final balance is only 4 ether + 2.95 ether = 6.95 ether
The feeReceiver and fixedDepositor have already withdrawn, but there is still ETH left in the contract.

Mitigation

stakesAmountOwed at LidoVault.sol:534 should exclude the protocolFee

diff

            uint256 protocolFee = ethAmountOwed.mulDiv(protocolFeeBps, 10000);
            totalProtocolFee += protocolFee;
-           uint256 stakesAmountOwed = lido.getSharesByPooledEth(ethAmountOwed);
+           uint256 stakesAmountOwed = lido.getSharesByPooledEth(ethAmountOwed - protocolFee );

totalEarnings at LidoVault.sol:775 should exclude the deduction of totalProtocolFee

diff

-     uint256 totalEarnings = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes,vaultEndingStakesAmount) - totalProtocolFee + vaultEndedStakingEarnings;
+     uint256 totalEarnings = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes,vaultEndingStakesAmount) + vaultEndedStakingEarnings;