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

[Leothosine]

Summary

Closes two critical authorization gaps (issue #144), post-#138's vault refactor.

C-01 — Three of four tier vaults gated mutating functions on the wrong address

VaultL3, VaultL6, and VaultL12's deposit/withdraw/early_exit/relock all gated on user.require_auth() — only VaultFlex correctly gated on admin.require_auth() (admin = the registered VaultRouter address). This let a caller invoke a tier vault directly, bypassing VaultRouter's pause flag and asset allowlist, and — most severely for deposit — the actual token transfer VaultRouter performs before invoking the vault (each vault's deposit is documented as "bookkeeping only," trusting the router already moved the tokens). A direct call let a caller mint themselves shares/balance for free with no principal ever deposited.

Fix: all four functions in all three vaults now gate on admin.require_auth(), matching VaultFlex's already-correct pattern exactly.

C-02 — GuardianMultisig::confirm_veto never attributed confirmations to a real caller

confirm_veto set caller = env.current_contract_address() (the multisig's own address — Soroban has no implicit msg.sender), never called require_auth() on anything meaningful, and pushed a clone of governance_contract (not the confirming owner) into the confirmations list — meaning the first-ever confirmation for a proposal made every later confirmation attempt panic with "already confirmed." With any threshold > 1, a veto could never actually execute; only the existing 1-of-1 test masked this.

Fix: confirm_veto now takes an explicit owner: Address, calls owner.require_auth(), verifies membership in the registered owner set, and tracks confirmations by the real owner address — the standard Soroban pattern (no implicit invoker exists for a direct call; the caller must name and authorize the address they're acting as), matching the shape the TypeScript SDK's Signer/WalletAdapter interfaces already use.

What's committed alongside the fixes

• Regression tests in all four vaults and guardian_multisig that mock auth for the wrong address only (never admin, never the true owner) and assert the call is now rejected — every one of these would have passed silently against the pre-fix code. A companion positive test per contract proves the correctly-authorized path still works. guardian_multisig additionally gets: non-owner rejection, same-owner double-confirm rejection, exactly-once-at-threshold dispatch verification (via a MockGovernance that counts calls), per-(governance_contract, proposal_id) scoping, and a test with no auth mocking at all proving require_auth() is genuinely enforced (this specific test would not have panicked against the pre-fix placeholder).
• audit/authorization-boundaries-2026-08.md — the committed authorization-boundary review the issue's Definition of Done requires: a who-can-call-what table for every payment-moving entrypoint (deposit, withdraw, early_exit, relock, set_max_tvl, pause/unpause, confirm_veto), a re-entrancy review of the two-hop transfer pattern under Soroban's host-function model (extending internal-2026-01.md's stated Governance-focused follow-up to the payment path), and a documented known limitation: no owner-rotation function exists in GuardianMultisig yet, so the "owner rotated mid-veto" edge case can't be exercised against real behavior — the fix is already robust to it in spirit (owner-set membership is re-read fresh on every call), but a future rotation function would need to explicitly decide how to handle in-flight confirmations naming a removed owner.
• One incidental finding noted in the audit doc but not fixed (out of this issue's authorization scope): VaultFlex doesn't implement max_tvl/remaining_capacity/set_max_tvl, so VaultRouter.vault_capacity(Tier::Flex) / set_max_tvl(Tier::Flex, ...) would fail at runtime — a functional gap, not an auth one, flagged for a separate issue.

Test plan

• Manual review of all five changed files (guardian_multisig, vault_flex, vault_l3, vault_l6, vault_l12) — brace-balance and structural correctness checked line by line.
• cargo test — could not run locally: this environment's Rust linker toolchain is broken (unrelated to this change — link.exe misresolution during build-script compilation). Flagging this honestly rather than claiming a local run that didn't happen. CI's "Soroban contracts" job itself only runs cargo build --target wasm32-unknown-unknown --release, which does not compile #[cfg(test)] code either way, so this PR's non-test code (the actual fixes) is what that job verifies; the new tests are the ones asking for a real cargo test run as a follow-up if this repo's CI is ever extended to include one.
• CI (contracts build / app / sdk) — pending on this PR; expect app and sdk jobs to pass trivially since no files in app/ or sdks/ changed.

Closes #144

[vercel]

@Leothosine is attempting to deploy a commit to the wumibals' projects Team on Vercel.

A member of the Team first needs to authorize it.

[wumibals]

Reviewed the payment authorization boundary hardening PR (closes #144). Both fixes address genuinely critical vulnerabilities, not defense-in-depth hardening.

• C-01 is the more severe of the two: three of four tier vaults gating deposit/withdraw/early_exit/relock on user.require_auth() instead of admin.require_auth() (where admin = the registered VaultRouter) meant anyone could call a tier vault directly, bypassing VaultRouter's pause flag and asset allowlist entirely. For deposit specifically, since each vault trusts that VaultRouter already moved the tokens before invoking it (documented as "bookkeeping only"), a direct call let a caller mint themselves shares/balance for free with zero principal ever deposited — a real fund-draining vulnerability, not a theoretical one. The fix correctly brings all three vaults in line with VaultFlex's already-correct pattern rather than inventing a new one.
• C-02 is a subtler but still serious bug: confirm_veto recording env.current_contract_address() (the multisig's own address) instead of the actual confirming owner meant every confirmation after the first panicked as "already confirmed" — so with any threshold > 1, a veto could never execute at all. That's a silent governance-availability failure that only a 1-of-1 test could have masked, which is exactly what happened. Requiring an explicit owner: Address parameter with owner.require_auth() is the correct Soroban pattern here (no implicit msg.sender exists), and it's good that this is called out explicitly as the standard idiom rather than a one-off fix.
• The regression tests are well-designed in principle: mocking auth for the wrong address and asserting rejection, with a companion positive test proving the correct path still works, plus a test with no auth mocking at all for confirm_veto specifically to prove require_auth() is genuinely enforced (not just present in the source). That's a meaningfully stronger test than most authorization regression suites bother with.
• The committed audit doc (audit/authorization-boundaries-2026-08.md) — a who-can-call-what table across every payment-moving entrypoint, a re-entrancy review of the two-hop transfer pattern, and an honestly-documented known limitation (no owner-rotation function exists yet, so that edge case can't be exercised against real behavior) — is real due diligence, not a checkbox. Flagging the separate VaultFlex max_tvl functional gap as out-of-scope rather than scope-creeping it into this PR is the right call for a focused security fix.
• The disclosure on testing is honest and appreciated: cargo test could not be run locally due to a broken linker (unrelated to this change), and this repo's CI only runs cargo build, which doesn't compile #[cfg(test)] code at all. That means the new regression tests — including the "no auth mocking" test that's the strongest evidence the fix actually works — have not been executed by anyone yet. The actual production code paths (the require_auth() changes themselves) did compile successfully under CI, and manual line-by-line review is a reasonable stopgap for a security fix of this kind, but the test suite itself remains unverified pending a real cargo test run — worth prioritizing given both bugs being fixed here are genuinely severe.

CI is green across Soroban contracts (build), Next.js dashboard, and TypeScript SDK. Vercel's FAILURE is the usual unauthorized deployment integration link, unrelated to the code.

Approving — these are correct, well-reasoned fixes for two critical authorization gaps; get cargo test running against this branch (or wire it into CI) as a priority follow-up before relying on the new regression suite as the safety net it's meant to be.