From 23dfb956a9b18925a7fe13e1b1bc334f471bb333 Mon Sep 17 00:00:00 2001 From: Andre Popovitch Date: Wed, 27 Nov 2024 12:52:31 -0600 Subject: [PATCH] fix(sns): SNS Gov should not accept an upgrade path from SNS-W that contains duplicate versions (#2839) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An upgrade path with duplicate versions from SNS-W would almost certainly be erroneous, and likely to cause the SNS to enter an infinite upgrade loop, which could be unrecoverable. We also may make the assumption in various places that the upgrade steps will not have duplicates, so it's good to enforce that. [← Previous PR](https://github.com/dfinity/ic/pull/2836) --- rs/sns/governance/src/cached_upgrade_steps.rs | 20 ++++-- .../advance_target_sns_version_tests.rs | 66 +++++++++++++++++++ rs/sns/governance/tests/governance.rs | 14 +++- 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/rs/sns/governance/src/cached_upgrade_steps.rs b/rs/sns/governance/src/cached_upgrade_steps.rs index 2c706789393..39c59632dcf 100644 --- a/rs/sns/governance/src/cached_upgrade_steps.rs +++ b/rs/sns/governance/src/cached_upgrade_steps.rs @@ -336,7 +336,22 @@ impl Governance { .await; let upgrade_steps = match upgrade_steps { - Ok(upgrade_steps) => upgrade_steps, + Ok(upgrade_steps) => { + // Check for duplicate versions in the response + let mut seen = std::collections::HashSet::new(); + for version in &upgrade_steps { + if !seen.insert(version) { + log!( + ERROR, + "Cannot refresh cached_upgrade_steps: SNS-W response contains duplicate versions" + ); + return; + } + } + Versions { + versions: upgrade_steps, + } + } Err(err) => { log!( ERROR, @@ -346,9 +361,6 @@ impl Governance { return; } }; - let upgrade_steps = Versions { - versions: upgrade_steps, - }; // Ensure `cached_upgrade_steps` is initialized let cached_upgrade_steps = self diff --git a/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs b/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs index f70d1221c80..3df4d9876b1 100644 --- a/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs +++ b/rs/sns/governance/src/governance/advance_target_sns_version_tests.rs @@ -746,6 +746,72 @@ fn test_upgrade_periodic_task_lock_times_out() { assert!(!gov.acquire_upgrade_periodic_task_lock()); } +#[tokio::test] +async fn test_refresh_cached_upgrade_steps_rejects_duplicate_versions() { + // Step 1: Prepare the world. + let root_canister_id = *TEST_ROOT_CANISTER_ID; + let governance_canister_id = *TEST_GOVERNANCE_CANISTER_ID; + + let mut env = NativeEnvironment::new(Some(governance_canister_id)); + + let version = SnsVersion { + root_wasm_hash: vec![1, 2, 3], + governance_wasm_hash: vec![2, 3, 4], + ledger_wasm_hash: vec![3, 4, 5], + swap_wasm_hash: vec![4, 5, 6], + archive_wasm_hash: vec![5, 6, 7], + index_wasm_hash: vec![6, 7, 8], + }; + + // Set up environment to return upgrade steps with a duplicate version + env.set_call_canister_response( + SNS_WASM_CANISTER_ID, + "list_upgrade_steps", + Encode!(&ListUpgradeStepsRequest { + starting_at: Some(version.clone()), + sns_governance_canister_id: Some(governance_canister_id.into()), + limit: 0, + }) + .unwrap(), + Ok(Encode!(&ListUpgradeStepsResponse { + steps: vec![ + ListUpgradeStep { + version: Some(version.clone()) + }, + ListUpgradeStep { + version: Some(version.clone()) // Duplicate version + }, + ] + }) + .unwrap()), + ); + + let mut governance = Governance::new( + GovernanceProto { + root_canister_id: Some(root_canister_id.get()), + deployed_version: Some(Version::from(version.clone())), + ..basic_governance_proto() + } + .try_into() + .unwrap(), + Box::new(env), + Box::new(DoNothingLedger {}), + Box::new(DoNothingLedger {}), + Box::new(FakeCmc::new()), + ); + + // Step 2: Run code under test. + assert_eq!(governance.proto.cached_upgrade_steps, None); + governance.temporarily_lock_refresh_cached_upgrade_steps(); + governance.refresh_cached_upgrade_steps().await; + + // Step 3: Verify that the cached_upgrade_steps was not updated due to duplicate versions + assert_eq!( + governance.proto.cached_upgrade_steps.unwrap().upgrade_steps, + None + ); +} + #[test] fn test_upgrade_periodic_task_lock_doesnt_get_stuck_during_overflow() { let env = NativeEnvironment::new(Some(*TEST_GOVERNANCE_CANISTER_ID)); diff --git a/rs/sns/governance/tests/governance.rs b/rs/sns/governance/tests/governance.rs index 49277c7982a..dda1fc021c7 100644 --- a/rs/sns/governance/tests/governance.rs +++ b/rs/sns/governance/tests/governance.rs @@ -2909,7 +2909,17 @@ async fn test_refresh_cached_upgrade_steps_noop_if_deployed_version_none() { async fn test_refresh_cached_upgrade_steps() { let mut canister_fixture = GovernanceCanisterFixtureBuilder::new().create(); - let expected_upgrade_steps = vec![Version::default(), Version::default(), Version::default()]; + let v1 = Version::default(); + let v2 = Version { + governance_wasm_hash: vec![1], + ..v1.clone() + }; + let v3 = Version { + governance_wasm_hash: vec![1, 2], + ..v2.clone() + }; + + let expected_upgrade_steps = vec![v1.clone(), v2.clone(), v3.clone()]; // Set up the fixture state { @@ -2927,7 +2937,7 @@ async fn test_refresh_cached_upgrade_steps() { canister_fixture .environment_fixture .push_mocked_canister_reply(ListUpgradeStepsResponse { steps }); - canister_fixture.governance.proto.deployed_version = Some(Version::default()); + canister_fixture.governance.proto.deployed_version = Some(v1); } // Check that the initial state is None