Step fi2

[BigMick03]

Closes #84

PR Request: Harden Parameters-Contract Multisig Security

Summary

This PR should harden the parameters-contract multisig implementation against three related authorization flaws that currently undermine the security guarantees of the signer/threshold mechanism:

1. Admin bypass during multisig configuration

2. Stale approvals surviving signer-set changes

3. Signer-controlled threshold reduction

The goal is to ensure that a compromised signer or administrator cannot unilaterally weaken the multisig and subsequently modify protocol parameters that directly affect creditline economics.

This work should be implemented as a security-focused, additive change while preserving the existing event surface wherever practical.

---

Problem Statement

The current implementation of the parameters-contract multisig can provide the appearance of multi-party authorization without reliably enforcing it.

1. Admin bypass

configure_multisig() currently authenticates only the single admin key before changing the multisig configuration.

This creates a fundamental trust issue: the same administrative authority can potentially replace the signer set or threshold without requiring the multisig approval that is supposed to protect sensitive configuration.

The implementation should therefore make multisig configuration changes observable and significantly harder to perform silently.

At minimum:

• Emit a prominent event whenever multisig configuration changes.

• Evaluate whether configuration should use a two-step propose → confirm flow.

• Ensure the resulting authorization model cannot silently undermine the multisig.

2. Stale approvals

Proposals currently retain approvals as Vec<Address>.

The critical issue is that an approval is effectively treated as valid based on its presence in the vector and the number of approvals:

approvals.len() >= threshold

If the signer set changes after a proposal receives approvals, an address that was previously eligible can remain recorded as an approver even after being removed from the signer set.

That creates an authorization state where a revoked signer can continue contributing to the approval threshold for an in-flight proposal.

The implementation must instead bind approvals to the signer membership that existed when the proposal was created and ensure that every approval remains valid against the current signer configuration.

A removed signer must never be counted toward execution.

3. Threshold downgrade / self-serving signer changes

A signer set configured as 2-of-3 can currently approve an UpdateSigners action that installs a weaker configuration, such as 2-of-2 or another signer set that makes future authorization easier.

The proposal is evaluated against the current configuration, meaning the old threshold can authorize the installation of a weaker future threshold.

This defeats the security purpose of the multisig because the protected parties can use the existing quorum to reduce the security requirement for subsequent operations.

UpdateSigners therefore requires an elevated quorum.

---

Required Implementation

1. Snapshot signer membership at proposal creation

Extend the proposal state so that it records the actual eligible signer addresses at the time the proposal is created.

Do not store only the signer count or threshold.

The snapshot should allow the contract to determine:

• Which addresses were eligible when the proposal was created.

• Whether an address attempting to approve was part of that original signer set.

• Whether that same address is still an active signer.

• Whether an approval can legitimately contribute toward execution.

The snapshot should be persisted as part of the proposal's storage representation.

Approval validation

approve() must validate the caller against:

1. The proposal's signer snapshot.

2. The current signer set.

An address that was a signer when the proposal was created but has since been removed must be rejected.

An address that is currently a signer but was not part of the proposal's snapshot must also be rejected for that proposal.

This prevents both stale approvals and newly introduced signers from being retroactively attached to an existing proposal.

Execution validation

execute() must independently revalidate every recorded approval.

Do not rely exclusively on:

approvals.len() >= threshold

Instead, execution should only count approvals where the approver:

• Exists in the proposal's signer snapshot.

• Is still a current signer.

• Has not otherwise become ineligible under the current multisig configuration.

This is important because execution may occur significantly later than approval collection.

The invariant should be:

A removed signer can never contribute to the execution quorum of an in-flight proposal.

---

2. Elevated quorum for UpdateSigners

Implement a stricter quorum specifically for signer-set changes.

Recommended policy

Use:

threshold + 1 approvals for every UpdateSigners action.

This must be capped/validated against the available signer count so the configuration cannot produce an impossible quorum.

The implementation should explicitly document this policy in code and tests.

For example:

Current configuration | Normal action | UpdateSigners -- | -- | -- 2-of-3 | 2 approvals | 3 approvals 3-of-5 | 3 approvals | 4 approvals 1-of-2 | 1 approval | 2 approvals

The exact validation rules should account for invalid configurations where threshold + 1 exceeds the available signer count.

