fix(soroban): prevent low-liquidity withdrawal stalls#425
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR changes the vault kernel's idle-payout planning from returning Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Entrypoint as Entrypoints
participant CuratorVault as CuratorVault
participant Kernel as Kernel
participant State as VaultState
Caller->>Entrypoint: execute(AbortWithdrawing { caller, op_id })
Entrypoint->>Entrypoint: require_signed()
Entrypoint->>CuratorVault: abort_withdrawing(caller, op_id, now_ns)
CuratorVault->>CuratorVault: authorize(caller)
CuratorVault->>Kernel: KernelAction::abort_withdrawing(op_id)
Kernel->>State: validate op_id matches active Withdrawing
Kernel->>State: refund escrowed shares
Kernel->>State: dequeue withdrawal head
Kernel->>State: transition op → Idle
Kernel-->>CuratorVault: EffectSummary { WithdrawalStopped, ... }
CuratorVault-->>Entrypoint: Result<EffectSummary, RuntimeError>
Entrypoint-->>Caller: Unit (success)
sequenceDiagram
participant Executor
participant Entrypoint as Entrypoints
participant CuratorVault as CuratorVault
participant Kernel as Kernel
participant State as VaultState
rect rgba(255, 100, 100, 0.5)
Executor->>Entrypoint: execute(ExecuteWithdraw { ... })
Entrypoint->>CuratorVault: execute_withdraw()
CuratorVault->>Kernel: plan_idle_payout(state, min_withdrawal_assets)
Kernel->>State: classify queue head with available_assets
alt idle_liquidity < min_withdrawal_assets
Kernel-->>CuratorVault: Err(WithdrawalLiquidityBelowMinimum)
CuratorVault-->>Entrypoint: RuntimeError::KernelError
Entrypoint-->>Executor: Error (Withdraw blocked)
else idle_liquidity >= min_withdrawal_assets
Kernel-->>CuratorVault: IdlePayoutPlan
CuratorVault->>Kernel: settle & execute effects
Kernel-->>CuratorVault: EffectSummary
CuratorVault-->>Entrypoint: ExecuteWithdrawStatus { ... }
Entrypoint-->>Executor: ExecuteWithdrawStatus
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
carrion256
changed the title
|
@coderabbitai review |
|
@coderabbitai for the benefit of the human reviewers - please output a description of this PR and any critique on the usability of the design when you are reviewing. |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes.
PR
|
| Area | Change |
|---|---|
| Withdrawal gate | execute_withdraw now rejects queue execution when the projected payout for the head item is below MIN_WITHDRAWAL_ASSETS, returning an error instead of creating a new Withdrawing state |
| Partial payout | Partial payouts that meet or exceed MIN_WITHDRAWAL_ASSETS are still permitted, preserving progress when only part of a withdrawal can be serviced |
| Recovery command | VaultCommand::AbortWithdrawing { caller, op_id } is wired through the compact execute(payload) entrypoint so a keeper/admin can cancel a stale Withdrawing op and return the escrow to the queue |
| Idle payout planner | Signature tightened: now returns a concrete PayoutPlan or a typed error; min_payout_threshold is passed explicitly rather than being baked into the function body |
| Kernel reuse | No new kernel logic; the existing AbortWithdrawing transition in templar-vault-kernel is reused unchanged |
| Tests | Deterministic tests for the prevention invariant (test_execute_withdraw_respects_min_withdrawal_assets) and recovery (test_abort_withdrawing), plus a property-based test (prop_low_liquidity) |
| Docs | STRIDE threat model and Soroban docs updated to document the retained recovery command |
The deployed WASM binary remains under the 128 KiB size gate (96,956 bytes).
Design Usability Critique
Below are observations and open questions human reviewers may want to probe.
1. MIN_WITHDRAWAL_ASSETS is a hard constant — what is its value and who owns it?
The PR passes the threshold explicitly to the planner (good for testability), but reviewers should confirm:
- What is the actual constant value and in what denomination/precision?
- Is it governance-configurable or hard-coded? If hard-coded, any future market where the underlying asset's decimals differ could make the threshold wrong by orders of magnitude.
- Is it the same value used across the idle path, the queue gate, and the
AbortWithdrawingauthorization check, or are there silent divergences?
2. Who calls AbortWithdrawing and is its authorization model correct?
The command struct carries caller explicitly. Reviewers should verify:
- Does the entrypoint require
caller.require_auth()before executing the abort? - Can an arbitrary keeper abort any op, or only their own? The PR description says "stale-state recovery backstop" but doesn't specify the trust model.
- Is there a time-lock or minimum age on the
Withdrawingstate before abort is permitted, or can a malicious keeper abort a legitimately in-flight op immediately after it entersWithdrawing?
3. Stuck withdrawers have no on-chain notification
When the queue gate refuses to advance because idle liquidity is too low, the withdrawer stays in Queued silently. There is no event emitted that a frontend or indexer can surface. Human reviewers should check whether a LowLiquidity or WithdrawalDeferred event is emitted so operators and users can detect the condition off-chain.
4. The two-mechanism design (prevent + abort) requires keepers to actively monitor
The prevention path stops new stuck states from being created, but existing or edge-case stuck Withdrawing ops require a keeper to observe and manually call AbortWithdrawing. This is operationally reasonable, but reviewers should confirm:
- Is
AbortWithdrawingidempotent? (i.e., safe to call on an op that is already aborted or already completed) - Is there any keeper incentive or obligation to call it, or is it purely a manual governance action?
- What happens to the withdrawer's escrow between the stall and the abort — is it locked and unavailable for any other op?
5. Interaction with the idle-only atomic path
Per the existing learning on this codebase, withdraw() and redeem() include an idle-only atomic path that bypasses the queued withdrawal lifecycle entirely. Reviewers should confirm whether MIN_WITHDRAWAL_ASSETS is also enforced on that fast path, or whether a small withdrawal can still slip through it below the threshold (creating an asymmetry between the two paths).
6. Error type ergonomics
The idle payout planner now returns a typed error. Reviewers should check that the error type exposed through execute(payload) is actually surfaced as a distinguishable Soroban contract error (not swallowed into a generic InternalError), so off-chain tooling can identify "deferred due to low liquidity" vs. "failed due to a bug."
7. Property test coverage scope
The prop_low_liquidity property test is a positive addition. Reviewers should check whether the property also covers the boundary condition at exactly MIN_WITHDRAWAL_ASSETS (i.e., that amount == threshold is accepted, not rejected), and whether the fuzzer is seeded with edge cases around u128::MAX share amounts.
Note: The usability analysis above is based on the PR description, objectives, and the stored learnings about this vault's architecture (idle-only atomic path,
AbortWithdrawinglifecycle,OpStatetransitions). Reviewers with access to the actual diff should verify each point against the code directly.
🧠 Learnings used
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/contract/entrypoints.rs : withdraw() and redeem() include an idle-only atomic path that bypasses the queued withdrawal lifecycle. Do not change queue semantics without checking both paths
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Serialized VaultState remains a practical resource boundary because Soroban persists a single StateBlob. Pending withdrawals are the main long-lived growth vector
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:16.597Z
Learning: Watch for `OpState` transitions, escrow accounting, keeper-routed withdrawals, callback ordering, idle-balance resync, fee accrual, and reconciliation after partial failures in the vault contract
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/** : Read README.md, STRIDE.md, SIZE_BUDGET.md, src/contract/entrypoints.rs, src/effects/mod.rs, and src/storage/mod.rs before making non-trivial changes to the Soroban vault runtime
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/src/** : RemoveMarket, skim recipient changes, and share-token authority changes can all become authority drift or asset-loss bugs if altered casually
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: contract/vault/soroban/AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:42.034Z
Learning: Applies to contract/vault/soroban/** : If behavior changes around governance bridging, withdrawal lifecycle, or share-token auth, update the docs in contract/vault/soroban/ in the same change
Learnt from: peer2f00l
Repo: Templar-Protocol/contracts PR: 404
File: contract/vault/kernel/src/actions/mod.rs:1869-1871
Timestamp: 2026-04-14T07:35:09.869Z
Learning: In `contract/vault/kernel/src/actions/mod.rs` (Templar-Protocol/contracts, Rust), the vault kernel crate is compiled for Soroban (wasm32 target). Extracting helper functions from large dispatcher functions like `dispatch::apply_action` risks increasing wasm binary size due to additional call frames and reduced compiler inlining. Therefore, `#[allow(clippy::too_many_lines)]` suppressions on large dispatch/entrypoint functions in this file are intentional and should not be flagged as needing refactoring.
Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: contract/vault/soroban/src/contract/entrypoints.rs:806-836
Timestamp: 2026-04-28T08:29:30.164Z
Learning: In `contract/vault/soroban/src/contract/entrypoints.rs` (Templar-Protocol/contracts, Soroban vault), the `SorobanVaultContract::initialize()` entrypoint intentionally has no caller-authorization check. Deployment is expected to go through a controlled/factory flow where `initialize` is called atomically with contract deployment, eliminating any front-running window. Do not flag the missing auth check on `initialize` in this contract as a security issue.
Learnt from: carrion256
Repo: Templar-Protocol/contracts PR: 417
File: contract/vault/curator-primitives/src/policy/cooldown/mod.rs:17-23
Timestamp: 2026-04-27T09:33:55.992Z
Learning: In Templar-Protocol/contracts, the Soroban vault (`contract/vault/soroban/`) and NEAR vault (`contract/vault/near/`) are greenfield deployments with no live persisted state. Do not flag internal serialization format changes (e.g., field type changes in structs like `Cooldown`, `RefreshThrottle`, or policy state) as breaking migration risks — there is no existing stored data to break.
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-24T14:44:16.597Z
Learning: Check invariants around refunds, withdrawals, account deletion, authorization, and replay or double-execution risks in smart contract code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract/vault/soroban/README.md`:
- Around line 113-119: Clarify the recovery semantics: explicitly state that
VaultCommand::AbortWithdrawing { caller, op_id } may be invoked only by actors
in the allocator-emergency role set, and that when processed the handler
restores any amount in Withdrawing.collected back to the vault's idle liquidity
before emitting the kernel WithdrawalStopped event and dequeuing the request,
returning the vault to Idle; update the README paragraph describing the
AbortWithdrawing transition to name the allowed caller role
(allocator-emergency) and to explicitly mention the restoration of
Withdrawing.collected to idle prior to dequeuing and returning to Idle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22bf469b-fc98-4083-a69c-099d1353af4f
📒 Files selected for processing (10)
contract/vault/kernel/src/actions/mod.rscontract/vault/kernel/src/actions/tests.rscontract/vault/kernel/src/error.rscontract/vault/soroban/Cargo.tomlcontract/vault/soroban/README.mdcontract/vault/soroban/STRIDE.mdcontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/tests.rs
carrion256
force-pushed
the
audit/withdrawal-recovery-repro
branch
from
be90ea5 to
76a0240
Compare
carrion256
force-pushed
the
audit/withdrawal-recovery-repro
branch
from
76a0240 to
66e68f8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…-4b87-9e22-844ce7913a9c) Trace: 76a0240
carrion256
force-pushed
the
audit/withdrawal-recovery-repro
branch
from
66e68f8 to
1b2c519
Compare
1 participant
Summary
MIN_WITHDRAWAL_ASSETS, so Soroban no longer creates a new low-liquidityWithdrawingstateVaultCommand::AbortWithdrawing { caller, op_id }through the compactexecute(payload)path as a stale-state recovery backstopAbortWithdrawingrecovery transitionWithdrawingrecoveryAudit Mapping
withdrawal-recoverycontracts-audit-clusters/withdrawal-recovery.audit.mdincontracts-audit-trackerspr/refactor/vault-ergonomics/4f330057, notdevVerification
cargo fmtCARGO_TARGET_DIR=/data/projects/contracts/.shared-target cargo test -p templar-vault-kernel low_liquidity -- --nocaptureCARGO_TARGET_DIR=/data/projects/contracts/.shared-target cargo test -p templar-vault-kernel execute_withdraw -- --nocaptureCARGO_TARGET_DIR=/data/projects/contracts/.shared-target cargo test -p templar-vault-kernel finish_allocating -- --nocaptureCARGO_TARGET_DIR=/data/projects/contracts/.shared-target cargo test -p templar-soroban-runtime test_execute_withdraw_respects_min_withdrawal_assets -- --nocaptureCARGO_TARGET_DIR=/data/projects/contracts/.shared-target cargo test -p templar-soroban-runtime test_abort_withdrawing -- --nocaptureCARGO_TARGET_DIR=/data/projects/contracts/.shared-target cargo test -p templar-soroban-runtime prop_low_liquidity -- --nocaptureCARGO_TARGET_DIR=/data/projects/contracts/.shared-target cargo test -p templar-soroban-shared-types -- --nocaptureCARGO_TARGET_DIR=/data/projects/contracts/.shared-target just -f contract/vault/soroban/justfile size-budget-checkSize gate: runtime deploy WASM
96956bytes <=131072bytes.Halborn Finding IDs
Included for Halborn SSC GitHub remediation detection:
39906066-2ebb-4b87-9e22-844ce7913a9c— Low-liquidity withdrawal execution can trap vault inWithdrawingwith no recovery pathThis change is