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

88347 sc medium rescue writes a terminal claimed flag onto a still open withdrawal bucket permanently trapping the account s next withdrawal

Submitted on Aug 12th 2026 at 20:07:23 UTC by @johanw for Audit Comp | Firelight

  • Report ID: #88347

  • Report Type: Smart Contract

  • Report severity: Medium

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

  • Impacts:

    • Permanent freezing of funds

Description

Summary

rescueWithdrawFromBlocklisted sets isWithdrawClaimed[period][from] = true at FirelightVault.sol:870 on a withdrawal bucket that is still open for new entries. _requestWithdraw writes to currentPeriod() + 1 (:1008), which is the only bucket a fresh request can land in, so a user blocklisted right after requesting can only ever be rescued on an open bucket.

If that account is later removed from the blocklist while the same period is still running and queues a new withdrawal, the new withdrawal can never be claimed. claimWithdraw checks the flag at :794 and reverts AlreadyClaimedPeriod forever. The shares were already burned inside _requestWithdraw at :1025, so the loss is realised at request time and the user only discovers it two periods later.

No AccessControl role can repair it. The rescue cannot undo its own damage: the guard at :864 refuses a second rescue of the same from. Nothing in the contract clears the flag.

The fix is a one-line deletion, and this report ships the counterfactual that proves it is safe: a byte-identical copy of the vault with only line 870 removed, plus 7 adversarial tests against it.

This is a variant of OpenZeppelin M-02, not a restatement of it. M-02 described this exact sink, in these words: "their shares would be burned during the call to _requestWithdraw, but the subsequent call to claimWithdraw would revert due to the isWithdrawClaimed check." Its trigger was a periodDuration change causing period-number reuse, it is marked Resolved, and that trigger is genuinely dead today because _addPeriodConfiguration:1096 keeps startingPeriod continuous and :1093 forces newEpoch >= nextPeriodEnd(), so period numbers are monotonic. The rescue path is nowhere in M-02, and the M-02 fix does nothing for it. The delta is spelled out in full below.

There is no attacker and this report claims none. Two individually correct privileged operations compose into unrecoverable state for an unprivileged third party, with no revert and no signal.

Vulnerability Details

Contracts:

  • contracts/core/FirelightVault.sol - rescueWithdrawFromBlocklisted() (L845-876, the defect at L870, the guard added by an earlier fix at L865), claimWithdraw() (L788-811, the open-bucket bound at L791, the flag read at L794, the flag write at L806), _requestWithdraw() (L994-1028, the target bucket at L1008, the share burn at L1025)

  • contracts/core/FirelightVaultStorage.sol - isWithdrawClaimed (L71)

Named in the competition's Assets in Scope table.

Bug Class: State-machine defect. A terminal, never-cleared flag is written to a mutable bucket that is still accepting entries, so a later legitimate entry into that bucket is silently unclaimable.

Severity: Medium. CVSS 4.0 vector: CVSS:4.0/AV:N/AC:L/AT:P/PR:H/UI:P/VC:N/VI:H/VA:H/SC:N/SI:N/SA:N = 5.8, computed with the cvss library rather than estimated. The filed severity and the CVSS band agree. AT:P for the operational sequence inside one period. PR:H because the blocklist and rescue calls require BLOCKLIST_ROLE and RESCUER_ROLE, and CVSS measures the privileges of the party performing the action even when that party is acting correctly. UI:P because the victim's own routine withdraw is what destroys the funds. VA:H for permanent unavailability of that user's funds. Selected impact is Permanent freezing of funds; that impact normally sits higher, and the severity is calibrated to Medium because the frozen amount is one user's single request rather than pooled funds, other LPs are provably unharmed, and the trigger needs a specific operational sequence.

Root Cause

claimWithdraw can never set the flag on an open bucket, because it is bounded to closed ones:

The rescue has no such bound, and writes the same terminal flag to any period the caller names:

A fresh withdrawal can only ever land in the open bucket:

so the claim is then permanently refused:

isWithdrawClaimed has exactly two writers in the contract, :806 and :870, and both only ever set it to true. No function clears it.

The team already recognised this hazard class and fixed half of it. Commit a2def47, "Fix rescuing withdrawal to a user that already claimed them", added the to-side guard now at L865:

That addresses a stale flag on the destination. The source side, where the rescue itself sets the flag on a still-open bucket, was never addressed.

Sequence, every step a documented operation:

  1. Period 1. Alice queues a withdrawal of 500 USDC. It lands in bucket 2, and her shares are burned.

  2. Still period 1. Compliance screening flags alice, so BLOCKLIST_ROLE blocklists her and RESCUER_ROLE runs rescueWithdrawFromBlocklisted on bucket 2, which is the only bucket her request could be in. Her bucket shares move to the recovery address and isWithdrawClaimed[2][alice] is set.

  3. Still period 1. The flag turns out to be a false positive, so BLOCKLIST_ROLE calls removeFromBlocklist, a first-class function with its own event.

  4. Alice, in good standing, queues another 500 USDC withdrawal. It lands in bucket 2 again, because bucket 2 is still the open bucket. Her shares are burned again. The call succeeds with no revert and no warning.

  5. Period 3. The recovery address claims its 500 USDC normally. Alice's claimWithdraw(2) reverts AlreadyClaimedPeriod(2), and will do so forever.

