fix(sns): SNS Gov should not accept an upgrade path from SNS-W that c…

…ontains duplicate versions (#2839)

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](#2836)

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

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));

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