Security invariant

A signer-set update must never be able to lower the security threshold using only the existing ordinary quorum.

For example:

Current: 2-of-3
Proposal: change to 2-of-2
Required approvals: 3

The proposal must not be executable with only two approvals.

This elevated quorum should apply specifically to the action that changes signer membership/threshold, rather than unnecessarily changing authorization requirements for unrelated parameter updates.

---

3. Harden configure_multisig()

configure_multisig() should receive additional security treatment because it is capable of changing the authorization boundary itself.

At minimum, the implementation must:

• Preserve the existing behavior where required by compatibility.

• Emit a prominent event when the multisig configuration changes.

• Include sufficient information in the event to identify the configuration transition.

• Make it obvious from emitted events that the signer set and/or threshold has changed.

Two-step confirmation

Please evaluate implementing configuration as a two-step operation:

configure/propose
        ↓
pending multisig configuration
        ↓
confirm
        ↓
active multisig configuration

The purpose is to prevent a single administrative call from silently replacing the security boundary.

If a two-step mechanism is implemented, the pending configuration should be explicit in storage and should have appropriate authorization checks on confirmation.

If architecture or backward compatibility makes a two-step flow inappropriate, document the reasoning in the PR and ensure the event/authorization model still provides strong protection against silent configuration replacement.

---

4. Handle in-flight proposals after signer-set changes

When the signer set changes, existing proposals must not retain authorization power from the previous signer configuration.

Implement one of the following safe mechanisms, preferably the simplest one consistent with the existing architecture:

Preferred behavior

Invalidate/clear in-flight proposals whose authorization depends on the previous signer set when UpdateSigners successfully executes.

Alternatively, retain them but ensure they are impossible to execute because all approvals are revalidated against current membership.

The important invariant is:

A signer-set transition must not leave behind executable proposals whose quorum can be satisfied by the old signer configuration.

This should apply regardless of whether the proposal was:

• created before the signer change,

• partially approved before the signer change,

• fully approved but not yet executed before the signer change.

The tests should explicitly cover each relevant lifecycle state.

---

5. Storage changes

Update:

contracts/parameters-contract/src/storage.rs

to support the signer snapshot and any additional state required by the configuration confirmation/invalidation mechanism.

Storage changes should:

• Follow the existing storage conventions.

• Preserve compatibility where possible.

• Avoid unnecessary changes to unrelated storage.

• Clearly document any new storage keys/types.

• Ensure proposal state can be loaded and validated consistently.

---

6. Type changes

Update:

contracts/parameters-contract/src/types.rs

as necessary.

Proposal should contain the signer snapshot required to validate approvals.

For example, conceptually:

Proposal {
    ...
    approvals: Vec<Address>,
    signer_snapshot: Vec<Address>,
    ...
}

The actual field naming and representation should follow the project's conventions.

MultisigConfig should remain the canonical source of the active signer set and threshold.

Avoid duplicating mutable authorization state unnecessarily.

---

7. Test coverage

Update:

contracts/parameters-contract/src/tests.rs

with comprehensive security regression tests.

The test suite must cover every branch introduced by the fix.

A. Stale approval exploit reproduction

Add an end-to-end regression test reproducing the vulnerability.

The test should conceptually perform:

1. Configure 2-of-3 signer set: A, B, C
2. Create proposal P
3. A approves P
4. Change signer set and remove A
5. B approves P
6. Attempt to execute P
7. Verify A's approval is NOT counted
8. Verify P cannot execute with only B + stale A approval

This test is especially important because it demonstrates the original security failure and proves the new implementation closes it.

The PR description should explicitly state that this test represents the pre-fix exploit semantics and now verifies the corrected behavior.

B. Removed signer cannot approve

Test that a signer removed from the active signer set after proposal creation cannot call approve() successfully.

Expected result:

approve() → rejected

C. New signer cannot retroactively approve

Create a proposal under signer set:

A, B, C

Change the signer set to:

B, C, D

Verify that D cannot add an approval to the old proposal.

This confirms that proposals are bound to their creation-time signer snapshot.

D. Execution revalidates approvals

Create a proposal, collect approvals, remove one of the approving signers, and attempt execution.

Verify that execution rejects the stale approval rather than relying on approvals.len().

