Repository navigation
fix(bridge): re-land reserved-redemption (#1121) with escrow P0s fixed - #1129
piotr-roslaniec wants to merge 2 commits into
Conversation
Two P0 escrow bugs found in the #1116 multi-agent review of #1121 (reverted via #1122 pending a fix), fixed before re-landing: 1. unwindPendingAction had no branch for ActionType.Redemption: a pending redemption superseded by a late acceptance/reanchor/ redemption settlement on the same position was marked Superseded with its escrowed mintedAmount never refunded, permanently stranding it as unbacked Bank balance on the Bridge. Added the missing refund branch, matching notifyReservationRedemptionTimedOut. 2. loadSettleableAction allowed late (post-TimedOut) settlement of a Redemption action. notifyReservationRedemptionTimedOut already refunds the full escrowed claim to the redeemer and returns the reservation to Active on timeout; permitting a late proof afterward let a wallet that confirms the redemption transaction after the timeout deliver the BTC while the redeemer keeps the refunded TBTC, burning nothing. Reachable via ordinary slow confirmation, no attacker required. Restricted loadSettleableAction so a Redemption action must still be Pending to settle; a TimedOut redemption can never be proven again. Deleted the now-unreachable resolveLateRedemptionAgainstPending and the dead late-branch of submitReservedRedemptionProof. Adds the escrow-path test coverage that was previously zero for submitReservedRedemptionProof: regression tests for both fixes at the loadSettleableAction/unwindPendingAction unit level and through the real submitReservationProof dispatcher entry point.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
External audit (V12) targeted this exact headV12 audited The two escrow P0s this commit set out to fix are confirmed fixed. Verified against the parent:
No spurious burn fires, and no escrow is stranded on supersession. The important scoping point
So 10 of the 11 findings are inherited, not introduced here, and most cannot be fixed in this PR since their defect sites are outside its diff. Notably this PR does not touch The one finding that concerns this PR's own change: 184199Reported Critical, "Timeout race enables BTC and TBTC double claim". Assessment splits three ways:
Refund-plus-delivered-BTC with nothing burned is therefore a pre-existing, deliberately accepted property of tBTC redemptions, not something reservations introduced. That reframes 184199 away from "new Critical" and onto three narrower reservation-specific deltas:
This looks like a design decision rather than a patch. The rationale comment says a post-timeout broadcast "becomes a fresh anchor requiring a new acceptance", but the output script is redeemer-chosen at request time, so the protocol cannot reclaim that UTXO, and nothing constrains a wallet from broadcasting close to Also worth notingThis PR is currently Findings inventory with per-finding permalinks and routing: see the comment on #1116. |
Summary
Re-lands the reserved-redemption and veto surface from #1121 (reverted
via #1122 for shipping two P0 escrow bugs found in the #1116 multi-agent
review), with both bugs fixed and regression-tested before re-landing.
Scope note: this is milestone-2 work, not part of the milestone-1 A-H
decomposition. This branch was originally based on reconciliation commit
52bf2822. The current M1 tip is9f8f5ef1and has diverged fromdevagain; this PR is therefore currently conflicting with
reservations-upgrade.The pending M1 review fix is #1161. Do not merge this PR ahead of milestone 1
landing; it remains a draft for that reason.
What changed
Commit 1 (
f362d6b3) re-applies #1121's original diff unmodified, as thebase for the fixes below.
Commit 2 (
d826c1ec) fixes two P0 escrow issues:unwindPendingActionmissingRedemptionbranch. The functionhandled
ReanchorandAcceptanceaction types when unwinding asuperseded pending generation, but had no branch for
Redemption.A pending redemption superseded by a late acceptance, late reanchor,
or late redemption settlement on the same position was marked
Supersededwith its escrowedmintedAmountnever refunded -permanently stranding it as unbacked Bank balance on the Bridge.
Fixed by adding the missing refund branch
(
self.bank.transferBalance(pendingAction.redeemer, pendingAction.amount)),matching what
notifyReservationRedemptionTimedOutalready does on anordinary timeout.
Late-settlement double-claim on redemption timeout.
loadSettleableActionallowed aTimedOutRedemptionaction to besettled late, same as
Acceptance/Reanchor. ButnotifyReservationRedemptionTimedOutalready refunds the fullescrowed claim to the redeemer and returns the reservation to
Activeon timeout - unlikeAcceptance/Reanchor, whose timeoutsleave the position open for late resolution. Permitting a late proof
afterward let a wallet that confirms the redemption transaction after
the timeout deliver the BTC while the redeemer keeps the refunded
TBTC, burning nothing. This is reachable via ordinary slow Bitcoin
confirmation - no attacker required, honest parties trigger it too.
Fixed by restricting
loadSettleableActionso aRedemptionactionmust still be
Pendingto settle; aTimedOutredemption can neverbe proven again (a wallet that broadcasts anyway just produces an
output the protocol no longer recognizes - it becomes a fresh anchor
requiring a new acceptance). This removed the vulnerable code path
entirely rather than adding new Bank-integration machinery to
force-collect from an already-refunded redeemer (the Bank's
decreaseBalanceonly burns the caller's own balance, so the Bridgecannot force-burn a third party without new Bank surface). Deleted
the now-unreachable
resolveLateRedemptionAgainstPendingand thedead late-settlement branch of
submitReservedRedemptionProof.Test coverage.
submitReservedRedemptionProofhad zero testcoverage before this PR (across both
ReservationProofs.test.tsandBridge.ReservedRedemption.test.ts). Added regression tests for bothfixes above at the
loadSettleableAction/unwindPendingActionunitlevel, plus a dispatcher-level test proving the timeout guard is
wired through the real
submitReservationProofproduction entrypoint.
Verification
hardhat compile: clean.ReservationProofs,Bridge.ReservedRedemption,Reservation,Bridge.Reservation*,RedemptionWatchtower,Bridge.Redemption,ReservationVault): 592 passing, 0 failing.prettier --checkandsolhinton touched files: clean (no newwarnings beyond 3 pre-existing, unrelated ordering warnings already
present in the re-landed baseline).
Not in scope
Everything else already covered by the #1116 review of #1121 remains as
originally landed; this PR only touches the two P0 escrow findings and
adds the missing settlement-proof test coverage.