feat(security): automated incident response and on-chain circuit breaker for critical pool alerts - #259
Conversation
The security scan already detected critical alerts and persisted them, but nothing acted on them. This adds the circuit breaker: enough critical alerts against one pool trips it, the pool is paused so no further money moves, and an incident is recorded for an admin to review. The decision logic is a pure module with no database, clock or network, so the cases that matter most are exercised in tests: below threshold, during cooldown, already paused, unknown pool, and dry-run. Execution against Supabase lives separately, writing the incident before the pause so a crash midway leaves a record with no action rather than a paused pool nobody can explain. Cooldown is a gate checked before the action, not a warning after it: with the defaults a pool is auto-paused at most once a day, and if it trips again it stays paused for an admin instead of flapping. Only pauses that actually happened count towards it, so a dry-run period does not consume a pool's allowance. Dry-run is the default and the intended rollout path. Both scan endpoints report whether an action would have fired regardless of dry-run state, so the flag produces real data on threshold accuracy before anyone arms it. One constraint worth stating plainly: the on-chain half of the pause cannot be automatic. `rotational::pause` asserts `admin.require_auth()` and that the caller is the pool's stored admin, which is the creator's own wallet; the platform holds no key that satisfies it, since `SPONSOR_SECRET_KEY` only pays fees. So the platform pause is immediate and automatic, and the contract call is prepared for the admin to sign, tracked as `onchain_status`. Automating it would mean adding a guardian role to a deployed funds-holding contract, which is the maintainers' call, not this layer's. No contract was changed. `emergency_withdraw` stays manual and admin-only. The breaker's action type has exactly two values, and a test asserts that set has not grown. Closes JointSave-org#254
The previous wording said automating the contract call would require adding a guardian role to the contract. That is wrong, and worth fixing before anyone designs around it. The gap is key custody, not the contract. A SorobanAuthorizationEntry is signed independently of the transaction envelope, so an admin can pre-sign one covering pause(admin) and the backend can submit it later, paying the fee itself. @stellar/stellar-sdk already exports authorizeEntry and the wallet modules in @creit.tech/stellar-wallets-kit implement signAuthEntry, so both halves are available in this repo today. require_auth on a classic G address also honours Stellar multisig at the medium threshold, which is a second route. Still not implemented here: entries are single-use and expire, so it needs a signing flow, storage and expiry handling, and a submission path of its own. onchain_status is the hook it plugs into.
…ed authorization Completes the circuit breaker. It now carries the pause through to the contract instead of stopping at the platform level and asking an admin to finish the job. The obstacle was never the contract. `rotational::pause` asserts `admin.require_auth()` and the admin is the creator's own wallet, so the platform cannot call it on its own keys, and `SPONSOR_SECRET_KEY` cannot stand in because a fee bump authorises nothing inside a transaction. That is key custody, not a contract limitation. A SorobanAuthorizationEntry is signed independently of the transaction envelope, so the party that authorises a call and the party that submits it can differ. The admin signs one entry covering exactly pause(admin) on exactly their pool's contract; the platform stores it and, when the breaker trips, wraps it in a transaction it pays for. Two signatures, two jobs: the admin authorises the call, the platform authorises the fee. No contract change, no shared key, and the credential the platform holds can do exactly one thing. The entry is validated on arrival rather than trusted. It must be address-credentialed, invoke pause, take the signer as its only argument, and carry no sub-invocations, so it cannot smuggle a second call. It is matched against the pool's contract and admin, and refused if it expires too soon to be useful. Tests build real entries with the XDR library and assert each refusal, including an entry authorising emergency_withdraw and one hiding it in a sub-invocation. Entries are single-use and expire, so selection is its own tested decision: spent, revoked, expired, expiring inside a safety margin, or signed for another contract or a rotated admin are each rejected for their own reason, and the entry expiring soonest is spent first. An authorization is marked used before submission, since its nonce may reach the network even when the response does not. Without a usable authorization nothing is lost: the platform pause still happens immediately and the incident stays at onchain_status 'pending' for the admin to sign. The stored XDR is never returned by the API and the table has no read policy outside the service role, because whoever holds it can pause the pool.
…tion Revoking took the admin address from the request body and compared it to the pool's creator_address. Both are public, so anyone who could read a pool could disarm its automatic on-chain pause. That is the worst place in this feature for a spoofable check: an attacker preparing to drain a pool could switch off the defence first, using only data the app already publishes. Registering never had this problem and still does not need a session: an entry that was not signed by the pool's real admin is refused by the inspector no matter who posts it, and the contract would reject it anyway. Revoking has no such self-validation, so it now asks for proof instead of a claim. The wallet signs a short message naming the exact authorization and the moment it was signed. The server rebuilds that message and verifies it under SEP-53 against the pool's admin as recorded, never against an address from the request, so a spoofed admin_address buys nothing. A captured proof goes stale in five minutes and does not transfer to another authorization; replaying it against the same one is a no-op, since revoking a revoked entry changes nothing. No challenge table was needed: the signed message carries its own timestamp and names its target, and the action is idempotent. The revocation is also written to pool_activity now, so disarming a pool is as auditable as arming it.
|
@diegoveme kindly resolve conflict |
Two conflicts, both from main and this branch adding to the same spot. frontend/lib/supabase.ts: main added the bridge_transactions table to the generated Database types while this branch added pause_authorizations and incidents. They are independent table definitions, so all three are kept. frontend/package.json: both sides appended suites to test:unit. Kept as a union, with this branch's incident-response, pause-onchain and wallet-proof suites next to pending-transactions, the file they extend. Neither lockfile was touched by this branch and both still match main, so pnpm install --frozen-lockfile is unaffected.
Both endpoints decided who the caller was from an admin_address field in the request body and compared it to the pool's creator. A pool id and a creator address are both public, so that check could be satisfied by anyone willing to type the right address, and archiving a pool takes it out of Explore and out of every member's active list. The endpoints now verify a wallet signature against the creator address as the database records it, the same proof this branch already requires before revoking a pause authorization. The body's admin_address is no longer what authorises anything. The messages name the action and the pool and carry a timestamp, so a proof gathered for one pool cannot be used on another, a proof to archive cannot be replayed to unarchive, and a captured one stops working within minutes. Five tests cover exactly those cases. The archive banner signs before it calls, so the admin flow keeps working. The daily sweep in /api/cron/archive-pools is unaffected: it writes through the admin client and never touches these routes.
|
Conflict resolved and pushed. Two files were involved: While merging I noticed the new I have pushed a fix in the same branch, since it reuses the wallet-proof helper this PR already introduces:
Happy to split that into its own PR if you would rather review it separately, though it would need the proof helper from this branch to land first.
|
Sendi0011
left a comment
There was a problem hiding this comment.
Review — Automated Incident Response with On-Chain Circuit Breaker
This is the strongest PR in the project so far. The design solves a genuinely hard key-custody problem — submitting an on-chain pause that calls rotational::pause (which asserts admin.require_auth()) — without the platform ever holding an admin key, no contract change, and with the only automatic action being a pause. Thoroughly reasoned, thoroughly tested, and the security boundaries are enforced by a test rather than by convention.
The parts I verified most carefully
1. The boundary is real, not just claimed. IncidentAction has exactly "pause" | "none" and the test at lib/incident-response.test.ts asserts that union cannot have grown. emergency_withdraw and any funds-moving call are unreachable from the automated path.
2. The bearer-credential validation is correct and honest. inspectPauseAuthorization checks the entry is address-credentialed (a source-account credential authorises whoever submits — correctly refused), has no sub-invocations (so a hidden emergency_withdraw can't be smuggled), invokes pause, takes the signer as its only argument, and is matched against the pool's actual contract and admin. The tests build real XDR with the SDK and assert each refusal, including both a root emergency_withdraw and one nested as a sub-invocation. This is exactly the right thing to check on a bearer credential the platform will submit later.
3. Single-use accounting is done the honest way. The authorization is marked spent before submission (incident-actions.ts: the conditional claim with .is("used_at", null) prevents two racing scans from spending the same entry), because a consumed nonce can never succeed again regardless of whether the response reaches us. Good.
4. Ordering is safe under partial failure. Incident row is written before the pause ("decided, not carried out"), the pause write is conditional on status = 'active' (closes the decision→write race), and execution is contained per-pool so one failure doesn't stop the next. A crash midway leaves a recoverable record rather than an unexplained paused pool.
5. RLS on pause_authorizations is deliberately closed. No select policy, unlike incidents/security_alerts — the right call, since the stored XDR is a bearer credential and only the service-role routes may read it.
6. Dry-run is a real rollout mechanism, not a switch. In dry-run it still decides, records, and notifies ("would have paused"), and wouldFire is reported independently of execution — so you get genuine threshold data before arming INCIDENT_AUTO_PAUSE_ENABLED.
7. High-stakes endpoints got signature proof. Revocation of a pause authorization (which disables a pool's defence) and archive/unarchive (which change what everyone sees) now require a SEP-53 wallet signature verified against the pool's creator — closing the "caller-supplied admin_address is just a claim" gap where it actually matters, while leaving the lower-stakes read endpoints on the existing convention.
Non-blocking suggestions
-
verifySignedMessageaccepts both SEP-53 framings — the SHA-256-prefixed digest and the raw prefixed bytes passed to ed25519. Accepting both is defensible for wallet compatibility, but it is a wider acceptance surface than the strict spec. If the specific wallets you support all implement the strict SEP-53 framing, consider narrowing it to the spec form to reject malformed signatures deterministically. Purely an availability/consistency trade-off, not a security regression. -
incidentsisSELECT USING (true)— consistent withsecurity_alerts, but it exposes admin addresses (resolved_by) and pool IDs publicly. If you ever want that tightened, a separate policy mirroring the audit-log convention would fit. Not blocking. -
submitOnChainPauseis not covered end-to-end — as you correctly note in the PR body, the live submission path needs a deployed pool + funded sponsor + RPC. Its callers' error paths are covered and it degrades correctly. Worth adding an integration test in CI against a local testnet instance if one becomes available, soassembleTransactionauth-entry preservation is verified against a live simulation.
Verification
- All 5 CI checks green (Lint/Format, Component tests, Build Soroban, Node unit, Playwright)
- Mergeable state
clean - 56 new unit tests covering cooldown, dry-run, escalation, grouping, config bounds, auth selection, entry inspection, revocation proof, and the boundary test
- Docs (
INCIDENT_RESPONSE.md) and.env.exampleare thorough and accurate test:unitscript correctly registers all three new test files
Approved. Ready to merge. Closes #254.
Description
The security scan already detected critical alerts and wrote them to
security_alerts, but nothing ever acted on them. This adds the layer thatdoes: when enough critical alerts land against one pool, the circuit breaker
trips, the pool is paused, and an incident is recorded for an admin to review,
resolve and resume.
The pause is real on both sides. The platform halts the pool immediately, and the
contract's own
pauseis invoked automatically too, without the platform everholding an admin key and without changing the contract.
Closes #254
How the on-chain pause is automatic
This deserves spelling out, because the obvious reading is that it cannot be.
rotational::pauseassertsadmin.require_auth()and that the caller is thepool's stored admin, which is the creator's own wallet (set in
initialize,passed as
admin: addressfrom the create-group forms). The platform cannot callit on its own keys, and
SPONSOR_SECRET_KEYcannot stand in: a fee bump pays fora transaction, it authorises nothing inside it.
That is a key-custody problem, not a contract limitation. A
SorobanAuthorizationEntryis signed independently of the transaction envelope,so the party that authorises a call and the party that submits it can be
different. The admin signs one entry covering exactly
pause(admin)on exactlytheir pool's contract. The platform stores it and, when the breaker trips, wraps
it in a transaction it pays for and signs the envelope of.
Two signatures doing two jobs: the admin authorises the call, the platform
authorises the fee. No contract change, no shared key, and the credential the
platform holds can do exactly one thing.
An alternative exists and was deliberately not taken.
require_authfor aclassic
Gaddress uses Stellar multisig at the medium threshold, so an admincould add a platform signer with enough weight instead. Simpler to operate, but a
far wider grant, since that weight applies to the account in general rather than
to a single call.
The entry is validated, not trusted
A stored entry is a bearer credential the platform will submit later, so what it
refuses matters more than what it accepts. On arrival an entry must be
address-credentialed (a source-account credential authorises whoever submits,
which is not a delegation), invoke
pause, take the signer as its only argument,and carry no sub-invocations, so it cannot smuggle a second call alongside the
pause. It is then matched against the pool's contract and admin, and refused if
it expires too soon to be useful.
The tests build real entries with the XDR library and assert each refusal,
including an entry that authorises
emergency_withdrawand one that hides it ina sub-invocation.
The XDR is never returned by the API, and the table has no read policy outside
the service role. Whoever holds it can pause the pool, which would be a griefing
vector against the pool's own members.
When no authorization exists
Nothing is lost. The platform pause still happens immediately, the incident stays
at
onchain_status = 'pending', the admin is told why in their notification, andthey sign the contract call themselves from the review screen. Pre-authorising
only removes the wait.
Entries are single-use and expire, so an admin re-signs one occasionally.
GET /api/admin/pause-authorizationsreportsarmed: truewhile a usable oneexists.
emergency_withdraw
Nothing in the automated path can move funds, and that is enforced in two places
rather than promised. The breaker's
IncidentActionhas exactly two values,pauseandnone, with a test asserting the set has not grown. And anauthorization entry that names
emergency_withdraw, at the root or nested, isrefused before it is ever stored.
Type of Change
How Has This Been Tested?
cargo testpasses (smart contracts): not applicable, no contract changedpnpm buildsucceeds (frontend): all three new routes appear in the outputpnpm lintpasses (frontend): no errorspnpm test:unitpasses: 299 tests, 56 of them added herenpx tsc --noEmitreports no errors in any file this PR touchesOn formatting:
pnpm format:checkpasses for every file in this PR. Running itacross the whole repo on a Windows checkout reports 373 files, including ones
this PR never touches, because git converts to CRLF locally while Prettier
defaults to LF. That does not happen on CI.
What the tests do and do not cover. The decision logic, the authorization
lifecycle, and the entry inspection are covered directly, and the inspection
tests run against real signed XDR rather than fixtures. What they do not cover is
a live submission: that needs a deployed pool, a funded sponsor and an RPC, so
submitOnChainPauseis exercised through its callers' error paths rather thanend to end. It is written to degrade to "an admin needs to sign it" on every
failure, and the platform pause has already happened before it runs.
emergency_withdrawat the root refused;emergency_withdrawnested as a sub-invocation refused; pausing on behalf of another address refused; source-account credentials refusedWhat is in here
frontend/lib/incident-response.tsfrontend/lib/server/incident-actions.tsfrontend/lib/server/pause-onchain.tsfrontend/lib/pause-authorization.tsfrontend/lib/wallet-proof.ts,frontend/lib/server/wallet-proof.tsfrontend/app/api/admin/incidents/frontend/app/api/admin/pause-authorizations/app/api/cron/security-scan,app/api/admin/security/scansupabase/migrations/20260827120000_incident_response.sql,20260827130000_pause_authorizations.sqldocs/INCIDENT_RESPONSE.mdAcceptance criteria
decideAutoPauseandrunIncidentResponse, platform pause plus the contract call viasubmitOnChainPause;INCIDENT_AUTO_PAUSE_ENABLEDarms itincidentsrow written before the pause, plus apool_activityrow so it shows in/api/admin/audit-logdecideAutoPause;GET /api/admin/incidentsandPOST /api/admin/incidents/[id]lib/incident-response.test.tsandlib/server/pause-onchain.test.ts, both registered intest:unitDesign notes
not carried out", and promoted only once the pool is actually paused. A crash
midway leaves a record saying no action was taken, which is true and
recoverable. The other ordering leaves a paused pool with no explanation.
network even when the response never reaches us, and a consumed nonce can never
succeed again, so burning it on an uncertain outcome is the honest accounting.
The claim is a conditional update, so two scans racing cannot both spend it.
not consume a pool's allowance, or arming the breaker later would find it
already spent.
write a row for every pool with a single alert. Everything that met the
thresholds is recorded, including what dry-run and cooldown held back.
does not lose the scan, since the alerts are already persisted.
race between the decision and the write.
Rollout
Dry-run is the default, and both scan endpoints report
incidentResponsewith awouldFirecount regardless of dry-run state, so the flag produces real data onthreshold accuracy before anyone sets
INCIDENT_AUTO_PAUSE_ENABLED=true.Defaults and tuning are documented in
.env.exampleanddocs/INCIDENT_RESPONSE.md.Checklist
On authorization of the admin endpoints
Most admin endpoints here compare a
callerAddressfrom the request against thepool's
creator_address, mirroring/api/admin/audit-logand/api/disputes/[id]/resolve. That is a claim rather than a proof, and this PRkeeps the convention where the stakes match it.
It does not keep it for revoking a pause authorization. Revoking disarms the
automatic on-chain pause, and both the pool id and the creator address are
public, so a spoofable check there would let an attacker switch off a pool's
defence before draining it. Revocation asks the wallet to sign a short,
timestamped message naming the exact authorization, verified under SEP-53 against
the pool's admin as recorded. No challenge table was needed: the message carries
its own timestamp and target, and the action is idempotent.
Registering needs no such proof and is left alone deliberately: an entry that was
not signed by the pool's real admin is refused by the inspector regardless of who
posted it, and the contract would reject it anyway.
The wider convention still applies to
resumeand to the read endpoints.Changing it would touch every admin endpoint in the codebase, so it did not
belong in this PR, but I am glad to open an issue or a follow-up if you want it
hardened across the board.