[Identity] Remove double encoding username signature payload (parityt…

…ech#4646)

In order to receive a username in `pallet-identity`, users have to,
among other things, provide a signature of the desired username. Right
now, there is an [extra encoding
step](https://github.com/paritytech/polkadot-sdk/blob/4ab078d6754147ce731523292dd1882f8a7b5775/substrate/frame/identity/src/lib.rs#L1119)
when generating the payload to sign.

Encoding a `Vec` adds extra bytes related to the length, which changes
the payload. This is unnecessary and confusing as users expect the
payload to sign to be just the username bytes. This PR fixes this issue
by validating the signature directly against the username bytes.

---------

Signed-off-by: georgepisaltu <[email protected]>

prdoc/pr_4646.prdoc

@@ -0,0 +1,20 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: "[Identity] Remove double encoding username signature payload"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      The signature payload for setting a username for an account in `pallet-identity` is now just
+      the raw bytes of said username (still including the suffix), removing the need to first
+      encode these bytes before signing.
+  - audience: Runtime User
+    description: |
+      The signature payload for setting a username for an account in `pallet-identity` is now just
+      the raw bytes of said username (still including the suffix), removing the need to first
+      encode these bytes before signing.
+
+crates:
+  - name: pallet-identity
+    bump: major

substrate/frame/identity/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "pallet-identity"
-version = "28.0.0"
+version = "29.0.0"
 authors.workspace = true
 edition.workspace = true
 license = "Apache-2.0"

substrate/frame/identity/src/benchmarking.rs

@@ -22,7 +22,6 @@
 use super::*;
 
 use crate::Pallet as Identity;
-use codec::Encode;
 use frame_benchmarking::{account, v2::*, whitelisted_caller, BenchmarkError};
 use frame_support::{
 	assert_ok, ensure,
@@ -623,17 +622,17 @@ mod benchmarks {
 
 		let username = bench_username();
 		let bounded_username = bounded_username::<T>(username.clone(), suffix.clone());
-		let encoded_username = Encode::encode(&bounded_username.to_vec());
 
 		let public = sr25519_generate(0.into(), None);
 		let who_account: T::AccountId = MultiSigner::Sr25519(public).into_account().into();
 		let who_lookup = T::Lookup::unlookup(who_account.clone());
 
-		let signature =
-			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
+		let signature = MultiSignature::Sr25519(
+			sr25519_sign(0.into(), &public, &bounded_username[..]).unwrap(),
+		);
 
 		// Verify signature here to avoid surprise errors at runtime
-		assert!(signature.verify(&encoded_username[..], &public.into()));
+		assert!(signature.verify(&bounded_username[..], &public.into()));
 
 		#[extrinsic_call]
 		_(RawOrigin::Signed(authority.clone()), who_lookup, username, Some(signature.into()));

substrate/frame/identity/src/lib.rs

@@ -1116,8 +1116,7 @@ pub mod pallet {
 			if let Some(s) = signature {
 				// Account has pre-signed an authorization. Verify the signature provided and grant
 				// the username directly.
-				let encoded = Encode::encode(&bounded_username.to_vec());
-				Self::validate_signature(&encoded, &s, &who)?;
+				Self::validate_signature(&bounded_username[..], &s, &who)?;
 				Self::insert_username(&who, bounded_username);
 			} else {
 				// The user must accept the username, therefore, queue it.
@@ -1267,12 +1266,12 @@ impl<T: Config> Pallet<T> {
 
 	/// Validate a signature. Supports signatures on raw `data` or `data` wrapped in HTML `<Bytes>`.
 	pub fn validate_signature(
-		data: &Vec<u8>,
+		data: &[u8],
 		signature: &T::OffchainSignature,
 		signer: &T::AccountId,
 	) -> DispatchResult {
 		// Happy path, user has signed the raw data.
-		if signature.verify(&data[..], &signer) {
+		if signature.verify(data, &signer) {
 			return Ok(())
 		}
 		// NOTE: for security reasons modern UIs implicitly wrap the data requested to sign into

substrate/frame/identity/src/tests.rs

@@ -1042,13 +1042,13 @@ fn set_username_with_signature_without_existing_identity_should_work() {
 
 		// set up username
 		let (username, username_to_sign) = test_username_of(b"42".to_vec(), suffix);
-		let encoded_username = Encode::encode(&username_to_sign.to_vec());
 
 		// set up user and sign message
 		let public = sr25519_generate(0.into(), None);
 		let who_account: AccountIdOf<Test> = MultiSigner::Sr25519(public).into_account().into();
-		let signature =
-			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
+		let signature = MultiSignature::Sr25519(
+			sr25519_sign(0.into(), &public, &username_to_sign[..]).unwrap(),
+		);
 
 		assert_ok!(Identity::set_username_for(
 			RuntimeOrigin::signed(authority),
@@ -1093,13 +1093,13 @@ fn set_username_with_signature_with_existing_identity_should_work() {
 
 		// set up username
 		let (username, username_to_sign) = test_username_of(b"42".to_vec(), suffix);
-		let encoded_username = Encode::encode(&username_to_sign.to_vec());
 
 		// set up user and sign message
 		let public = sr25519_generate(0.into(), None);
 		let who_account: AccountIdOf<Test> = MultiSigner::Sr25519(public).into_account().into();
-		let signature =
-			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
+		let signature = MultiSignature::Sr25519(
+			sr25519_sign(0.into(), &public, &username_to_sign[..]).unwrap(),
+		);
 
 		// Set an identity for who. They need some balance though.
 		Balances::make_free_balance_be(&who_account, 1000);
@@ -1156,13 +1156,13 @@ fn set_username_with_bytes_signature_should_work() {
 		let unwrapped_username = username_to_sign.to_vec();
 
 		// Sign an unwrapped version, as in `username.suffix`.
-		let unwrapped_encoded = Encode::encode(&unwrapped_username);
-		let signature_on_unwrapped =
-			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &unwrapped_encoded).unwrap());
+		let signature_on_unwrapped = MultiSignature::Sr25519(
+			sr25519_sign(0.into(), &public, &unwrapped_username[..]).unwrap(),
+		);
 
 		// Trivial
 		assert_ok!(Identity::validate_signature(
-			&unwrapped_encoded,
+			&unwrapped_username,
 			&signature_on_unwrapped,
 			&who_account
 		));
@@ -1174,15 +1174,15 @@ fn set_username_with_bytes_signature_should_work() {
 		let mut wrapped_username: Vec<u8> =
 			Vec::with_capacity(unwrapped_username.len() + prehtml.len() + posthtml.len());
 		wrapped_username.extend(prehtml);
-		wrapped_username.extend(unwrapped_encoded.clone());
+		wrapped_username.extend(&unwrapped_username);
 		wrapped_username.extend(posthtml);
 		let signature_on_wrapped =
 			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &wrapped_username).unwrap());
 
 		// We want to call `validate_signature` on the *unwrapped* username, but the signature on
 		// the *wrapped* data.
 		assert_ok!(Identity::validate_signature(
-			&unwrapped_encoded,
+			&unwrapped_username,
 			&signature_on_wrapped,
 			&who_account
 		));
@@ -1401,9 +1401,8 @@ fn setting_primary_should_work() {
 
 		// set up username
 		let (first_username, first_to_sign) = test_username_of(b"42".to_vec(), suffix.clone());
-		let encoded_username = Encode::encode(&first_to_sign.to_vec());
 		let first_signature =
-			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
+			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &first_to_sign[..]).unwrap());
 
 		assert_ok!(Identity::set_username_for(
 			RuntimeOrigin::signed(authority.clone()),
@@ -1427,9 +1426,8 @@ fn setting_primary_should_work() {
 
 		// set up username
 		let (second_username, second_to_sign) = test_username_of(b"101".to_vec(), suffix);
-		let encoded_username = Encode::encode(&second_to_sign.to_vec());
 		let second_signature =
-			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
+			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &second_to_sign[..]).unwrap());
 
 		assert_ok!(Identity::set_username_for(
 			RuntimeOrigin::signed(authority),
@@ -1510,10 +1508,8 @@ fn must_own_primary() {
 		let pi_account: AccountIdOf<Test> = MultiSigner::Sr25519(pi_public).into_account().into();
 		let (pi_username, pi_to_sign) =
 			test_username_of(b"username314159".to_vec(), suffix.clone());
-		let encoded_pi_username = Encode::encode(&pi_to_sign.to_vec());
-		let pi_signature = MultiSignature::Sr25519(
-			sr25519_sign(0.into(), &pi_public, &encoded_pi_username).unwrap(),
-		);
+		let pi_signature =
+			MultiSignature::Sr25519(sr25519_sign(0.into(), &pi_public, &pi_to_sign[..]).unwrap());
 		assert_ok!(Identity::set_username_for(
 			RuntimeOrigin::signed(authority.clone()),
 			pi_account.clone(),
@@ -1525,10 +1521,8 @@ fn must_own_primary() {
 		let e_public = sr25519_generate(1.into(), None);
 		let e_account: AccountIdOf<Test> = MultiSigner::Sr25519(e_public).into_account().into();
 		let (e_username, e_to_sign) = test_username_of(b"username271828".to_vec(), suffix.clone());
-		let encoded_e_username = Encode::encode(&e_to_sign.to_vec());
-		let e_signature = MultiSignature::Sr25519(
-			sr25519_sign(1.into(), &e_public, &encoded_e_username).unwrap(),
-		);
+		let e_signature =
+			MultiSignature::Sr25519(sr25519_sign(1.into(), &e_public, &e_to_sign[..]).unwrap());
 		assert_ok!(Identity::set_username_for(
 			RuntimeOrigin::signed(authority.clone()),
 			e_account.clone(),
@@ -1633,13 +1627,13 @@ fn removing_dangling_usernames_should_work() {
 
 		// set up username
 		let (username, username_to_sign) = test_username_of(b"42".to_vec(), suffix.clone());
-		let encoded_username = Encode::encode(&username_to_sign.to_vec());
 
 		// set up user and sign message
 		let public = sr25519_generate(0.into(), None);
 		let who_account: AccountIdOf<Test> = MultiSigner::Sr25519(public).into_account().into();
-		let signature =
-			MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
+		let signature = MultiSignature::Sr25519(
+			sr25519_sign(0.into(), &public, &username_to_sign[..]).unwrap(),
+		);
 
 		// Set an identity for who. They need some balance though.
 		Balances::make_free_balance_be(&who_account, 1000);
@@ -1657,11 +1651,10 @@ fn removing_dangling_usernames_should_work() {
 
 		// Now they set up a second username.
 		let (username_two, username_two_to_sign) = test_username_of(b"43".to_vec(), suffix);
-		let encoded_username_two = Encode::encode(&username_two_to_sign.to_vec());
 
 		// set up user and sign message
 		let signature_two = MultiSignature::Sr25519(
-			sr25519_sign(0.into(), &public, &encoded_username_two).unwrap(),
+			sr25519_sign(0.into(), &public, &username_two_to_sign[..]).unwrap(),
 		);
 
 		assert_ok!(Identity::set_username_for(