You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
(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().
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.
Problem / Context
contracts/guardian_multisig/src/lib.rs'sconfirm_vetocontains a literal placeholder instead of real caller attribution:(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
confirm_vetoto use the actual transaction invoker (Soroban's auth-context / invoker mechanism, or require the caller to pass andrequire_auth()their own address), and verify that address is a registered owner before recording a confirmation.Governance::vetoexactly once, exactly at the threshold — not before, not repeatedly after.require_auth()chain across the (post-Payments 1/8: Fix VaultRouter <-> tier-vault ABI mismatch and unify asset-scoped accounting #138) vault interfaces: confirm thatVaultRouter.deposit/withdraw/early_exit'suser.require_auth()cannot be bypassed by calling a tier vault directly. Today, tier vaults gate their mutating functions onadmin.require_auth()whereadmin=VaultRouter(seecontracts/vault_flex/src/lib.rsline 37 and the equivalent invault_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'sdeposit/withdrawdirectly.user -> vaulton deposit,vault -> useron 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.mdline 74) to the payment path specifically, not just governance.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 extendingaudit/or a newSECURITY.md-adjacent note).Important edge cases and failure scenarios
confirm_vetomust not allow a stale confirmation count to persist incorrectly against the new owner set.deposit/withdrawdirectly (bypassingVaultRouter) must be rejected by the vault's own admin-gatedrequire_auth().contracts/vault_router/src/lib.rslines 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_vetocorrectly attributes confirmations to the actual calling owner; a dedicated test suite proves non-owner and duplicate-owner confirmations are both rejected.deposit,withdraw,early_exit,relock,pause,unpause,harvest,confirm_veto.require_auth()check, verified by tests that attempt to bypass each one directly.Definition of done
PR merged; the
guardian_multisigplaceholder is removed and replaced with a real, tested invoker check; the authorization-boundary document is committed;cargo testis green including the new negative-authorization tests.