From ccfb229d66640a3fe02fdde864dc7a48324e71a8 Mon Sep 17 00:00:00 2001 From: Zakari Maureen Date: Sun, 18 Oct 2026 22:02:07 +0100 Subject: [PATCH 1/2] fix(vault): bind admin in storage migration and align ForceUpgrade checkpoints (#1633) --- contracts/vault/src/lib.rs | 29 +++++++++++---- contracts/vault/src/proxy_tests.rs | 58 ++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 8 deletions(-) diff --git a/contracts/vault/src/lib.rs b/contracts/vault/src/lib.rs index b5bd3f42d..c084bf5f2 100644 --- a/contracts/vault/src/lib.rs +++ b/contracts/vault/src/lib.rs @@ -597,10 +597,23 @@ impl YieldVault { let pending = get_pending_upgrade(&env).ok_or(VaultError::NoPendingWithdrawal)?; Self::assert_timelock_ready(&env, pending.eta)?; - let pre_version = get_storage_version(&env); - Self::run_storage_migration(&env, STORAGE_VERSION)?; + set_pending_upgrade(&env, &None); + Self::upgrade_with_checkpoint(&env, &admin, pending.wasm_hash) + } + + /// Shared by `execute_upgrade` and the `ForceUpgrade` emergency action: + /// migrates storage to `STORAGE_VERSION`, verifies the version checkpoint, + /// swaps the WASM, and emits `upg_chkpt` with + /// `(old_wasm_hash, new_wasm_hash, old_storage_version, new_storage_version)`. + fn upgrade_with_checkpoint( + env: &Env, + caller: &Address, + new_wasm_hash: BytesN<32>, + ) -> Result<(), VaultError> { + let pre_version = get_storage_version(env); + Self::run_storage_migration(env, STORAGE_VERSION)?; // Checkpoint: storage version must equal STORAGE_VERSION after migration. - let post_version = get_storage_version(&env); + let post_version = get_storage_version(env); assert_eq!( post_version, STORAGE_VERSION, "storage version checkpoint failed: expected {}, got {}", @@ -614,8 +627,12 @@ impl YieldVault { post_version ); - set_pending_upgrade(&env, &None); - Self::apply_upgrade(&env, &admin, pending.wasm_hash); + let previous = get_implementation(env); + Self::apply_upgrade(env, caller, new_wasm_hash.clone()); + env.events().publish( + (symbol_short!("upg_chkpt"), caller.clone()), + (previous, new_wasm_hash, pre_version, post_version), + ); Ok(()) } @@ -1203,7 +1220,7 @@ impl YieldVault { emergency::EmergencyActionKind::ForceUpgrade => { let hash = proposal.wasm_hash.clone().expect("wasm hash required"); - Self::apply_upgrade(&env, &confirmer, hash); + Self::upgrade_with_checkpoint(&env, &confirmer, hash)?; } } diff --git a/contracts/vault/src/proxy_tests.rs b/contracts/vault/src/proxy_tests.rs index 4ab464735..e5fedeb7b 100644 --- a/contracts/vault/src/proxy_tests.rs +++ b/contracts/vault/src/proxy_tests.rs @@ -274,8 +274,8 @@ fn test_migrate_storage_version_checkpoint() { let pre = vault.storage_version(); // Idempotent migration to current version must pass the checkpoint. - vault.migrate_storage(&2); - assert_eq!(vault.storage_version(), 2); + vault.migrate_storage(&STORAGE_VERSION); + assert_eq!(vault.storage_version(), STORAGE_VERSION); assert!(vault.storage_version() >= pre); } @@ -448,3 +448,57 @@ fn generate_storage_fingerprint(env: &Env) -> &str { // (Rust Vec of strings is hard to return here, so we just use it for internal assertion) "Admin TokenAsset Initialized" } + +#[test] +fn test_execute_upgrade_emits_checkpoint_event_with_versions() { + let env = Env::default(); + let (vault, admin, hash) = setup_upgrade(&env); + + let eta = vault.propose_upgrade(&hash); + env.ledger().set_timestamp(eta); + vault.execute_upgrade(); + + let events = env.events().all(); + assert!(events.iter().any(|(contract, topics, data)| { + contract == vault.address + && topics == (symbol_short!("upg_chkpt"), admin.clone()).into_val(&env) + && <(Option>, BytesN<32>, u32, u32)>::try_from_val(&env, &data) + == Ok((None, hash.clone(), STORAGE_VERSION, STORAGE_VERSION)) + })); +} + +#[test] +fn test_force_upgrade_runs_migration_checkpoint_and_emits_event() { + let env = Env::default(); + let (vault, _admin, hash) = setup_upgrade(&env); + let primary = Address::generate(&env); + let secondary = Address::generate(&env); + vault.set_emergency_approvers(&primary, &secondary); + + // Simulate a vault whose storage predates the current schema. + env.as_contract(&vault.address, || set_storage_version(&env, 1)); + assert_eq!(vault.storage_version(), 1); + + let id = vault.propose_emergency_action( + &primary, + &emergency::EmergencyActionKind::ForceUpgrade, + &0u32, + &None, + &Some(hash.clone()), + ); + let window = vault.emergency_dispute_window(); + env.ledger() + .set_timestamp(env.ledger().timestamp() + window + 1); + vault.confirm_emergency_action(&secondary, &id); + let events = env.events().all(); + + assert_eq!(vault.storage_version(), STORAGE_VERSION); + assert_eq!(vault.implementation(), Some(hash.clone())); + + assert!(events.iter().any(|(contract, topics, data)| { + contract == vault.address + && topics == (symbol_short!("upg_chkpt"), secondary.clone()).into_val(&env) + && <(Option>, BytesN<32>, u32, u32)>::try_from_val(&env, &data) + == Ok((None, hash.clone(), 1, STORAGE_VERSION)) + })); +} From 457b4ed30e0cf5252849ff74332874609939a088 Mon Sep 17 00:00:00 2001 From: Zakari Maureen Date: Mon, 19 Oct 2026 06:08:06 +0100 Subject: [PATCH 2/2] fix(contract): guard whitelist_strategy removal against active and retired strategies (#1637) --- contracts/vault/src/strategy_registration.rs | 23 +++++++ contracts/vault/src/test.rs | 69 +++++++++++++++++++- contracts/vault/src/whitelist.rs | 50 +++++++++++++- 3 files changed, 136 insertions(+), 6 deletions(-) diff --git a/contracts/vault/src/strategy_registration.rs b/contracts/vault/src/strategy_registration.rs index 5f7685c32..461a2a919 100644 --- a/contracts/vault/src/strategy_registration.rs +++ b/contracts/vault/src/strategy_registration.rs @@ -143,6 +143,29 @@ pub fn require_active_registration( } } +/// Guarded removal used by `whitelist_strategy(false)`. Mirrors the guards of +/// `retire_strategy_internal`: the vault's current strategy and Active +/// strategies cannot be removed, and Retired strategies are terminal so their +/// record must survive (removing it would let them re-register as Pending). +/// Only unregistered (no-op) and Pending strategies can be removed. +pub(crate) fn remove_registration_guarded( + env: &Env, + strategy: &Address, + active_vault_strategy: Option
, +) -> Result<(), StrategyRegistrationError> { + if active_vault_strategy.as_ref() == Some(strategy) { + return Err(StrategyRegistrationError::ActiveStrategyInUse); + } + match read_registration_state(env, strategy) { + Some(STATE_ACTIVE) => Err(StrategyRegistrationError::ActiveStrategyInUse), + Some(STATE_RETIRED) => Err(StrategyRegistrationError::InvalidTransition), + _ => { + remove_registration_state(env, strategy); + Ok(()) + } + } +} + pub fn remove_registration(env: &Env, strategy: &Address) { remove_registration_state(env, strategy); } diff --git a/contracts/vault/src/test.rs b/contracts/vault/src/test.rs index 50c5d94b3..2806f146d 100644 --- a/contracts/vault/src/test.rs +++ b/contracts/vault/src/test.rs @@ -2352,9 +2352,56 @@ fn test_whitelist_consistency_with_set_strategy() { // Verify it was set assert_eq!(vault.strategy().unwrap(), benji_strategy); - // Remove from whitelist and verify - vault.whitelist_strategy(&benji_strategy, &false); - assert!(!vault.is_strategy_whitelisted(&benji_strategy)); + // Removal of the active vault strategy is rejected (#1637) + assert!(vault.try_whitelist_strategy(&benji_strategy, &false).is_err()); + assert!(vault.is_strategy_whitelisted(&benji_strategy)); +} + +#[test] +fn test_whitelist_removal_rejects_active_strategy() { + let env = Env::default(); + env.mock_all_auths(); + let (vault, _, _, _admin) = setup_vault(&env); + let strategy = Address::generate(&env); + + vault.register_strategy(&strategy); + vault.activate_strategy_registration(&strategy); + + assert!(vault.try_whitelist_strategy(&strategy, &false).is_err()); + assert_eq!( + vault.strategy_registration_state(&strategy), + Some(STATE_ACTIVE) + ); +} + +#[test] +fn test_retired_strategy_cannot_return_to_pending_via_whitelist() { + let env = Env::default(); + env.mock_all_auths(); + let (vault, _, _, _admin) = setup_vault(&env); + let strategy = Address::generate(&env); + + vault.whitelist_strategy(&strategy, &true); + vault.retire_strategy(&strategy); + assert_eq!( + vault.strategy_registration_state(&strategy), + Some(STATE_RETIRED) + ); + + // Removal must be rejected, leaving the Retired record intact... + assert!(vault.try_whitelist_strategy(&strategy, &false).is_err()); + assert_eq!( + vault.strategy_registration_state(&strategy), + Some(STATE_RETIRED) + ); + + // ...so re-adding cannot revive it to Pending. + assert!(vault.try_whitelist_strategy(&strategy, &true).is_err()); + assert_eq!( + vault.strategy_registration_state(&strategy), + Some(STATE_RETIRED) + ); + assert!(!vault.is_strategy_whitelisted(&strategy)); } // ─── Strategy heartbeat ─────────────────────────────────────────────────────── @@ -2929,3 +2976,19 @@ fn test_overflow_protection_near_limits() { let res_assets = vault.try_calculate_assets(&i128::MAX); assert_eq!(res_assets, Err(Ok(crate::errors::VaultError::MathOverflow))); } + +#[test] +fn test_map_registration_error_variants() { + use crate::strategy_registration::StrategyRegistrationError as E; + let cases = [ + (E::NotRegistered, VaultError::StrategyNotWhitelisted), + (E::InvalidTransition, VaultError::InvalidMigrationTarget), + (E::StrategyNotActive, VaultError::InvalidMigrationTarget), + (E::ActiveStrategyInUse, VaultError::WhitelistOperationFailed), + (E::AlreadyRegistered, VaultError::WhitelistOperationFailed), + (E::Unauthorized, VaultError::UnauthorizedStrategy), + ]; + for (input, expected) in cases { + assert_eq!(YieldVault::map_registration_error(input), expected); + } +} diff --git a/contracts/vault/src/whitelist.rs b/contracts/vault/src/whitelist.rs index bb8145f2e..370cf043c 100644 --- a/contracts/vault/src/whitelist.rs +++ b/contracts/vault/src/whitelist.rs @@ -6,6 +6,7 @@ use soroban_sdk::{Address, Env}; use crate::strategy_registration::{self, STATE_RETIRED}; use crate::upgrade::get_admin; +use crate::DataKey; /// Errors that can occur during whitelist operations #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -30,7 +31,8 @@ impl SecureWhitelist { match strategy_registration::read_registration_state(env, strategy) { None => { - let _ = strategy_registration::register_strategy_internal(env, strategy); + strategy_registration::register_strategy_internal(env, strategy) + .map_err(|_| WhitelistError::OperationFailed)?; } Some(STATE_RETIRED) => { return Err(WhitelistError::OperationFailed); @@ -51,8 +53,10 @@ impl SecureWhitelist { return Err(WhitelistError::Unauthorized); } - strategy_registration::remove_registration(env, strategy); - Ok(()) + let active_vault_strategy: Option
= + env.storage().instance().get(&DataKey::Strategy); + strategy_registration::remove_registration_guarded(env, strategy, active_vault_strategy) + .map_err(|_| WhitelistError::OperationFailed) } pub fn is_strategy_whitelisted(env: &Env, strategy: &Address) -> bool { @@ -196,6 +200,46 @@ mod tests { }); } + #[test] + fn test_remove_strategy_rejects_active_and_retired() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(crate::YieldVault, ()); + + env.as_contract(&contract_id, || { + let admin = Address::generate(&env); + let active = Address::generate(&env); + let retired = Address::generate(&env); + let in_use = Address::generate(&env); + set_admin(&env, &admin); + + assert_eq!(SecureWhitelist::add_strategy(&env, &admin, &active), Ok(())); + assert_eq!(activate_strategy_internal(&env, &active), Ok(STATE_ACTIVE)); + assert_eq!( + SecureWhitelist::remove_strategy(&env, &admin, &active), + Err(WhitelistError::OperationFailed) + ); + assert_eq!(read_registration_state(&env, &active), Some(STATE_ACTIVE)); + + assert_eq!(SecureWhitelist::add_strategy(&env, &admin, &retired), Ok(())); + assert_eq!(retire_strategy_internal(&env, &retired, None), Ok(STATE_RETIRED)); + assert_eq!( + SecureWhitelist::remove_strategy(&env, &admin, &retired), + Err(WhitelistError::OperationFailed) + ); + assert_eq!(read_registration_state(&env, &retired), Some(STATE_RETIRED)); + + // Pending strategy that is the vault's current strategy is protected too + assert_eq!(SecureWhitelist::add_strategy(&env, &admin, &in_use), Ok(())); + env.storage().instance().set(&DataKey::Strategy, &in_use); + assert_eq!( + SecureWhitelist::remove_strategy(&env, &admin, &in_use), + Err(WhitelistError::OperationFailed) + ); + assert_eq!(read_registration_state(&env, &in_use), Some(STATE_PENDING)); + }); + } + #[test] fn test_set_whitelist_status_add_and_remove() { let env = Env::default();