Skip to content

fix: ask the admin to sign each call once - #274

Open
hpmaxi wants to merge 11 commits into
refactor/guardsfrom
fix/single-admin-signature
Open

hpmaxi wants to merge 11 commits into
refactor/guardsfrom
fix/single-admin-signature

Conversation

@hpmaxi

@hpmaxi hpmaxi commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #325

Admin calls asked the admin to sign twice. In the vault, the share token, the oracle and the identity verifier, 28 entrypoints checked the stored admin's signature with #[only_admin], then asked their caller argument to sign too, without comparing it with the admin. The admin signed each call twice, and any other caller passed as long as both signed. Tests checked that a call without the admin's signature fails, not what each call asks to sign.

Now each admin call asks for one signature. The kit's own admin entrypoints take no caller argument, and the stored admin signs. pause, unpause, set_compliance and set_identity_verifier keep the argument their OpenZeppelin trait fixes: they refuse any other account with AdminRequired and ask the admin to sign once. Each contract has tests that record what every privileged call asks to sign.

Changes

  • The kit's own admin entrypoints in all five contracts take no caller argument; #[only_admin] asks the stored admin to sign
  • The vault's unpause and the token's pause, unpause, set_compliance and set_identity_verifier refuse a caller other than the admin with AdminRequired, then ask it to sign once
  • A caller_is_admin guard in the vault and the token makes that check, with a unit test
  • AdminRequired's doc in the vault and the token covers the new refusal
  • Tests assert exactly what each privileged call asks to sign; the three tests that pinned two identical admin entries now pin one
  • Refusal tests sign as the other account instead of naming it as the caller
  • The test-only migrate of the token and oracle probes takes no caller either
  • The harness, the authorization matrix, the kit spec and the testing guide follow the new signatures

Deviations

  • The compliance contract's and the verifier's OpenZeppelin trait methods keep their unread operator, as before: they already ask for one signature, and the matrix says why.
  • The signature tests also cover role calls (renounce_role included), the co-signed legal_transfer and recover_balance, hand-overs and the compliance hooks, not only the 28.
  • New harness check: an outsider cannot pause the share token (AdminRequired).
  • The kit spec's accept_admin_transfer row named a caller the call does not take; it now says the pending admin signs. Its runbook's grant_role and bind_token lines now show every argument.

Acceptance criteria

  • No production entrypoint combines an admin check with a second signature request for the same account
  • Every privileged entrypoint has a test that asserts exactly what it asks to sign
  • The harness and the authorization docs follow the new signatures, and the harness passes on a local network

Test plan

Automated tests

  1. cargo test --locked: 677 passed (431 vault, 98 oracle, 80 token, 36 compliance, 7 identity verifier, 19 timelock, 6 storage), 16 of them new. Each of the six commits passes on its own.
  2. cargo clippy --locked --all-targets --workspace -- -D warnings, and on the vault with --features short_wind_down: clean.
  3. The Wasm upgrade tests (cargo test -p async-vault -p nav-oracle --features upgrade_wasm with_wasm, after the builds in docs/TESTING.md): vault 5 and oracle 2 pass against the new builds and probes.
  4. CI runs the harness deploy, happy path and guards on a local network.

Manual verification

  1. Start a local network: docker run --rm -d -p 8000:8000 --name stellar-local stellar/quickstart --local --enable core,rpc.
  2. Run stellar contract build, then CARGO_TARGET_DIR=target/short-wind-down stellar contract build --package async-vault --features short_wind_down.
  3. Run npm run build:clients, then HARNESS_NETWORK=local npm run deploy -w scripts/harness.
  4. Run test:happy-path, test:guards and test:wind-down with npm run <script> -w scripts/harness.
  5. You should see 24, 49 and 6 tests pass. The Wasm sizes in bytes: vault 71,319 (was 71,553), token 42,490 (was 42,464), oracle 32,937 (was 33,215), compliance 24,371 (was 24,536), identity verifier 4,115 (was 4,151).

Breaking changes

Fresh deployments only. These entrypoints lose their last argument, caller:

  • async_vault: set_deposit_cap(cap), set_custodian(custodian), set_wind_down_delay(secs), propose_wind_down(), cancel_wind_down_proposal(), set_notice(secs), finish_setup(), propose_change(change), cancel_change(id), apply_change(id)
  • share_token: bind_vault(vault), finish_setup(), propose_change(change), cancel_change(id), apply_change(id)
  • nav_oracle: bind_vault(vault), finish_setup(), propose_change(change), cancel_change(id), apply_change(id), set_nav_paused(paused), clear_latest()
  • compliance: bind_vault(vault), finish_setup(), propose_change(change), cancel_change(id), apply_change(id)
  • identity_verifier: allow(account, allowed)

The vault's unpause and the token's pause, unpause, set_compliance and set_identity_verifier keep their arguments, but now refuse a caller other than the admin with AdminRequired (6040 on the vault, 7000 on the token), even when it signs. Each admin call now carries one admin authorization entry instead of two. No event changes. The console is not changed here; its calls to these entrypoints still pass caller.

Checklist

  • Self-reviewed my own diff
  • Tests added or updated
  • Docs updated (if applicable)
  • No unrelated changes bundled in

Screenshots

None.

The token's own admin calls drop the caller argument that made the
admin sign twice. pause, unpause, set_compliance and
set_identity_verifier keep the argument their OpenZeppelin trait fixes:
they refuse any account but the admin with AdminRequired and ask it to
sign once.
The vault's own admin calls drop the caller argument that made the
admin sign twice. unpause keeps the caller the Pausable trait fixes: it
refuses any account but the admin with AdminRequired and asks it to
sign once.
@vercel

vercel Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
strata-vault-kit-app Ready Ready Preview Oct 23, 2026 3:32pm UTC

Request Review

@hpmaxi
hpmaxi marked this pull request as ready for review October 20, 2026 02:21
Brings main into this branch through feedback. Conflict in
docs/TESTING.md: keep main's two layers of authorization tests, and add
this branch's signature tests to the second one: signing as another
account only, and reading `e.auths()` for the exact signers a call needs.

This branch was successfully deployed

1 active deployment
Preview — c741c385 Deployed Oct 23, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants