Skip to content

fix(globe-wallet): enforce minimum timelock delays for upgrades and recovery - #106

Merged
ndii-dev merged 2 commits into
Orbit-Wal:mainfrom
christabel888:fix/timelock-minimum-delay-issue-84
Aug 29, 2026
Merged

fix(globe-wallet): enforce minimum timelock delays for upgrades and recovery#106
ndii-dev merged 2 commits into
Orbit-Wal:mainfrom
christabel888:fix/timelock-minimum-delay-issue-84

Conversation

@christabel888

Copy link
Copy Markdown

Summary

Closes #84.

propose_upgrade and set_recovery_config both accepted delay_in_ledgers = 0, silently turning either "timelock" into an atomic, zero-reaction-time operation — defeating the one documented purpose each exists for (giving observers time to react to a proposed upgrade, and giving the admin a window to notice and cancel a malicious/mistaken recovery).

This adds a real floor to each:

  • GlobeWallet::MIN_UPGRADE_DELAY_LEDGERS = LEDGERS_PER_DAY * 3propose_upgrade is the only defense against a compromised admin key entrenching itself via a malicious code swap (no guardian quorum gates it), so it gets a longer floor.
  • GlobeWallet::MIN_RECOVERY_DELAY_LEDGERS = LEDGERS_PER_DAY — recovery already has an independent first layer (guardian quorum), so the post-quorum timelock only needs to be a sufficient second layer.

Both constants reuse the contract's existing LEDGERS_PER_DAY (already defined and documented for TTL bookkeeping), rather than introducing a new unrelated magic number, and both new error variants (UpgradeDelayTooShort, RecoveryDelayTooShort) slot into the existing 1001+ namespaced error range (see the WalletError doc comment).

