fix(liquidation): v17 pre-submit re-verification must fail closed (#373)

[dcccrypto]

Closes #373.

The bug

The v17 pre-submit re-verification wrapped its entire block — the still-undercollateralized margin recheck, the maintenance-margin re-parse, and the oracle-drift guard — in try { … } catch { /* proceed cautiously */ } with an empty catch. Any throw fell straight through to the send with no checks applied.

The v12.x path directly below is unwrapped and already fails closed. v17 is the live mainnet layout, so the asymmetry ran the wrong way.

The "proceed cautiously — the on-chain program will reject" justification covers the closed/topped-up case. It does not cover the drift guard, which the code itself documents as having no on-chain counterpart:

The on-chain Liquidate instruction carries no price bound, so keeper-side drift detection is the only available mitigation.

So on a throw, a liquidation could be submitted at unbounded drift with no backstop anywhere — keeper-side skipped, on-chain nonexistent. Triggers aren't attacker-controlled (malformed wrapper bytes in parseWrapperConfigV17, a cluster-time RPC error, the M-6 owner-check failure in fetchSlabWithRetry) but they're reachable.

The fix

Narrow the try to the fetch/parse of re-verification data and return null on failure. The drift and margin checks now run outside it, so their failure aborts the submit. Matches the H-8 pattern already applied to the inner reMmBps parse.

Trade-off, stated plainly: during RPC instability the keeper now skips these liquidations instead of submitting them blind. That's the intended posture and the one v12.x already had — the next scan cycle retries. It trades some liquidation coverage under RPC flakiness for never submitting an unverified liquidation.

Verification

The issue asked for a test where parseWrapperConfigV17 throws and keeperSend is never called. Added that, plus the slab-re-fetch throw, plus two contrast tests proving the checks still run on the healthy path (otherwise "fail closed" could be implemented as "always return null" and the first two would still pass).

I verified these against the original code, not a hand-rolled mutation — my first mutation attempt produced a broken hybrid that passed and would have let a useless test through:

ORIGINAL fail-open liquidation.ts + new tests  → 2 failed   ✅ they catch the bug
fixed liquidation.ts + new tests               → 4 passed

Second finding: the existing drift test was vacuous

While building the contrast case I found the pre-existing test — aborts v17 liquidation when fresh wrapper price drifts beyond the configured limit — passes with the drift guard entirely commented out.

Its wrapper config had oracleTargetPublishTime: 0n, so resolveV17WrapperPrice treated the EWMA as stale and returned 0n. The test aborted on the freshPrice === 0n guard and never reached the drift check.

So the mitigation this whole issue exists to protect had no working test coverage. Fixed by giving it a fresh publish time; the 100% drift (1.0 → 2.0 against a 150bps limit) is now what actually aborts. Mutation-verified: disabling the guard fails it.

Checks

• Full keeper suite: 979 passed, 0 failures
• npx tsc --noEmit clean

Touches liquidation safety logic on the live mainnet path — wants a security review before merge, not just a functional one.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved liquidation safety by aborting submissions when pre-submit verification data is missing, invalid, or cannot be refreshed.
  • Prevented liquidations from proceeding when market data, pricing, or risk information cannot be reliably verified.
  • Ensured drift and margin safeguards run only with validated, up-to-date inputs.
• Tests
  • Added coverage for malformed data, failed market refreshes, successful re-verification, and price drift scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3596488b-85cb-47da-a3e4-5598db0b3fe7

📥 Commits

Reviewing files that changed from the base of the PR and between 1d2cb11 and 8e3d778.

📒 Files selected for processing (2)

• src/services/liquidation.ts
• tests/services/liquidation.test.ts

---

📝 Walkthrough

Walkthrough

V17 liquidation pre-submit re-verification now fails closed on data-fetch, parsing, or price-resolution errors. Tests verify aborted submission on failures and continued drift and margin validation after successful re-verification.

Changes

V17 liquidation pre-submit re-verification

Layer / File(s)	Summary
Fail-closed re-verification and guarded submission path src/services/liquidation.ts	The v17 flow now returns null when required re-verification inputs cannot be fetched or resolved, and runs drift, margin, and crank rebuilding only after successful validation.
Failure and success-path tests tests/services/liquidation.test.ts	Tests cover wrapper parsing and slab-fetch failures, blocked keeperSend, successful guard execution, skipped margin rechecks after failure, and fresh oracle timestamps for drift testing.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Issue 352 — Directly addresses fail-closed v17 pre-submit re-verification and prevents validation failures from bypassing liquidation guards.
• Issue 349 — Concerns another v17 pre-submit price re-verification and oracle-drift failure mode in the same service.

Possibly related PRs

• dcccrypto/percolator-keeper#6 — Both modify v17 re-verification pricing and validation before liquidation submission.
• dcccrypto/percolator-keeper#282 — The fail-closed path builds on v17 risk-parameter corruption handling in the same service and tests.
• dcccrypto/percolator-keeper#345 — Changes v17 maintenance-margin parsing behavior that affects this re-verification path.

Suggested reviewers: morenikeoa, rehannek, 6figpsolseeker

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately describes the main v17 liquidation fix.
Linked Issues check	✅ Passed	The code and tests match #373 by failing closed on re-verification errors and preserving the healthy-path checks.
Out of Scope Changes check	✅ Passed	The changes stay focused on the v17 liquidation fix and its tests, with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/373-v17-presubmit-fail-closed

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}