E. Normal proposals retain normal quorum

Verify that unrelated parameter updates still use the configured normal threshold.

For example:

2-of-3

should still allow a normal parameter proposal to execute after two currently valid approvals.

The elevated quorum must not accidentally break ordinary parameter updates.

F. UpdateSigners requires elevated quorum

For a:

2-of-3

configuration, verify that:

2 approvals → insufficient
3 approvals → sufficient

for UpdateSigners.

G. Threshold downgrade is prevented

Explicitly test:

2-of-3 → 2-of-2

and verify that two approvals cannot execute the signer-set update.

Three valid approvals should be required.

H. Signer-set expansion is protected

Test a change such as:

A, B, C
→
A, B, C, D

and verify that the elevated quorum is still enforced.

I. Configuration event

Verify that configure_multisig() emits the new/prominent event with the expected information.

If a two-step configuration flow is implemented, add tests for:

• proposal creation,

• confirmation,

• unauthorized confirmation,

• replacement/cancellation of pending configuration,

• activation of the confirmed configuration.

J. In-flight proposal invalidation

Test proposals that are:

• unapproved,

• partially approved,

• fully approved but not executed,

when a signer-set change occurs.

None should be executable using approvals from removed signers.

---

8. Event compatibility

Preserve the existing event surface wherever possible.

Changes should be additive, not destructive.

Do not unnecessarily rename or remove existing events consumed by:

• indexers,

• frontend applications,

• monitoring,

• integration tests,

• downstream contracts.

If additional security events are required, add them alongside the existing events.

The configuration event should make signer-set changes highly visible for monitoring and incident response.

---

9. Context and project standards

Before implementation, read the following files in full:

context/architecture-context.md
context/code-standards.md
context/progress-tracker.md

Also read the complete parameters-contract implementation:

contracts/parameters-contract/src/lib.rs
contracts/parameters-contract/src/storage.rs
contracts/parameters-contract/src/types.rs
contracts/parameters-contract/src/tests.rs

The implementation must follow the project's established architecture and coding conventions exactly.

Do not introduce a new abstraction or storage pattern without first confirming that it fits the existing contract architecture.

---

10. Progress tracker

Update:

context/progress-tracker.md

with:

• The security issue addressed.

• The affected contract.

• The implementation status.

• The tests added.

• Any architectural decision made around configure_multisig().

• Any follow-up work that remains.

The tracker should accurately reflect the final implementation rather than simply marking the task complete.

---

Security Invariants

The implementation should enforce these invariants:

Invariant 1 — Current membership

A removed signer can never contribute an approval toward execution.

Invariant 2 — Proposal snapshot

A signer must have been eligible when the proposal was created to approve that proposal.

Invariant 3 — Execution revalidation

Proposal execution must independently validate all approvals against the proposal snapshot and current signer membership.

Invariant 4 — Protected signer configuration

UpdateSigners requires a strictly higher quorum than ordinary parameter updates.

Invariant 5 — No self-cheapening

An existing quorum cannot be used to install a signer configuration that bypasses the elevated authorization requirement.

Invariant 6 — Configuration visibility

Changes to the multisig authorization boundary must generate an explicit, prominent event.

Invariant 7 — No stale authorization

Signer-set changes cannot leave previously collected approvals capable of satisfying a future execution quorum.

---

Files To Touch

The implementation should be limited to the following files unless a necessary dependency requires otherwise:

contracts/parameters-contract/src/lib.rs
contracts/parameters-contract/src/storage.rs
contracts/parameters-contract/src/types.rs
contracts/parameters-contract/src/tests.rs
context/progress-tracker.md

Avoid unrelated refactors.

---

Mandatory Verification

Before opening the PR, verify all of the following:

• context/architecture-context.md read in full.

• context/code-standards.md read in full.

• context/progress-tracker.md read in full.

• parameters-contract/src/lib.rs read in full.

• parameters-contract/src/storage.rs read in full.

• parameters-contract/src/types.rs read in full.

• Existing tests reviewed before adding new tests.

• Signer snapshot implemented.

• approve() validates against both snapshot and current membership.

• execute() revalidates every approval.

• Removed signer approvals are never counted.

• UpdateSigners requires threshold + 1 valid approvals.

• Threshold downgrade is explicitly tested.

