fix: enforce homogeneous policies and commit every matching policy

README.md

@@ -91,14 +91,32 @@ cargo test
 | Role | Capabilities | Use Cases |
 |------|-------------|-----------|
 | **Admin** | Full access, can upgrade contracts | System administrators, emergency access |
-| **Standard** | Normal operations, cannot modify signers, optional policy restrictions | Regular users, application accounts, AI agents with policies |
+| **Standard** | Normal operations, cannot modify signers, optional policy restrictions (see [Policy semantics](#policy-semantics)) | Regular users, application accounts, AI agents with policies |
 
 ### Policy Types
 
 - **External Delegation**: Delegate authorization decisions to external policy contracts
 - **Token Transfer Policy**: Restrict signers to specific token transfers with cumulative spending limits, reset windows, recipient allowlists, and per-policy expiration
 - **Extensible**: Add custom policies by implementing the `AuthorizationCheck` and `PolicyCallback` traits
 
+### Policy semantics
+
+A Standard signer's `policies` vector must contain a single kind of policy
+(all `TokenTransferPolicy` or all `ExternalValidatorPolicy`); mixing kinds
+is rejected at `add_signer` / `update_signer` time with `InvalidPolicy`.
+
+Within a homogeneous set, policies are evaluated with **OR semantics** —
+the signer is authorized if **any** policy allows the operation. Every
+matching policy's `on_authorized` hook fires, so state trackers
+(e.g. spending counters) advance for **all** matching policies, not just
+the first.
+
+This is a deliberate design choice for the current release. It supports
+natural multi-currency configurations (e.g. one `TokenTransferPolicy` per
+token) while preventing the dangerous pattern of stacking a permissive
+external policy with a restrictive token cap. A future release will
+introduce a richer composition model.
+
 ### Signer Expiration
 
 Standard signers can have an expiration timestamp. Once the ledger timestamp exceeds the expiration, the signer is rejected. A value of `0` means no expiration.

contracts/smart-account/src/account.rs

@@ -159,6 +159,9 @@ impl SmartAccountInterface for SmartAccount {
                 return Err(SmartAccountError::InvalidPolicy);
             }
         }
+        if let SignerRole::Standard(ref policies, _) = signer.role() {
+            Self::validate_homogeneous_policies(policies)?;
+        }
 
         let key = signer.clone().into();
         let storage = Storage::persistent();
@@ -194,6 +197,9 @@ impl SmartAccountInterface for SmartAccount {
                 return Err(SmartAccountError::InvalidPolicy);
             }
         }
+        if let SignerRole::Standard(ref policies, _) = signer.role() {
+            Self::validate_homogeneous_policies(policies)?;
+        }
 
         let key = signer.clone().into();
         let storage = Storage::persistent();
@@ -414,6 +420,37 @@ impl SmartAccount {
         Ok(())
     }
 
