From 189c92e30ec72d427a19a1bcf2405d3780d8d2c6 Mon Sep 17 00:00:00 2001
From: Hector Bulgarini <hbulgarini@gmail.com>
Date: Thu, 13 Apr 2023 11:18:56 +0200
Subject: [PATCH] First version of review

---
 pallets/bridge-registry/src/weights.rs      |  8 ++++++++
 pallets/dkg-metadata/src/lib.rs             | 21 +++++++++++++++++++++
 pallets/dkg-metadata/src/weights.rs         |  8 ++++++++
 pallets/dkg-proposal-handler/src/lib.rs     | 11 +++++++++++
 pallets/dkg-proposal-handler/src/weights.rs |  9 +++++++++
 pallets/dkg-proposals/src/weights.rs        |  8 ++++++++
 6 files changed, 65 insertions(+)

diff --git a/pallets/bridge-registry/src/weights.rs b/pallets/bridge-registry/src/weights.rs
index bf9245df8..c67c6f216 100644
--- a/pallets/bridge-registry/src/weights.rs
+++ b/pallets/bridge-registry/src/weights.rs
@@ -50,6 +50,14 @@ pub trait WeightInfo {
 	fn force_reset_indices() -> Weight;
 }
 
+	/// <HB SBP Review M2
+	///
+	/// The code is running under the release 0.9.39 however the weights are still using weights v1
+	/// since it is considering only one value (ref time). The output of the CLI should configure the weights
+	/// to use the `from_parts(ref time,proof size)` fn
+	///
+	/// HB SBP Review M2>
+
 /// Weights for pallet_bridge_registry using the Substrate node and recommended hardware.
 pub struct WebbWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for WebbWeight<T> {
diff --git a/pallets/dkg-metadata/src/lib.rs b/pallets/dkg-metadata/src/lib.rs
index e8c63e283..f574e06d2 100644
--- a/pallets/dkg-metadata/src/lib.rs
+++ b/pallets/dkg-metadata/src/lib.rs
@@ -1364,6 +1364,18 @@ pub mod pallet {
 		}
 	}
 
+	/// <HB SBP Review M2
+	///
+	/// Is the validate_unsigned fn used anywhere? I'm not able to find if this is used in any OCW.
+	///
+	/// We detected a vector atack with unsigned transactions from OCW: A malissuous validator might
+	/// push tempered data and the validators wouldn't be punished for this. Validators are only
+	/// subject to slashing if they create a block that violates the STF. Creating valid
+	/// transactions with tampered data would not result in slashing, and therefore the possibility
+	/// of malicious Validators in this context is real.
+	///
+	/// HB SBP Review M2>
+
 	#[pallet::validate_unsigned]
 	impl<T: Config> ValidateUnsigned for Pallet<T> {
 		type Call = Call<T>;
@@ -1454,6 +1466,15 @@ pub mod pallet {
 	}
 }
 
+/// <HB SBP Review M2
+///
+/// I would recommend extracting these functions into it's own file to help the pallet's readability
+/// and to reduce the number of lines to this file.
+///
+/// Here there is a proposal about a code organization for a big pallet like this one:
+/// https://github.com/paritytech/substrate/blob/master/frame/assets/src
+///
+/// HB SBP Review M2>
 impl<T: Config> Pallet<T> {
 	/// Return the current active DKG authority set.
 	pub fn authority_set() -> AuthoritySet<T::DKGId, T::MaxAuthorities> {
diff --git a/pallets/dkg-metadata/src/weights.rs b/pallets/dkg-metadata/src/weights.rs
index b9e67138b..a816d6b6a 100644
--- a/pallets/dkg-metadata/src/weights.rs
+++ b/pallets/dkg-metadata/src/weights.rs
@@ -55,6 +55,14 @@ pub trait WeightInfo {
 	fn force_unjail_keygen() -> Weight;
 }
 
+	/// <HB SBP Review M2
+	///
+	/// The code is running under the release 0.9.39 however the weights are still using weights v1
+	/// since it is considering only one value (ref time). The output of the CLI should configure the weights
+	/// to use the `from_parts(ref time,proof size)` fn
+	///
+	/// HB SBP Review M2>
+
 /// Weight functions for `pallet_dkg_metadata`.
 pub struct WebbWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for WebbWeight<T> {
diff --git a/pallets/dkg-proposal-handler/src/lib.rs b/pallets/dkg-proposal-handler/src/lib.rs
index 165b864bd..f0c4fce98 100644
--- a/pallets/dkg-proposal-handler/src/lib.rs
+++ b/pallets/dkg-proposal-handler/src/lib.rs
@@ -463,6 +463,17 @@ pub mod pallet {
 		}
 	}
 
+	/// <HB SBP Review M2
+	///
+	/// Is the validate_unsigned fn used anywhere? I'm not able to find if this is used in any OCW.
+	///
+	/// We detected a vector atack with unsigned transactions from OCW: A malissuous validator might
+	/// push tempered data and the validators wouldn't be punished for this. Validators are only
+	/// subject to slashing if they create a block that violates the STF. Creating valid
+	/// transactions with tampered data would not result in slashing, and therefore the possibility
+	/// of malicious Validators in this context is real.
+	///
+	/// HB SBP Review M2>
 	#[pallet::validate_unsigned]
 	impl<T: Config> ValidateUnsigned for Pallet<T> {
 		type Call = Call<T>;
diff --git a/pallets/dkg-proposal-handler/src/weights.rs b/pallets/dkg-proposal-handler/src/weights.rs
index 3ee6ae16b..706d72f1b 100644
--- a/pallets/dkg-proposal-handler/src/weights.rs
+++ b/pallets/dkg-proposal-handler/src/weights.rs
@@ -51,6 +51,15 @@ pub trait WeightInfo {
 	fn submit_signed_proposals(n: u32) -> Weight;
 	fn force_submit_unsigned_proposal() -> Weight;
 }
+
+	/// <HB SBP Review M2
+	///
+	/// The code is running under the release 0.9.39 however the weights are still using weights v1
+	/// since it is considering only one value (ref time). The output of the CLI should configure the weights
+	/// to use the `from_parts(ref time,proof size)` fn
+	///
+	/// HB SBP Review M2>
+
 /// Weight functions for `pallet_dkg_proposal_handler`.
 pub struct WebbWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config>WeightInfo for WebbWeight<T> {
diff --git a/pallets/dkg-proposals/src/weights.rs b/pallets/dkg-proposals/src/weights.rs
index b6d548d17..a934c0ecf 100644
--- a/pallets/dkg-proposals/src/weights.rs
+++ b/pallets/dkg-proposals/src/weights.rs
@@ -61,6 +61,14 @@ pub trait WeightInfo {
 	fn eval_vote_state(_c: u32,) -> Weight;
 }
 
+	/// <HB SBP Review M2
+	///
+	/// The code is running under the release 0.9.39 however the weights are still using weights v1
+	/// since it is considering only one value (ref time). The output of the CLI should configure the weights
+	/// to use the `from_parts(ref time,proof size)` fn
+	///
+	/// HB SBP Review M2>
+
 /// Weight functions for `pallet_dkg_proposals`.
 pub struct WebbWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for WebbWeight<T> {