• Stale approval exploit has an end-to-end regression test.

• In-flight proposals are invalidated or made unexecutable after signer-set changes.

• configure_multisig() emits a prominent event.

• Two-step configuration was evaluated and the decision documented.

• Existing events remain compatible wherever possible.

• context/progress-tracker.md updated.

• Build completes with zero errors.

• Entire test suite passes.

• No unrelated files or behavior were modified.

• PR references this issue number exactly.

---

Definition of Done

This PR is complete when the parameters-contract multisig can no longer be bypassed through stale approvals or weakened through an ordinary signer quorum.

In particular, the following scenario must be impossible:

2-of-3 multisig
      ↓
A + B approve proposal
      ↓
A is removed
      ↓
A's old approval remains stored
      ↓
B executes using A + B

It must instead behave as:

2-of-3 multisig
      ↓
A + B approve proposal
      ↓
A is removed
      ↓
A's approval becomes invalid
      ↓
B alone cannot satisfy quorum
      ↓
execution is rejected

Likewise, this must be impossible:

2-of-3
  ↓
2 signers approve UpdateSigners
  ↓
install weaker 2-of-2 configuration

The corrected behavior must require the elevated quorum:

2-of-3
  ↓
UpdateSigners
  ↓
requires 3 valid approvals
  ↓
only then can signer configuration change

The final implementation should provide a clear, test-backed authorization boundary where signer membership changes invalidate stale authority and signer-set changes require stronger consensus than ordinary parameter changes.

[EmeditWeb]

⚠️ Automated Audit: partial

@BigMick03 Good start — please look into the gaps identified below.

The PR addresses all three root causes described in #84 with the exact mechanisms the issue requested: (1) configure_multisig is now two-step (staged pending config + confirm_multisig, both requiring fresh admin auth) with a prominent MSIGPEND event carrying the full signer list; (2) proposals persist an eligible_signers snapshot, approve() validates against both snapshot and current membership, and execute() revalidates every recorded approver against both sets, panicking StaleApproval so removed signers can never count toward quorum; (3) UpdateSigners requires an elevated quorum (threshold+1, capped at unanimity — a documented, justified deviation) and in-flight signer-targeting proposals are invalidated with PROPINVL on signer changes. Events are additive (MSCONFIG preserved), new errors/storage keys follow conventions, and progress-tracker.md is updated with honest disclosure of the XDR layout break. Caveats: the claimed test results (37 parameters-contract tests, exploit-reproduction test failing pre-fix) are UNVERIFIED — the independent sandbox run was inconclusive and no CI exists — and the PR bundles substantial unrelated liquidity-pool defense-in-depth work, violating the issue's 'avoid unrelated refactors' rule.

⚖️ Adjusted by bot policy: confidence 78% is below the 90% threshold for a full approval; gaps were still identified.

Gaps identified:

• Unrelated liquidity-pool vendor-registry/outflow-cap/merchant-cap feature work is bundled into this security PR; should be split out per the issue's 'no unrelated refactors' rule
• invalidate_signer_proposals() iterates every proposal id ever created on each signer-set change — unbounded scan that risks exceeding Soroban instruction budgets as proposal history grows
• Proposal struct layout change renders pre-upgrade in-flight proposals undecodable; documented but no concrete migration/upgrade sequencing provided

---

CI checks: none configured

_{Audited by stepfi-audit-bot 🤖}

[EmeditWeb]

⚠️ Automated Audit: partial

@BigMick03 Good start — please look into the gaps identified below.

The parameters-contract changes genuinely target the root causes: eligible-signer snapshots bound into Proposal, approve()/execute() validating against both snapshot and current membership (StaleApproval/NotEligibleSigner), elevated threshold+1-capped-at-unanimity quorum for UpdateSigners, invalidation of in-flight signer proposals, and a two-step configure_multisig with prominent events — matching the issue's acceptance criteria. However, full approval is blocked because the PR title ('Step fi2') is vague and fails the mandatory description-quality bar, and the PR bundles substantial unrelated liquidity-pool/vendor-registry hardening despite the issue's explicit 'avoid unrelated refactors' constraint. The claimed '37 tests green' result could not be independently verified since no CI is configured; the claim is treated as unverified, though the visible diffs (new error/event/test plumbing, updated creditline integration test call-site) are consistent with real regression tests having been added.