Changes

  • New constants MIN_UPGRADE_DELAY_LEDGERS, MIN_RECOVERY_DELAY_LEDGERS with rationale doc comments matching the existing MAX_GUARDIANS style.
  • propose_upgrade rejects delay_in_ledgers < MIN_UPGRADE_DELAY_LEDGERS with WalletError::UpgradeDelayTooShort, checked after the existing UpgradeAlreadyPending gate (preserves existing error precedence).
  • set_recovery_config rejects delay_in_ledgers < MIN_RECOVERY_DELAY_LEDGERS with WalletError::RecoveryDelayTooShort, checked after the existing threshold/guardian-count gates (same reasoning).
  • New tests: zero-delay rejection (issue's own repro, for both paths), below-minimum rejection, and at-minimum acceptance with full end-to-end execution (no regression to the happy path).
  • Updated every existing test that previously exercised these paths with a sub-minimum placeholder delay (0, 5, 10) to use the new minimums, including re-deriving the ledger-advance amounts needed to actually reach ready_at under the new, much larger delays.
  • setup()'s test helper now proactively extends the contract's instance storage TTL once at setup. This is a test-only accommodation: the new multi-day minimums mean tests now advance the ledger sequence far enough that the test host's default instance-entry TTL would otherwise archive Admin/RecoveryConfig/etc. mid-test. The contract itself has no equivalent self-extension for instance storage (unlike UserAssets/SpendLimit, which already self-extend on every write) — worth its own follow-up issue, but out of scope here.
  • Also fixed a one-line pre-existing compile error in test_propose_upgrade_accepts_any_hash_without_validation (Ok(()) where the try_* client method's actual return type needs Ok(Ok(()))) — this test could never have compiled before, in any environment where the crate actually built, since it's untouched by any of the above and was already there.

Test plan

  • MIN_UPGRADE_DELAY_LEDGERS / MIN_RECOVERY_DELAY_LEDGERS added with documented rationale
  • propose_upgrade rejects delay below minimum with a typed error (UpgradeDelayTooShort)
  • set_recovery_config rejects delay below minimum with a typed error (RecoveryDelayTooShort)
  • Tests proving both zero-delay and below-minimum-delay calls are rejected (both paths)
  • Tests proving a delay at the minimum still works exactly as before, no regression, including a full end-to-end execute_recovery after waiting exactly the new minimum
  • cargo test --workspace output below
running 75 tests
...
test tests::test_propose_upgrade_rejects_zero_delay ... ok
test tests::test_propose_upgrade_rejects_delay_below_minimum ... ok
test tests::test_propose_upgrade_accepts_delay_at_minimum - should panic ... ok
test tests::test_set_recovery_config_rejects_zero_delay ... ok
test tests::test_set_recovery_config_rejects_delay_below_minimum ... ok
test tests::test_set_recovery_config_accepts_delay_at_minimum_and_recovery_executes ... ok
test tests::test_recovery_happy_path_2_of_3 ... ok
test tests::test_execute_recovery_rejects_new_admin_same_as_current_admin ... ok
test tests::test_revoking_approval_below_threshold_resets_timelock ... ok
... (all other existing upgrade/recovery tests, updated to the new minimums, also pass)

test result: FAILED. 73 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out

On the 2 failures: test_max_assets_limit and test_migrate_user_assets_within_limit_does_nothing fail with HostError: Error(Contract, #1029) (InvalidAssetInfo) — both construct a non-native AssetInfo with issuer: None, which the asset-code validation added by a previously-merged PR now rejects. This is unrelated to this issue (asset registry, not timelocks), reproduces identically on a clean main checkout with no changes of mine applied, and isn't touched by anything in this diff. Recommend filing a separate issue for it rather than folding an unrelated fix into this PR.

Environment note: in the sandbox this was developed in, cargo test --workspace doesn't build at all against a fresh cargo resolution — soroban-env-host 21.2.1 resolves against ed25519-dalek 3.0.0, which breaks its own trait bounds (ChaCha20Rng no longer satisfies the CryptoRng bound SigningKey::generate needs). This reproduces identically on a pristine main with no changes of mine, and is why test_propose_upgrade_accepts_any_hash_without_validation's pre-existing Ok(())/Ok(Ok(())) bug (fixed here) had apparently never actually been compiled before. Worked around locally for verification via cargo update -p ed25519-dalek@3.0.0 --precise 2.2.0 (not part of this diff — Cargo.lock is gitignored in this repo). Flagging in case CI hits the same resolution once ed25519-dalek 3.0.0's index entry is picked up wherever CI runs.

@christabel888

Copy link
Copy Markdown
Author

Added a second commit: committed the regenerated `test_snapshots/*.json` files affected by this change.

Every test that goes through the shared `setup()` helper now bakes in the larger instance-storage TTL that helper sets (needed so tests advancing the ledger by the new multi-day minimums don't hit mid-test archival — see the code comment on `setup()`), so their snapshots differ mechanically from before. This repo's history already checks these in alongside the code changes that produce them, so I've done the same here: the previously-tracked snapshots this change affects, plus snapshots for the tests this PR actually added or modified.

Left alone: snapshots for tests unrelated to this issue that happened to never be committed before (verified — only 48 of 78 snapshot files in `contracts/globe-wallet/test_snapshots/tests/` were tracked prior to this branch). Backfilling those is unrelated repo hygiene, not something this PR should bundle in.

…ecovery

propose_upgrade and set_recovery_config accepted delay_in_ledgers = 0,
letting either timelock be silently configured to provide zero reaction
time. Add MIN_UPGRADE_DELAY_LEDGERS (3 days) and MIN_RECOVERY_DELAY_LEDGERS
(1 day), reject delays below them with typed errors, and cover both the
new floors and the unaffected happy path with tests.

Closes Orbit-Wal#84
test_snapshots/*.json are checked in alongside the code change that
produces them per this repo's existing convention. Includes exactly the
snapshots for tests this PR added or modified (new minimum-delay tests,
plus every pre-existing test whose delay/advance values changed to stay
above the new minimums, including two tests from the separately-merged
recovery_completed-event PR that also needed updating here).

Deliberately excludes snapshots for unrelated tests (token-wrapper, and
globe-wallet's send/token-wrapper-allowlist tests) that happened to also
be stale against main's current code — that staleness predates this
branch and isn't caused by anything in this diff.
@christabel888

Copy link
Copy Markdown
Author

Rebased onto main to resolve conflicts with the two PRs merged since this was opened (#107 reentrancy-safe wiring, #105 recovery_completed event):

Also re-scoped the snapshot commit: on rebasing I found several `test_snapshots/*.json` already committed on `main` were stale against `main`'s own current code (old un-namespaced error codes, old TTL values) — from #107, unrelated to anything here. Excluded those; the snapshot commit now contains exactly the 33 files this PR's actual code changes touch, no more.

`cargo test --workspace`: 86 passed (globe-wallet) + 1 (reentrancy integration test) + 11 (token-wrapper), 0 failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Neither timelock (upgrade proposals, guardian recovery) enforces a minimum delay — both can be set to 0, defeating their entire purpose

3 participants