For the complete documentation index, see llms.txt. This page is also available as Markdown.

88595 sc high rescuing a not yet claimable withdrawal request permanently poisons the period account pair freezing any later request on that period

Submitted on Aug 14th 2026 at 02:02:31 UTC by @Kalila95146 for Audit Comp | Firelight

  • Report ID: #88595

  • Report Type: Smart Contract

  • Report severity: High

  • Target: https://github.com/immunefi-team/audit-comp-firelight/blob/v1_audit_ready/contracts/core/FirelightVault.sol

  • Impacts:

    • Permanent freezing of funds

Description

Brief/Intro

rescueWithdrawFromBlocklisted marks the source account as already claimed for the rescued period. It accepts any period, including one that is not yet claimable — which is the normal case, since withdrawal requests always target currentPeriod() + 1.

_requestWithdraw never reads that flag. claimWithdraw does, and reverts on it.

So after a rescue, if the account files a new withdrawal request that lands on the same period, the contract accepts the request and burns the shares for a payout it can never honour. The flag is never cleared anywhere in the contract, so the assets are frozen permanently.

Vulnerability Details

FirelightVault uses delayed withdrawals: withdraw/redeem burn the caller's shares and create a pending request targeting currentPeriod() + 1, claimable later via claimWithdraw(period).

1 — The rescue sets a permanent flag, on any period

FirelightVault.sol:845-876:

There is no periods[i] < currentPeriod() check. Rescuing a request for a period that has not yet become claimable therefore sets isWithdrawClaimed[futurePeriod][from] = true.

2 — A new request never consults that flag

FirelightVault.sol:994-1028:

3 — But the claim does

FirelightVault.sol:788-811:

isWithdrawClaimed is never set back to false anywhere in the contract (verified: the only writes are :806 and :870, both to true). The pair (period, account) is poisoned permanently.

Why this is a contract defect, not an operational choice