Proof of Concept

See the Proof of Concept field below for the full runnable Foundry tests and output.

Impact

An unprivileged user permanently loses the funds they committed to a withdrawal, having already surrendered the corresponding shares. In the PoC that is 500 USDC of a 1,000 USDC position, and the amount is whatever the user queues in that window.

The failure is silent in both directions that matter. The user's withdraw succeeds, so nothing signals that the position is already doomed, and the contract state that would reveal it, isWithdrawClaimed(period, account), is a public getter no interface would think to read before withdrawing. The user discovers the loss two periods later at claim time, when it is already terminal.

No AccessControl role can repair it. Re-blocklisting and rescuing again is refused by the guard at L864, so the rescue is barred from undoing its own damage, and the position ends up both unclaimable and unrescuable.

Stated so it is not overread:

  • Other LPs are unharmed. The share burn at :1025 and pendingWithdrawAssets += assets at :1019 are consistent, so share price does not move and totalAssets() cannot underflow. This is not a protocol-wide loss and is not insolvency.

  • The loss is bounded to the amount queued in that one window. The user can withdraw normally in any later period.

  • A secondary and conditional effect: pendingWithdrawAssets carries a reservation for a claim that can never execute, so totalAssets() under-reports. This is not unconditional. payout decrements pendingWithdrawAssets at :960 and rewrites withdrawAssets[capturePeriod + 1] at :954, so an incident payout that drains that bucket inside its payout window clears the drift. The victim stays frozen either way.

  • Recovery by contract upgrade is possible. The production vault uses the Transparent Upgradeable Proxy pattern, so the ProxyAdmin owner could upgrade and clear the mapping. The PoC deploys a bare ERC1967Proxy with no admin and does not model that escape.

Limitations, stated up front

  • The trigger needs the un-blocklisting. Every writer of withdrawSharesOf was checked: only _requestWithdraw:1017, which credits receiver, and the rescue itself. withdraw:765 and redeem:727 both carry notBlocklisted(receiver), so while the account is blocklisted nothing can credit it. There is no alternative path.

  • The window is the remainder of the current period. SMALLEST_PERIOD_DURATION is 1 day, so the window can be short.

  • A rescue that targets an already-closed bucket is safe, because currentPeriod() is monotonic and no closed bucket can receive a new entry. That boundary is asserted by its own test.

  • The PoC grants one address every role. In production BLOCKLIST_ROLE and RESCUER_ROLE may be held separately, which makes the sequence a two-party operation rather than a one-party one.

Distinctness From Disclosed Issues

OpenZeppelin M-02, the closest prior art, named first on purpose. M-02 describes this sink in the same words used above, and is marked Resolved. Its trigger was an increase to periodDuration causing currentPeriod() to underreport and a period number to be reused. That trigger is dead in the current design: _addPeriodConfiguration:1096 derives startingPeriod continuously from the previous configuration and :1093 requires newEpoch >= nextPeriodEnd(), so period numbers only ever advance and a closed bucket never re-opens. The rescue path is absent from M-02, and the M-02 remediation does nothing about it. Because M-02 is Resolved rather than unfixed, the ineligibility rule for unfixed audit findings does not apply.

Macro, 2026-07-29. All 18 findings reviewed. None mentions the rescue path, isWithdrawClaimed, claimWithdraw, or unclaimable withdrawals.

The November 2025 Immunefi competition ran on the legacy vault, which ships this identical code at contracts/legacy/FirelightVaultPredeposits.sol:760. Of its published reports, the only two touching this function are a gas note on repeated array access inside rescueWithdrawFromBlocklisted and an accuracy note on the withdrawalsOf() view. Neither is this defect.

Known issues. The three listed known issues concern the first-loss buffer's peg, the first-loss buffer's custody model, and aggregate claims not being capped on-chain. None touches the blocklist or rescue path.

Recommended Fix

The minimal fix is to delete line 870:

Line 869 already zeroes the source's bucket balance, and a zero balance alone makes claimWithdraw revert NoWithdrawalAmount at :803. The flag write adds no protection on any of the three read sites, and it costs two things: the trap described here, and the permanent disqualification of that account from ever being re-rescued for that period. The Proof of Concept field ships a byte-identical copy of the vault with only that line removed and 7 adversarial tests against it, covering double rescue, duplicate periods in one call, rescue chains and re-rescue after a re-queue. All pass, and share conservation holds.

Two alternatives, if removing the flag write is unwelcome:

  1. Mirror the bound that already makes the claim path safe. Add if (periods[i] > currentPeriod()) revert InvalidPeriod(); to the rescue loop. claimWithdraw:791 carries exactly this bound, which is precisely why the claim path can never flag an open bucket. A dedicated test asserts that property.

  2. Defence in depth. Make _requestWithdraw revert when isWithdrawClaimed[period][receiver] is already set. That converts a silent permanent loss into a revert, and is worth adding even alongside the deletion.

