Zellic - Audit Reports

Category: Protocol Risks

Unnecessary precision loss in protocol fee

Informational Severity

Informational Impact

High Likelihood

Description

When a variable-ongoing withdrawal is requested, the variable variableToWithdrawnStakingEarnings is multiplied by the inverse of the protocol fee and then set to the current state minus the protocol fee applied to the current state:

if (lidoStETHBalance > fixedETHDeposits) {
  (uint256 currentState, uint256 ethAmountOwed) = calculateVariableWithdrawState(
    (lidoStETHBalance - fixedETHDeposits) + withdrawnStakingEarnings + totalProtocolFee,
    variableToWithdrawnStakingEarnings[msg.sender].mulDiv(10000, 10000 - protocolFeeBps) // rounds down!
  );

  if (ethAmountOwed >= minStETHWithdrawalAmount()) {
    // estimate protocol fee and update total - will actually be applied on withdraw finalization
    uint256 protocolFee = ethAmountOwed.mulDiv(protocolFeeBps, 10000);
    totalProtocolFee += protocolFee;

    withdrawnStakingEarnings += ethAmountOwed - protocolFee;
    variableToWithdrawnStakingEarnings[msg.sender] = currentState - currentState.mulDiv(protocolFeeBps, 10000);

However, precision loss happens when a quantity is multiplied by 10000/(10000 - protocolFeeBps), then an amount is added to it, and then the amount is subtracted from itself multiplied again by protocolFeeBps / 10000.

This precision loss is entirely unnecessary, because more exact quantities already exist in memory in the ethAmountOwed and protocolFee variables.

Impact

Unnecessary precision loss affects the estimated protocol fee. These extra multiplications and divisions cost gas while reducing the accuracy of the estimation.

Recommendations

Note that because of the implementation of calculateVariableWithdrawState, the return value will have the relation that ethAmountOwed = currentState - previousWithdrawnAmount.

So, what the new variableToWithdrawnStakingEarnings[msg.sender] is set to is equal to this:

= currentState - currentState.mulDiv(protocolFeeBps, 10000);
= currentState.mulDiv(10000 - protocolFeeBps, 10000)
= (ethAmountOwed + previousWithdrawnAmount).mulDiv(10000 - protocolFeeBps, 10000)
= ethAmountOwed.mulDiv(10000 - protocolFeeBps, 10000) + previousWithdrawnAmount.mulDiv(10000 - protocolFeeBps, 10000)
= ethAmountOwed.mulDiv(10000 - protocolFeeBps, 10000) +
      variableToWithdrawnStakingEarnings[msg.sender].mulDiv(10000, 10000 - protocolFeeBps).mulDiv(10000 - protocolFeeBps, 10000)
// (assuming no precision loss)
= ethAmountOwed.mulDiv(10000 - protocolFeeBps, 10000) + variableToWithdrawnStakingEarnings[msg.sender]
= variableToWithdrawnStakingEarnings[msg.sender] + ethAmountOwed.mulDiv(10000 - protocolFeeBps, 10000)
= variableToWithdrawnStakingEarnings[msg.sender] + ethAmountOwed - ethAmountOwed.mulDiv(protocolFeeBps, 10000)
= variableToWithdrawnStakingEarnings[msg.sender] + ethAmountOwed - protocolFee

Therefore, assuming no precision loss, that line of code is equivalent to this:

variableToWithdrawnStakingEarnings[msg.sender] += ethAmountOwed - protocolFee;

Additionally, this matches the line of code before it.

Remediation

This issue has been acknowledged by Saffron, and a fix was implemented in commit 4828dd90↗.