Skip to content

Payments 7/8: Security hardening of payment authorization boundaries (guardian multisig invoker fix + auth audit) #144

Description

@wumibals

Problem / Context

contracts/guardian_multisig/src/lib.rs's confirm_veto contains a literal placeholder instead of real caller attribution:

pub fn confirm_veto(env: Env, governance_contract: Address, proposal_id: u32) {
    let caller = env.current_contract_address(); // placeholder — real impl uses invoker

(line 79-80). env.current_contract_address() returns the multisig contract's own address, not the calling owner's — meaning veto confirmations are not actually attributed to distinct owners today. This breaks the N-of-M threshold guarantee that the Guardian Multisig exists to provide: per the README's own risk model, the Guardian's veto over Strategist proposals is one of only two privileged, fund-adjacent authority paths in the protocol, and it is currently non-functional as a distinct-owner threshold check.

Separately, #138 changes the storage model and call patterns across all four tier vaults (asset-scoped keys, standardized signatures). Every payment-moving require_auth() boundary in the router-to-vault-to-token call chain should get a dedicated security pass once that structural change lands, rather than treating authorization correctness as an implicit side effect of a feature PR.

Objective

Close the guardian-multisig authorization gap, and verify that every payment-moving code path enforces the correct, minimal authorization boundary consistent with the protocol's stated non-custodial design.

Technical approach

  • Fix confirm_veto to use the actual transaction invoker (Soroban's auth-context / invoker mechanism, or require the caller to pass and require_auth() their own address), and verify that address is a registered owner before recording a confirmation.
  • Add tests proving: a non-owner cannot confirm; the same owner cannot double-count toward the threshold by calling repeatedly; the veto dispatches to Governance::veto exactly once, exactly at the threshold — not before, not repeatedly after.
  • Audit the require_auth() chain across the (post-Payments 1/8: Fix VaultRouter <-> tier-vault ABI mismatch and unify asset-scoped accounting #138) vault interfaces: confirm that VaultRouter.deposit/withdraw/early_exit's user.require_auth() cannot be bypassed by calling a tier vault directly. Today, tier vaults gate their mutating functions on admin.require_auth() where admin = VaultRouter (see contracts/vault_flex/src/lib.rs line 37 and the equivalent in vault_l3) — verify this pattern holds, unchanged in spirit, across all four vaults after Payments 1/8: Fix VaultRouter <-> tier-vault ABI mismatch and unify asset-scoped accounting #138's refactor, so a caller can't skip router-level checks (the pause flag, the asset allowlist) by invoking a vault's deposit/withdraw directly.
  • Review the two-hop transfer pattern (user -> vault on deposit, vault -> user on withdraw) for re-entrancy exposure under Soroban's host-function model — extending the internal audit's own stated follow-up ("Review of Governance execution path for re-entrancy... under Soroban's host-function model," audit/internal-2026-01.md line 74) to the payment path specifically, not just governance.
  • Document the authorization model explicitly as part of this PR: who can call what, and what each require_auth() call actually proves at each hop of the payment path. This currently exists only implicitly in code with no single reference.

Detailed scope

contracts/guardian_multisig/src/lib.rs, contracts/vault_router, contracts/vault_flex/vault_l3/vault_l6/vault_l12 (authorization-boundary verification, building on #138's structural changes — not a re-implementation), and a new authorization-boundary document (either extending audit/ or a new SECURITY.md-adjacent note).

Important edge cases and failure scenarios

  • Owner removed or rotated mid-veto-proposal — confirm_veto must not allow a stale confirmation count to persist incorrectly against the new owner set.
  • Regression coverage: existing tests must be extended so they fail against the current placeholder behavior, proving the fix actually changes the outcome rather than just adding parallel, unused code.
  • A malicious or buggy adjacent contract attempting to call a tier vault's deposit/withdraw directly (bypassing VaultRouter) must be rejected by the vault's own admin-gated require_auth().
  • Guardian pause/unpause (contracts/vault_router/src/lib.rs lines 114-133) is itself authorization-critical — confirm it remains callable only by the registered Guardian address after Payments 1/8: Fix VaultRouter <-> tier-vault ABI mismatch and unify asset-scoped accounting #138's changes, with no incidental widening of who can trigger it.

Dependencies

Depends on #138 — the authorization-boundary audit needs to review the final, post-refactor vault interfaces, not the current mismatched ones.

Acceptance criteria

  • confirm_veto correctly attributes confirmations to the actual calling owner; a dedicated test suite proves non-owner and duplicate-owner confirmations are both rejected.
  • A committed authorization-boundary review covers every payment-moving entrypoint: deposit, withdraw, early_exit, relock, pause, unpause, harvest, confirm_veto.
  • No payment-moving contract function is callable without its intended require_auth() check, verified by tests that attempt to bypass each one directly.

Definition of done

PR merged; the guardian_multisig placeholder is removed and replaced with a real, tested invoker check; the authorization-boundary document is committed; cargo test is green including the new negative-authorization tests.

Activity

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

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardThird CampaignCampaign: Third CampaignbugSomething isn't workingcontract

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions