pallet-identity: Be more paranoid ;) (#12170)

* pallet-identity: Be more paranoid ;)

Check that a registrar is providing judgement for the correct identity.

* Fixes

* Fix alliance

* 🤦

* Fixes

* ...

substrate/frame/alliance/src/mock.rs

@@ -297,20 +297,62 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
 			twitter: Data::default(),
 		};
 		assert_ok!(Identity::set_identity(Origin::signed(1), Box::new(info.clone())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 1, Judgement::KnownGood));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(1),
+			0,
+			1,
+			Judgement::KnownGood,
+			BlakeTwo256::hash_of(&info)
+		));
 		assert_ok!(Identity::set_identity(Origin::signed(2), Box::new(info.clone())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 2, Judgement::KnownGood));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(1),
+			0,
+			2,
+			Judgement::KnownGood,
+			BlakeTwo256::hash_of(&info)
+		));
 		assert_ok!(Identity::set_identity(Origin::signed(3), Box::new(info.clone())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 3, Judgement::KnownGood));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(1),
+			0,
+			3,
+			Judgement::KnownGood,
+			BlakeTwo256::hash_of(&info)
+		));
 		assert_ok!(Identity::set_identity(Origin::signed(4), Box::new(info.clone())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 4, Judgement::KnownGood));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(1),
+			0,
+			4,
+			Judgement::KnownGood,
+			BlakeTwo256::hash_of(&info)
+		));
 		assert_ok!(Identity::set_identity(Origin::signed(5), Box::new(info.clone())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 5, Judgement::KnownGood));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(1),
+			0,
+			5,
+			Judgement::KnownGood,
+			BlakeTwo256::hash_of(&info)
+		));
 		assert_ok!(Identity::set_identity(Origin::signed(6), Box::new(info.clone())));
 		assert_ok!(Identity::set_identity(Origin::signed(8), Box::new(info.clone())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 8, Judgement::KnownGood));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(1),
+			0,
+			8,
+			Judgement::KnownGood,
+			BlakeTwo256::hash_of(&info)
+		));
 		assert_ok!(Identity::set_identity(Origin::signed(9), Box::new(info.clone())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 9, Judgement::KnownGood));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(1),
+			0,
+			9,
+			Judgement::KnownGood,
+			BlakeTwo256::hash_of(&info)
+		));
 
 		// Joining before init should fail.
 		assert_noop!(

substrate/frame/benchmarking/src/lib.rs

@@ -86,12 +86,12 @@ macro_rules! whitelist {
 /// ```
 ///
 /// Note that due to parsing restrictions, if the `from` expression is not a single token (i.e. a
-/// literal or constant), then it must be parenthesised.
+/// literal or constant), then it must be parenthesized.
 ///
 /// The macro allows for a number of "arms", each representing an individual benchmark. Using the
 /// simple syntax, the associated dispatchable function maps 1:1 with the benchmark and the name of
 /// the benchmark is the same as that of the associated function. However, extended syntax allows
-/// for arbitrary expresions to be evaluated in a benchmark (including for example,
+/// for arbitrary expressions to be evaluated in a benchmark (including for example,
 /// `on_initialize`).
 ///
 /// Note that the ranges are *inclusive* on both sides. This is in contrast to ranges in Rust which

substrate/frame/identity/src/benchmarking.rs

@@ -76,9 +76,11 @@ fn create_sub_accounts<T: Config>(
 	}
 
 	// Set identity so `set_subs` does not fail.
-	let _ = T::Currency::make_free_balance_be(who, BalanceOf::<T>::max_value() / 2u32.into());
-	let info = create_identity_info::<T>(1);
-	Identity::<T>::set_identity(who_origin.into(), Box::new(info))?;
+	if IdentityOf::<T>::get(who).is_none() {
+		let _ = T::Currency::make_free_balance_be(who, BalanceOf::<T>::max_value() / 2u32.into());
+		let info = create_identity_info::<T>(1);
+		Identity::<T>::set_identity(who_origin.into(), Box::new(info))?;
+	}
 
 	Ok(subs)
 }
@@ -138,7 +140,7 @@ benchmarks! {
 
 			// Add an initial identity
 			let initial_info = create_identity_info::<T>(1);
-			Identity::<T>::set_identity(caller_origin.clone(), Box::new(initial_info))?;
+			Identity::<T>::set_identity(caller_origin.clone(), Box::new(initial_info.clone()))?;
 
 			// User requests judgement from all the registrars, and they approve
 			for i in 0..r {
@@ -147,7 +149,8 @@ benchmarks! {
 					RawOrigin::Signed(account("registrar", i, SEED)).into(),
 					i,
 					caller_lookup.clone(),
-					Judgement::Reasonable
+					Judgement::Reasonable,
+					T::Hashing::hash_of(&initial_info),
 				)?;
 			}
 			caller
@@ -200,13 +203,13 @@ benchmarks! {
 			let caller: T::AccountId = whitelisted_caller();
 			let _ = add_sub_accounts::<T>(&caller, s)?;
 		};
-		let x in 1 .. T::MaxAdditionalFields::get() => {
-			// Create their main identity with x additional fields
-			let info = create_identity_info::<T>(x);
-			let caller: T::AccountId = whitelisted_caller();
-			let caller_origin = <T as frame_system::Config>::Origin::from(RawOrigin::Signed(caller));
-			Identity::<T>::set_identity(caller_origin, Box::new(info))?;
-		};
+		let x in 1 .. T::MaxAdditionalFields::get();
+
+		// Create their main identity with x additional fields
+		let info = create_identity_info::<T>(x);
+		let caller: T::AccountId = whitelisted_caller();
+		let caller_origin = <T as frame_system::Config>::Origin::from(RawOrigin::Signed(caller.clone()));
+		Identity::<T>::set_identity(caller_origin.clone(), Box::new(info.clone()))?;
 
 		// User requests judgement from all the registrars, and they approve
 		for i in 0..r {
@@ -215,7 +218,8 @@ benchmarks! {
 				RawOrigin::Signed(account("registrar", i, SEED)).into(),
 				i,
 				caller_lookup.clone(),
-				Judgement::Reasonable
+				Judgement::Reasonable,
+				T::Hashing::hash_of(&info),
 			)?;
 		}
 		ensure!(IdentityOf::<T>::contains_key(&caller), "Identity does not exist.");
@@ -328,15 +332,16 @@ benchmarks! {
 		let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
 
 		let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::<T>(r)?;
-		let x in 1 .. T::MaxAdditionalFields::get() => {
-			let info = create_identity_info::<T>(x);
-			Identity::<T>::set_identity(user_origin.clone(), Box::new(info))?;
-		};
+		let x in 1 .. T::MaxAdditionalFields::get();
+
+		let info = create_identity_info::<T>(x);
+		let info_hash = T::Hashing::hash_of(&info);
+		Identity::<T>::set_identity(user_origin.clone(), Box::new(info))?;
 
 		let registrar_origin = T::RegistrarOrigin::successful_origin();
 		Identity::<T>::add_registrar(registrar_origin, caller_lookup)?;
 		Identity::<T>::request_judgement(user_origin, r, 10u32.into())?;
-	}: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable)
+	}: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable, info_hash)
 	verify {
 		assert_last_event::<T>(Event::<T>::JudgementGiven { target: user, registrar_index: r }.into())
 	}
