From 5ad9decd57f53671d9e6e4f6383a8051b0cc1ab4 Mon Sep 17 00:00:00 2001 From: tecch-wiz Date: Sun, 6 Sep 2026 18:50:25 +0100 Subject: [PATCH 1/2] fix(registry): tighten admin and metadata behavior --- contracts/pool/src/test.rs | 10 +++ contracts/registry/src/events.rs | 5 -- contracts/registry/src/lib.rs | 105 +++++++++++++------------------ contracts/registry/src/test.rs | 82 +++++++++++++++++++++++- 4 files changed, 133 insertions(+), 69 deletions(-) diff --git a/contracts/pool/src/test.rs b/contracts/pool/src/test.rs index c3e24554..a37ddf4b 100644 --- a/contracts/pool/src/test.rs +++ b/contracts/pool/src/test.rs @@ -1223,6 +1223,16 @@ fn test_fund_invoice_rejects_above_cap() { te.pool.fund_invoice(&invoice_id); } +#[test] +#[should_panic(expected = "Error(Contract, #12)")] +fn test_fund_invoice_rejects_when_utilization_cap_is_zero() { + let te = setup(); + te.pool.set_max_utilization(&te.admin, &0); + te.pool.deposit(&te.lp, &10_000_000_000); + let invoice_id = create_and_list(&te, &te.usdc_id); + te.pool.fund_invoice(&invoice_id); +} + #[test] fn test_fund_invoice_allowed_when_below_cap() { let te = setup(); diff --git a/contracts/registry/src/events.rs b/contracts/registry/src/events.rs index da433bca..fb775fe4 100644 --- a/contracts/registry/src/events.rs +++ b/contracts/registry/src/events.rs @@ -17,11 +17,6 @@ pub fn buyer_registered(env: &Env, address: &Address) { .publish((Symbol::new(env, "buyer_registered"), address.clone()), ()); } -pub fn metadata_updated(env: &Env, address: &Address) { - env.events() - .publish((Symbol::new(env, "metadata_updated"), address.clone()), ()); -} - pub fn address_revoked(env: &Env, address: &Address) { env.events() .publish((Symbol::new(env, "address_revoked"), address.clone()), ()); diff --git a/contracts/registry/src/lib.rs b/contracts/registry/src/lib.rs index 6c6827e9..6396a5d0 100644 --- a/contracts/registry/src/lib.rs +++ b/contracts/registry/src/lib.rs @@ -120,11 +120,7 @@ impl RegistryContract { panic_with_error!(&env, RegistryError::BatchSizeExceeded); } - let admin: Address = env - .storage() - .instance() - .get(&DataKey::Admin) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + let admin = Self::require_admin(&env); admin.require_auth(); // Pre-validate ALL entries' metadata before processing any of them. @@ -181,7 +177,7 @@ impl RegistryContract { /// /// # Panics /// * `RegistryError::BatchSizeExceeded` if `entries.len() > 50`. - /// * `RegistryError::NotFound` if the contract admin is not set. + /// * `RegistryError::NotInitialized` if the contract admin is not set. /// /// # Returns /// * `Vec
` - The list of addresses that were skipped (already @@ -194,11 +190,7 @@ impl RegistryContract { panic_with_error!(&env, RegistryError::BatchSizeExceeded); } - let admin: Address = env - .storage() - .instance() - .get(&DataKey::Admin) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + let admin = Self::require_admin(&env); admin.require_auth(); let mut skipped: Vec
= Vec::new(&env); @@ -329,6 +321,7 @@ impl RegistryContract { .persistent() .extend_ttl(&key, TTL_THRESHOLD, TTL_EXTEND_TO); events::profile_updated(&env, &address); + Self::extend_instance_ttl(&env); true } @@ -347,7 +340,7 @@ impl RegistryContract { /// `MAX_METADATA_SIZE` entries, contains an empty key or value, or has a /// key longer than `MAX_METADATA_KEY_LEN` or a value longer than /// `MAX_METADATA_VALUE_LEN`. - /// * `RegistryError::NotFound` if the address is not registered. + /// * `RegistryError::NotRegistered` if the address is not registered. /// /// # Example /// ```ignore @@ -361,13 +354,13 @@ impl RegistryContract { .storage() .persistent() .get(&key) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotRegistered)); profile.metadata = metadata; env.storage().persistent().set(&key, &profile); env.storage() .persistent() .extend_ttl(&key, TTL_THRESHOLD, TTL_EXTEND_TO); - events::metadata_updated(&env, &address); + events::profile_updated(&env, &address); Self::extend_instance_ttl(&env); true } @@ -485,11 +478,7 @@ impl RegistryContract { /// let result = client.revoke(&issuer); /// ``` pub fn revoke(env: Env, address: Address) -> bool { - let admin: Address = env - .storage() - .instance() - .get(&DataKey::Admin) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + let admin = Self::require_admin(&env); admin.require_auth(); let key = DataKey::Profile(address.clone()); let mut profile: Profile = env @@ -528,8 +517,8 @@ impl RegistryContract { /// (read from `DataKey::Admin`) may reinstate a profile. /// /// # Panics - /// * `RegistryError::NotFound` if the contract admin is not set (contract - /// was never initialized). + /// * `RegistryError::NotInitialized` if the contract admin is not set + /// (contract was never initialized). /// * `RegistryError::NotFound` if no profile is stored for `address`. /// /// # Returns @@ -540,11 +529,7 @@ impl RegistryContract { /// let ok = client.reinstate(&issuer); /// ``` pub fn reinstate(env: Env, address: Address) -> bool { - let admin: Address = env - .storage() - .instance() - .get(&DataKey::Admin) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + let admin = Self::require_admin(&env); admin.require_auth(); let key = DataKey::Profile(address.clone()); let mut profile: Profile = env @@ -564,11 +549,7 @@ impl RegistryContract { } pub fn verify_profile(env: Env, address: Address, verify: bool) -> bool { - let admin: Address = env - .storage() - .instance() - .get(&DataKey::Admin) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + let admin = Self::require_admin(&env); admin.require_auth(); let key = DataKey::Profile(address.clone()); let mut profile: Profile = env @@ -591,28 +572,22 @@ impl RegistryContract { true } + /// Transfers admin ownership to a new address. + /// + /// Requires authentication from both the current admin and the incoming + /// new admin, preventing accidental transfers to a wrong address. Callers + /// that intentionally need unilateral key rotation may use + /// [`transfer_admin`](Self::transfer_admin), but it does not provide this + /// dual-authorization safety guarantee. + + /// # Arguments + /// * `env` - The Soroban environment. + /// * `new_admin` - The address that will become the new admin. + /// + /// # Panics + /// * `RegistryError::NotInitialized` if the contract has not been initialized. pub fn transfer_ownership(env: Env, new_admin: Address) { - // Transfers admin ownership to a new address. - // - // Requires authentication from BOTH the current admin and the incoming - // new admin, preventing accidental transfers to wrong addresses. - // - // # Arguments - // * `env` - The Soroban environment. - // * `new_admin` - The address that will become the new admin. - // - // # Panics - // * `NotFound` if the admin is not set. - // - // # Example - // ```ignore - // client.transfer_ownership(&new_admin); - // ``` - let admin: Address = env - .storage() - .instance() - .get(&DataKey::Admin) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + let admin = Self::require_admin(&env); admin.require_auth(); new_admin.require_auth(); env.storage().instance().set(&DataKey::Admin, &new_admin); @@ -624,9 +599,12 @@ impl RegistryContract { /// /// Unlike `transfer_ownership`, this function only requires auth from the /// current admin — the new admin does not need to sign. This is useful - /// for key rotation scenarios where the current admin key may be - /// compromised or needs to be rotated without the new key holder's - /// involvement. + /// for key rotation scenarios where the new key holder's involvement is + /// unavailable. This is an intentional lower-security escape hatch: the + /// current admin can use it to bypass `transfer_ownership`'s dual-auth + /// guarantee, including transferring to an address that never authorized + /// the transfer. Integrators requiring that safety property must use + /// `transfer_ownership` and ensure both signatures are collected. /// /// # Arguments /// * `env` - The Soroban environment. @@ -637,8 +615,8 @@ impl RegistryContract { /// contract admin may call this function. /// /// # Panics - /// * `RegistryError::NotFound` if the contract has not been initialized - /// (no admin is stored under `DataKey::Admin`). + /// * `RegistryError::NotInitialized` if the contract has not been + /// initialized. /// /// # Returns /// * `()` - No value is returned. @@ -648,11 +626,7 @@ impl RegistryContract { /// client.transfer_admin(&new_admin); /// ``` pub fn transfer_admin(env: Env, new_admin: Address) { - let admin: Address = env - .storage() - .instance() - .get(&DataKey::Admin) - .unwrap_or_else(|| panic_with_error!(&env, RegistryError::NotFound)); + let admin = Self::require_admin(&env); admin.require_auth(); env.storage().instance().set(&DataKey::Admin, &new_admin); events::admin_transferred(&env, &admin, &new_admin); @@ -696,6 +670,13 @@ impl RegistryContract { } impl RegistryContract { + fn require_admin(env: &Env) -> Address { + env.storage() + .instance() + .get(&DataKey::Admin) + .unwrap_or_else(|| panic_with_error!(env, RegistryError::NotInitialized)) + } + fn require_initialized(env: &Env) { if !env.storage().instance().has(&DataKey::Admin) { panic_with_error!(env, RegistryError::NotInitialized); diff --git a/contracts/registry/src/test.rs b/contracts/registry/src/test.rs index 94674428..5b11e280 100644 --- a/contracts/registry/src/test.rs +++ b/contracts/registry/src/test.rs @@ -93,6 +93,41 @@ fn test_register_buyer_before_initialize_panics() { client.register_buyer(&Address::generate(&env), &map![&env]); } +#[test] +#[should_panic(expected = "Error(Contract, #4)")] +fn test_batch_register_issuers_before_initialize_panics() { + let (env, client) = setup(); + client.batch_register_issuers(&vec![&env]); +} + +#[test] +#[should_panic(expected = "Error(Contract, #4)")] +fn test_revoke_before_initialize_panics() { + let (env, client) = setup(); + client.revoke(&Address::generate(&env)); +} + +#[test] +#[should_panic(expected = "Error(Contract, #4)")] +fn test_reinstate_before_initialize_panics() { + let (env, client) = setup(); + client.reinstate(&Address::generate(&env)); +} + +#[test] +#[should_panic(expected = "Error(Contract, #4)")] +fn test_verify_profile_before_initialize_panics() { + let (env, client) = setup(); + client.verify_profile(&Address::generate(&env), &true); +} + +#[test] +#[should_panic(expected = "Error(Contract, #4)")] +fn test_transfer_ownership_before_initialize_panics() { + let (env, client) = setup(); + client.transfer_ownership(&Address::generate(&env)); +} + #[test] fn test_is_verified_returns_false_for_registered_but_unverified() { let (env, client) = setup(); @@ -355,7 +390,7 @@ fn test_update_metadata_self_succeeds() { } #[test] -#[should_panic(expected = "Error(Contract, #3)")] +#[should_panic(expected = "Error(Contract, #7)")] fn test_update_metadata_unregistered_panics() { let (env, client) = setup(); let admin = Address::generate(&env); @@ -1182,6 +1217,49 @@ fn test_transfer_admin_changes_admin() { assert_eq!(client.get_admin(), new_admin); } +#[test] +fn test_transfer_admin_bypasses_transfer_ownership_dual_auth() { + let env = Env::default(); + let contract_id = env.register_contract(None, RegistryContract); + let client = RegistryContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let new_admin = Address::generate(&env); + + env.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &admin, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &contract_id, + fn_name: "initialize", + args: (admin.clone(),).into_val(&env), + sub_invokes: &[], + }, + }]); + client.initialize(&admin); + + env.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &admin, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &contract_id, + fn_name: "transfer_ownership", + args: (new_admin.clone(),).into_val(&env), + sub_invokes: &[], + }, + }]); + assert!(client.try_transfer_ownership(&new_admin).is_err()); + + env.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &admin, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &contract_id, + fn_name: "transfer_admin", + args: (new_admin.clone(),).into_val(&env), + sub_invokes: &[], + }, + }]); + client.transfer_admin(&new_admin); + assert_eq!(client.get_admin(), new_admin); +} + #[test] #[should_panic(expected = "Error(Auth, InvalidAction)")] fn test_transfer_admin_by_non_admin_panics() { @@ -1194,7 +1272,7 @@ fn test_transfer_admin_by_non_admin_panics() { } #[test] -#[should_panic(expected = "Error(Contract, #3)")] +#[should_panic(expected = "Error(Contract, #4)")] fn test_transfer_admin_before_initialize_panics() { let (env, client) = setup(); let new_admin = Address::generate(&env); From 98fc92470eccec04c6a6e9b88b01624900533396 Mon Sep 17 00:00:00 2001 From: tecch-wiz Date: Sun, 6 Sep 2026 22:22:29 +0100 Subject: [PATCH 2/2] fix(registry): satisfy clippy rustdoc lint --- contracts/registry/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/registry/src/lib.rs b/contracts/registry/src/lib.rs index 6396a5d0..95412e00 100644 --- a/contracts/registry/src/lib.rs +++ b/contracts/registry/src/lib.rs @@ -579,7 +579,6 @@ impl RegistryContract { /// that intentionally need unilateral key rotation may use /// [`transfer_admin`](Self::transfer_admin), but it does not provide this /// dual-authorization safety guarantee. - /// # Arguments /// * `env` - The Soroban environment. /// * `new_admin` - The address that will become the new admin.