+    /// Ensures a Standard signer's policy vector contains only one variant.
+    ///
+    /// Mixing `ExternalValidatorPolicy` with `TokenTransferPolicy` is rejected:
+    /// the OR-short-circuit in `SignerRole::is_authorized` would let the
+    /// permissive policy authorize a transaction the strict policy intended
+    /// to cap, and the strict policy's spending tracker would never observe
+    /// the spend. Same-variant sets (multi-token caps, multiple external
+    /// validators) remain allowed.
+    fn validate_homogeneous_policies(
+        policies: &Option<Vec<SignerPolicy>>,
+    ) -> Result<(), SmartAccountError> {
+        let Some(policies) = policies else {
+            return Ok(());
+        };
+        if policies.len() <= 1 {
+            return Ok(());
+        }
+
+        let first_is_external = matches!(
+            policies.get(0).unwrap(),
+            SignerPolicy::ExternalValidatorPolicy(_)
+        );
+        for policy in policies.iter() {
+            let is_external = matches!(policy, SignerPolicy::ExternalValidatorPolicy(_));
+            if is_external != first_is_external {
+                return Err(SmartAccountError::InvalidPolicy);
+            }
+        }
+        Ok(())
+    }
+
     /// Validates multisig signer configuration
     fn validate_multisig(env: &Env, multisig: &MultisigSigner) -> Result<(), SmartAccountError> {
         if multisig.members.is_empty() || multisig.threshold == 0 {

contracts/smart-account/src/auth/permissions.rs

@@ -99,15 +99,21 @@ impl AuthorizationCheck for SignerRole {
                     match policies {
                         // No policies = no restrictions (beyond admin check)
                         None => true,
-                        // At least one policy must authorize the full transaction
+                        // OR semantics: at least one policy must authorize.
+                        // Every matching policy's on_authorized fires so that
+                        // overlapping policies (e.g. multiple TokenTransferPolicy
+                        // entries on the same token) all commit their state.
+                        // Mixed-variant policy sets are rejected at registration
+                        // (see SmartAccount::validate_homogeneous_policies).
                         Some(policies) => {
+                            let mut authorized = false;
                             for policy in policies.iter() {
                                 if policy.is_authorized(env, signer_key, contexts) {
                                     policy.on_authorized(env, signer_key, contexts);
-                                    return true;
+                                    authorized = true;
                                 }
                             }
-                            false
+                            authorized
                         }
                     }
                 }

contracts/smart-account/src/tests/policy_test.rs

@@ -2,15 +2,17 @@ use crate::tests::test_utils::TestSignerTrait as _;
 use smart_account_interfaces::Ed25519Signer;
 pub use smart_account_interfaces::SmartAccountInterface;
 use soroban_sdk::auth::Context;
-use soroban_sdk::testutils::Events;
+use soroban_sdk::testutils::{Address as _, BytesN as _, Events};
 use soroban_sdk::{
-    contract, contractimpl, symbol_short, vec, Address, Env, Symbol, TryFromVal, Vec,
+    contract, contractimpl, symbol_short, vec, Address, BytesN, Env, Symbol, TryFromVal, Vec,
 };
 
 use crate::account::SmartAccount;
 use crate::error::Error;
 use crate::tests::test_utils::{setup, Ed25519TestSigner};
-use smart_account_interfaces::{ExternalPolicy, Signer, SignerKey, SignerPolicy, SignerRole};
+use smart_account_interfaces::{
+    ExternalPolicy, Signer, SignerKey, SignerPolicy, SignerRole, TokenTransferPolicy,
+};
 
 #[contract]
 pub struct DummyExternalPolicy;
@@ -213,3 +215,131 @@ fn test_update_signer_with_external_polic_lifecycle_with_same_policy() {
     .unwrap();
     ensure_policy_event_is_not_emmited(&env, policy_id_1.clone(), symbol_short!("ON_REVOKE"));
 }
+
+// ============================================================================
+// Homogeneous policies — mixing TokenTransferPolicy with ExternalValidatorPolicy
+// is rejected at registration to prevent the OR-short-circuit from letting a
+// permissive external policy hide a strict spending cap.
+// ============================================================================
+
+fn make_ttp(env: &Env) -> TokenTransferPolicy {
+    TokenTransferPolicy {
+        policy_id: BytesN::random(env),
+        token: Address::generate(env),
+        limit: Some(100),
+        reset_window_secs: 0,
+        allowed_recipients: None,
+        expiration: 0,
+    }
+}
+
+#[test]
+#[should_panic(expected = "#80")]
+fn test_mixed_policy_variants_rejected_at_constructor() {
+    let env = setup();
+    let ext_addr = env.register(DummyExternalPolicy, ());
+    let ext = SignerPolicy::ExternalValidatorPolicy(ExternalPolicy {
+        policy_address: ext_addr,
+    });
+    let ttp = SignerPolicy::TokenTransferPolicy(make_ttp(&env));
+
+    let admin = Ed25519TestSigner::generate(SignerRole::Admin).into_signer(&env);
+    let mixed = Ed25519TestSigner::generate(SignerRole::Standard(
+        Some(vec![&env, ext, ttp]),
+        0,
+    ));
+    env.register(
+        SmartAccount,
+        (
+            vec![&env, admin, mixed.into_signer(&env)],
+            Vec::<Address>::new(&env),
+        ),
+    );
+}
+
+#[test]
+fn test_mixed_policy_variants_rejected_on_update_signer() {
+    let env = setup();
+    let ext_addr = env.register(DummyExternalPolicy, ());
+    let ext = SignerPolicy::ExternalValidatorPolicy(ExternalPolicy {
+        policy_address: ext_addr.clone(),
+    });
+
+    // Start homogeneous: external-only.
+    let admin = Ed25519TestSigner::generate(SignerRole::Admin).into_signer(&env);
+    let standard =
+        Ed25519TestSigner::generate(SignerRole::Standard(Some(vec![&env, ext.clone()]), 0));
+    let standard_signer = standard.into_signer(&env);
+    let contract_id = env.register(
+        SmartAccount,
+        (
+            vec![&env, admin, standard_signer.clone()],
+            Vec::<Address>::new(&env),
+        ),
+    );
+    env.mock_all_auths();
+
+    // Now try to update with a mixed set — must fail with InvalidPolicy.
+    let ttp = SignerPolicy::TokenTransferPolicy(make_ttp(&env));
+    let pubkey = match standard_signer {
+        Signer::Ed25519(ref s, _) => s.public_key.clone(),
+        _ => unreachable!(),
+    };
+    let mixed_update = Signer::Ed25519(
+        Ed25519Signer::new(pubkey),
+        SignerRole::Standard(Some(vec![&env, ext, ttp]), 0),
+    );
+
+    let err = env
+        .as_contract(&contract_id, || {
+            SmartAccount::update_signer(&env, mixed_update)
+        })
+        .unwrap_err();
+    assert_eq!(err, Error::InvalidPolicy);
+}
+
+#[test]
+fn test_two_token_transfer_policies_same_variant_allowed() {
+    let env = setup();
+    // Two TokenTransferPolicy entries — multi-currency use case still works.
+    let ttp1 = SignerPolicy::TokenTransferPolicy(make_ttp(&env));
+    let ttp2 = SignerPolicy::TokenTransferPolicy(make_ttp(&env));
+
+    let admin = Ed25519TestSigner::generate(SignerRole::Admin).into_signer(&env);
+    let standard = Ed25519TestSigner::generate(SignerRole::Standard(
+        Some(vec![&env, ttp1, ttp2]),
+        0,
+    ));
+    // Should register without panicking.
+    env.register(
+        SmartAccount,
+        (
+            vec![&env, admin, standard.into_signer(&env)],
+            Vec::<Address>::new(&env),
+        ),
+    );
+}
+
+#[test]
+fn test_two_external_policies_same_variant_allowed() {
+    let env = setup();
+    let ext1 = SignerPolicy::ExternalValidatorPolicy(ExternalPolicy {
+        policy_address: env.register(DummyExternalPolicy, ()),
+    });
+    let ext2 = SignerPolicy::ExternalValidatorPolicy(ExternalPolicy {
+        policy_address: env.register(DummyExternalPolicy, ()),
+    });
+
+    let admin = Ed25519TestSigner::generate(SignerRole::Admin).into_signer(&env);
+    let standard = Ed25519TestSigner::generate(SignerRole::Standard(
+        Some(vec![&env, ext1, ext2]),
+        0,
+    ));
+    env.register(
+        SmartAccount,
+        (
+            vec![&env, admin, standard.into_signer(&env)],
+            Vec::<Address>::new(&env),
+        ),
+    );
+}

contracts/smart-account/src/tests/token_transfer_policy_test.rs

@@ -928,3 +928,68 @@ fn test_updating_allowed_recipients_preserves_spending_tracker() {
     let contexts = vec![&env, make_transfer_context(&env, &token, &allowed_2, 300)];
     check_auth(&env, &contract_id, &signer, &contexts).unwrap();
 }
+
+// ============================================================================
+// Regression: overlapping TokenTransferPolicy entries on the same token must
+// each have their `on_authorized` fire so every spending tracker advances.
+// Before the commit-every-match fix, only the FIRST matching policy committed.
+// ============================================================================
+
+#[test]
+fn test_overlapping_token_policies_both_commit_spend() {
+    let env = setup();
+    let token = Address::generate(&env);
+
+    let policy_a = TokenTransferPolicy {
+        policy_id: BytesN::random(&env),
+        token: token.clone(),
+        limit: Some(10_000),
+        reset_window_secs: 0,
+        allowed_recipients: None,
+        expiration: 0,
+    };
+    let policy_b = TokenTransferPolicy {
+        policy_id: BytesN::random(&env),
+        token: token.clone(),
+        limit: Some(10_000),
+        reset_window_secs: 0,
+        allowed_recipients: None,
+        expiration: 0,
+    };
+
+    let admin_signer = Ed25519TestSigner::generate(SignerRole::Admin).into_signer(&env);
+    let signer_policies = vec![
+        &env,
+        SignerPolicy::TokenTransferPolicy(policy_a.clone()),
+        SignerPolicy::TokenTransferPolicy(policy_b.clone()),
+    ];
+    let standard_signer = Ed25519TestSigner::generate(SignerRole::Standard(
+        Some(signer_policies),
+        0,
+    ));
+    let contract_id = env.register(
+        SmartAccount,
+        (
+            vec![&env, admin_signer, standard_signer.into_signer(&env)],
+            Vec::<Address>::new(&env),
+        ),
+    );
+
+    let to = Address::generate(&env);
+    let contexts = vec![&env, make_transfer_context(&env, &token, &to, 400)];
+    check_auth(&env, &contract_id, &standard_signer, &contexts).unwrap();
+
+    let signer_key = SignerKey::Ed25519(standard_signer.public_key(&env));
+    let key_a = SpendTrackerKey::TokenSpend(policy_a.policy_id, signer_key.clone());
+    let key_b = SpendTrackerKey::TokenSpend(policy_b.policy_id, signer_key);
+
+    env.as_contract(&contract_id, || {
+        let a: SpendingTracker = env.storage().persistent().get(&key_a).unwrap();
+        let b: SpendingTracker = env.storage().persistent().get(&key_b).unwrap();
+        assert_eq!(a.spent, 400, "policy A tracker should reflect the spend");
+        assert_eq!(
+            b.spent, 400,
+            "policy B tracker must ALSO reflect the spend (regression)"
+        );
+    });
+}