fix: enforce homogeneous policies and commit every matching policy - #116
fix: enforce homogeneous policies and commit every matching policy#116alberto-crossmint wants to merge 1 commit into
Conversation
Two interlocking fixes that together close a Standard-signer authorization-bypass and a partial-state-commit hazard. 1. Reject mixed SignerPolicy variants on Standard signers (account.rs::validate_homogeneous_policies). A signer's policy vector must be either all ExternalValidatorPolicy or all TokenTransferPolicy. Mixing them let the OR-short-circuit in SignerRole::is_authorized authorize through a permissive external policy while a strict TokenTransferPolicy spending cap was never consulted. 2. Run on_authorized for every matching policy, not just the first (permissions.rs). With the homogeneity check in place, OR semantics are kept (multi-currency configurations work as before), but overlapping same-variant policies (e.g. daily and monthly cap on the same token) all advance their spending trackers instead of only the first matching one. The README is rewritten to match the implemented semantics — earlier text said "all policies must pass" while the code implements OR.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60f1e01800
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for policy in policies.iter() { | ||
| if policy.is_authorized(env, signer_key, contexts) { | ||
| policy.on_authorized(env, signer_key, contexts); | ||
| return true; | ||
| authorized = true; |
There was a problem hiding this comment.
When a signer has duplicate policy identities (e.g. two TokenTransferPolicy entries sharing the same policy_id), this loop can call on_authorized multiple times in one auth pass. Because token spend tracking is keyed by policy_id, one transfer can be recorded twice (or more), prematurely exhausting limits and making authorization/order effects depend on vector ordering. This regression appears after removing the early return; consider deduplicating policies on add/update or ensuring each policy identity is committed at most once per authorization.
Useful? React with 👍 / 👎.
1 participant
Summary
Two interlocking fixes for a Standard-signer authorization-bypass and a partial-state-commit hazard found during security review.
SignerPolicyvariants on Standard signers. A signer's policy vector must contain a single kind — either allExternalValidatorPolicyor allTokenTransferPolicy. Mixing them letSignerRole::is_authorized's OR-short-circuit authorize through a permissive external policy while a strictTokenTransferPolicycap was never consulted (itsis_authorized/on_authorizedwere skipped).on_authorizedfor every matching policy. With the homogeneity gate in place, OR remains the decision rule (multi-currency configurations still work) but the loop no longer short-circuits on the first match — every policy that authorizes commits its state. Overlapping same-token policies (e.g. daily + monthly cap) now both advance their trackers.Why this shape
A blanket switch to AND semantics would have broken the multi-currency use case the existing tests lock in (
test_multi_policy_usdc_transfer_passesetc.). Locking out the dangerous combination at registration is a minimal change that preserves the working configurations and removes the bypass surface.The full design discussion (alternatives B/C/D — strict AND, tri-state policies, explicit composition) is deferred to a separate RFC. This PR is the safe, minimal fix.
Test plan
test_mixed_policy_variants_rejected_at_constructor—[ExternalPolicy, TokenTransferPolicy]panics withInvalidPolicy (#80)at register time.test_mixed_policy_variants_rejected_on_update_signer— same viaupdate_signer.test_two_token_transfer_policies_same_variant_allowed— multi-currency use case remains valid.test_two_external_policies_same_variant_allowed— defense-in-depth external-only stack still allowed.test_overlapping_token_policies_both_commit_spend— regression guard for the commit-every-match change; both same-token tracker entries advance.cargo test -p smart-accountreports 131 passed, 0 failed.Backwards compatibility
__check_authstill works (auth path doesn't re-validate the signer set on every call). Onlyadd_signer/update_signerenforce the new rule, which acts as upgrade pressure to a safe configuration. If any production accounts are in this state, an enumeration script can find them viaget_signerover known signer keys.convert_roleinmigration/v1_to_v2.rsalready producesSome(vec![external_only])orNone— always homogeneous. No migration changes needed.Out of scope
is_authorized(Allow/Deny/NotApplicable), tracked separately.on_authorizedcalls per auth (gas concern). Common configurations have ≤ 2 policies; abuse is admin-self-paid.handle_policy_set_changes(update-signer policy diff) is unchanged — the new validator runs ahead of it.