The most likely objection is "the rescuer could simply have waited until the period became claimable". Four reasons that does not hold:

  1. The function explicitly accepts any period. rescueWithdrawFromBlocklisted takes an arbitrary periods[] array with no guard whatsoever on its values. The contract permits the dangerous path and signals nothing.

  2. Waiting defeats the purpose of the rescue. It would leave a blocklisted account's value in limbo for one to two full periods — the opposite of what a compliance rescue exists to do.

  3. Nothing communicates a timing constraint. Not the signature, not the NatSpec, not any revert. The only way to know is to have already found this bug.

  4. A batch rescue cannot avoid it. An account with requests spread over several periods is rescued in a single call (trigger condition #5), and such a call necessarily includes the not-yet-claimable period.

A rescuer following the obvious, documented behaviour produces the broken state.

Exploit scenario

Every role action below is legitimate and squarely within its documented privileges — this is the normal compliance workflow, not misuse.

#
Action
Resulting state

1

Victim requests a withdrawal during period C

withdrawSharesOf[C+1][victim] > 0

2

Victim is blocklisted (compliance suspicion)

—

3

Rescuer calls rescueWithdrawFromBlocklisted(victim, safe, [C+1])

withdrawSharesOf[C+1][victim] = 0, isWithdrawClaimed[C+1][victim] = true

4

Suspicion cleared, victim unblocklisted (still period C)

—

5

Victim files a new withdrawal → targets currentPeriod()+1 = C+1

withdrawSharesOf[C+1][victim] > 0, shares burned

6

Period advances, victim calls claimWithdraw(C+1)

revert AlreadyClaimedPeriod

Step 3 is not an odd operator choice: a blocklisted user cannot claim (claimWithdraw carries notBlocklisted(_msgSender()), reverting BlocklistedAddress), so rescuing is the only mechanism available to move that pending value. Verified in test R1.

No recovery path exists:

  • claimWithdraw reverts forever — the flag is never cleared;

  • a second rescue requires isWithdrawClaimed[period][from] == false → also reverts;

  • the shares are already burned, so the amount cannot be re-requested.

Exact trigger conditions

The bug fires only when a rescue targets a not-yet-claimable period and a later request lands on that same period. Both edges are asserted:

#
Condition
Result

1

Rescue a period that is already claimable

No bug — a new request can only target currentPeriod()+1, never a past period

2

Re-request one period later

No bug — lands on a fresh, unpoisoned period

3

Re-request via redeem()

Reproduces identically — same _requestWithdraw

4

rescueSharesFromBlocklisted (shares only)

No poisoning — never touches isWithdrawClaimed

5

Rescue passing several periods in one call

Every period is poisoned at once — damage scales from one transaction

Rows 1, 2 and 4 are included to show the finding is precisely scoped rather than over-generalised. Row 5 matters for severity: cleaning up an account with requests spread over several periods turns each of them into a permanent trap.

Either option suffices:

  1. In _requestWithdraw, revert if isWithdrawClaimed[period][receiver] is already true; or

  2. In rescueWithdrawFromBlocklisted, set the claimed flag only for periods that are already claimable — for a not-yet-claimable period, zeroing withdrawSharesOf is enough, since the flag guards nothing once the balance is zero.

Why this was not caught earlier

  • test/rescue.js covers rescuing shares and rescuing withdrawals, but never files a new withdrawal request after a rescue on the same period — the exact trigger.

  • FirelightVault.sol line coverage is 91.2% (npx hardhat coverage); _requestWithdraw's interaction with a pre-existing isWithdrawClaimed flag is among the uncovered behaviours.

This is an oversight, not an accepted design trade-off.

Impact Details

Severity: Critical — Permanent freezing of funds (impact id 17). Fallback: High — Temporary freezing of funds ≥ 24h (id 6261).

  1. Permanent loss for the user. Shares burned, assets unclaimable forever. The victim can lose their entire remaining position in a single call — test R2 redeems the full balance into the poisoned period and ends with balanceOf == 0 and maxWithdraw == 0.

  2. Assets permanently stranded, owned by nobody. pendingWithdrawAssets is incremented at step 5 and never decremented (only claimWithdraw decrements it, and it reverts). Since totalAssets() = super.totalAssets() - pendingWithdrawAssets (:427-429), the tokens physically remain in the vault while totalAssets() excludes them forever: not claimable by the victim, not redistributed to anyone else. Asserted: asset.balanceOf(vault) - vault.totalAssets() >= frozenAmount, and it does not heal over subsequent periods.

  3. Reduced cover capacity. CoverOrderAllocator._computeAvailableCapacity (CoverOrderAllocator.sol:710-714) values the vault leg as vault.totalAssetsAt(vault.currentPeriodStart()) — a checkpoint of totalAssets() = super.totalAssets() - pendingWithdrawAssets, which permanently excludes the stranded amount. The capacity computation therefore values the vault below the collateral it actually holds, for good.

    Asserted in test R5, on the exact quantity the allocator reads: the checkpoint returned by totalAssetsAt(currentPeriodStart()) equals the live totalAssets() (so there is no temporal skew between the two readings), and asset.balanceOf(vault) - checkpoint equals the frozen amount exactly — still short five periods later.

  4. The withdrawal path gives no warning. maxWithdraw, maxRedeem, balanceOf, previewWithdraw and withdrawalsOf all return healthy values, and withdraw() succeeds and emits WithdrawRequest normally. The raw isWithdrawClaimed mapping is a public getter and does return true — but nothing on the withdrawal path consults or surfaces it: the request is accepted and the shares burned regardless. A user following the documented ERC-4626 flow has no signal. This is why it is a protocol defect, not user error.

Bounds of the claim (test C). The victim is not locked out of withdrawing in general: a later request naming a different receiver claims normally. What is permanently frozen is the amount already committed to the doomed request — shares burned, assets unclaimable, no way back. And since nothing on the withdrawal path signals the poisoning (Impact 4), the victim has no reason to name a different receiver until after the loss has already occurred.

Explicitly NOT claimed (verified, stated for accuracy): the share price is not distorted for other depositors. When the doomed request was filed, the victim's shares were burned at the same moment pendingWithdrawAssets grew, so totalAssets/totalSupply is preserved — previewDeposit returns an identical quote before and after the freeze. Remaining holders are neither diluted nor enriched; the loss falls entirely on the victim.

Scope justification

  • No role acts maliciously or incorrectly. Blocklist → rescue → unblocklist is the expected operational sequence of a compliance system. The victim is a legitimate user.

  • The rescue is forced, not optional. A blocklisted account cannot call claimWithdraw (reverts BlocklistedAddress), so its pending value can only be moved by rescueWithdrawFromBlocklisted.

  • Rescuing a not-yet-claimable period is the normal case. Requests target currentPeriod() + 1, so a request stays non-claimable throughout the period in which it was filed and the next one. A rescuer reacting promptly necessarily lands in that window; nothing in the signature, NatSpec or revert conditions suggests waiting, and the function explicitly accepts any periods[i].

  • It bypasses an intended constraint. The implicit invariant "an accepted withdrawal request remains claimable" is broken even though _requestWithdraw accepted the request, burned the shares and took the value.

References

  • contracts/core/FirelightVault.sol:845-876 — rescueWithdrawFromBlocklisted, the write at :870

  • contracts/core/FirelightVault.sol:994-1028 — _requestWithdraw, no read of the flag

  • contracts/core/FirelightVault.sol:788-811 — claimWithdraw, the revert at :794

  • contracts/core/FirelightVault.sol:427-429 — totalAssets()

  • contracts/core/CoverOrderAllocator.sol:710-714 — _computeAvailableCapacity

Proof of Concept

Proof of Concept

Step-by-step reproduction

Every role acts strictly within its documented privileges. This is the normal compliance workflow of the protocol, not misuse.

  1. A user files a normal withdrawal request. During period C, the victim calls withdraw(amount, victim, victim). _requestWithdraw burns the shares and credits withdrawSharesOf[C+1][victim], because a request always targets currentPeriod() + 1.

  2. Compliance blocklists the account. The blocklister calls addToBlocklist(victim) on suspicion. From this moment the victim can no longer call claimWithdraw — it carries notBlocklisted(_msgSender()) and reverts BlocklistedAddress. The pending value can now only be moved by a rescue.

  3. The rescuer moves the pending request to a safe address. rescueWithdrawFromBlocklisted(victim, safe, [C+1]) zeroes withdrawSharesOf[C+1][victim] and credits safe. It also sets isWithdrawClaimed[C+1][victim] = true (FirelightVault.sol:870), with no check that C+1 is already claimable — and it is not, since we are still inside period C.

  4. The suspicion is cleared and the account is un-blocklisted. removeFromBlocklist(victim), still within period C.

  5. The victim files a new withdrawal request. It targets currentPeriod() + 1 = C+1 — the poisoned period. _requestWithdraw never reads isWithdrawClaimed, so the request is accepted, the shares are burned, pendingWithdrawAssets grows, and WithdrawRequest is emitted normally. maxWithdraw, maxRedeem, balanceOf and previewWithdraw all keep reporting healthy values.

  6. The period advances and the claim reverts, permanently. claimWithdraw(C+1) hits if (isWithdrawClaimed[period][sender]) revert AlreadyClaimedPeriod(period) (:794). There is no way out: the flag is never written back to false anywhere in the contract (the only two writes, :806 and :870, both set true); a second rescue requires the flag to be false and therefore reverts too; and the shares are already burned, so the amount cannot be re-requested.

The assets stay physically in the vault while totalAssets() excludes them forever — claimable by nobody, redistributed to nobody.

Running the tests

The suite is a single self-contained file written against the project's own Hardhat fixtures (test/setup/fixtures.js). No external harness, no mocks beyond what the repository already ships. Drop it into test/ and run:

Test file — test/FL02_full_suite.js

Output — 16 passing, 0 failing

What each test establishes

  • POC / CONTROL — the freeze reproduces, and the identical flow without the rescue claims successfully. The defect is attributable to the rescue, not to the delayed-withdrawal mechanism.

  • A, B — the funds are in limbo and no admin or role path clears the flag (second rescue, re-blocklist, pause/unpause cycle all fail).

  • C — bounds the claim honestly: a later request naming a different receiver does claim. What is frozen is the amount already committed.

  • D — the doomed request is accepted silently, with WithdrawRequest emitted normally.

  • 1, 2, 4 — the negative controls. Rescuing an already-claimable period, re-requesting one period later, and rescueSharesFromBlocklisted alone all produce no bug. The finding is precisely scoped rather than over-generalised.

  • 3 — reproduces identically through redeem().

  • 5 — a multi-period rescue poisons every period passed in one call, so the damage scales from a single transaction.

  • R1–R5 — each closes one anticipated objection, including R4 which states explicitly what is not claimed (the share price is unaffected).

Was this helpful?