fix(agent-vault): add no-op rotation rejection for admin and orchestr…

contracts/agent-vault/src/lib.rs

@@ -132,6 +132,7 @@ pub enum VaultError {
     NotYourTask = 16,
     NotYourOrchestrator = 17,
     TooManyActiveTasks = 18,
+    NoChange = 19,
 }
 
 // Storage keys
@@ -567,6 +568,7 @@ impl AgentVault {
 
     /// Update the registered orchestrator for a user. Requires no active tasks so
     /// in-flight task authorization cannot be stranded on the old orchestrator.
+    /// Rejects if new_orchestrator equals the current orchestrator (no-op check).
     pub fn update_orchestrator(
         env: Env,
         user: Address,
@@ -581,7 +583,6 @@ impl AgentVault {
             .persistent()
             .get(&config_key)
             .ok_or(VaultError::OrchestratorNotRegistered)?;
-        Self::extend_persistent_ttl(&env, &config_key);
 
         if config.active_tasks_count != 0 {
             return Err(VaultError::ActiveTaskExists);
@@ -594,12 +595,20 @@ impl AgentVault {
 
         let new_owner_key = DataKey::OrchestratorOwner(new_orchestrator.clone());
         if let Some(existing_owner) = env.storage().persistent().get::<_, Address>(&new_owner_key) {
-            Self::extend_persistent_ttl(&env, &new_owner_key);
             if existing_owner != user {
                 return Err(VaultError::OrchestratorAlreadyRegistered);
             }
         }
 
+        // Reject no-op rotation: new_orchestrator must differ from old orchestrator
+        // This check comes BEFORE any TTL refreshes or state mutations
+        if new_orchestrator == old_orchestrator {
+            return Err(VaultError::NoChange);
+        }
+
+        // All validations passed; now refresh TTLs and perform state writes
+        Self::extend_persistent_ttl(&env, &config_key);
+
         let old_owner_key = DataKey::OrchestratorOwner(old_orchestrator.clone());
         env.storage().persistent().remove(&old_owner_key);
 
@@ -1226,7 +1235,7 @@ impl AgentVault {
         result
     }
 
-    // ── Pause / Unpause ─────────────────────────────────────────────────
+    // Pause / Unpause
 
     /// Pauses the contract, blocking deposit, create_task, and release_payment.
     pub fn pause(env: Env, admin: Address) -> Result<(), VaultError> {
@@ -1281,7 +1290,7 @@ impl AgentVault {
         paused
     }
 
-    // ── Stale Task Threshold Management ────────────────────────────────
+    // Stale Task Threshold Management
 
     /// Admin updates the threshold (in seconds) after which a task is considered stale.
     pub fn set_stale_threshold(env: Env, admin: Address, seconds: u64) -> Result<(), VaultError> {
@@ -1319,7 +1328,7 @@ impl AgentVault {
         threshold
     }
 
-    // ── Max Active Tasks Management ────────────────────────────────────
+    // Max Active Tasks Management
 
     /// Admin updates the cap on concurrent active tasks a single user may hold.
     pub fn set_max_active_tasks(env: Env, admin: Address, max: u32) -> Result<(), VaultError> {
@@ -1356,6 +1365,7 @@ impl AgentVault {
     }
 
     /// Rotates the admin key. Only the current admin can call this.
+    /// Rejects if new_admin equals the current admin (no-op check).
     /// Emits UpdateAdminEvent on success.
     pub fn update_admin(env: Env, admin: Address, new_admin: Address) -> Result<(), VaultError> {
         admin.require_auth();
@@ -1367,10 +1377,14 @@ impl AgentVault {
         if admin != stored_admin {
             return Err(VaultError::Unauthorized);
         }
+        // Reject no-op rotation: new_admin must differ from current admin
+        if new_admin == stored_admin {
+            return Err(VaultError::NoChange);
+        }
         env.storage().instance().set(&DataKey::Admin, &new_admin);
         Self::extend_instance_ttl(&env);
         UpdateAdminEvent {
-            old_admin: admin,
+            old_admin: stored_admin,
             new_admin,
         }
         .publish(&env);

contracts/agent-vault/src/tests.rs

@@ -1,4 +1,5 @@
-use crate::{AgentVault, AgentVaultClient, DataKey, VaultError};
+ use crate::{AgentVault, AgentVaultClient, VaultError};
+use crate::{AgentVault, AgentVaultClient, VaultError};
 use soroban_sdk::testutils::storage::Persistent as _;
 use soroban_sdk::testutils::{Address as _, Events, Ledger as _};
 use soroban_sdk::{token, Address, Env};
@@ -2056,8 +2057,7 @@ fn test_lowering_cap_below_current_count_does_not_affect_existing_tasks() {
     let third_task_id = t.client.create_task(&orchestrator, &t.usdc_sac, &100);
     assert_eq!(third_task_id, 3);
 }
-
-// ── Admin key rotation tests ─────────────────────────────────────────────────
+// Admin key rotation tests
 
 /// Positive: admin rotates to new_admin successfully.
 #[test]
@@ -2641,3 +2641,158 @@ fn test_version_returns_contract_version() {
     let test_env = setup_test();
     assert_eq!(test_env.client.version(), 2);
 }
+
+
+// No-op rotation rejection tests
+
+/// Verify that no-op admin rotation is rejected without state changes.
+#[test]
+fn test_update_admin_rejects_no_op_rotation() {
+    let t = setup_test();
+    t.client.init(&t.admin, &t.usdc_sac);
+
+    // Store admin before attempting no-op
+    let before = t.client.get_admin();
+    assert_eq!(before, t.admin);
+
+    // Attempt to rotate to the same admin - should return NoChange error
+    let result = t.client.try_update_admin(&t.admin, &t.admin);
+    assert!(result == Err(Ok(VaultError::NoChange)));
+
+    // Verify admin is unchanged
+    let after = t.client.get_admin();
+    assert_eq!(after, before);
+}
+
+/// Verify that no-op orchestrator rotation is rejected without state changes.
+#[test]
+fn test_update_orchestrator_rejects_no_op_rotation() {
+    let test_env = setup_test();
+    test_env.client.init(&test_env.admin, &test_env.usdc_sac);
+
+    let user = Address::generate(&test_env.env);
+    let orchestrator = Address::generate(&test_env.env);
+    let name = soroban_sdk::String::from_str(&test_env.env, "MyOrchestrator");
+
+    // Register orchestrator first
+    test_env
+        .client
+        .register_orchestrator(&user, &orchestrator, &name);
+
+    // Store state before attempting no-op
+    let config_before = test_env.client.get_user_config(&user).unwrap();
+    assert_eq!(config_before.orchestrator, Some(orchestrator.clone()));
+    assert_eq!(config_before.orchestrator_name, name.clone());
+
+    // Attempt to update to the same orchestrator - should return NoChange error
+    let result = test_env
+        .client
+        .try_update_orchestrator(&user, &orchestrator, &name);
+    assert!(result == Err(Ok(VaultError::NoChange)));
+
+    // Verify orchestrator is unchanged
+    let config_after = test_env.client.get_user_config(&user).unwrap();
+    assert_eq!(config_after.orchestrator, Some(orchestrator.clone()));
+    assert_eq!(config_after.orchestrator_name, name);
+}
+
+/// Positive: legitimate admin rotation still works after no-op check.
+#[test]
+fn test_update_admin_legitimate_rotation_still_works() {
+    let t = setup_test();
+    t.client.init(&t.admin, &t.usdc_sac);
+    let new_admin = Address::generate(&t.env);
+    let another_admin = Address::generate(&t.env);
+
+    // First rotation succeeds
+    t.client.update_admin(&t.admin, &new_admin);
+    assert_eq!(t.client.get_admin(), new_admin);
+
+    // Second legitimate rotation also succeeds
+    t.client.update_admin(&new_admin, &another_admin);
+    assert_eq!(t.client.get_admin(), another_admin);
+}
+
+/// Positive: legitimate orchestrator rotation still works after no-op check.
+#[test]
+fn test_update_orchestrator_legitimate_rotation_still_works() {
+    let test_env = setup_test();
+    test_env.client.init(&test_env.admin, &test_env.usdc_sac);
+
+    let user = Address::generate(&test_env.env);
+    let old_orchestrator = Address::generate(&test_env.env);
+    let new_orchestrator = Address::generate(&test_env.env);
+    let old_name = soroban_sdk::String::from_str(&test_env.env, "OldOrchestrator");
+    let new_name = soroban_sdk::String::from_str(&test_env.env, "NewOrchestrator");
+
+    // Register original orchestrator
+    test_env
+        .client
+        .register_orchestrator(&user, &old_orchestrator, &old_name);
+
+    // First legitimate rotation succeeds
+    test_env
+        .client
+        .update_orchestrator(&user, &new_orchestrator, &new_name);
+    let config = test_env.client.get_user_config(&user).unwrap();
+    assert_eq!(config.orchestrator, Some(new_orchestrator.clone()));
+    assert_eq!(config.orchestrator_name, new_name);
+
+    // Second legitimate rotation also succeeds (update to a different orchestrator)
+    let another_orchestrator = Address::generate(&test_env.env);
+    let another_name = soroban_sdk::String::from_str(&test_env.env, "AnotherOrchestrator");
+    test_env
+        .client
+        .update_orchestrator(&user, &another_orchestrator, &another_name);
+    let config2 = test_env.client.get_user_config(&user).unwrap();
+    assert_eq!(config2.orchestrator, Some(another_orchestrator.clone()));
+    assert_eq!(config2.orchestrator_name, another_name);
+}
+
+/// Verify that legitimate admin rotation emits exactly one event.
+#[test]
+fn test_update_admin_legitimate_rotation_emits_event() {
+    let t = setup_test();
+    t.client.init(&t.admin, &t.usdc_sac);
+    let new_admin = Address::generate(&t.env);
+
+    // Clear any init events
+    let _ = t.env.events().all();
+
+    // Perform legitimate rotation
+    t.client.update_admin(&t.admin, &new_admin);
+
+    // Verify exactly one event was emitted
+    let events = t.env.events().all();
+    assert_eq!(events.events().len(), 1);
+}
+
+/// Verify that legitimate orchestrator rotation emits exactly one event.
+#[test]
+fn test_update_orchestrator_legitimate_rotation_emits_event() {
+    let test_env = setup_test();
+    test_env.client.init(&test_env.admin, &test_env.usdc_sac);
+
+    let user = Address::generate(&test_env.env);
+    let orchestrator = Address::generate(&test_env.env);
+    let name = soroban_sdk::String::from_str(&test_env.env, "MyOrchestrator");
+
+    test_env
+        .client
+        .register_orchestrator(&user, &orchestrator, &name);
+
+    // Clear any registration events
+    let _ = test_env.env.events().all();
+
+    let new_orchestrator = Address::generate(&test_env.env);
+    let new_name = soroban_sdk::String::from_str(&test_env.env, "NewOrchestrator");
+
+    // Perform legitimate rotation
+    test_env
+        .client
+        .update_orchestrator(&user, &new_orchestrator, &new_name);
+
+    // Verify exactly one event was emitted
+    let events = test_env.env.events().all();
+    assert_eq!(events.events().len(), 1);
+}