eeyore - Invalid calculations of total earnings can lead to a DoS due to an `ETF` error when withdrawing or cause a lock of funds for early withdrawal users.

[sherlock-admin3]

eeyore

High

Invalid calculations of total earnings can lead to a DoS due to an ETF error when withdrawing or cause a lock of funds for early withdrawal users.

Summary

In the Saffron Lido Vault, variable users can withdraw any accrued income at any time. However, withdrawing stETH has consequences, including a reduced APR for all users. The Saffron protocol aims to compensate users who do not withdraw their income until the Vault’s end by implementing the following mechanism:

This withdrawal decreases the vault's daily income, reducing the income for all remaining users. To address this, we ensured that variable users receive the same amount at the end of the Saffron Lido Vault, regardless of whether anyone withdraws before the end. This was achieved by using ‘stakes’ in our calculations for variable users' income.

As a result, users who withdraw early are penalized and compensate other users' APR for those early income withdrawals, ensuring that users who do not withdraw still receive the same APR as if all the fixed stETH shares remained in the Vault for its entire duration.

The stETH operates as a rebalancing token, where the yield occurs on the shares the Vault holds. Therefore, early income withdrawals reduce the overall number of shares held by the Vault.

Unfortunately, there is an issue with the equations used for the calculations.

First, in the withdrawal() function:

          (uint256 currentState, uint256 ethAmountOwed) = calculateVariableWithdrawState(
            (lidoStETHBalance.mulDiv(currentStakes + withdrawnStakingEarningsInStakes, currentStakes) - fixedETHDeposits),
            variableToWithdrawnStakingEarningsInShares[msg.sender].mulDiv(lidoStETHBalance, currentStakes)
          );

In this case, user withdrawals are overinflated with each consecutive income claim. If a user claims all their income through withdrawal() right before the Vault ends, it creates a situation where more income and fees are accounted for than the actual ETH available after Lido withdrawal and fixed users' ETH returns. As a result, the last claiming variable user or protocol fee recipient may be unable to claim without an ETH donation.

Second, in the vaultEndedWithdraw() function:

      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 case, users who withdrew prematurely and did not withdraw all their income before the Vault’s end will have their earnings over-reduced.

Root Cause

1. The root cause of the overinflation appears to be that shares have different values depending on when they were withdrawn from the LidoVault, and this discrepancy is not accounted for. https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L526-L529

2. The issue with the fund lock seems to arise from protocol fees being accounted for too early when calculating vaultEndedStakingEarnings. https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L682-L683 https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol#L775

Internal pre-conditions

• Users withdraw income before the Vault ends.

External pre-conditions

• Lido stETH share price increases as expected.

Attack Path

This issue occurs naturally.

Case 1:

1. A user withdraws all income continuously before the Vault ends.
2. Vault calculations become overinflated, and there will be insufficient ETH for the last user.

Case 2:

1. A user withdraws some income before the Vault ends, but a significant portion of the income is withdrawn after the Vault ends.
2. The user is undercompensated, and part of their assets will be locked in the Vault.

Since both cases can counterbalance each other in a multi-user Vault, this issue might go unnoticed for some time. However, users who withdraw income early will eventually have part of their income locked.

Impact

• Funds could be locked.
• DoS could occur due to insufficient withdrawal funds.

PoC

Add to `1.LidoVault.test.ts` to demonstrate the scenario described above:

