Description

In multiple places, isStarted() and isEnded() are called in the same line of code that an event is emitted. For example, when a variable-side user withdraws before the vault starts:

function withdraw(uint256 side) external {
  // [...] [ case !isStarted() && side == VARIABLE ]

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

  variableBearerToken[msg.sender] -= sendAmount;
  variableBearerTokenTotalSupply -= sendAmount;

  (bool sent, ) = msg.sender.call{value: sendAmount}("");
  require(sent, "ETF");

  emit VariableFundsWithdrawn(sendAmount, msg.sender, isStarted(), isEnded());
  return;

However, the call to the sender to send sendAmount of native ETH can reenter the LidoVault. If during this reentrancy, a deposit is issued that completes the vault despite the withdrawal, then isStarted() called after the reentrancy returns will incorrectly be true.

Impact

Reentrancy attacks can cause the isStarted field of emitted events to be falsified, confusing external subscribers to these events.

Recommendations

For emit lines where an event is emitted and where isStarted() and isEnded() at the beginning of the call are known because of conditionals around that line of code, such as the above example where clearly the vault has not started yet, replacing them with constants would save gas and fix this issue.

For cases like finalizeVaultNotStartedFixedWithdrawals, where reentrancy is possible through transferWithdrawnFunds but it is unknown based on context whether the vault has started or not, consider emitting the event first or saving the values of isStarted() and isEnded() prior to the call to sender.

Remediation

This issue has been acknowledged by Saffron, and fixes were implemented in the following commits:

• 4c971a17↗
• 44a82869↗
• c77079b3↗