@@ -352,7 +357,7 @@ benchmarks! {
 		let _ = T::Currency::make_free_balance_be(&target, BalanceOf::<T>::max_value());
 
 		let info = create_identity_info::<T>(x);
-		Identity::<T>::set_identity(target_origin.clone(), Box::new(info))?;
+		Identity::<T>::set_identity(target_origin.clone(), Box::new(info.clone()))?;
 		let _ = add_sub_accounts::<T>(&target, s)?;
 
 		// User requests judgement from all the registrars, and they approve
@@ -362,7 +367,8 @@ benchmarks! {
 				RawOrigin::Signed(account("registrar", i, SEED)).into(),
 				i,
 				target_lookup.clone(),
-				Judgement::Reasonable
+				Judgement::Reasonable,
+				T::Hashing::hash_of(&info),
 			)?;
 		}
 		ensure!(IdentityOf::<T>::contains_key(&target), "Identity not set");

substrate/frame/identity/src/lib.rs

@@ -79,7 +79,7 @@ mod types;
 pub mod weights;
 
 use frame_support::traits::{BalanceStatus, Currency, OnUnbalanced, ReservableCurrency};
-use sp_runtime::traits::{AppendZerosInput, Saturating, StaticLookup, Zero};
+use sp_runtime::traits::{AppendZerosInput, Hash, Saturating, StaticLookup, Zero};
 use sp_std::prelude::*;
 pub use weights::WeightInfo;
 
@@ -236,6 +236,8 @@ pub mod pallet {
 		NotSub,
 		/// Sub-account isn't owned by sender.
 		NotOwned,
+		/// The provided judgement was for a different identity.
+		JudgementForDifferentIdentity,
 	}
 
 	#[pallet::event]