```javascript it('PoC: Invalid calculations in total earnings', async function () { const { lidoVault, deployer, addr1, addr2, addr3, lidoVaultAddress, } = await loadFixture(deployLidoVaultFixture) const { lidoMock, lidoWithdrawalQueueMock } = await setupMockLidoContracts(lidoVault) await lidoVault.connect(addr1).deposit(SIDE.FIXED, { value: parseEther('1000') }) await lidoVault .connect(addr2) .deposit(SIDE.VARIABLE, { value: (variableDeposit * BigInt(5)) / BigInt(10) }) await lidoVault .connect(addr3) .deposit(SIDE.VARIABLE, { value: (variableDeposit * BigInt(5)) / BigInt(10) }) await lidoVault.connect(addr1).claimFixedPremium() expect(await lidoVault.isStarted()).to.be.true const balanceAddr2Before = await ethers.provider.getBalance(addr2) const balanceAddr3Before = await ethers.provider.getBalance(addr3) let gasAddr2 = BigInt(0); let gasAddr3 = BigInt(0); const stakingEarnings = parseEther('1') let shares = await lidoMock.sharesOf(lidoVaultAddress) let balance = await lidoMock.balanceOf(lidoVaultAddress) await lidoMock.addStakingEarningsForTargetETH( balance + stakingEarnings, lidoVaultAddress ) gasAddr2 += calculateGasFees(await (await lidoVault.connect(addr2).withdraw(SIDE.VARIABLE)).wait()) gasAddr2 += calculateGasFees(await (await lidoVault.connect(addr2).finalizeVaultOngoingVariableWithdrawals()).wait()) shares = await lidoMock.sharesOf(lidoVaultAddress) balance = await lidoMock.balanceOf(lidoVaultAddress) await lidoMock.addStakingEarningsForTargetETH( balance + ((stakingEarnings * shares) / parseEther('1000')), lidoVaultAddress ) // COMMENT two lines below for Case 2: // ↓↓↓ From here ↓↓↓ gasAddr2 += calculateGasFees(await (await lidoVault.connect(addr2).withdraw(SIDE.VARIABLE)).wait()) gasAddr2 += calculateGasFees(await (await lidoVault.connect(addr2).finalizeVaultOngoingVariableWithdrawals()).wait()) // ↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑↑ // end vault const { endTime } = await getTimeState(lidoVault) await time.increaseTo(endTime + BIG_INT_ONE) gasAddr2 += calculateGasFees(await (await lidoVault.connect(addr2).withdraw(SIDE.VARIABLE)).wait()) gasAddr2 += calculateGasFees(await (await lidoVault.connect(addr2).finalizeVaultEndedWithdrawals(SIDE.VARIABLE)).wait()) gasAddr3 += calculateGasFees(await (await lidoVault.connect(addr3).finalizeVaultEndedWithdrawals(SIDE.VARIABLE)).wait()) const balanceAddr2After = await ethers.provider.getBalance(addr2) const balanceAddr3After = await ethers.provider.getBalance(addr3) const balanceAddr1Before = await ethers.provider.getBalance(addr1) const gas = calculateGasFees(await (await lidoVault.connect(addr1).finalizeVaultEndedWithdrawals(SIDE.FIXED)).wait()) const balanceAddr1After = await ethers.provider.getBalance(addr1) console.log(`User 1 :`, balanceAddr2After - balanceAddr2Before + gasAddr2) console.log(`User 2 :`, balanceAddr3After - balanceAddr3Before + gasAddr3) console.log(`Fixed side :`, balanceAddr1After - balanceAddr1Before + gas) console.log(`LidoVault :`, await ethers.provider.getBalance(lidoVault)) console.log(`Protocol fee :`, await lidoVault.appliedProtocolFee()) await lidoVault.connect(deployer).finalizeVaultEndedWithdrawals(SIDE.VARIABLE) console.log(await ethers.provider.getBalance(lidoVault)) }) ```

Note: There are two cases within the same PoC.

Console log for Case 1:

User 1       : 989505494505494505n <-- overinflated
User 2       : 990002497502497501n <-- overinflated
Fixed side   : 1000000000000000000000n
lidoVault    : 19992507492517493n
Protocol fee : 19995004995004994n

Error: VM Exception while processing transaction: reverted with reason string 'ETF'

Console log for Case 2:

User 1       : 984502997002997003n <-- 0.005 ETH loss
User 2       : 990002497502497501n
Fixed side   : 1000000000000000000000n
LidoVault    : 24995004995014995n  <-- stuck in Vault
Protocol fee : 19995004995004994n

Mitigation

In the withdraw() function, when handling premature income extraction, consider that shares withdrawn before the end of the Vault are worth less than those at the end.

In the vaultEndedWithdraw() function, ensure that protocol fees are not incorrectly reducing total earnings for users who withdrew prematurely.

Ensure all calculations favor the protocol without unfairly penalizing users.