Fix upgrade path check (informalsystems#1298)

* fix upgrade_path prefix

* fix conversion

* add more check

* user-defined upgrade path

* fix a test

* Update ibc-clients/ics07-tendermint/src/client_state/common.rs

Co-authored-by: Sean Chen <[email protected]>
Signed-off-by: Yuji Ito <[email protected]>

* Update ibc-clients/ics07-tendermint/src/client_state/common.rs

Co-authored-by: Sean Chen <[email protected]>
Signed-off-by: Yuji Ito <[email protected]>

* fmt

* disallow empty upgrade_path

* remove unused import

* add InvalidUpgradePath

* add changelogs

---------

Signed-off-by: Yuji Ito <[email protected]>
Co-authored-by: Sean Chen <[email protected]>

Author: yito88

.changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md

@@ -0,0 +1,3 @@
+- [ibc-client-tendermint] Fix client verification panic on upgrades when the
+  `upgrade_path` size is 1.
+  ([\#1297](https://github.com/cosmos/ibc-rs/issues/1297))

.changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md

@@ -0,0 +1,3 @@
+- [ibc-client-tendermint] Use the user-defined upgrade path for client upgrades,
+instead of defaulting to `UPGRADED_IBC_STATE`.
+  ([\#1303](https://github.com/cosmos/ibc-rs/issues/1303))

ibc-clients/ics07-tendermint/src/client_state/common.rs

@@ -13,7 +13,9 @@ use ibc_core_commitment_types::merkle::{MerklePath, MerkleProof};
 use ibc_core_commitment_types::proto::ics23::{HostFunctionsManager, HostFunctionsProvider};
 use ibc_core_commitment_types::specs::ProofSpecs;
 use ibc_core_host::types::identifiers::ClientType;
-use ibc_core_host::types::path::{Path, PathBytes, UpgradeClientPath};
+use ibc_core_host::types::path::{
+    Path, PathBytes, UpgradeClientStatePath, UpgradeConsensusStatePath,
+};
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::Any;
 use ibc_primitives::{Timestamp, ToVec};
@@ -56,20 +58,51 @@ impl ClientStateCommon for ClientState {
     ) -> Result<(), ClientError> {
         let last_height = self.latest_height().revision_height();
 
-        let upgrade_client_path_bytes = self.serialize_path(Path::UpgradeClient(
-            UpgradeClientPath::UpgradedClientState(last_height),
-        ))?;
-
-        let upgrade_consensus_path_bytes = self.serialize_path(Path::UpgradeClient(
-            UpgradeClientPath::UpgradedClientConsensusState(last_height),
-        ))?;
+        // The client state's upgrade path vector needs to parsed into a tuple in the form
+        // of `(upgrade_path_prefix, upgrade_path)`. Given the length of the client
+        // state's upgrade path vector, the following determinations are made:
+        // 1: The commitment prefix is left empty and the upgrade path is used as-is.
+        // 2: The commitment prefix and upgrade path are both taken as-is.
+        let upgrade_path = &self.inner().upgrade_path;
+        let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() {
+            0 => {
+                return Err(UpgradeClientError::InvalidUpgradePath {
+                    reason: "no upgrade path has been set".to_string(),
+                }
+                .into());
+            }
+            1 => (CommitmentPrefix::empty(), upgrade_path[0].clone()),
+            2 => (
+                upgrade_path[0].as_bytes().to_vec().into(),
+                upgrade_path[1].clone(),
+            ),
+            _ => {
+                return Err(UpgradeClientError::InvalidUpgradePath {
+                    reason: "upgrade path is too long".to_string(),
+                }
+                .into());
+            }
+        };
+
+        let upgrade_client_path_bytes =
+            self.serialize_path(Path::UpgradeClientState(UpgradeClientStatePath {
+                upgrade_path: upgrade_path.clone(),
+                height: last_height,
+            }))?;
+
+        let upgrade_consensus_path_bytes =
+            self.serialize_path(Path::UpgradeConsensusState(UpgradeConsensusStatePath {
+                upgrade_path,
+                height: last_height,
+            }))?;
 
         verify_upgrade_client::<HostFunctionsManager>(
             self.inner(),
             upgraded_client_state,
             upgraded_consensus_state,
             proof_upgrade_client,
             proof_upgrade_consensus_state,
+            upgrade_path_prefix,
             upgrade_client_path_bytes,
             upgrade_consensus_path_bytes,
             root,
@@ -207,6 +240,7 @@ pub fn verify_upgrade_client<H: HostFunctionsProvider>(
     upgraded_consensus_state: Any,
     proof_upgrade_client: CommitmentProofBytes,
     proof_upgrade_consensus_state: CommitmentProofBytes,
+    upgrade_path_prefix: CommitmentPrefix,
     upgrade_client_path_bytes: PathBytes,
     upgrade_consensus_path_bytes: PathBytes,
     root: &CommitmentRoot,
@@ -225,22 +259,11 @@ pub fn verify_upgrade_client<H: HostFunctionsProvider>(
     // the height
     if latest_height >= upgraded_tm_client_state_height {
         Err(UpgradeClientError::LowUpgradeHeight {
-            upgraded_height: latest_height,
-            client_height: upgraded_tm_client_state_height,
+            upgraded_height: upgraded_tm_client_state_height,
+            client_height: latest_height,
         })?
     }
 
-    // Check to see if the upgrade path is set
-    let mut upgrade_path = client_state.upgrade_path.clone();
-
-    if upgrade_path.pop().is_none() {
-        return Err(ClientError::ClientSpecific {
-            description: "cannot upgrade client as no upgrade path has been set".to_string(),
-        });
-    };
-
-    let upgrade_path_prefix = CommitmentPrefix::from(upgrade_path[0].clone().into_bytes());
-
     // Verify the proof of the upgraded client state
     verify_membership::<H>(
         &client_state.proof_specs,

ibc-core/ics02-client/types/src/error.rs

@@ -148,6 +148,8 @@ pub enum UpgradeClientError {
         upgraded_height: Height,
         client_height: Height,
     },
+    /// Invalid upgrade path: `{reason}`
+    InvalidUpgradePath { reason: String },
     /// invalid upgrade proposal: `{reason}`
     InvalidUpgradeProposal { reason: String },
     /// invalid upgrade plan: `{reason}`

ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs

@@ -7,7 +7,7 @@
 
 use ibc_core_client_context::ClientValidationContext;
 use ibc_core_client_types::error::UpgradeClientError;
-use ibc_core_host_types::path::UpgradeClientPath;
+use ibc_core_host_types::path::{UpgradeClientStatePath, UpgradeConsensusStatePath};
 
 use super::Plan;
 
@@ -28,13 +28,13 @@ pub trait UpgradeValidationContext {
     /// Returns the upgraded client state at the specified upgrade path.
     fn upgraded_client_state(
         &self,
-        upgrade_path: &UpgradeClientPath,
+        upgrade_path: &UpgradeClientStatePath,
     ) -> Result<UpgradedClientStateRef<Self>, UpgradeClientError>;
 
     /// Returns the upgraded consensus state at the specified upgrade path.
     fn upgraded_consensus_state(
         &self,
-        upgrade_path: &UpgradeClientPath,
+        upgrade_path: &UpgradeConsensusStatePath,
     ) -> Result<UpgradedConsensusStateRef<Self>, UpgradeClientError>;
 }
 
@@ -50,14 +50,14 @@ pub trait UpgradeExecutionContext: UpgradeValidationContext {
     /// Stores the upgraded client state at the specified upgrade path.
     fn store_upgraded_client_state(
         &mut self,
-        upgrade_path: UpgradeClientPath,
+        upgrade_path: UpgradeClientStatePath,
         client_state: UpgradedClientStateRef<Self>,
     ) -> Result<(), UpgradeClientError>;
 
     /// Stores the upgraded consensus state at the specified upgrade path.
     fn store_upgraded_consensus_state(
         &mut self,
-        upgrade_path: UpgradeClientPath,
+        upgrade_path: UpgradeConsensusStatePath,
         consensus_state: UpgradedConsensusStateRef<Self>,
     ) -> Result<(), UpgradeClientError>;
 }

ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs

@@ -1,6 +1,6 @@
 use ibc_client_tendermint::types::ClientState as TmClientState;
 use ibc_core_client_types::error::UpgradeClientError;
-use ibc_core_host_types::path::UpgradeClientPath;
+use ibc_core_host_types::path::UpgradeClientStatePath;
 use ibc_primitives::prelude::*;
 use tendermint::abci::Event as TmEvent;
 
@@ -37,7 +37,7 @@ where
 
     ctx.schedule_upgrade(plan.clone())?;
 
-    let upgraded_client_state_path = UpgradeClientPath::UpgradedClientState(plan.height);
+    let upgraded_client_state_path = UpgradeClientStatePath::new_with_default_path(plan.height);
 
     ctx.store_upgraded_client_state(upgraded_client_state_path, client_state.into())?;
 

ibc-core/ics24-host/types/src/path.rs

@@ -99,7 +99,8 @@ pub enum Path {
     Commitment(CommitmentPath),
     Ack(AckPath),
     Receipt(ReceiptPath),
-    UpgradeClient(UpgradeClientPath),
+    UpgradeClientState(UpgradeClientStatePath),
+    UpgradeConsensusState(UpgradeConsensusStatePath),
 }
 
 #[cfg_attr(
@@ -693,13 +694,51 @@ impl ReceiptPath {
     derive(borsh::BorshSerialize, borsh::BorshDeserialize)
 )]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-/// Paths that are specific for client upgrades.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)]
-pub enum UpgradeClientPath {
-    #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_STATE}")]
-    UpgradedClientState(u64),
-    #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_CONSENSUS_STATE}")]
-    UpgradedClientConsensusState(u64),
+#[display(fmt = "{upgrade_path}/{height}/{UPGRADED_CLIENT_STATE}")]
+pub struct UpgradeClientStatePath {
+    pub upgrade_path: String,
+    pub height: u64,
+}
+
+impl UpgradeClientStatePath {
+    /// Create with the default upgrade path
+    pub fn new_with_default_path(height: u64) -> Self {
+        Self {
+            upgrade_path: UPGRADED_IBC_STATE.to_string(),
+            height,
+        }
+    }
+}
+
+#[cfg_attr(
+    feature = "parity-scale-codec",
+    derive(
+        parity_scale_codec::Encode,
+        parity_scale_codec::Decode,
+        scale_info::TypeInfo
+    )
+)]
+#[cfg_attr(
+    feature = "borsh",
+    derive(borsh::BorshSerialize, borsh::BorshDeserialize)
+)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)]
+#[display(fmt = "{upgrade_path}/{height}/{UPGRADED_CLIENT_CONSENSUS_STATE}")]
+pub struct UpgradeConsensusStatePath {
+    pub upgrade_path: String,
+    pub height: u64,
+}
+
+impl UpgradeConsensusStatePath {
+    /// Create with the default upgrade path
+    pub fn new_with_default_path(height: u64) -> Self {
+        Self {
+            upgrade_path: UPGRADED_IBC_STATE.to_string(),
+            height,
+        }
+    }
 }
 
 #[cfg_attr(
@@ -760,7 +799,8 @@ impl FromStr for Path {
             .or_else(|| parse_commitments(&components))
             .or_else(|| parse_acks(&components))
             .or_else(|| parse_receipts(&components))
-            .or_else(|| parse_upgrades(&components))
+            .or_else(|| parse_upgrade_client_state(&components))
+            .or_else(|| parse_upgrade_consensus_state(&components))
             .ok_or(PathError::ParseFailure {
                 path: s.to_string(),
             })
@@ -1084,28 +1124,52 @@ fn parse_receipts(components: &[&str]) -> Option<Path> {
     )
 }
 
-fn parse_upgrades(components: &[&str]) -> Option<Path> {
+fn parse_upgrade_client_state(components: &[&str]) -> Option<Path> {
     if components.len() != 3 {
         return None;
     }
 
-    let first = *components.first()?;
+    let last = *components.last()?;
 
-    if first != UPGRADED_IBC_STATE {
+    if last != UPGRADED_CLIENT_STATE {
         return None;
     }
 
-    let last = *components.last()?;
+    let upgrade_path = components.first()?.to_string();
 
-    let height = components[1].parse::<u64>().ok()?;
+    let height = u64::from_str(components[1]).ok()?;
 
-    match last {
-        UPGRADED_CLIENT_STATE => Some(UpgradeClientPath::UpgradedClientState(height).into()),
-        UPGRADED_CLIENT_CONSENSUS_STATE => {
-            Some(UpgradeClientPath::UpgradedClientConsensusState(height).into())
+    Some(
+        UpgradeClientStatePath {
+            upgrade_path,
+            height,
         }
-        _ => None,
+        .into(),
+    )
+}
+
+fn parse_upgrade_consensus_state(components: &[&str]) -> Option<Path> {
+    if components.len() != 3 {
+        return None;
+    }
+
+    let last = *components.last()?;
+
+    if last != UPGRADED_CLIENT_CONSENSUS_STATE {
+        return None;
     }
+
+    let upgrade_path = components.first()?.to_string();
+
+    let height = u64::from_str(components[1]).ok()?;
+
+    Some(
+        UpgradeConsensusStatePath {
+            upgrade_path,
+            height,
+        }
+        .into(),
+    )
 }
 
 #[cfg(test)]
@@ -1207,11 +1271,17 @@ mod tests {
     )]
     #[case(
         "upgradedIBCState/0/upgradedClient",
-        Path::UpgradeClient(UpgradeClientPath::UpgradedClientState(0))
+        Path::UpgradeClientState(UpgradeClientStatePath {
+            upgrade_path: UPGRADED_IBC_STATE.to_string(),
+            height: 0,
+        })
     )]
     #[case(
         "upgradedIBCState/0/upgradedConsState",
-        Path::UpgradeClient(UpgradeClientPath::UpgradedClientConsensusState(0))
+        Path::UpgradeConsensusState(UpgradeConsensusStatePath {
+            upgrade_path: UPGRADED_IBC_STATE.to_string(),
+            height: 0,
+        })
     )]
     fn test_successful_parsing(#[case] path_str: &str, #[case] path: Path) {
         // can be parsed into Path
@@ -1419,25 +1489,30 @@ mod tests {
     }
 
     #[test]
-    fn test_parse_upgrades_fn() {
+    fn test_parse_upgrade_client_state_fn() {
         let path = "upgradedIBCState/0/upgradedClient";
         let components: Vec<&str> = path.split('/').collect();
 
         assert_eq!(
-            parse_upgrades(&components),
-            Some(Path::UpgradeClient(UpgradeClientPath::UpgradedClientState(
-                0
-            ))),
+            parse_upgrade_client_state(&components),
+            Some(Path::UpgradeClientState(UpgradeClientStatePath {
+                upgrade_path: UPGRADED_IBC_STATE.to_string(),
+                height: 0,
+            })),
         );
+    }
 
+    #[test]
+    fn test_parse_upgrade_consensus_state_fn() {
         let path = "upgradedIBCState/0/upgradedConsState";
         let components: Vec<&str> = path.split('/').collect();
 
         assert_eq!(
-            parse_upgrades(&components),
-            Some(Path::UpgradeClient(
-                UpgradeClientPath::UpgradedClientConsensusState(0)
-            )),
+            parse_upgrade_consensus_state(&components),
+            Some(Path::UpgradeConsensusState(UpgradeConsensusStatePath {
+                upgrade_path: UPGRADED_IBC_STATE.to_string(),
+                height: 0,
+            })),
         )
     }
 }