@@ -746,6 +748,7 @@ pub mod pallet {
 		/// - `target`: the account whose identity the judgement is upon. This must be an account
 		///   with a registered identity.
 		/// - `judgement`: the judgement of the registrar of index `reg_index` about `target`.
+		/// - `identity`: The hash of the [`IdentityInfo`] for that the judgement is provided.
 		///
 		/// Emits `JudgementGiven` if successful.
 		///
@@ -765,6 +768,7 @@ pub mod pallet {
 			#[pallet::compact] reg_index: RegistrarIndex,
 			target: AccountIdLookupOf<T>,
 			judgement: Judgement<BalanceOf<T>>,
+			identity: T::Hash,
 		) -> DispatchResultWithPostInfo {
 			let sender = ensure_signed(origin)?;
 			let target = T::Lookup::lookup(target)?;
@@ -772,10 +776,14 @@ pub mod pallet {
 			<Registrars<T>>::get()
 				.get(reg_index as usize)
 				.and_then(Option::as_ref)
-				.and_then(|r| if r.account == sender { Some(r) } else { None })
+				.filter(|r| r.account == sender)
 				.ok_or(Error::<T>::InvalidIndex)?;
 			let mut id = <IdentityOf<T>>::get(&target).ok_or(Error::<T>::InvalidTarget)?;
 
+			if T::Hashing::hash_of(&id.info) != identity {
+				return Err(Error::<T>::JudgementForDifferentIdentity.into())
+			}
+
 			let item = (reg_index, judgement);
 			match id.judgements.binary_search_by_key(&reg_index, |x| x.0) {
 				Ok(position) => {

substrate/frame/identity/src/tests.rs

@@ -279,27 +279,70 @@ fn registration_should_work() {
 fn uninvited_judgement_should_work() {
 	new_test_ext().execute_with(|| {
 		assert_noop!(
-			Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable),
+			Identity::provide_judgement(
+				Origin::signed(3),
+				0,
+				10,
+				Judgement::Reasonable,
+				H256::random()
+			),
 			Error::<Test>::InvalidIndex
 		);
 
 		assert_ok!(Identity::add_registrar(Origin::signed(1), 3));
 		assert_noop!(
-			Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable),
+			Identity::provide_judgement(
+				Origin::signed(3),
+				0,
+				10,
+				Judgement::Reasonable,
+				H256::random()
+			),
 			Error::<Test>::InvalidTarget
 		);
 
 		assert_ok!(Identity::set_identity(Origin::signed(10), Box::new(ten())));
 		assert_noop!(
-			Identity::provide_judgement(Origin::signed(10), 0, 10, Judgement::Reasonable),
+			Identity::provide_judgement(
+				Origin::signed(3),
+				0,
+				10,
+				Judgement::Reasonable,
+				H256::random()
+			),
+			Error::<Test>::JudgementForDifferentIdentity
+		);
+
+		let identity_hash = BlakeTwo256::hash_of(&ten());
+
+		assert_noop!(
+			Identity::provide_judgement(
+				Origin::signed(10),
+				0,
+				10,
+				Judgement::Reasonable,
+				identity_hash
+			),
 			Error::<Test>::InvalidIndex
 		);
 		assert_noop!(
-			Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::FeePaid(1)),
+			Identity::provide_judgement(
+				Origin::signed(3),
+				0,
+				10,
+				Judgement::FeePaid(1),
+				identity_hash
+			),
 			Error::<Test>::InvalidJudgement
 		);
 
-		assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(3),
+			0,
+			10,
+			Judgement::Reasonable,
+			identity_hash
+		));
 		assert_eq!(Identity::identity(10).unwrap().judgements, vec![(0, Judgement::Reasonable)]);
 	});
 }
@@ -309,7 +352,13 @@ fn clearing_judgement_should_work() {
 	new_test_ext().execute_with(|| {
 		assert_ok!(Identity::add_registrar(Origin::signed(1), 3));
 		assert_ok!(Identity::set_identity(Origin::signed(10), Box::new(ten())));
-		assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(3),
+			0,
+			10,
+			Judgement::Reasonable,
+			BlakeTwo256::hash_of(&ten())
+		));
 		assert_ok!(Identity::clear_identity(Origin::signed(10)));
 		assert_eq!(Identity::identity(10), None);
 	});
@@ -411,7 +460,13 @@ fn cancelling_requested_judgement_should_work() {
 		assert_eq!(Balances::free_balance(10), 90);
 		assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::<Test>::NotFound);
 
-		assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(3),
+			0,
+			10,
+			Judgement::Reasonable,
+			BlakeTwo256::hash_of(&ten())
+		));
 		assert_noop!(
 			Identity::cancel_request(Origin::signed(10), 0),
 			Error::<Test>::JudgementGiven
@@ -438,7 +493,13 @@ fn requesting_judgement_should_work() {
 			Identity::request_judgement(Origin::signed(10), 0, 10),
 			Error::<Test>::StickyJudgement
 		);
-		assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Erroneous));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(3),
+			0,
+			10,
+			Judgement::Erroneous,
+			BlakeTwo256::hash_of(&ten())
+		));
 		// Registrar got their payment now.
 		assert_eq!(Balances::free_balance(3), 20);
 
@@ -453,7 +514,13 @@ fn requesting_judgement_should_work() {
 		assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10));
 
 		// Re-requesting after the judgement has been reduced works.
-		assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::OutOfDate));
+		assert_ok!(Identity::provide_judgement(
+			Origin::signed(3),
+			0,
+			10,
+			Judgement::OutOfDate,
+			BlakeTwo256::hash_of(&ten())
+		));
 		assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10));
 	});
 }