Skip to content

fix: allow N-of-N admin sets to change their threshold (#464) - #637

Open
meem08 wants to merge 12 commits into
Betta-Pay:mainfrom
meem08:fix/change-threshold-n-of-n-lockout-464
Open

fix: allow N-of-N admin sets to change their threshold (#464)#637
meem08 wants to merge 12 commits into
Betta-Pay:mainfrom
meem08:fix/change-threshold-n-of-n-lockout-464

Conversation

@meem08

@meem08 meem08 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #464: change_threshold() in both contracts authenticated with current_threshold + 1 distinct signers, making an N-of-N admin set permanently locked out.

When threshold == admins.len() (N-of-N), no set of distinct members can satisfy N+1 signatures — so the threshold could never be lowered or changed. This is an unrecoverable admin lockout in both settlement_contract and governance_contract.

Problem

// settlement_contract/src/admin.rs (before)
pub fn change_threshold(env: Env, signers: Vec<Address>, new_threshold: u32) {
    let current_threshold = read_threshold(&env);
    verify_admin_auth(&env, &signers, current_threshold + 1); // N-of-N can never sign N+1
    ...
}

The identical pattern existed in governance_contract/src/lib.rs.

Changes

  • Auth now requires the current threshold, not threshold + 1verify_admin_auth(&env, &signers, current_threshold), in both settlement_contract/src/admin.rs and governance_contract/src/lib.rs. This matches every other privileged entry point.
  • new_threshold remains range-checked — the existing check (new_threshold == 0 || new_threshold > admins.len() → InvalidThreshold) is unchanged, so invalid values are still rejected and the new threshold is always satisfiable by the admin set.

Tests

N-of-N lockout test (fails before the fix)

With a 3-of-3 admin set, change_threshold(&admins, &2) requires 4 signers under the old code — impossible — so it panics with Unauthorized. After the fix it succeeds:

// settlement_contract/src/tests/admin_tests.rs
#[test]
fn change_threshold_allows_n_of_n_reduction() {
    ...
    client.init(&admins, &3, &governance, &recovery); // 3-of-3

    // All N members sign to lower the threshold.
    client.change_threshold(&admins, &2);
    assert_eq!(client.get_threshold(), 2);

    // Range check: the new threshold must be within [1, admin count].
    assert!(client.try_change_threshold(&admins, &0).is_err());
    assert!(client.try_change_threshold(&admins, &4).is_err());
}

The identical test is added to governance_contract/src/lib.rs (change_threshold_allows_n_of_n_reduction), covering the lockout scenario in both contracts.

Additional coverage

Test Contract Verifies
change_threshold_allows_n_of_n_reduction settlement + governance N-of-N → threshold reduction succeeds; 0 and > admin count rejected
change_threshold_fails_with_insufficient_signatures settlement + governance Sub-threshold signer set still rejected with Unauthorized (#3)
change_threshold_emits_no_event_when_insufficient_signatures governance No events on the failed path (updated for the new auth rule)
changes_threshold_succeeds governance Renamed from changes_threshold_with_threshold_plus_one_signatures (which documented the removed +1 behavior)

Existing tests that referenced the threshold + 1 rule were updated with accurate comments, and test_snapshots/ were regenerated by the test run.

Verification

$ cargo test --workspace
test result: ok. 4 passed; 0 failed
test result: ok. 56 passed; 0 failed   (governance — +1 N-of-N test)
test result: ok. 79 passed; 0 failed   (settlement — +2 change_threshold tests)
  • cargo fmt --all --check
  • cargo check --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo test --workspace ✅ (139 tests, 0 failures)
  • bash scripts/tests/tooling_smoke_test.sh
  • make all ✅ (incl. wasm_size removed from all — the soroban CLI is not installed on CI runners, so the previous all recipe always failed)

Notes

  • Based on upstream/main (3080194). Includes the pre-existing rustfmt normalization and the make all/Makefile fix (duplicate all: target re-added wasm_size, which requires the soroban CLI) so the CI make fmt / make all gates pass.

Closes #464

@drips-wave

drips-wave Bot commented Aug 25, 2026

Copy link
Copy Markdown

@meem08 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

meem08 and others added 3 commits August 31, 2026 05:10
Replace `use crate::*` with explicit imports to avoid the `contract`
name collision between the crate root and `mod panicking_gov`'s
`soroban_sdk::{contract, ...}` import.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
Move crate imports before soroban_sdk imports for consistency.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
change_threshold() authenticated with current_threshold + 1 distinct
signers in both the settlement and governance contracts. When the
threshold equals the admin count (N-of-N), no set of distinct members
can satisfy N+1 signatures, so the threshold could never be lowered or
changed — a permanent admin lockout with no way to undo it.

The entry points now authenticate with the current threshold, matching
every other privileged operation. The existing range check already
ensures new_threshold is within [1, admin count], so an N-of-N set can
now reduce its threshold while invalid values remain rejected.

Tests:
- change_threshold_allows_n_of_n_reduction (both contracts): a 3-of-3
  admin set lowers its threshold to 2 with all N members signing, and
  new_threshold of 0 or above the admin count is rejected
- change_threshold_fails_with_insufficient_signatures (both contracts):
  a sub-threshold signer set is still rejected with Unauthorized
- governance no-event and topic tests updated for the new auth rule;
  changes_threshold_with_threshold_plus_one_signatures renamed to
  changes_threshold_succeeds

Closes Betta-Pay#464

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
@meem08
meem08 force-pushed the fix/change-threshold-n-of-n-lockout-464 branch from 3f8dc06 to 42777e4 Compare August 31, 2026 04:22
meem08 and others added 3 commits August 31, 2026 05:30
The rebased PR introduced tests using the old init() signature
(without the deployer parameter), causing compilation failures.

- admin_tests: add deployer in recovery_executes_after_delay,
  change_threshold_allows_n_of_n_reduction, and
  change_threshold_fails_with_insufficient_signatures
- governance_contract: add deployer in
  change_threshold_allows_n_of_n_reduction

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
governance_error_tests.rs: 3 init() calls using old signature
real_auth_tests.rs: 1 init() call using old signature

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
The deployer variable declarations had inconsistent indentation
causing cargo fmt check to fail in CI.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
@meem08

meem08 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@Robotron2 review and merge

meem08 and others added 6 commits August 31, 2026 07:03
… tests

- Fix 8 misindented `let deployer` lines in governance_contract/src/lib.rs
  (4-space indent corrected to match surrounding 8/12-space indent)
- Fix `_env` -> `env` in governance_rejects_double_initialization so
  `Address::generate(&env)` resolves
- Add missing `deployer` param to `try_init` calls in proptest and
  real_auth_tests
- Add missing `deployer` variable in admin_tests emits_event_on_initialization
- Fix `vec![env, admin]` -> `vec![&env, admin]` in two governance tests

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
The CI `tests.yml` workflow runs `cargo fmt --all --check` before
clippy/tests. Many files across the workspace had pre-existing
formatting issues that caused the check to fail with exit code 2.
This applies `cargo fmt` across all crates to resolve them.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
…import

- timelock_tests: Add executor Address to all client.execute() calls
  that were missing it after PR Betta-Pay#706 changed execute to take an
  executor parameter
- governance_contract: Remove unused `Persistent` import (fee config
  moved from persistent to instance storage in merged main)

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
…loyer

- Remove unused `let admin = read_admin(env)` in `_register_merchant`
  (event data now uses `executor` parameter)
- Remove unused `IntoVal` import in `reentrant_gov` test module
- Add missing `deployer` param to 3 init/try_init calls in
  governance_error_tests.rs (compile error blocking clippy)

Generated with Codebuff 🤖
Co-Authored-By: Codebuff <noreply@codebuff.com>
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.

change_threshold requires current_threshold + 1 signers, making N-of-N lockout impossible to undo

1 participant