search

sherlock-audit / 2024-08-saffron-finance-judging

9 stars 5 forks source link

YowiSec - Improper Handling of the Zero Address in LidoVault.sol Withdrawal Arrays Will Cause Denial of Service for Vault Participants #156

Open sherlock-admin2 opened 2 months ago

sherlock-admin2 commented 2 months ago

YowiSec

Medium

Improper Handling of the Zero Address in LidoVault.sol Withdrawal Arrays Will Cause Denial of Service for Vault Participants

Summary

Improper handling of the zero address during the finalization of withdrawal requests leads to a Denial of Service (DoS). Once a withdrawal request is finalized using the delete keyword, the user’s address is replaced with address(0). During subsequent finalization attempts, the contract encounters this address and reverts with the "WNR" (Withdrawal Not Ready) error, halting the withdrawal process and effectively preventing any further withdrawals or vault finalizations.

Root Cause

This issue arises from the delete keyword being used to remove user addresses from the fixedOngoingWithdrawalUsers and variableToVaultOngoingWithdrawalRequestIds arrays during withdrawal finalization. Instead of removing the entry, delete sets the entry to 0x0000000000000000000000000000000000000000. When the contract later encounters this invalid address in finalization processes, it reverts, stopping all future withdrawal requests from being processed.

The issue affects the following functions:

In particular, the line of interest where the deletion occurs is: https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol?plain=1#L601 and https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol?plain=1#L695. This vulnerability leads to a temporary Denial of Service, where users cannot successfully withdraw their funds due to the contract's reversion when encountering the zero address.

Internal pre-conditions

The contract must have users with ongoing withdrawals in fixedOngoingWithdrawalUsers or variableToVaultOngoingWithdrawalRequestIds. Withdrawal requests must be initiated by calling withdraw function, leading to addresses being pushed to respective arrays.

External pre-conditions

External interaction or protocol change is not directly required for this issue to manifest, as it is contingent solely on the state management within the LidoVault contract itself.

Attack Path

  1. User calls withdraw(SIDE.FIXED) to initiate a withdrawal.
  2. User then calls finalizeVaultOngoingFixedWithdrawals, setting their address in the array to the zero address.
  3. Any user attempting to finalize withdrawals thereafter encounters the zero address, causing the contract to revert with the WNR error, resulting in a denial of service.

Impact

The affected parties (users attempting to finalize withdrawals) cannot execute withdrawals or finalize them, causing funds to be locked without resolution unless the contract is updated or managed to bypass or handle zero address entries effectively.

PoC

Paste the following code in the withdraw section of tests within the 1.LidoVault.test.ts file.

async function setupWithdrawalFixture() {
  const { lidoVault, addr1, addr2 } = await loadFixture(deployLidoVaultFixture)

  const depositAmount = parseEther('200')

  await lidoVault.connect(addr1).deposit(SIDE.FIXED, { value: depositAmount })
  await lidoVault.connect(addr2).deposit(SIDE.FIXED, { value: depositAmount })

  return { lidoVault, addr1, addr2 }
}

it('Should handle ZERO_ADDRESS in the withdrawal array without revert (Fixed DoS Test)', async function () {
  const { lidoVault, addr1, addr2 } = await setupWithdrawalFixture()

  await lidoVault.connect(addr1).withdraw(SIDE.FIXED)
  let fixedOngoingUsers = await lidoVault.fixedOngoingWithdrawalUsers(0)

  expect(fixedOngoingUsers).to.equal(addr1.address)

  await lidoVault.connect(addr1).finalizeVaultOngoingFixedWithdrawals()

  fixedOngoingUsers = await lidoVault.fixedOngoingWithdrawalUsers(0)
  expect(fixedOngoingUsers).to.equal(ZERO_ADDRESS)

  await lidoVault.connect(addr2).withdraw(SIDE.FIXED)
  fixedOngoingUsers = await lidoVault.fixedOngoingWithdrawalUsers(1)
  expect(fixedOngoingUsers).to.equal(addr2.address)

  await expect(lidoVault.connect(addr1).finalizeVaultOngoingFixedWithdrawals()).to.not.be.reverted

  const withdrawalIds = await lidoVault.getFixedOngoingWithdrawalRequestIds(addr2.address)
  expect(withdrawalIds.length).to.equal(0)
})

it('Should handle ZERO_ADDRESS in the withdrawal array without revert (Variable DoS Test)', async function () {
  const { lidoVault, addr1, addr2 } = await setupWithdrawalFixture()

  await lidoVault.connect(addr1).withdraw(SIDE.VARIABLE)
  let variableOngoingRequestIds = await lidoVault.variableToVaultOngoingWithdrawalRequestIds(
    addr1.address,
    0
  )

  expect(variableOngoingRequestIds).to.equal(1)

  await lidoVault.connect(addr1).finalizeVaultOngoingVariableWithdrawals()

  variableOngoingRequestIds = await lidoVault.variableToVaultOngoingWithdrawalRequestIds(
    addr1.address,
    0
  )
  expect(variableOngoingRequestIds).to.equal(ZERO_ADDRESS)

  await lidoVault.connect(addr2).withdraw(SIDE.VARIABLE)
  variableOngoingRequestIds = await lidoVault.variableToVaultOngoingWithdrawalRequestIds(
    addr2.address,
    0
  )
  expect(variableOngoingRequestIds).to.equal(1)

  await expect(lidoVault.connect(addr1).finalizeVaultOngoingVariableWithdrawals()).to.not.be
    .reverted

  const variableWithdrawalIds = await lidoVault.getVariableToVaultOngoingWithdrawalRequestIds(
    addr2.address
  )
  expect(variableWithdrawalIds.length).to.equal(0)
})

it('Should revert when trying to finalize an ended vault withdrawal containing ZERO_ADDRESS (Fixed)', async function () {
  const { lidoVault, addr1 } = await loadFixture(endVaultFixture)

  await lidoVault.connect(addr1).withdraw(SIDE.FIXED)

  await lidoVault.connect(addr1).finalizeVaultOngoingFixedWithdrawals()

  await expect(lidoVault.finalizeVaultEndedWithdrawals(SIDE.FIXED)).to.be.revertedWith('WNR')
})

it('Should revert when trying to finalize an ended vault withdrawal containing ZERO_ADDRESS (Variable)', async function () {
  const { lidoVault, addr1 } = await loadFixture(endVaultFixture)

  await lidoVault.connect(addr1).withdraw(SIDE.VARIABLE)

  await lidoVault.connect(addr1).finalizeVaultOngoingVariableWithdrawals()

  await expect(lidoVault.finalizeVaultEndedWithdrawals(SIDE.VARIABLE)).to.be.revertedWith('WNR')
})

Mitigation

Let claimFixedVaultOngoingWithdrawal return 0 if fixedUser is address(0) (see https://github.com/sherlock-audit/2024-08-saffron-finance/blob/main/lido-fiv/contracts/LidoVault.sol?plain=1#L690-697).