feat(soroban): harden circuit breaker, liquidation, token allowances, and staking metadata - #1068
Merged
Ceejaytech25 merged 4 commits intoAug 28, 2026
Conversation
This was referenced Aug 27, 2026
The circuit breaker could be tripped by governance, authorized bots, and autonomous oracle/volume triggers, but the only way back out was the timelocked initiate_unpause/execute_unpause pair. That timelock is correct for routine resumption, but it means a false-positive trip (a benign volume spike, a transient oracle deviation) keeps the protocol halted for the full timelock window, where the outage is the incident. Add an admin-only unpause(admin) escape hatch that resumes immediately: - unpause() requires admin auth, rejects the not-paused case, clears the tier, and emits (cb, resumed) with (admin, timestamp). - It also zeroes UnpauseUnlocksAt, so a previously scheduled unpause cannot fire afterwards and silently re-tier the protocol from a stale target. - Bots are deliberately excluded: they may trip the breaker but must not be able to resume it. Track an explicit IsPaused boolean in instance storage, set in initialize() and updated in apply_trip(), execute_unpause(), and unpause() so it can never drift from PauseTier, and expose it via is_paused(). Note that execute_unpause() to a narrower tier keeps IsPaused true, since the protocol is still halted. Tests cover both directions of the state machine: is_paused after initialize, trip/unpause round-trips across all three tiers, immediate resume without advancing the ledger, pending-unpause cancellation, the stale execute_unpause path panicking, non-admin and bot rejection, repeated pause/unpause cycles with trip-count accounting, tier-narrowing, and the exact resumed event payload. 41 tests pass (31 pre-existing, 10 new).
The liquidation crate did not compile: create_vault bound `id` immutably and then reassigned it, and in doing so wrote the same vault under two different ids while advancing the counter twice, leaking an id per vault. Beyond that, both liquidation paths computed an unguarded health factor before reaching the safe one. liquidate() and partial_liquidate() each fetched a u128 collateral price and divided by vault.debt_amount directly, so any vault with zero debt panicked on division by zero. Each function then recomputed the health factor via get_health_factor and asserted again, leaving the first computation as dead code whose only reachable effect was that panic. partial_liquidate additionally duplicated its ratio, incentive, and vault-mutation logic, and used unchecked arithmetic throughout. Route both paths exclusively through get_health_factor, which already returns i128::MAX for zero debt and is oracle-priced, so a debt-free vault now reports infinite health and fails the healthy-vault assertion instead of panicking. Delete the duplicated pre-checks and dead bindings, and use checked arithmetic for the seized collateral, incentive, and vault updates. Name the magic thresholds: HF_ONE_BPS (10_000 = 1.0), LIQUIDATION_THRESHOLD_BPS (12_000), PARTIAL_LIQUIDATION_THRESHOLD_BPS (15_000), and add is_liquidatable(), which encodes the rule that a vault at or above a health factor of 1.0 is never liquidatable. Note the health factor is i128, not u128, so the infinite-health sentinel is i128::MAX. Tests cover zero debt with and without collateral, zero debt after a full liquidation, both liquidation entry points rejecting a debt-free vault, the exactly-1.0 and just-below-1.0 boundary, sequential vault ids, and a partial liquidation reducing debt and collateral. 18 tests pass (9 pre-existing, 9 new); the crate previously did not build.
Allowances were stored as a bare i128 with no lifetime, so an approval
stayed spendable forever unless the owner remembered to overwrite it. A
spender approved once retained that authority indefinitely, and abandoned
approvals kept paying storage rental with no way to reclaim them.
Store allowances as AllowanceValue { amount, expiration_ledger } and gate
every read on env.ledger().sequence() <= expiration_ledger. The bound is
inclusive: an allowance is still spendable on its expiration ledger.
approve() now takes expiration_ledger and rejects one already in the past,
so a non-zero approval cannot be created dead. Approving zero is treated as
a revocation: the record is removed rather than stored as a zero, and any
expiration is accepted, since refusing to revoke over a stale argument
would be the more dangerous failure.
permit() carries expiration_ledger inside the signed auth payload, so a
relayer cannot extend the lifetime of an approval beyond what the owner
signed.
Storage is reclaimed on two paths: transfer_from purges a lapsed record as
it reads it, and an allowance spent down to zero is removed instead of
being written back. Neither covers the common case of an expired approval
that is simply never used again, and a spend attempt against an expired
allowance panics, so Soroban reverts the invocation and takes that purge
with it. purge_expired_allowance() therefore exists as its own entry point.
It is permissionless because it can only delete a record that has already
lapsed and is unspendable, and it is idempotent.
Operator approval (set_approval_for_all) is unchanged and remains
independent of per-token allowance expiry.
allowance() keeps returning i128 and now reports 0 once expired, so
existing callers stay correct; allowance_expiration() exposes the deadline.
46 tests pass (31 pre-existing, 15 new), covering the inclusive boundary,
reads and spends after expiry, both purge paths, revocation, re-approval
extending a deadline, partial spends preserving it, and operator bypass.
update_contract_meta already required admin authorization: it calls require_auth() on the caller and compares it against the stored admin, panicking with OnlyAdmin otherwise, and test_update_contract_meta_non_admin covered that. The issue's premise that any caller can rewrite the staking position metadata does not hold against the current code, so the authorization check is left as it is and covered by additional tests rather than reimplemented. What was genuinely missing is any bound on what the admin can store. The description, icon_url, and website strings were written to instance storage at whatever length was supplied, so a single update could commit the contract to unbounded storage rental. Reject any field longer than MAX_METADATA_LEN (256 bytes) with a new MetadataUriTooLong error. The bound is inclusive, so a URI of exactly 256 bytes is accepted. The emitted event carried only the ContractMetadata struct, so a subscriber wanting the new URI had to destructure it. Lead the event data with icon_url, keeping the full struct behind it so existing consumers continue to work. Also add the missing `Events` import to the test module, without which the crate's tests did not compile at all. Tests cover a non-admin update being rejected and leaving prior metadata intact, a URI at exactly the limit being accepted, each of the three fields being rejected one byte over, an oversized update leaving state untouched, and the update emitting an event. 25 tests pass (17 pre-existing, 8 new).
0xDeon
force-pushed
the
feat/soroban-contract-hardening
branch
from
August 27, 2026 14:27
c6b8b43 to
39de66f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #982
Closes #984
Closes #986
Closes #987
Combines PRs #1055, #1056, #1057, #1058 into a single branch. Four independent Soroban contract fixes, one commit each, no overlapping files. Rebased onto current
main.feat(soroban): add emergency pause/resume toggle in Circuit Breakersrc/circuit_breaker/lib.rsfix(soroban): make collateral health factor evaluation zero-debt safesrc/liquidation/src/lib.rsfeat(soroban): expire operator approval allowances by ledger sequencesrc/token/lib.rsfeat(soroban): validate and bound NFT metadata updates in Liquid Stakingcontracts/liquid_staking/src/lib.rsTest evidence
Note on
contracts/liquid_stakingIts 25 tests could not be run locally: the
contracts/workspace has a pre-existing break onmain—soroban-env-hostfails to compile againsted25519-dalek 3.0.0(ChaCha20Rng: CryptoRngunsatisfied). Verified by checking out unmodifiedmainin a clean worktree, so it is independent of this branch.cargo check --tests -p liquid_stakingpasses against a locally pinned dalek 2.2.0; that pin is deliberately not committed.The liquid staking commit's diff is now
+195/-3. The original PR #1058 showed+1291/-1099because it also reformatted the whole file; that reformatting has been dropped so the diff is reviewable.Rationale for each change is in the individual commit messages.