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

88420 sc high rescuewithdrawfromblocklisted sets the same flag claimwithdraw reads as already claimed and never clears it permanently freezing any later withdrawal the account makes into t

Submitted on Aug 13th 2026 at 04:37:23 UTC by @daivo for Audit Comp | Firelight

  • Report ID: #88420

  • 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

isWithdrawClaimed[period][account] carries two different meanings. claimWithdraw reads it as "this account has already claimed this period"; rescueWithdrawFromBlocklisted writes it as "this account's withdrawal was rescued away". It is never cleared. Because a rescue can execute while the target bucket is still the upcoming one, the same account can afterwards accrue a fresh, fully legitimate withdrawal for exactly that period — and can then never claim it. No role can recover it either: the rescue path reverts on the very same flag. The assets remain counted in pendingWithdrawAssets, so they are also permanently subtracted from totalAssets() and from every staker's share price. In the PoC, 600 tokens sit in the vault while totalAssets() reads 0.

Vulnerability Details

claimWithdraw treats the flag as the replay guard (FirelightVault.sol:788-811):

if (isWithdrawClaimed[period][sender]) revert AlreadyClaimedPeriod(period);   // 794
...
isWithdrawClaimed[period][sender] = true;                                     // 806

rescueWithdrawFromBlocklisted reuses the same slot as a "moved away" marker (FirelightVault.sol:861-873):

There is no path anywhere in the contract that sets this mapping back to false.

Why the normal claim path can never collide, but the rescue path can. claimWithdraw(period) requires period < currentPeriod(), and _requestWithdraw always books into currentPeriod() + 1. So once an account has genuinely claimed period P, any new request necessarily lands in a bucket strictly greater than P. The two can never meet — the flag is safe there.

The rescue has no such constraint. periods is arbitrary caller-supplied input, gated only on withdrawSharesOf[period][from] != 0. A rescue therefore commonly executes while the bucket is still currentPeriod() + 1 — i.e. before the period in which further requests would also be booked into it. Once the account is removed from the blocklist, its next withdraw/redeem in that same period books into the poisoned bucket:

_requestWithdraw never consults isWithdrawClaimed, so the request succeeds, the shares are burned, and withdrawalsOf(period, account) reports a positive owed balance. Only at claim time does line 794 reject it — permanently.

There is no recovery path for the user.

  • claimWithdraw — reverts AlreadyClaimedPeriod forever.

  • rescueWithdrawFromBlocklisted again (after re-blocklisting) — reverts at line 864 on the same flag.

  • rescueSharesFromBlocklisted — operates on ERC-20 share balances, but the shares were already burned by the withdrawal request. Irrelevant.

  • No admin setter, no sweep, no force-claim exists. Nothing short of a contract upgrade returns the assets to the victim.

One precise caveat, stated so it cannot be used as a rebuttal. The stranded assets are not literally untouchable by the protocol for the whole of their life: while capturePeriod == P - 1 and currentPeriod ∈ {P - 1, P}, payout() can still draw from withdrawAssets[P] and decrement pendingWithdrawAssets, so an incident payout in that window can consume part of the balance. That is not a recovery — the victim receives nothing either way, and the assets go to the payout receiver. Once currentPeriod > P the window closes and the balance is stranded permanently, unreachable by every function in the contract.

This is live today, not only in the audited version. The defect is not introduced by V2. contracts/legacy/FirelightVaultPredeposits.sol:754-760 — the implementation currently behind proxy 0x4C18Ff3C89632c3Dd62E796c0aFA5c07c4c1B2b3 — contains the identical overload:

and the same replay guard in claimWithdraw at lines 694/706. The live vault holds 58,284,732 FXRP with pendingWithdrawAssets = 226,259.11 FXRP — the withdrawal buckets this bug operates on are non-empty right now. The V1→V2 upgrade carries the defect forward unchanged.

The triggering sequence uses only documented, in-permission actions. BLOCKLIST_ROLE blocklists a flagged account; RESCUER_ROLE rescues the pending withdrawal to a compliant beneficiary — exactly what the function is for; BLOCKLIST_ROLE later removes the account once the compliance issue is resolved, which is why removeFromBlocklist exists. The user then performs an ordinary withdrawal. No party misbehaves at any point, and the loss lands on the innocent user.

The deployed vault runs 1-day periods (periodConfigurations(0) = (1764682969, 86400, 0) on proxy 0x4C18Ff3C89632c3Dd62E796c0aFA5c07c4c1B2b3), so "the same period" is a 24-hour window — well within a same-day resolution of a false-positive screening hit.

The project's own test/rescue.js contains 30 tests covering this function but never removes an account from the blocklist after a rescue, so the state is unreachable by the existing suite.

Impact Details

Direct, permanent, unrecoverable loss of the victim's withdrawal, plus a permanent reduction of the vault's usable collateral.

Measured in the PoC (1,000 staked, 400 rescued away, 600 withdrawn afterwards):

