fix critical vulnerability in ismp-grandpa

polytope-labs · Jan 28, 2025 · f0e85db · f0e85db
1 parent 5613582
commit f0e85db
Show file tree

Hide file tree

Showing 9 changed files with 35 additions and 39 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -283,13 +283,13 @@ geth-primitives = { path = "./modules/consensus/geth-primitives", default-featur
 sync-committee-primitives = { path = "./modules/consensus/sync-committee/primitives", default-features = false }
 sync-committee-prover = { path = "./modules/consensus/sync-committee/prover" }
 sync-committee-verifier = { path = "./modules/consensus/sync-committee/verifier", default-features = false }
-grandpa-verifier-primitives = { version = "0.1.1", path = "./modules/consensus/grandpa/primitives", default-features = false }
-grandpa-verifier = { version = "0.1.1", path = "./modules/consensus/grandpa/verifier", default-features = false }
+grandpa-verifier-primitives = { version = "0.1.2", path = "./modules/consensus/grandpa/primitives", default-features = false }
+grandpa-verifier = { version = "0.1.2", path = "./modules/consensus/grandpa/verifier", default-features = false }
 grandpa-prover = { path = "./modules/consensus/grandpa/prover" }
 
 # consensus clients
 ismp-bsc = { path = "./modules/ismp/clients/bsc", default-features = false }
-ismp-grandpa = { version = "15.0.0", path = "./modules/ismp/clients/grandpa", default-features = false }
+ismp-grandpa = { version = "15.0.1", path = "./modules/ismp/clients/grandpa", default-features = false }
 ismp-parachain = { version = "15.0.1", path = "./modules/ismp/clients/parachain/client", default-features = false }
 ismp-parachain-inherent = { version = "15.0.0", path = "./modules/ismp/clients/parachain/inherent" }
 ismp-parachain-runtime-api = { version = "15.0.0", path = "./modules/ismp/clients/parachain/runtime-api", default-features = false }

diff --git a/modules/consensus/grandpa/primitives/Cargo.toml b/modules/consensus/grandpa/primitives/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "grandpa-verifier-primitives"
-version = "0.1.1"
+version = "0.1.2"
 edition = "2021"
 authors = ["Polytope Labs <hello@polytope.technology>"]
 license = "Apache-2.0"

diff --git a/modules/consensus/grandpa/primitives/src/justification.rs b/modules/consensus/grandpa/primitives/src/justification.rs
@@ -26,7 +26,7 @@ use sp_consensus_grandpa::{
 	ScheduledChange, SetId, GRANDPA_ENGINE_ID,
 };
 use sp_core::ed25519;
-use sp_runtime::{generic::OpaqueDigestItemId, traits::Header as HeaderT};
+use sp_runtime::{generic::OpaqueDigestItemId, traits::Header as HeaderT, RuntimeAppPublic};
 use sp_std::prelude::*;
 
 /// A GRANDPA justification for block finality, it includes a commit message and
@@ -243,20 +243,9 @@ where
 	H: Encode,
 	N: Encode,
 {
-	log::trace!(target: "pallet_grandpa", "Justification Message {:?}", (round, set_id));
 	let buf = (message, round, set_id).encode();
 
-	let signature_bytes: &[u8] = signature.as_ref();
-	let sp_finality_signature: ed25519::Signature = signature_bytes
-		.try_into()
-		.map_err(|err| anyhow!("Failed to convert ed25519 signature: {err:#?}"))?;
-
-	let id_bytes: &[u8] = id.as_ref();
-	let pub_key: ed25519::Public = id_bytes
-		.try_into()
-		.map_err(|err| anyhow!("Failed to convert public key: {err:#?}"))?;
-
-	if !sp_io::crypto::ed25519_verify(&sp_finality_signature, &buf, &pub_key) {
+	if !id.verify(&buf, signature) {
 		Err(anyhow!("invalid signature for precommit in grandpa justification"))?
 	}
 

diff --git a/modules/consensus/grandpa/prover/src/lib.rs b/modules/consensus/grandpa/prover/src/lib.rs
@@ -158,7 +158,7 @@ where
 
 		Ok(ConsensusState {
 			current_authorities,
-			current_set_id: current_set_id + 1,
+			current_set_id,
 			latest_height,
 			latest_hash: hash.into(),
 			slot_duration,

diff --git a/modules/consensus/grandpa/verifier/Cargo.toml b/modules/consensus/grandpa/verifier/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "grandpa-verifier"
-version = "0.1.1"
+version = "0.1.2"
 edition = "2021"
 authors = ["Polytope Labs <hello@polytope.technology>"]
 license = "Apache-2.0"
@@ -14,7 +14,7 @@ keywords = ["substrate", "polkadot-sdk", "ISMP", "interoperability", "GRANDPA"]
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
-codec = { workspace = true, features = ["derive"]}
+codec = { workspace = true, features = ["derive"] }
 anyhow = { workspace = true, default-features = false }
 finality-grandpa = { version = "0.16.0", features = ["derive-codec"], default-features = false }
 serde = { workspace = true, features = ["derive"] }

diff --git a/modules/consensus/grandpa/verifier/src/tests.rs b/modules/consensus/grandpa/verifier/src/tests.rs
@@ -44,7 +44,7 @@ async fn follow_grandpa_justifications() {
 	let relay_ws_url = std::env::var("RELAY_HOST")
 		.unwrap_or_else(|_| "wss://hyperbridge-paseo-relay.blockops.network:443".to_string());
 
-	let para_ids = vec![2000];
+	let para_ids = vec![1000];
 
 	println!("Connecting to relay chain {relay_ws_url}");
 	let prover = GrandpaProver::<subxt_utils::BlakeSubstrateChain>::new(ProverOptions {
@@ -85,7 +85,7 @@ async fn follow_grandpa_justifications() {
 
 	// slot duration in milliseconds for parachains
 	let slot_duration = 6000;
-	let hash = prover.client.rpc().block_hash(Some(10u64.into())).await.unwrap().unwrap();
+	let hash = prover.client.rpc().finalized_head().await.unwrap();
 	let mut consensus_state = prover.initialize_consensus_state(slot_duration, hash).await.unwrap();
 	println!("Grandpa proofs are now available");
 	while let Some(Ok(_)) = subscription.next().await {

diff --git a/modules/ismp/clients/grandpa/Cargo.toml b/modules/ismp/clients/grandpa/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "ismp-grandpa"
-version = "15.0.0"
+version = "15.0.1"
 edition = "2021"
 authors = ["Polytope Labs <hello@polytope.technology>"]
 license = "Apache-2.0"
@@ -15,9 +15,7 @@ readme = "./README.md"
 anyhow = { workspace = true }
 codec = { workspace = true, features = ["derive"] }
 primitive-types = { workspace = true }
-scale-info = { version = "2.1.1", default-features = false, features = ["derive"] }
 merkle-mountain-range = { workspace = true }
-finality-grandpa = { version = "0.16.0", features = ["derive-codec"], default-features = false }
 frame-benchmarking = { workspace = true, optional = true }
 sp-std = { workspace = true }
 
@@ -39,6 +37,16 @@ sp-core = { workspace = true }
 # cumulus
 substrate-state-machine = { workspace = true }
 
+[dependencies.scale-info]
+version = "2.1.1"
+default-features = false
+features = ["derive"]
+
+[dependencies.finality-grandpa]
+version = "0.16.0"
+features = ["derive-codec"]
+default-features = false
+
 [features]
 default = ["std"]
 std = [

diff --git a/parachain/runtimes/nexus/src/lib.rs b/parachain/runtimes/nexus/src/lib.rs
@@ -335,8 +335,8 @@ impl Contains<RuntimeCall> for IsTreasurySpend {
 	fn contains(c: &RuntimeCall) -> bool {
 		matches!(
 			c,
-			RuntimeCall::Treasury(pallet_treasury::Call::spend { .. })
-				| RuntimeCall::Treasury(pallet_treasury::Call::spend_local { .. })
+			RuntimeCall::Treasury(pallet_treasury::Call::spend { .. }) |
+				RuntimeCall::Treasury(pallet_treasury::Call::spend_local { .. })
 		)
 	}
 }
@@ -764,20 +764,19 @@ impl InstanceFilter<RuntimeCall> for ProxyType {
 	fn filter(&self, c: &RuntimeCall) -> bool {
 		match self {
 			ProxyType::Any => true,
-			ProxyType::NonTransfer => {
-				!matches!(c, RuntimeCall::Balances { .. } | RuntimeCall::Assets { .. })
-			},
+			ProxyType::NonTransfer =>
+				!matches!(c, RuntimeCall::Balances { .. } | RuntimeCall::Assets { .. }),
 			ProxyType::CancelProxy => matches!(
 				c,
-				RuntimeCall::Proxy(pallet_proxy::Call::reject_announcement { .. })
-					| RuntimeCall::Utility { .. }
-					| RuntimeCall::Multisig { .. }
+				RuntimeCall::Proxy(pallet_proxy::Call::reject_announcement { .. }) |
+					RuntimeCall::Utility { .. } |
+					RuntimeCall::Multisig { .. }
 			),
 			ProxyType::Collator => matches!(
 				c,
-				RuntimeCall::CollatorSelection { .. }
-					| RuntimeCall::Utility { .. }
-					| RuntimeCall::Multisig { .. }
+				RuntimeCall::CollatorSelection { .. } |
+					RuntimeCall::Utility { .. } |
+					RuntimeCall::Multisig { .. }
 			),
 		}
 	}