From f4ee5e93202f310531015e2f0ef946ec32770797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 2 Sep 2022 14:22:29 +0200 Subject: [PATCH 1/6] pallet-identity: Be more paranoid ;) Check that a registrar is providing judgement for the correct identity. --- frame/identity/src/lib.rs | 12 +++++- frame/identity/src/tests.rs | 85 +++++++++++++++++++++++++++++++++---- 2 files changed, 86 insertions(+), 11 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 0f80acceb949c..bc3887b2b0833 100644 --- a/frame/identity/src/lib.rs +++ b/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, Saturating, StaticLookup, Zero, Hash}; 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, judgement: Judgement>, + identity: T::Hash, ) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let target = T::Lookup::lookup(target)?; @@ -772,10 +776,14 @@ pub mod pallet { >::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::::InvalidIndex)?; let mut id = >::get(&target).ok_or(Error::::InvalidTarget)?; + if T::Hashing::hash_of(&id.info) != identity { + return Err(Error::::JudgementForDifferentIdentity.into()) + } + let item = (reg_index, judgement); match id.judgements.binary_search_by_key(®_index, |x| x.0) { Ok(position) => { diff --git a/frame/identity/src/tests.rs b/frame/identity/src/tests.rs index a0773e9904a1c..4c1ced0a83324 100644 --- a/frame/identity/src/tests.rs +++ b/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::::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::::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::::JudgementForDifferentIdentity + ); + + let identity_hash = BlakeTwo256::hash_of(&ten()); + + assert_noop!( + Identity::provide_judgement( + Origin::signed(10), + 0, + 10, + Judgement::Reasonable, + identity_hash + ), Error::::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::::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::::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::::JudgementGiven @@ -438,7 +493,13 @@ fn requesting_judgement_should_work() { Identity::request_judgement(Origin::signed(10), 0, 10), Error::::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)); }); } From 6a4a4087fa72b349d5e2f6fb3943a4ce7a31f824 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 2 Sep 2022 14:37:20 +0200 Subject: [PATCH 2/6] Fixes --- frame/identity/src/benchmarking.rs | 38 +++++++++++++++++------------- frame/identity/src/lib.rs | 2 +- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 5d409f48bf567..42cad69eee445 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -138,7 +138,7 @@ benchmarks! { // Add an initial identity let initial_info = create_identity_info::(1); - Identity::::set_identity(caller_origin.clone(), Box::new(initial_info))?; + Identity::::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 +147,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 +201,13 @@ benchmarks! { let caller: T::AccountId = whitelisted_caller(); let _ = add_sub_accounts::(&caller, s)?; }; - let x in 1 .. T::MaxAdditionalFields::get() => { - // Create their main identity with x additional fields - let info = create_identity_info::(x); - let caller: T::AccountId = whitelisted_caller(); - let caller_origin = ::Origin::from(RawOrigin::Signed(caller)); - Identity::::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::(x); + let caller: T::AccountId = whitelisted_caller(); + let caller_origin = ::Origin::from(RawOrigin::Signed(caller.clone())); + Identity::::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 +216,8 @@ benchmarks! { RawOrigin::Signed(account("registrar", i, SEED)).into(), i, caller_lookup.clone(), - Judgement::Reasonable + Judgement::Reasonable, + T::Hashing::hash_of(&info), )?; } ensure!(IdentityOf::::contains_key(&caller), "Identity does not exist."); @@ -328,15 +330,16 @@ benchmarks! { let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; - let x in 1 .. T::MaxAdditionalFields::get() => { - let info = create_identity_info::(x); - Identity::::set_identity(user_origin.clone(), Box::new(info))?; - }; + let x in 1 .. T::MaxAdditionalFields::get(); + + let info = create_identity_info::(x); + let info_hash = T::Hashing::hash_of(&info); + Identity::::set_identity(user_origin.clone(), Box::new(info))?; let registrar_origin = T::RegistrarOrigin::successful_origin(); Identity::::add_registrar(registrar_origin, caller_lookup)?; Identity::::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::(Event::::JudgementGiven { target: user, registrar_index: r }.into()) } @@ -352,7 +355,7 @@ benchmarks! { let _ = T::Currency::make_free_balance_be(&target, BalanceOf::::max_value()); let info = create_identity_info::(x); - Identity::::set_identity(target_origin.clone(), Box::new(info))?; + Identity::::set_identity(target_origin.clone(), Box::new(info.clone()))?; let _ = add_sub_accounts::(&target, s)?; // User requests judgement from all the registrars, and they approve @@ -362,7 +365,8 @@ benchmarks! { RawOrigin::Signed(account("registrar", i, SEED)).into(), i, target_lookup.clone(), - Judgement::Reasonable + Judgement::Reasonable, + T::Hashing::hash_of(&info), )?; } ensure!(IdentityOf::::contains_key(&target), "Identity not set"); diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index bc3887b2b0833..d24b17eb86ee0 100644 --- a/frame/identity/src/lib.rs +++ b/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, Hash}; +use sp_runtime::traits::{AppendZerosInput, Hash, Saturating, StaticLookup, Zero}; use sp_std::prelude::*; pub use weights::WeightInfo; From 9edbfee36410800d3eb2e930f641d9c871ad73eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 2 Sep 2022 14:42:40 +0200 Subject: [PATCH 3/6] Fix alliance --- frame/alliance/src/mock.rs | 56 +++++++++++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/frame/alliance/src/mock.rs b/frame/alliance/src/mock.rs index f2632ec3db08c..921f83611aec0 100644 --- a/frame/alliance/src/mock.rs +++ b/frame/alliance/src/mock.rs @@ -305,20 +305,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, + T::Hashing::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, + T::Hashing::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, + T::Hashing::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, + T::Hashing::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, + T::Hashing::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, + T::Hashing::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, + T::Hashing::hash_of(&info) + )); // Joining before init should fail. assert_noop!( From 9baf5da98eb3c7c5c35398b4d52c54a53fe52bf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 2 Sep 2022 16:03:56 +0200 Subject: [PATCH 4/6] :facepalm: --- frame/alliance/src/mock.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/frame/alliance/src/mock.rs b/frame/alliance/src/mock.rs index 921f83611aec0..d5b861c2be0ef 100644 --- a/frame/alliance/src/mock.rs +++ b/frame/alliance/src/mock.rs @@ -310,7 +310,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 1, Judgement::KnownGood, - T::Hashing::hash_of(&info) + BlakeTwo256::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(2), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -318,7 +318,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 2, Judgement::KnownGood, - T::Hashing::hash_of(&info) + BlakeTwo256::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(3), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -326,7 +326,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 3, Judgement::KnownGood, - T::Hashing::hash_of(&info) + BlakeTwo256::Hashing::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(4), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -334,7 +334,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 4, Judgement::KnownGood, - T::Hashing::hash_of(&info) + BlakeTwo256::Hashing::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(5), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -342,7 +342,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 5, Judgement::KnownGood, - T::Hashing::hash_of(&info) + BlakeTwo256::Hashing::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()))); @@ -351,7 +351,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 8, Judgement::KnownGood, - T::Hashing::hash_of(&info) + BlakeTwo256::Hashing::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(9), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -359,7 +359,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 9, Judgement::KnownGood, - T::Hashing::hash_of(&info) + BlakeTwo256::Hashing::hash_of(&info) )); // Joining before init should fail. From abd6f980bfe0885f8aea1118b1879dc32daa13db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 2 Sep 2022 18:48:22 +0200 Subject: [PATCH 5/6] Fixes --- frame/alliance/src/mock.rs | 10 +++++----- frame/benchmarking/src/lib.rs | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/frame/alliance/src/mock.rs b/frame/alliance/src/mock.rs index d5b861c2be0ef..89f660cf401fe 100644 --- a/frame/alliance/src/mock.rs +++ b/frame/alliance/src/mock.rs @@ -326,7 +326,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 3, Judgement::KnownGood, - BlakeTwo256::Hashing::hash_of(&info) + BlakeTwo256::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(4), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -334,7 +334,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 4, Judgement::KnownGood, - BlakeTwo256::Hashing::hash_of(&info) + BlakeTwo256::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(5), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -342,7 +342,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 5, Judgement::KnownGood, - BlakeTwo256::Hashing::hash_of(&info) + 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()))); @@ -351,7 +351,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 8, Judgement::KnownGood, - BlakeTwo256::Hashing::hash_of(&info) + BlakeTwo256::hash_of(&info) )); assert_ok!(Identity::set_identity(Origin::signed(9), Box::new(info.clone()))); assert_ok!(Identity::provide_judgement( @@ -359,7 +359,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { 0, 9, Judgement::KnownGood, - BlakeTwo256::Hashing::hash_of(&info) + BlakeTwo256::hash_of(&info) )); // Joining before init should fail. diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 18472595f15b9..614216a96940a 100644 --- a/frame/benchmarking/src/lib.rs +++ b/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 From 496d6d5368b90e13ddacf3d091a4a0417b20ac6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 2 Sep 2022 19:44:23 +0200 Subject: [PATCH 6/6] ... --- frame/identity/src/benchmarking.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 42cad69eee445..6596f227383f2 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -76,9 +76,11 @@ fn create_sub_accounts( } // Set identity so `set_subs` does not fail. - let _ = T::Currency::make_free_balance_be(who, BalanceOf::::max_value() / 2u32.into()); - let info = create_identity_info::(1); - Identity::::set_identity(who_origin.into(), Box::new(info))?; + if IdentityOf::::get(who).is_none() { + let _ = T::Currency::make_free_balance_be(who, BalanceOf::::max_value() / 2u32.into()); + let info = create_identity_info::(1); + Identity::::set_identity(who_origin.into(), Box::new(info))?; + } Ok(subs) }