Consequences:

  1. The user's funds are permanently frozen. The full second withdrawal is unrecoverable by the victim, and by every privileged role, short of a contract upgrade. (See the caveat in Vulnerability Details: an incident payout() can consume part of the balance while the capture window is still open, but that never returns it to the victim.)

  2. The protocol's collateral is permanently reduced. Because totalAssets() = balanceOf(vault) - pendingWithdrawAssets, the stranded amount is excluded from the share price forever. Other stakers do not absorb it as yield — it is simply removed from the accounting. In the PoC the vault physically holds 600 tokens while reporting totalAssets() == 0.

  3. Cover capacity shrinks correspondingly and permanently. CoverOrderAllocator._computeAvailableCapacity values staked collateral via vault.totalAssetsAt(currentPeriodStart()), so every stranded amount permanently reduces how much cover the protocol can sell.

  4. The damage is unbounded in principle. The frozen amount equals whatever the account withdraws after being un-blocklisted — up to its entire remaining stake. Each occurrence is independent and cumulative across accounts and periods.

This is the in-scope Permanent freezing of funds impact, which the program classifies as Critical.

Honest note on feasibility. This requires a specific but entirely legitimate operational sequence: blocklist → rescue → un-blocklist → withdraw again, all within one vault period. It is not attacker-triggerable at will. If the program weighs that down, Temporary freezing of funds for at least 24 hours (High) understates it, since the freeze is genuinely permanent and no emergency action short of a contract upgrade recovers the funds — and the program states emergency actions are not an automatic reason to downgrade.

References

Recommended fix

Separate the two meanings. The rescue should zero the balance rather than mark it claimed, since withdrawSharesOf[period][from] = 0 already prevents a double rescue via the existing _withdrawOf == 0 guard:

If an explicit marker is preferred, use a distinct mapping (isWithdrawRescued) that claimWithdraw does not read. Alternatively, make claimWithdraw idempotent against balance rather than a boolean — zero withdrawSharesOf[period][sender] on claim and drop the flag entirely.

Proof of Concept

Runnable PoC: test/integration/poc_rescue_poison.js (2 tests, both passing against v1_audit_ready). It uses the project's own test/setup/fixtures.js vault fixture with default settings.

Observed output:

Step-by-step

Let P = currentPeriod() + 1 — the bucket every withdrawal made this period lands in.

  1. Alice stakes 1,000. deposit(1000, alice).

  2. Alice requests a partial withdrawal of 400. withdraw(400, alice, alice). Now withdrawSharesOf[P][alice] > 0.

  3. Compliance flags Alice. BLOCKLIST_ROLE calls addToBlocklist(alice) — an ordinary in-permission action.

  4. RESCUER_ROLE rescues the pending withdrawal to Bob. rescueWithdrawFromBlocklisted(alice, bob, [P]) — exactly what the function exists for. State afterwards: withdrawSharesOf[P][alice] == 0, withdrawSharesOf[P][bob] > 0, and critically isWithdrawClaimed[P][alice] == true.

  5. The flag is resolved and Alice is un-blocklisted. removeFromBlocklist(alice). Still the same vault period — assert currentPeriod() == P - 1.

  6. Alice withdraws her remaining 600. withdraw(600, alice, alice) succeeds: shares are burned, withdrawSharesOf[P][alice] > 0, pendingWithdrawAssets increases, and withdrawalsOf(P, alice) reports 600 owed. _requestWithdraw never consults isWithdrawClaimed.

  7. Advance two periods so bucket P becomes claimable (currentPeriod() > P).

  8. Alice calls claimWithdraw(P) → reverts AlreadyClaimedPeriod(P). She has claimed nothing; the stale rescue marker blocks her.

  9. Confirm there is no recovery. Re-blocklist Alice and call rescueWithdrawFromBlocklisted(alice, bob, [P]) again → reverts AlreadyClaimedPeriod(P) at line 864, on the same flag. No other function can move the balance.

  10. Confirm the funds are stranded protocol-wide. Bob claims his rescued portion normally; afterwards the vault still physically holds 600 tokens, pendingWithdrawAssets still counts them, and totalAssets() reports 0. The assets are unreachable by anyone and permanently excluded from the share price and from cover-capacity accounting.

  11. Control (test 2). Without steps 3–5, the identical pair of withdrawals (400 then 600, same period) claims normally and Alice receives the full amount — isolating the rescue as the sole cause.

PoC source — full file, not present in the repo

These files do not exist in v1_audit_ready. Create them at the paths shown below (they import only fixtures that already ship with the project), then run the command above. No other change to the repository is required.

test/integration/poc_rescue_poison.js

Environment

  • Branch v1_audit_ready, local tree verified byte-identical to the in-scope files.

  • Full suite with the PoC added: 478 passing, 0 failing.

Previous88416 sc medium post payout share price collapse in firelightvault payout lets an unprivileged 100 wei deposit capture 99 of the slashed lps subsequent premium leading to theft of unclaimed
Next88425 sc high rescue marks an open withdrawal bucket claimed and permanently freezes later post unblock principal

Was this helpful?