diff --git a/.changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md b/.changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md new file mode 100644 index 000000000..7bfaeedb1 --- /dev/null +++ b/.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)) diff --git a/.changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md b/.changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md new file mode 100644 index 000000000..324daf793 --- /dev/null +++ b/.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)) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index becb9b8bb..ef5b04469 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/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,13 +58,43 @@ 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::( self.inner(), @@ -70,6 +102,7 @@ impl ClientStateCommon for ClientState { 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( 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( // 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::( &client_state.proof_specs, diff --git a/ibc-core/ics02-client/types/src/error.rs b/ibc-core/ics02-client/types/src/error.rs index dddd67a95..cda8b0f05 100644 --- a/ibc-core/ics02-client/types/src/error.rs +++ b/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}` diff --git a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs index 056bd4e46..1328355e8 100644 --- a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs +++ b/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, UpgradeClientError>; /// Returns the upgraded consensus state at the specified upgrade path. fn upgraded_consensus_state( &self, - upgrade_path: &UpgradeClientPath, + upgrade_path: &UpgradeConsensusStatePath, ) -> Result, 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, ) -> 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, ) -> Result<(), UpgradeClientError>; } diff --git a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs index 8c7ee72cf..3f27bad12 100644 --- a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs +++ b/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())?; diff --git a/ibc-core/ics24-host/types/src/path.rs b/ibc-core/ics24-host/types/src/path.rs index bd6b120de..61ce31813 100644 --- a/ibc-core/ics24-host/types/src/path.rs +++ b/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 { ) } -fn parse_upgrades(components: &[&str]) -> Option { +fn parse_upgrade_client_state(components: &[&str]) -> Option { 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::().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 { + 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, + })), ) } } diff --git a/ibc-query/src/core/client/query.rs b/ibc-query/src/core/client/query.rs index 076aa8329..27e64a33f 100644 --- a/ibc-query/src/core/client/query.rs +++ b/ibc-query/src/core/client/query.rs @@ -4,11 +4,12 @@ use ibc::core::client::context::client_state::ClientStateValidation; use ibc::core::client::context::ClientValidationContext; use ibc::core::client::types::error::ClientError; use ibc::core::host::types::path::{ - ClientConsensusStatePath, ClientStatePath, Path, UpgradeClientPath, + ClientConsensusStatePath, ClientStatePath, Path, UpgradeClientStatePath, + UpgradeConsensusStatePath, UPGRADED_IBC_STATE, }; use ibc::core::host::{ConsensusStateRef, ValidationContext}; use ibc::cosmos_host::upgrade_proposal::{UpgradeValidationContext, UpgradedConsensusStateRef}; -use ibc::primitives::prelude::format; +use ibc::primitives::prelude::{format, ToString}; use ibc::primitives::proto::Any; use super::{ @@ -205,6 +206,12 @@ where I: ValidationContext, U: UpgradeValidationContext + ProvableContext, { + let upgrade_path = match &request.upgrade_path { + Some(path) if !path.is_empty() => path, + _ => UPGRADED_IBC_STATE, + } + .to_string(); + let upgrade_revision_height = match request.upgrade_height { Some(height) => height.revision_height(), None => { @@ -215,8 +222,10 @@ where } }; - let upgraded_client_state_path = - UpgradeClientPath::UpgradedClientState(upgrade_revision_height); + let upgraded_client_state_path = UpgradeClientStatePath { + upgrade_path, + height: upgrade_revision_height, + }; let upgraded_client_state = upgrade_ctx .upgraded_client_state(&upgraded_client_state_path) @@ -230,7 +239,7 @@ where let proof = upgrade_ctx .get_proof( proof_height, - &Path::UpgradeClient(upgraded_client_state_path), + &Path::UpgradeClientState(upgraded_client_state_path), ) .ok_or_else(|| { QueryError::proof_not_found(format!( @@ -256,6 +265,12 @@ where U: UpgradeValidationContext + ProvableContext, UpgradedConsensusStateRef: Into, { + let upgrade_path = match &request.upgrade_path { + Some(path) if !path.is_empty() => path, + _ => UPGRADED_IBC_STATE, + } + .to_string(); + let upgrade_revision_height = match request.upgrade_height { Some(height) => height.revision_height(), None => { @@ -266,8 +281,10 @@ where } }; - let upgraded_consensus_state_path = - UpgradeClientPath::UpgradedClientConsensusState(upgrade_revision_height); + let upgraded_consensus_state_path = UpgradeConsensusStatePath { + upgrade_path, + height: upgrade_revision_height, + }; let upgraded_consensus_state = upgrade_ctx .upgraded_consensus_state(&upgraded_consensus_state_path) @@ -281,7 +298,7 @@ where let proof = upgrade_ctx .get_proof( proof_height, - &Path::UpgradeClient(upgraded_consensus_state_path), + &Path::UpgradeConsensusState(upgraded_consensus_state_path), ) .ok_or_else(|| { QueryError::proof_not_found(format!( diff --git a/ibc-query/src/core/client/types/request.rs b/ibc-query/src/core/client/types/request.rs index 1cc5e4769..8648c69bc 100644 --- a/ibc-query/src/core/client/types/request.rs +++ b/ibc-query/src/core/client/types/request.rs @@ -198,6 +198,8 @@ impl From for QueryClientParamsRequest { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] pub struct QueryUpgradedClientStateRequest { + /// The upgrade path + pub upgrade_path: Option, /// Height at which the chain is scheduled to halt for upgrade pub upgrade_height: Option, /// The height at which to query the upgraded client state. If not provided, @@ -208,6 +210,7 @@ pub struct QueryUpgradedClientStateRequest { impl From for QueryUpgradedClientStateRequest { fn from(_request: RawUpgradedClientStateRequest) -> Self { Self { + upgrade_path: None, upgrade_height: None, query_height: None, } @@ -220,6 +223,8 @@ impl From for QueryUpgradedClientStateRequest { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] pub struct QueryUpgradedConsensusStateRequest { + /// The upgrade path + pub upgrade_path: Option, /// The height at which the chain is scheduled to halt for upgrade. pub upgrade_height: Option, /// The height at which to query the upgraded consensus state. If not @@ -230,6 +235,7 @@ pub struct QueryUpgradedConsensusStateRequest { impl From for QueryUpgradedConsensusStateRequest { fn from(_request: RawUpgradedConsensusStateRequest) -> Self { Self { + upgrade_path: None, upgrade_height: None, query_height: None, }