From 942d0fc1c0d777c08ef17461748e63197ef155ec Mon Sep 17 00:00:00 2001 From: Ezedike-egwom Collins Date: Fri, 31 Jul 2026 00:13:39 +0100 Subject: [PATCH] fix: block finalize while paused, closing the challenge-window gap --- contracts/tholos/src/lib.rs | 32 +++++++++----- contracts/tholos/src/test.rs | 22 +++++++--- ...paused_then_succeeds_after_unpause.1.json} | 44 ++++++++++++++++++- ...blocks_assert_dispute_and_finalize.1.json} | 25 ++++++----- docs/src/ARCHITECTURE.md | 18 +++++--- docs/src/CHANGELOG.md | 5 +++ docs/src/CONTRACT.md | 28 ++++++------ docs/src/DEPLOYMENT.md | 13 +++--- docs/src/GLOSSARY.md | 2 +- docs/src/INTEGRATION.md | 14 +++--- 10 files changed, 137 insertions(+), 66 deletions(-) rename contracts/tholos/test_snapshots/test/{test_finalize_with_reward_works_while_paused.1.json => test_finalize_with_reward_blocked_while_paused_then_succeeds_after_unpause.1.json} (95%) rename contracts/tholos/test_snapshots/test/{test_paused_blocks_assert_dispute_and_resolve_but_not_finalize.1.json => test_paused_blocks_assert_dispute_and_finalize.1.json} (99%) diff --git a/contracts/tholos/src/lib.rs b/contracts/tholos/src/lib.rs index 7be6264..d287a1e 100644 --- a/contracts/tholos/src/lib.rs +++ b/contracts/tholos/src/lib.rs @@ -537,10 +537,12 @@ impl Tholos { Ok(()) } - /// Pauses or unpauses new assertions, disputes, and resolver votes. - /// Assertions already `Pending` can still be `finalize`d while paused, - /// so existing uncontested claims aren't stuck waiting on an unpause. - /// Only callable by the admin set at initialization. + /// Pauses or unpauses new assertions, disputes, resolver votes, and + /// finalization. A pending assertion may have had no real opportunity to + /// be disputed during its challenge window if that window overlapped a + /// pause, so `finalize` is blocked too rather than letting it finalize + /// uncontested; it becomes callable again once unpaused. Only callable by + /// the admin set at initialization. pub fn set_paused(env: Env, paused: bool) -> Result<(), Error> { let admin: Address = env .storage() @@ -653,15 +655,21 @@ impl Tholos { } /// Finalizes a pending assertion once its challenge window has elapsed - /// with no dispute. `caller` must authorize the call unconditionally — - /// regardless of whether `finalize_reward_bps` is zero — so the address - /// recorded in `Assertion.finalizer` and the `Finalized` event is always - /// a verified caller and cannot be spoofed. When `finalize_reward_bps` is - /// non-zero, `caller` also receives `bond * finalize_reward_bps / 10_000` - /// tokens as an incentive for prompt finalization and the asserter - /// receives the remainder; when it is zero the full bond is returned to - /// the asserter and no reward is paid. Returns the asserted outcome. + /// with no dispute. Fails with `Paused` if paused: a paused assertion may + /// have had no real opportunity to be disputed during its challenge + /// window (since `dispute` is also blocked while paused), so it must not + /// be able to finalize uncontested until unpaused. `caller` must + /// authorize the call unconditionally — regardless of whether + /// `finalize_reward_bps` is zero — so the address recorded in + /// `Assertion.finalizer` and the `Finalized` event is always a verified + /// caller and cannot be spoofed. When `finalize_reward_bps` is non-zero, + /// `caller` also receives `bond * finalize_reward_bps / 10_000` tokens as + /// an incentive for prompt finalization and the asserter receives the + /// remainder; when it is zero the full bond is returned to the asserter + /// and no reward is paid. Returns the asserted outcome. pub fn finalize(env: Env, caller: Address, id: u64) -> Result { + Self::require_not_paused(&env)?; + // Auth is required unconditionally: even when finalize_reward_bps is // zero and no reward is paid, the caller's address is written into // Assertion.finalizer and the Finalized event as the finalizer of diff --git a/contracts/tholos/src/test.rs b/contracts/tholos/src/test.rs index 4136d00..7a0bc71 100644 --- a/contracts/tholos/src/test.rs +++ b/contracts/tholos/src/test.rs @@ -617,12 +617,11 @@ fn test_resolvers_updated_mid_dispute_do_not_affect_it() { } #[test] -fn test_paused_blocks_assert_dispute_and_resolve_but_not_finalize() { +fn test_paused_blocks_assert_dispute_and_finalize() { let f = Fixture::new(); let asserter = f.funded_address(); let disputer = f.funded_address(); - // An assertion posted before the pause can still finalize normally. let pending_id = f.client.assert_outcome(&asserter, &true); f.client.set_paused(&true); @@ -636,12 +635,20 @@ fn test_paused_blocks_assert_dispute_and_resolve_but_not_finalize() { Err(Ok(Error::Paused)) ); + // A pending assertion may have had no real opportunity to be disputed + // during a challenge window that overlapped a pause (dispute is also + // blocked above), so it must not finalize uncontested while still paused. f.advance_past_window(); + assert_eq!( + f.client.try_finalize(&asserter, &pending_id), + Err(Ok(Error::Paused)) + ); + + f.client.set_paused(&false); let outcome = f.client.finalize(&asserter, &pending_id); assert!(outcome); assert_eq!(f.token.balance(&asserter), 1_000); - f.client.set_paused(&false); let id = f.client.assert_outcome(&asserter, &true); f.client.dispute(&disputer, &id); assert_eq!( @@ -1045,9 +1052,9 @@ fn test_finalize_requires_auth_when_reward_bps_is_zero() { } #[test] -fn test_finalize_with_reward_works_while_paused() { - // Finalize is deliberately exempt from the pause; reward payout must also - // work when the contract is paused. +fn test_finalize_with_reward_blocked_while_paused_then_succeeds_after_unpause() { + // Finalize is blocked while paused, same as assert_outcome and dispute; + // reward payout still works normally once unpaused. let (f, _) = fixture_with_reward(200); // 2 % = 2 tokens let asserter = f.funded_address(); let caller = f.generate(); @@ -1056,6 +1063,9 @@ fn test_finalize_with_reward_works_while_paused() { f.client.set_paused(&true); f.advance_past_window(); + assert_eq!(f.client.try_finalize(&caller, &id), Err(Ok(Error::Paused))); + + f.client.set_paused(&false); let outcome = f.client.finalize(&caller, &id); assert!(outcome); assert_eq!(f.token.balance(&caller), 2); diff --git a/contracts/tholos/test_snapshots/test/test_finalize_with_reward_works_while_paused.1.json b/contracts/tholos/test_snapshots/test/test_finalize_with_reward_blocked_while_paused_then_succeeds_after_unpause.1.json similarity index 95% rename from contracts/tholos/test_snapshots/test/test_finalize_with_reward_works_while_paused.1.json rename to contracts/tholos/test_snapshots/test/test_finalize_with_reward_blocked_while_paused_then_succeeds_after_unpause.1.json index 0719671..77393f1 100644 --- a/contracts/tholos/test_snapshots/test/test_finalize_with_reward_works_while_paused.1.json +++ b/contracts/tholos/test_snapshots/test/test_finalize_with_reward_blocked_while_paused_then_succeeds_after_unpause.1.json @@ -154,6 +154,26 @@ } ] ], + [], + [ + [ + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", + { + "function": { + "contract_fn": { + "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMDR4", + "function_name": "set_paused", + "args": [ + { + "bool": false + } + ] + } + }, + "sub_invocations": [] + } + ] + ], [ [ "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAATYON", @@ -460,7 +480,7 @@ ] }, "val": { - "bool": true + "bool": false } }, { @@ -526,6 +546,26 @@ }, "live_until": 6311999 }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", + "key": { + "ledger_key_nonce": { + "nonce": "4270020994084947596" + } + }, + "durability": "temporary", + "val": "void" + } + }, + "ext": "v0" + }, + "live_until": 6311999 + }, { "entry": { "last_modified_ledger_seq": 0, @@ -575,7 +615,7 @@ "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAATYON", "key": { "ledger_key_nonce": { - "nonce": "4270020994084947596" + "nonce": "8370022561469687789" } }, "durability": "temporary", diff --git a/contracts/tholos/test_snapshots/test/test_paused_blocks_assert_dispute_and_resolve_but_not_finalize.1.json b/contracts/tholos/test_snapshots/test/test_paused_blocks_assert_dispute_and_finalize.1.json similarity index 99% rename from contracts/tholos/test_snapshots/test/test_paused_blocks_assert_dispute_and_resolve_but_not_finalize.1.json rename to contracts/tholos/test_snapshots/test/test_paused_blocks_assert_dispute_and_finalize.1.json index d69280e..02ed9bb 100644 --- a/contracts/tholos/test_snapshots/test/test_paused_blocks_assert_dispute_and_resolve_but_not_finalize.1.json +++ b/contracts/tholos/test_snapshots/test/test_paused_blocks_assert_dispute_and_finalize.1.json @@ -178,20 +178,18 @@ ], [], [], + [], [ [ - "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAARQG5", + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", { "function": { "contract_fn": { "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMDR4", - "function_name": "finalize", + "function_name": "set_paused", "args": [ { - "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAARQG5" - }, - { - "u64": "0" + "bool": false } ] } @@ -200,18 +198,20 @@ } ] ], - [], [ [ - "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAARQG5", { "function": { "contract_fn": { "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMDR4", - "function_name": "set_paused", + "function_name": "finalize", "args": [ { - "bool": false + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAARQG5" + }, + { + "u64": "0" } ] } @@ -220,6 +220,7 @@ } ] ], + [], [ [ "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAARQG5", @@ -886,7 +887,7 @@ "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM", "key": { "ledger_key_nonce": { - "nonce": "6277191135259896685" + "nonce": "8370022561469687789" } }, "durability": "temporary", @@ -946,7 +947,7 @@ "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAARQG5", "key": { "ledger_key_nonce": { - "nonce": "8370022561469687789" + "nonce": "6277191135259896685" } }, "durability": "temporary", diff --git a/docs/src/ARCHITECTURE.md b/docs/src/ARCHITECTURE.md index f24e0fe..3e47345 100644 --- a/docs/src/ARCHITECTURE.md +++ b/docs/src/ARCHITECTURE.md @@ -81,13 +81,17 @@ one through. ## Pause is scoped, not absolute -`set_paused` blocks `assert_outcome`, `dispute`, and `resolve`, but deliberately -*not* `finalize` or `update_resolvers`. The intended scope is incident containment: -stop new bonds, disputes, and votes while still allowing an uncontested pending -assertion to return its bond through `finalize`. This is not funds-neutral. Open -disputes cannot progress while paused, and a pending assertion also cannot be -challenged even though its deadline continues and `finalize` remains available. -Pause must therefore be short-lived and is not a safe retirement switch. +`set_paused` blocks `assert_outcome`, `dispute`, `resolve`, and `finalize`, but +deliberately *not* `update_resolvers`. `finalize` is blocked alongside `dispute` +rather than exempted: a pending assertion may have had no real opportunity to be +disputed during a challenge window that overlapped a pause, so it must not be able +to finalize uncontested until unpaused. This is not funds-neutral. Open disputes +cannot progress while paused, and a pending assertion whose window elapses while +paused simply waits, it becomes finalizable again once unpaused, rather than +finalizing uncontested during the pause. Pause must therefore be short-lived and is +not a safe retirement switch. A future version may extend a pending assertion's +challenge window deadline by however long a pause overlapped it, so a paused +incident doesn't cost legitimate disputers their real window; v1 does not do this. If the pause was triggered because the live resolver committee is compromised, the admin can use `update_resolvers` while paused to protect disputes opened after diff --git a/docs/src/CHANGELOG.md b/docs/src/CHANGELOG.md index 7f0365a..0db92d0 100644 --- a/docs/src/CHANGELOG.md +++ b/docs/src/CHANGELOG.md @@ -75,6 +75,11 @@ All notable changes to this project are documented here. Format follows ### Fixed +- `finalize` is now blocked while paused, alongside `assert_outcome`, `dispute`, + and `resolve`. Previously a pending assertion could finalize uncontested even if + its entire challenge window overlapped a pause, during which `dispute` was + blocked, so it had no real opportunity to be challenged. Closes #36. + - `initialize` and `update_resolvers` now reject a resolver committee containing duplicate addresses. A committee like `[A, A, B]` previously passed the odd-length check while being an effective electorate of two, diff --git a/docs/src/CONTRACT.md b/docs/src/CONTRACT.md index 4f697ba..b04ab45 100644 --- a/docs/src/CONTRACT.md +++ b/docs/src/CONTRACT.md @@ -54,7 +54,7 @@ State of an assertion: `Pending`, `Disputed`, or `Resolved`. | `ChallengeWindowOpen` | Tried to finalize before the challenge window elapsed | | `NotAResolver` | Caller isn't in the committee snapshotted for this dispute | | `AlreadyVoted` | Resolver already voted on this assertion | -| `Paused` | Called `assert_outcome`, `dispute`, or `resolve` while paused | +| `Paused` | Called `assert_outcome`, `dispute`, `resolve`, or `finalize` while paused | | `InvalidBondAmount` | `bond_amount` is zero, negative, or greater than `MAX_BOND_AMOUNT` | | `InvalidChallengeWindow` | `challenge_window_secs` is zero or greater than 7 days | | `TooManyResolvers` | Resolver list has more than `MAX_RESOLVERS` (21) entries | @@ -135,16 +135,17 @@ guard), so a lost proposer key can't permanently block rotation. Emits ### `set_paused(paused)` -Pauses or unpauses `assert_outcome`, `dispute`, and `resolve`. Requires the stored -admin's signature. `finalize` is deliberately exempt: assertions already `Pending` -before a pause can still be finalized while paused, so an uncontested claim isn't -stuck waiting on an unpause. `update_resolvers` is also exempt, so a compromised -live committee can be replaced for future disputes without unpausing first; an -already disputed assertion keeps its snapshot. Emits `PauseUpdated`. +Pauses or unpauses `assert_outcome`, `dispute`, `resolve`, and `finalize`. Requires +the stored admin's signature. `finalize` is blocked alongside `dispute`, not +exempted: a pending assertion may have had no real opportunity to be disputed +during a challenge window that overlapped a pause, so it must not finalize +uncontested until unpaused, it becomes callable again once the contract is +unpaused. `update_resolvers` is exempt, so a compromised live committee can be +replaced for future disputes without unpausing first; an already disputed +assertion keeps its snapshot. Emits `PauseUpdated`. -Because `dispute` is blocked while `finalize` is not, leaving the contract paused -through a pending assertion's challenge deadline can make that assertion -uncontestable. Pause is an incident-control tool, not an atomic retirement gate. +Pause is an incident-control tool, not an atomic retirement gate: it can delay a +legitimate uncontested claim from finalizing for as long as the pause lasts. ### `assert_outcome(asserter, outcome) -> u64` @@ -162,9 +163,10 @@ assertion isn't pending (including if it's already disputed), or ### `finalize(caller, id) -> bool` Callable once a `Pending` assertion's challenge window has elapsed with no dispute. -`caller` must authorize the call unconditionally — regardless of whether -`finalize_reward_bps` is zero — so the address recorded in `Assertion.finalizer` -and the `Finalized` event is always a verified caller and cannot be spoofed. This +Fails with `Paused` if paused. `caller` must authorize the call unconditionally — +regardless of whether `finalize_reward_bps` is zero — so the address recorded in +`Assertion.finalizer` and the `Finalized` event is always a verified caller and +cannot be spoofed. This applies even when no reward is being paid: without enforced auth, any address could be passed as `caller`, permanently writing an unverifiable identity into the on-chain record. diff --git a/docs/src/DEPLOYMENT.md b/docs/src/DEPLOYMENT.md index 46c8e63..f250875 100644 --- a/docs/src/DEPLOYMENT.md +++ b/docs/src/DEPLOYMENT.md @@ -56,12 +56,13 @@ behavior looks off), pause first and investigate second: stellar contract invoke --id "$CONTRACT" --source admin --network testnet -- set_paused --paused true ``` -This stops `assert_outcome`, `dispute`, and `resolve` immediately, but not -`finalize`. That asymmetry matters: a `Pending` assertion cannot be challenged -while paused and may become finalizable if its window expires. Inventory open -assertions before pausing, minimize pause duration, and unpause with -`--paused false` as soon as incident handling permits. Do not use pause as a safe -migration or retirement switch. +This stops `assert_outcome`, `dispute`, `resolve`, and `finalize` immediately. A +`Pending` assertion whose challenge window elapses while paused simply waits, it +becomes finalizable once you unpause, rather than finalizing uncontested during +the incident. Minimize pause duration, since this delays legitimate uncontested +claims for as long as the pause lasts, and unpause with `--paused false` as soon +as incident handling permits. Do not use pause as a safe migration or retirement +switch. ### Rotating the resolver committee diff --git a/docs/src/GLOSSARY.md b/docs/src/GLOSSARY.md index 705772d..ec38f29 100644 --- a/docs/src/GLOSSARY.md +++ b/docs/src/GLOSSARY.md @@ -67,7 +67,7 @@ the winning side receives both bonds and the assertion moves to `Resolved`. **Pause** An admin-controlled switch (`set_paused`) that blocks new assertions, disputes, -and resolver votes, without affecting `finalize` or `update_resolvers`. See +resolver votes, and finalization, without affecting `update_resolvers`. See [ARCHITECTURE.md](ARCHITECTURE.md#pause-is-scoped-not-absolute). **SEP-41** diff --git a/docs/src/INTEGRATION.md b/docs/src/INTEGRATION.md index 6dc4d4a..bbf9f67 100644 --- a/docs/src/INTEGRATION.md +++ b/docs/src/INTEGRATION.md @@ -153,10 +153,10 @@ resolved; it is `None` while the assertion is still `Pending` or `Disputed`. finalization. When it is 0 (the default), no reward is paid and the full bond is returned to the asserter, but auth is still required to keep the recorded finalizer trustworthy. -- The admin can pause `assert_outcome`, `dispute`, and `resolve` at any time via - `set_paused`. Your integration should treat a `Paused` error as a distinct, - expected failure mode (surface it to the user as "resolution temporarily - unavailable") rather than an unexpected error. `finalize` and - `update_resolvers` stay callable while paused. A pending assertion can therefore - become finalizable while `dispute` is blocked; do not assume a pause also freezes - its challenge deadline. +- The admin can pause `assert_outcome`, `dispute`, `resolve`, and `finalize` at + any time via `set_paused`. Your integration should treat a `Paused` error as a + distinct, expected failure mode (surface it to the user as "resolution + temporarily unavailable") rather than an unexpected error. `update_resolvers` + stays callable while paused. A pending assertion whose challenge window elapses + while paused does not become finalizable until unpaused; do not assume a pause + only affects new assertions and disputes.