Gaps identified:

• PR title 'Step fi2' is too vague — must state the essence of the security fix (e.g. 'Harden parameters multisig: signer snapshots, elevated UpdateSigners quorum, two-step config')
• Unrelated scope creep: liquidity-pool-contract outflow/concentration caps and vendor-registry cross-check belong in a separate PR per the issue's Files-To-Touch constraints
• Confirm parameters-contract tests.rs covers every required branch (stale-approval exploit end-to-end, removed/new-signer rejection, downgrade prevention, in-flight invalidation lifecycle states) — its diff was not among the shown files and claimed results are unverified

---

CI checks: none configured
Merge conflicts: none, but the PR is blocked (failing/missing required checks or reviews).

_{Audited by stepfi-audit-bot 🤖}

[EmeditWeb]

⚠️ Automated Audit: partial

@BigMick03 Good start — please look into the gaps identified below.

The PR addresses all three root causes described in Issue #84: (1) admin bypass is mitigated by a two-step configure→confirm flow requiring separate transactions; (2) stale approvals are prevented by snapshotting eligible signers at proposal creation and revalidating on both approve() and execute(); (3) self-serving threshold reduction is blocked by requiring threshold+1 approvals for UpdateSigners actions. Comprehensive regression tests are present, including the end-to-end stale-approval exploit reproduction, elevated quorum tests, and proposal invalidation tests. The independent sandbox run passes. The PR title 'Step fi2' is vague and does not describe the security fix, which is an author-fixable gap but does not undermine the code-level fix.

⚖️ Adjusted by bot policy: gaps were still identified.

Gaps identified:

• PR title 'Step fi2' is vague — should describe the multisig security hardening
• Patch is truncated in lib.rs execute() — cannot fully verify the invalidation scan for in-flight UpdateSigners proposals in the diff, though tests suggest it is implemented
• CI checks not configured for the repo (not an author gap but noted neutrally)

---

CI checks: none configured
Merge conflicts: ✅ none — but the PR is blocked (failing/missing required checks or reviews).

Independent test run: PASSED

afe_math.rs:7:8
  |
7 | pub fn sub_i128(a: i128, b: i128) -> Result<i128, ParametersError> {
  |        ^^^^^^^^

warning: function `mul_i128` is never used
  --> contracts\parameters-contract\src\safe_math.rs:11:8
   |
11 | pub fn mul_i128(a: i128, b: i128) -> Result<i128, ParametersError> {
   |        ^^^^^^^^

warning: function `div_i128` is never used
  --> contracts\parameters-contract\src\safe_math.rs:15:8
   |
15 | pub fn div_i128(a: i128, b: i128) -> Result<i128, ParametersError> {
   |        ^^^^^^^^

warning: unused imports: `Val` and `Vec`
 --> contracts\vendor-registry-contract\src\tests.rs:4:36
  |
4 |     Address, Env, IntoVal, String, Val, Vec,
  |                                    ^^^  ^^^
  |
  = note: `#[warn(unused_imports)]` (part of `#[warn(unused)]`) on by default

warning: variable does not need to be mutable
    --> contracts\creditline-contract\src\tests.rs:3545:9
     |
3545 |     let mut loan = t.client.get_loan(&loan_id);
     |         ----^^^^
     |         |
     |         help: remove this `mut`
     |
     = note: `#[warn(unused_mut)]` (part of `#[warn(unused)]`) on by default

warning: unused variable: `share_price_before`
    --> contracts\creditline-contract\src\tests.rs:3896:9
     |
3896 |     let share_price_before = lp_client.get_pool_stats().share_price;
     |         ^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_share_price_before`
     |
     = note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

warning: function `div_u64` is never used
  --> contracts\creditline-contract\src\safe_math.rs:31:8
   |
31 | pub fn div_u64(a: u64, b: u64) -> Result<u64, CreditLineError> {
   |        ^^^^^^^
   |
   = note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default

warning: function `assert_event` is never used
   --> contracts\creditline-contract\src\tests.rs:616:4
    |
616 | fn assert_event(env: &Env, expected: soroban_sdk::Symbol) {
    |    ^^^^^^^^^^^^

_{Audited by stepfi-audit-bot 🤖}