Supporting Materials

  • poc-test/RescueTrapsLaterWithdrawal.t.sol - the defect, the redundancy observation, and the closed-bucket scope boundary. 3 tests.

  • poc-test/RefuteLine870.t.sol - the counterfactual suite against a vault with line 870 deleted, proving the fix is safe and that the accumulator drift is clearable by payout. 8 tests.

  • poc-test/FirelightVaultNoFlag.sol - generated from the in-scope source by a single sed, given in the Proof of Concept field, so no modified contract needs to be trusted.

Proof of Concept

Proof of Concept

Two suites, 11 tests, all passing. Self-contained: the vault and the mock token are deployed inside the test, so everything runs offline with no fork, no RPC endpoint and no API key, and nothing touches a deployed contract or real funds.

The second suite is the part that matters most. It runs against a copy of the in-scope vault with only line 870 removed and nothing else changed, which is what turns "the flag write is redundant" from an assertion into evidence. That copy is produced from the in-scope source by one sed, given below, so no modified contract has to be taken on trust.

Setup

Requires Foundry and git only.

Save the foundry.toml below to the repository root, and the two test files below to poc-test/. Then generate the counterfactual vault, which differs from the in-scope source only by the deleted line and the import paths and contract name needed to compile it from poc-test/:

The OpenZeppelin versions are the pair package.json pins: "@openzeppelin/contracts": "5.2.0" under overrides, and "@openzeppelin/contracts-upgradeable": "~5.4.0". foundry.toml skips contracts/fasset/** and contracts/oracle/**, the only sources importing the flare-smart-contracts npm packages.

Run

Scenario: a vault with a 7 day period and two 1,000 USDC depositors. Alice queues 500 USDC in period 1, is blocklisted and rescued while bucket 2 is still open, is un-blocklisted in the same period, and queues 500 USDC again. Every timestamp is derived from a fixed t0 rather than from block.timestamp after a warp, because solc common-subexpression eliminates block.timestamp across vm.warp under via_ir and a naive capture silently reads the pre-warp value.

Result (actual forge output)

What each test establishes

test_rescueOnAnOpenBucketPermanentlyTrapsALaterWithdrawal is the defect. It asserts the trap, then asserts there is no repair through any role: re-blocklisting and calling the rescue again reverts AlreadyClaimedPeriod, so the rescue cannot undo its own damage and the position is left both unclaimable and unrescuable.

test_scope_rescueOnAClosedBucketIsSafe states the boundary of the finding rather than leaving it to be discovered. A rescue targeting an already-closed bucket is harmless, because currentPeriod() is monotonic and a closed bucket can never receive a new entry. Alice's later withdrawal lands in bucket 3 and claims normally.

test_control_zeroedBucketSharesAlreadyBlockAClaimWithoutTheFlag shows that a zero bucket balance alone already causes claimWithdraw to revert NoWithdrawalAmount, which is the observation the remediation rests on. It is suggestive rather than conclusive on its own, which is why the second suite exists.

test_patchedContractHasNoTrap is the counterfactual: the identical sequence against the vault with line 870 deleted, where alice claims her second withdrawal normally.

test_attack_doubleRescueSamePeriodStillReverts, test_attack_duplicatePeriodInOneCallStillReverts, test_attack_fromCannotClaimAfterRescue and test_attack_rescueChainAndToSideGuard are the adversarial checks on the proposed fix. On the patched contract a second rescue of the same source still reverts, now via NoWithdrawalAmount from the zeroed balance instead of AlreadyClaimedPeriod; a duplicate period inside one call still reverts because the balance is re-read each iteration; the rescued source still cannot claim; and the to-side guard added by commit a2def47 still fires on a rescue chain. Nothing is lost by the deletion.

test_attack_reRescueAfterRequeueConservesValue shows the patched contract allowing the re-queued position to be rescued, with share conservation holding across all three accounts and pendingWithdrawAssets draining to zero. The same test logs that on the original contract that position is both unrescuable and unclaimable.

test_claimPathCannotFlagAnOpenBucket proves the alternative remediation is the right shape: the period >= currentPeriod() bound at claimWithdraw:791 is load-bearing, and is exactly what the rescue lacks.

test_attack_payoutCanDrainTheStuckAccumulator is included because it argues against part of my own case. It shows an incident payout decrementing pendingWithdrawAssets back to zero, which is why the accumulator drift is reported above as conditional rather than permanent. The victim remains frozen throughout it.

A first run compiles the whole tree, so the compile line differs from a cached run. Gas figures may vary slightly by Foundry version; the asserted values do not.

Build configuration, foundry.toml

poc-test/RescueTrapsLaterWithdrawal.t.sol

poc-test/RefuteLine870.t.sol

Previous88345 sc high inclusive period boundary checkpoint lets a staker underfund valid claims
Next88353 sc high a vault exit mined in the first second of a period writes that period s own exposure snapshot permanently short paying an approved cover claim

Was this helpful?