[NFTs] Improve offchain signature validation (#13960)

* Improve signature validation

* Rework

substrate/frame/nfts/src/common_functions.rs

@@ -18,6 +18,7 @@
 //! Various pieces of common functionality.
 
 use crate::*;
+use frame_support::pallet_prelude::*;
 
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// Get the owner of the item, if the item exists.
@@ -30,6 +31,30 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		Collection::<T, I>::get(collection).map(|i| i.owner)
 	}
 
+	/// Validate the `data` was signed by `signer` and the `signature` is correct.
+	pub fn validate_signature(
+		data: &Vec<u8>,
+		signature: &T::OffchainSignature,
+		signer: &T::AccountId,
+	) -> DispatchResult {
+		if signature.verify(&**data, &signer) {
+			return Ok(())
+		}
+
+		// NOTE: for security reasons modern UIs implicitly wrap the data requested to sign into
+		// <Bytes></Bytes>, that's why we support both wrapped and raw versions.
+		let prefix = b"<Bytes>";
+		let suffix = b"</Bytes>";
+		let mut wrapped: Vec<u8> = Vec::with_capacity(data.len() + prefix.len() + suffix.len());
+		wrapped.extend(prefix);
+		wrapped.extend(data);
+		wrapped.extend(suffix);
+
+		ensure!(signature.verify(&*wrapped, &signer), Error::<T, I>::WrongSignature);
+
+		Ok(())
+	}
+
 	#[cfg(any(test, feature = "runtime-benchmarks"))]
 	pub fn set_next_id(id: T::CollectionId) {
 		NextCollectionId::<T, I>::set(Some(id));

substrate/frame/nfts/src/lib.rs

@@ -50,7 +50,7 @@ use frame_support::traits::{
 };
 use frame_system::Config as SystemConfig;
 use sp_runtime::{
-	traits::{Saturating, StaticLookup, Zero},
+	traits::{IdentifyAccount, Saturating, StaticLookup, Verify, Zero},
 	RuntimeDebug,
 };
 use sp_std::prelude::*;
@@ -69,7 +69,6 @@ pub mod pallet {
 	use super::*;
 	use frame_support::{pallet_prelude::*, traits::ExistenceRequirement};
 	use frame_system::pallet_prelude::*;
-	use sp_runtime::traits::{IdentifyAccount, Verify};
 
 	/// The current storage version.
 	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
@@ -1841,8 +1840,7 @@ pub mod pallet {
 			signer: T::AccountId,
 		) -> DispatchResult {
 			let origin = ensure_signed(origin)?;
-			let msg = Encode::encode(&mint_data);
+			Self::validate_signature(&Encode::encode(&mint_data), &signature, &signer)?;
-			ensure!(signature.verify(&*msg, &signer), Error::<T, I>::WrongSignature);
 			Self::do_mint_pre_signed(origin, mint_data, signer)
 		}
 
@@ -1868,8 +1866,7 @@ pub mod pallet {
 			signer: T::AccountId,
 		) -> DispatchResult {
 			let origin = ensure_signed(origin)?;
-			let msg = Encode::encode(&data);
+			Self::validate_signature(&Encode::encode(&data), &signature, &signer)?;
-			ensure!(signature.verify(&*msg, &signer), Error::<T, I>::WrongSignature);
 			Self::do_set_attributes_pre_signed(origin, data, signer)
 		}
 	}

substrate/frame/nfts/src/tests.rs

@@ -3140,6 +3140,34 @@ fn add_remove_item_attributes_approval_should_work() {
 	})
 }
 
+#[test]
+fn validate_signature() {
+	new_test_ext().execute_with(|| {
+		let user_1_pair = sp_core::sr25519::Pair::from_string("//Alice", None).unwrap();
+		let user_1_signer = MultiSigner::Sr25519(user_1_pair.public());
+		let user_1 = user_1_signer.clone().into_account();
+		let mint_data: PreSignedMint<u32, u32, AccountId, u32> = PreSignedMint {
+			collection: 0,
+			item: 0,
+			attributes: vec![],
+			metadata: vec![],
+			only_account: None,
+			deadline: 100000,
+		};
+		let encoded_data = Encode::encode(&mint_data);
+		let signature = MultiSignature::Sr25519(user_1_pair.sign(&encoded_data));
+		assert_ok!(Nfts::validate_signature(&encoded_data, &signature, &user_1));
+
+		let mut wrapped_data: Vec<u8> = Vec::new();
+		wrapped_data.extend(b"<Bytes>");
+		wrapped_data.extend(&encoded_data);
+		wrapped_data.extend(b"</Bytes>");
+
+		let signature = MultiSignature::Sr25519(user_1_pair.sign(&wrapped_data));
+		assert_ok!(Nfts::validate_signature(&encoded_data, &signature, &user_1));
+	})
+}
+
 #[test]
 fn pre_signed_mints_should_work() {
 	new_test_ext().execute_with(|| {