fix(parameters): secure multisig against stale approvals, self-degrading quorum, and admin bypass (#107) - #108
Conversation
…ing quorum, and admin bypass (StepFi-app#107) - Snapshot the eligible signer set into every Proposal; approve() and execute() re-validate each approver against the snapshot AND current membership so removed signers' approvals are never counted and newly added signers cannot influence older proposals (ApproverNotEligible=19). - Require threshold + 1 approvals (capped at full signer count) for UpdateSigners actions so a committee cannot cheapen its own gate. - Split configure_multisig into a two-step propose->confirm flow with a prominent MSCONFPR event before any signer set is activated. - Invalidate in-flight UpdateSigners proposals when the signer set changes (ProposalInvalidated=18, PROPIVLD event). - Tests: 399 workspace tests green, including the stale-approval exploit reproduced end-to-end (fails pre-fix, passes post-fix).
EmeditWeb
left a comment
There was a problem hiding this comment.
✅ Automated Audit: solves
@KingFRANKHOOD Excellent work, thank you! 🎉
The PR genuinely addresses all three root causes from issue #107: (1) admin bypass is fixed via two-step configure/confirm flow where configure_multisig only proposes and emits MSCONFPR event, confirm_multisig applies the change; (2) stale approvals are prevented by snapshot validation at approve() and execute() time — every stored approver must be in the proposal-time snapshot AND current signer set; (3) self-degrading quorum is fixed by requiring threshold+1 for UpdateSigners actions, capped at unanimity. All 34 new tests pass, including the end-to-end exploit reproduction test that specifically verifies the stale-approval scenario from the issue fails pre-fix and passes post-fix. CI shows 399 passed, 0 failed. PR title and description are substantive and of high quality. CI integrity is maintained — no workflow modifications.
CI checks: ✅ PASSED: Build and Test Contracts
Merge conflicts: ✅ none — but the PR is blocked (failing/missing required checks or reviews).
Audited by stepfi-audit-bot 🤖
Summary
Closes #107
Replaces the decorative parameters-contract multisig with real security:
Proposalnow records asnapshotof the eligible signer set at proposal time.approve()rejects signers who were not members at proposal time, andexecute()re-validates every stored approver against both the snapshot and the current signer set before counting it. A signer removed since (or added after) approval instantly loses all approval/veto power over in-flight proposals.UpdateSignersactions needthreshold + 1approvals, capped at the full signer count (unanimity for already-unanimous sets). A 2-of-3 committee can no longer install a 2-of-2 gate (or admit a colluder) with only 2 old-threshold approvals.configure_multisig→confirm_multisig. The admin step now only records a pending config and emits a prominentMSCONFPRevent carrying the full proposed signer set; the set is activated only by the explicitconfirm_multisigstep. A single compromised admin key can no longer silently swap the signer set — the intent is broadcast on-chain first.do_update_signerssweep and void every other non-executedUpdateSignersproposal (ProposalInvalidated = 18,PROPIVLDevent). Non-signer proposals survive, but any stale approver blocks execution via the membership re-validation.New errors:
ProposalInvalidated = 18,ApproverNotEligible = 19. Existing event surface preserved; only additive events (MSCONFPR,PROPIVLD) were added.This repo is for Soroban smart contracts only
contracts/directoryType of change
What changed per file
contracts/parameters-contract/src/types.rs—Proposalgainssnapshot: Vec<Address>andinvalidated: bool.contracts/parameters-contract/src/errors.rs—ProposalInvalidated = 18,ApproverNotEligible = 19.contracts/parameters-contract/src/storage.rs— instance keysPMSCFG(pending multisig) andPROPIDS(active-proposal index) plus accessors.contracts/parameters-contract/src/events.rs— additiveMSCONFPR(prominent multisig-configure-proposed) andPROPIVLD(proposal invalidated) events.contracts/parameters-contract/src/lib.rs— two-stepconfigure_multisig/confirm_multisig; snapshot capture inpropose; snapshot+membership validation inapprove/execute; elevated quorum forUpdateSigners; invalidation sweep on signer-set change.contracts/parameters-contract/src/tests.rs— 34 tests, including 15 new security tests.contracts/creditline-contract/src/tests.rs— governance integration test now callsconfirm_multisig()between configure and propose.context/progress-tracker.md— updated.Security tests (new)
test_stale_approval_from_removed_signer_is_never_counted— 2-of-3 set; s1 + s2 approve a parameter rewrite; s2 is then removed viaUpdateSigners; the old proposal'sexecutemust now fail and parameters must remain untouched. Pre-fix this test fails (the proposal executed and rewrotemin_guarantee_percent— the exact exploit from the issue). Post-fix it passes.test_execute_rejects_proposal_whose_approver_was_removed(core: implement vendor payment escrow in creditline #19),test_removed_signer_cannot_approve_after_removal(core: implement atomic loan funding from liquidity pool #10),test_newly_added_signer_cannot_approve_old_proposal(core: implement vendor payment escrow in creditline #19).test_update_signers_requires_elevated_quorum(core: implement contract event replay and indexing support #17),test_update_signers_with_elevated_quorum_executes,test_signer_change_quorum_capped_at_full_committee_for_unanimous_set.test_configure_multisig_two_step_propose_confirm_emits_prominent_events,test_confirm_multisig_without_propose_fails(feat: implement approve_loan() Pending→Active transition #9),test_configure_multisig_cannot_repropose_without_confirm([8] Add TTLextend_ttl()to liquidity-pool and vendor-registry contracts #8),test_confirm_multisig_cannot_be_called_twice([8] Add TTLextend_ttl()to liquidity-pool and vendor-registry contracts #8).test_in_flight_signer_set_proposals_are_invalidated_on_signer_change(+PROPIVLD),test_approve_rejects_invalidated_signer_set_proposal(core: implement minimum reputation threshold enforcement #18),test_parameter_proposals_survive_signer_change_but_stale_approvals_revalidated.Acceptance criteria
approve()andexecute()against snapshot + current membership.threshold + 1, capped at unanimity.configure_multisigemits a prominentMSCONFPRevent and requires a secondconfirm_multisigstep.Testing
cargo test --locked→ 399 passed, 0 failed (parameters 34, creditline 143, liquidity-pool 109, reputation 60, vendor-registry 26, vouching 27)Context files reviewed