branch_indigo - withdraw() has Inconsistent protocol fee accounting between vault ongoing and vault ends

[sherlock-admin3]

branch_indigo

Medium

withdraw() has Inconsistent protocol fee accounting between vault ongoing and vault ends

Summary

withdraw() has Inconsistent protocol fee accounting between vault ongoing and vault ends. withdraw() will calculate incorrect post-protocol fee earnings for variable-side users.

Vulnerability Detail

Before vault ends, when a variable-side user requests a withdrawal of earnings, the post fee earning is: (proportional_totalEarnings - previousWithdrawnAmount) * (1 - protocolFeeBps). Note in this case both (proportional_totalEarnings and previousWithdrawnAmount are pre-fee values.

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

After vault ends, when a variable-side user requests a withdrawal of earnings, the protocol fee is simply applied on proportional totalEarnings. The post fee earning is: (proportional_totalEarnings - totalProtocolFee - previousWithdrawnAmount) . Note that in this case proportional_totalEarnings - totalProtocolFee is a post-fee value, but previousWithdrawnAmount is still pre-fee values.

  function vaultEndedWithdraw(uint256 side) internal {
...
       //@audit After vault ends, totalEarnings is a post-fee value, but variableToWithdrawnStakingEarningsInShares[msg.sender] is always pre-fee value. Inconsistent with withdraw before vault ends.
|>      uint256 totalEarnings = vaultEndingETHBalance.mulDiv(withdrawnStakingEarningsInStakes,vaultEndingStakesAmount) - totalProtocolFee  + vaultEndedStakingEarnings;
        (uint256 currentState, uint256 stakingEarningsShare) = calculateVariableWithdrawState(
          totalEarnings,
          variableToWithdrawnStakingEarningsInShares[msg.sender].mulDiv(vaultEndingETHBalance, vaultEndingStakesAmount)
        );

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

Since previousWithdrawnAmount is always pre-fee values, the earnings shouldn't include fees. And fees should be deducted afterwards.

Impact

When vault ends, withdraw() will calculate incorrect post-protocol fee earnings for variable-side users.

Code Snippet

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

Tool used

Manual Review

Recommendation

In vaultEndedWithdraw(), use pre-fee values for totalEarnings.