diff --git a/contracts/agent-vault/src/lib.rs b/contracts/agent-vault/src/lib.rs index 57f5b6e..a41568e 100644 --- a/contracts/agent-vault/src/lib.rs +++ b/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); diff --git a/contracts/agent-vault/src/tests.rs b/contracts/agent-vault/src/tests.rs index 27e0e51..62664e2 100644 --- a/contracts/agent-vault/src/tests.rs +++ b/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); +}