From f7b185c5a29798ae4168def963ef98d3e9885569 Mon Sep 17 00:00:00 2001 From: Esther Date: Sun, 18 Oct 2026 17:54:54 +0100 Subject: [PATCH] test(SC-002): verify upgrade storage integrity and prevent proxy/impl storage-slot collisions (#1623) - Add proxy state-persistence test covering admin, pending admin, registered wasm hashes, proposals, and storage version before/after execute_upgrade - Add storage-key collision test comparing XDR encodings of ProxyDataKey vs ImplDataKey and proving both key families coexist in contract storage - Add unauthorized register_wasm regression test - Add implementation_v2 test suite covering score/name storage isolation, pre-upgrade (V1) state readability, overflow saturation, and admin gating --- contracts/Cargo.lock | 2 + contracts/implementation_v2/Cargo.toml | 1 + contracts/implementation_v2/src/lib.rs | 3 + contracts/implementation_v2/src/tests.rs | 136 +++++++++++++++++++++++ contracts/proxy/Cargo.toml | 1 + contracts/proxy/src/tests.rs | 92 +++++++++++++++ 6 files changed, 235 insertions(+) create mode 100644 contracts/implementation_v2/src/tests.rs diff --git a/contracts/Cargo.lock b/contracts/Cargo.lock index 8b056de0e..c55dee4cd 100644 --- a/contracts/Cargo.lock +++ b/contracts/Cargo.lock @@ -1116,6 +1116,7 @@ dependencies = [ name = "implementation_v2" version = "0.0.0" dependencies = [ + "implementation_v1", "soroban-sdk", ] @@ -1447,6 +1448,7 @@ dependencies = [ name = "proxy" version = "0.0.0" dependencies = [ + "implementation_v1", "soroban-sdk", ] diff --git a/contracts/implementation_v2/Cargo.toml b/contracts/implementation_v2/Cargo.toml index 2784ec497..6786433c2 100644 --- a/contracts/implementation_v2/Cargo.toml +++ b/contracts/implementation_v2/Cargo.toml @@ -11,3 +11,4 @@ soroban-sdk = "28.0.0" [dev-dependencies] soroban-sdk = { version = "28.0.0", features = ["testutils"] } +implementation_v1 = { path = "../implementation_v1" } diff --git a/contracts/implementation_v2/src/lib.rs b/contracts/implementation_v2/src/lib.rs index ee323fa14..bf21698d3 100644 --- a/contracts/implementation_v2/src/lib.rs +++ b/contracts/implementation_v2/src/lib.rs @@ -133,3 +133,6 @@ impl StudentRecordV2 { env.storage().persistent().get(&ImplDataKey::Name(student)) } } + +#[cfg(test)] +mod tests; diff --git a/contracts/implementation_v2/src/tests.rs b/contracts/implementation_v2/src/tests.rs new file mode 100644 index 000000000..201705363 --- /dev/null +++ b/contracts/implementation_v2/src/tests.rs @@ -0,0 +1,136 @@ +#![cfg(test)] + +use super::*; +use soroban_sdk::{testutils::Address as _, Address, Env, String}; + +// The proxy contract owns Admin state in a real deployment; tests seed it +// directly to mimic proxy initialization (same pattern as implementation_v1). +fn setup() -> (Env, StudentRecordV2Client<'static>, Address) { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(StudentRecordV2, ()); + let client = StudentRecordV2Client::new(&env, &contract_id); + let admin = Address::generate(&env); + + env.as_contract(&contract_id, || { + env.storage().instance().set(&ProxyDataKey::Admin, &admin); + }); + + (env, client, admin) +} + +#[test] +fn test_add_and_get_score() { + let (env, client, admin) = setup(); + let student = Address::generate(&env); + + assert_eq!(client.get_score(&student), 0); + + client.add_score(&admin, &student, &50); + assert_eq!(client.get_score(&student), 50); + + client.add_score(&admin, &student, &25); + assert_eq!(client.get_score(&student), 75); +} + +#[test] +fn test_score_overflow_saturates() { + let (env, client, admin) = setup(); + let student = Address::generate(&env); + + client.add_score(&admin, &student, &u32::MAX); + assert_eq!(client.get_score(&student), u32::MAX); + + client.add_score(&admin, &student, &10); + assert_eq!(client.get_score(&student), u32::MAX); +} + +#[test] +fn test_set_and_get_name() { + let (env, client, admin) = setup(); + let student = Address::generate(&env); + + assert!(client.get_name(&student).is_none()); + + let name = String::from_str(&env, "Ada Lovelace"); + client.set_name(&admin, &student, &name); + assert_eq!(client.get_name(&student).unwrap(), name); +} + +#[test] +fn test_name_and_score_stored_without_collision() { + let (env, client, admin) = setup(); + let student = Address::generate(&env); + + client.add_score(&admin, &student, &88); + + let name = String::from_str(&env, "Grace Hopper"); + client.set_name(&admin, &student, &name); + + // The additive Name(Address) key must not clobber Score(Address) and + // vice versa. + assert_eq!(client.get_score(&student), 88); + assert_eq!(client.get_name(&student).unwrap(), name); + + client.add_score(&admin, &student, &12); + assert_eq!(client.get_score(&student), 100); + assert_eq!(client.get_name(&student).unwrap(), name); +} + +#[test] +fn test_v1_state_readable_after_upgrade() { + let (env, client, admin) = setup(); + let student = Address::generate(&env); + + // Write state using V1's key type, exactly as the V1 contract would have + // persisted it on-chain before the upgrade. + env.as_contract(&client.address, || { + env.storage().persistent().set( + &implementation_v1::ImplDataKey::Score(student.clone()), + &42u32, + ); + }); + assert_eq!(client.get_score(&student), 42); + + // V2 keeps reading the same Score(Address) key and adds Name(Address) + // alongside it without disturbing existing data. + let name = String::from_str(&env, "Alan Turing"); + client.set_name(&admin, &student, &name); + assert_eq!(client.get_score(&student), 42); + assert_eq!(client.get_name(&student).unwrap(), name); + + client.add_score(&admin, &student, &8); + assert_eq!(client.get_score(&student), 50); + assert_eq!(client.get_name(&student).unwrap(), name); +} + +#[test] +#[should_panic(expected = "Error(Contract, #2)")] +fn test_unauthorized_add_score() { + let (env, client, _admin) = setup(); + let student = Address::generate(&env); + let intruder = Address::generate(&env); + + client.add_score(&intruder, &student, &100); +} + +#[test] +#[should_panic(expected = "Error(Contract, #2)")] +fn test_unauthorized_set_name() { + let (env, client, _admin) = setup(); + let student = Address::generate(&env); + let intruder = Address::generate(&env); + + let name = String::from_str(&env, "Mallory"); + client.set_name(&intruder, &student, &name); +} + +#[test] +#[should_panic(expected = "Error(Contract, #2)")] +fn test_unauthorized_transfer_admin() { + let (env, client, _admin) = setup(); + let intruder = Address::generate(&env); + let new_admin = Address::generate(&env); + + client.transfer_admin(&intruder, &new_admin); +} diff --git a/contracts/proxy/Cargo.toml b/contracts/proxy/Cargo.toml index d2dbadc8b..32c5d66db 100644 --- a/contracts/proxy/Cargo.toml +++ b/contracts/proxy/Cargo.toml @@ -11,3 +11,4 @@ soroban-sdk = "28.0.0" [dev-dependencies] soroban-sdk = { version = "28.0.0", features = ["testutils"] } +implementation_v1 = { path = "../implementation_v1" } diff --git a/contracts/proxy/src/tests.rs b/contracts/proxy/src/tests.rs index 8d1b341c6..1522450ea 100644 --- a/contracts/proxy/src/tests.rs +++ b/contracts/proxy/src/tests.rs @@ -184,6 +184,98 @@ fn test_transfer_admin_revokes_old_admin() { client.upgrade_to(&admin, &wasm2); } +#[test] +#[should_panic(expected = "Error(Contract, #3)")] +fn test_unauthorized_register_wasm() { + let (env, client, admin) = setup(); + let wasm1 = dummy_wasm_hash(&env, 1); + let wasm2 = dummy_wasm_hash(&env, 2); + let non_admin = Address::generate(&env); + + client.init(&admin, &wasm1); + + client.register_wasm(&non_admin, &wasm2); +} + +#[test] +fn test_proxy_state_persists_across_upgrade() { + let (env, client, admin) = setup(); + let wasm1 = dummy_wasm_hash(&env, 1); + let wasm2 = dummy_wasm_hash(&env, 2); + let pending_admin = Address::generate(&env); + + client.init(&admin, &wasm1); + client.register_wasm(&admin, &wasm2); + + // Build up proxy state before the upgrade. + client.transfer_admin(&admin, &pending_admin); + let eta = client.propose_upgrade(&admin, &wasm2); + let co_signer = Address::generate(&env); + client.approve_upgrade(&co_signer, &wasm2); + assert_eq!(client.get_storage_version(), 1u32); + + env.ledger().set_timestamp(eta + 1); + client.execute_upgrade(&admin, &wasm2); + + // Implementation pointer changed... + assert_eq!(client.get_implementation(), wasm2); + assert_eq!(client.get_storage_version(), 2u32); + + // ...but every piece of proxy state survived the upgrade. + assert_eq!(client.get_admin(), admin); + assert_eq!(client.get_pending_admin().unwrap(), pending_admin); + assert!(client.is_wasm_registered(&wasm1)); + assert!(client.is_wasm_registered(&wasm2)); + let proposal = client.get_proposal(&wasm2).unwrap(); + assert_eq!(proposal.wasm_hash, wasm2); + assert_eq!(proposal.eta, eta); + assert!(proposal.executed); + + // The pending-admin ceremony still completes on the upgraded contract. + client.accept_admin(&pending_admin); + assert_eq!(client.get_admin(), pending_admin); + assert!(client.get_pending_admin().is_none()); +} + +#[test] +fn test_proxy_and_impl_storage_keys_do_not_collide() { + use soroban_sdk::xdr::ToXdr; + let (env, client, admin) = setup(); + let wasm = dummy_wasm_hash(&env, 1); + client.init(&admin, &wasm); + + // The UUPS implementation intentionally reuses the proxy's Admin / + // ImplementationWasm keys, so those encodings must match exactly. + let proxy_admin = ProxyDataKey::Admin.to_xdr(&env); + let proxy_impl = ProxyDataKey::ImplementationWasm.to_xdr(&env); + let impl_proxy_admin = implementation_v1::ProxyDataKey::Admin.to_xdr(&env); + let impl_proxy_wasm = implementation_v1::ProxyDataKey::ImplementationWasm.to_xdr(&env); + assert_eq!(proxy_admin, impl_proxy_admin); + assert_eq!(proxy_impl, impl_proxy_wasm); + + // Application-state keys must never alias any proxy key. + let score_key = implementation_v1::ImplDataKey::Score(admin.clone()).to_xdr(&env); + assert_ne!(score_key, proxy_admin); + assert_ne!(score_key, proxy_impl); + + // Behavioral proof: both key families coexist in the same contract storage + // without clobbering each other. + env.as_contract(&client.address, || { + env.storage().instance().set( + &implementation_v1::ImplDataKey::Score(admin.clone()), + &77u32, + ); + }); + assert_eq!(client.get_admin(), admin); + let score: u32 = env.as_contract(&client.address, || { + env.storage() + .instance() + .get(&implementation_v1::ImplDataKey::Score(admin.clone())) + .unwrap() + }); + assert_eq!(score, 77); +} + // ── SC-046 Security Council Tests ───────────────────────────────────────────── fn setup_council(