Merge pull request #1310 from galacticcouncil/fix_claim_account

fix: claim account with wrapped message

Author: enthusiastmartin

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "runtime-integration-tests"
-version = "1.61.0"
+version = "1.61.1"
 description = "Integration tests"
 authors = ["GalacticCouncil"]
 edition = "2021"

integration-tests/Cargo.toml

@@ -544,7 +544,7 @@ mod account_conversion {
 				.pre_dispatch(&account, &call, &info, len);
 			assert_ok!(&pre);
 
-			let result = call.clone().dispatch(RuntimeOrigin::signed(account.clone().into()));
+			let result = call.clone().dispatch(RuntimeOrigin::signed(account.clone()));
 			assert_ok!(result);
 
 			assert_ok!(
@@ -1249,7 +1249,7 @@ mod currency_precompile {
 				})
 			);
 
-			let stored = pallet_evm_accounts::Pallet::<Runtime>::get_allowance(HDX.into(), owner, spender);
+			let stored = pallet_evm_accounts::Pallet::<Runtime>::get_allowance(HDX, owner, spender);
 			assert_eq!(stored, amount);
 		});
 	}
@@ -1339,13 +1339,13 @@ mod currency_precompile {
 
 			approve(80u128 * UNITS);
 			assert_eq!(
-				pallet_evm_accounts::Pallet::<Runtime>::get_allowance(HDX.into(), owner, spender),
+				pallet_evm_accounts::Pallet::<Runtime>::get_allowance(HDX, owner, spender),
 				80u128 * UNITS
 			);
 
 			approve(0);
 			assert_eq!(
-				pallet_evm_accounts::Pallet::<Runtime>::get_allowance(HDX.into(), owner, spender),
+				pallet_evm_accounts::Pallet::<Runtime>::get_allowance(HDX, owner, spender),
 				0
 			);
 		});
@@ -2864,7 +2864,7 @@ mod account_marking {
 				10000 * UNITS as i128,
 			));
 
-			let state = frame_system::Pallet::<Runtime>::account(&user_acc.address());
+			let state = frame_system::Pallet::<Runtime>::account(user_acc.address());
 			assert_eq!(state.providers, 1);
 			assert_eq!(state.sufficients, 0);
 			assert_eq!(state.nonce, 0);
@@ -2914,7 +2914,7 @@ mod account_marking {
 			));
 
 			// Verify the nonce and sufficients were incremented through EVM transactions
-			let state = frame_system::Pallet::<Runtime>::account(&user_acc.address());
+			let state = frame_system::Pallet::<Runtime>::account(user_acc.address());
 			assert_eq!(state.providers, 1);
 			assert_eq!(state.sufficients, 1);
 			assert_eq!(state.nonce, 3,);
@@ -2979,7 +2979,7 @@ mod account_marking {
 				initial_amount as i128,
 			));
 
-			let state = frame_system::Pallet::<Runtime>::account(&user_acc.address());
+			let state = frame_system::Pallet::<Runtime>::account(user_acc.address());
 			assert_eq!(state.providers, 1);
 
 			assert_ok!(Omnipool::sell(
@@ -3042,7 +3042,7 @@ mod account_marking {
 			let free_hdx = Currencies::free_balance(HDX, &user_acc.address());
 			assert_eq!(free_hdx, 0);
 
-			let state = frame_system::Pallet::<Runtime>::account(&user_acc.address());
+			let state = frame_system::Pallet::<Runtime>::account(user_acc.address());
 			assert_eq!(state.providers, 0);
 			assert_eq!(state.sufficients, 1);
 			assert_eq!(state.nonce, 3);
@@ -3120,7 +3120,7 @@ mod account_marking {
 				initial_amount as i128,
 			));
 
-			let state = frame_system::Pallet::<Runtime>::account(&user_acc.address());
+			let state = frame_system::Pallet::<Runtime>::account(user_acc.address());
 			assert_eq!(state.providers, 1);
 
 			assert_ok!(Omnipool::sell(
@@ -3185,7 +3185,7 @@ mod account_marking {
 			let free_hdx = Currencies::free_balance(HDX, &user_acc.address());
 			assert!(free_hdx > 0);
 
-			let state = frame_system::Pallet::<Runtime>::account(&user_acc.address());
+			let state = frame_system::Pallet::<Runtime>::account(user_acc.address());
 			assert_eq!(state.providers, 1);
 			assert_eq!(state.sufficients, 1);
 			assert_eq!(state.nonce, 0);
@@ -3915,7 +3915,7 @@ fn raw_eip1559_eth_call_with_params(is_batch: bool, nonce: U256, input: Vec<u8>)
 		gas_limit: gas_limit.into(),
 		action: TransactionAction::Call(DISPATCH_ADDR),
 		value: U256::zero(),
-		input: input_data.clone().into(),
+		input: input_data.clone(),
 		access_list: vec![],
 	};
 
@@ -3937,7 +3937,7 @@ fn raw_eip1559_eth_call_with_params(is_batch: bool, nonce: U256, input: Vec<u8>)
 		gas_limit: gas_limit.into(),
 		action: TransactionAction::Call(DISPATCH_ADDR),
 		value: U256::zero(),
-		input: input_data.into(),
+		input: input_data,
 		access_list: vec![],
 		odd_y_parity,
 		r: H256::from(rs.r.b32()),
@@ -4043,7 +4043,7 @@ fn raw_legacy_eth_call_with_params(is_batch: bool, nonce: U256, input: Vec<u8>)
 		gas_limit: gas_limit.into(),
 		action: TransactionAction::Call(DISPATCH_ADDR),
 		value: U256::zero(),
-		input: input_data.clone().into(),
+		input: input_data.clone(),
 		chain_id: None,
 	};
 
@@ -4067,7 +4067,7 @@ fn raw_legacy_eth_call_with_params(is_batch: bool, nonce: U256, input: Vec<u8>)
 		gas_limit: gas_limit.into(),
 		action: TransactionAction::Call(DISPATCH_ADDR),
 		value: U256::zero(),
-		input: input_data.into(),
+		input: input_data,
 		signature,
 	};
 
@@ -4667,7 +4667,7 @@ mod evm_error_decoder {
 				..Config::default()
 			});
 
-			let _ = runner
+			runner
 				.run(&random_error_string(), |value| {
 					let call_result = CallResult {
 						exit_reason: ExitReason::Error(ExitError::Other("Some error".into())),
@@ -5003,7 +5003,7 @@ mod evm_error_decoder {
 
 	#[test]
 	fn test_scale_decode_malicious_payload() {
-		let malicious_payloads = vec![
+		let malicious_payloads = [
 			// Looks like Module error (discriminant 3) with crafted data
 			vec![0x03, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00],
 			// Looks like Other variant (discriminant 0) with invalid string data
@@ -5026,7 +5026,7 @@ mod evm_error_decoder {
 	}
 
 	#[test]
-	fn dispatch_decode_with_malformed_scawle_payloads_should_not_panic() {
+	fn dispatch_decode_with_malformed_scale_payloads_should_not_panic() {
 		// Test various malicious/malformed SCALE-encoded payloads
 		// that could trigger panics in decode_with_depth_limit
 		let test_cases = vec![
@@ -5062,12 +5062,12 @@ mod evm_error_decoder {
 			};
 
 			let _result = EvmErrorDecoder::convert(call_result.clone());
-			DispatchError::decode_with_depth_limit(MAX_DECODE_DEPTH, &mut &call_result.value[..]);
+			let _ = DispatchError::decode_with_depth_limit(MAX_DECODE_DEPTH, &mut &call_result.value[..]);
 		}
 	}
 
 	#[test]
-	fn dispatch_decode_cannot_pani_for_different_multi_byte_patterns() {
+	fn dispatch_decode_cannot_panic_for_different_multi_byte_patterns() {
 		for byte1 in [0x00, 0x03, 0x06, 0x07, 0xFF].iter() {
 			for byte2 in [0x00, 0xFF].iter() {
 				let call_result = CallResult {

integration-tests/src/evm.rs

@@ -1,6 +1,6 @@
 [package]
 name = "pallet-evm-accounts"
-version = "1.4.2"
+version = "1.4.3"
 authors = ["GalacticCouncil"]
 edition = "2021"
 license = "Apache-2.0"

pallets/evm-accounts/Cargo.toml

@@ -236,16 +236,7 @@ pub mod pallet {
 	{
 		type Call = Call<T>;
 
-		fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity {
-			match source {
-				TransactionSource::External => {
-					// receiving unsigned transaction from network - disallow
-					return InvalidTransaction::Call.into();
-				}
-				TransactionSource::Local => {}   // produced by off-chain worker
-				TransactionSource::InBlock => {} // some other node included it in a block
-			};
-
+		fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity {
 			let valid_tx = |user| {
 				ValidTransaction::with_tag_prefix("evm-accounts")
 					.priority(UNSIGNED_TXS_PRIORITY)
@@ -265,8 +256,8 @@ pub mod pallet {
 				} => {
 					// validate transaction
 					match (
-						Self::verify_claim_account(&account, asset_id.clone(), signature.clone()),
-						Self::validate_bind_evm_address(&account, &Self::evm_address(&account)),
+						Self::verify_claim_account(account, *asset_id, signature.clone()),
+						Self::validate_bind_evm_address(account, &Self::evm_address(&account)),
 					) {
 						(Ok(()), Ok(())) => valid_tx(account),
 						_ => InvalidTransaction::Call.into(),
@@ -480,31 +471,51 @@ where
 	fn verify_claim_account(account: &T::AccountId, asset_id: T::AssetId, signature: Signature) -> DispatchResult {
 		let msg = Self::create_claim_account_message(account, asset_id);
 
-		ensure!(
-			signature.verify(msg.as_slice(), &account.clone().into()),
-			Error::<T>::InvalidSignature
-		);
+		Self::validate_signature(msg.as_slice(), &signature, account)?;
 
 		T::FeeCurrency::is_payment_currency(asset_id)?;
 
 		ensure!(
-			!frame_system::Pallet::<T>::account_exists(&account),
+			!frame_system::Pallet::<T>::account_exists(account),
 			Error::<T>::AccountAlreadyExists
 		);
 
 		ensure!(
-			T::Currency::balance(asset_id, &account) >= T::ExistentialDeposits::get(&asset_id),
+			T::Currency::balance(asset_id, account) >= T::ExistentialDeposits::get(&asset_id),
 			Error::<T>::InsufficientAssetBalance
 		);
 
 		Ok(())
 	}
 
 	/// Creates a message that can be used to prove ownership of an account.
-	fn create_claim_account_message(account: &T::AccountId, asset_id: T::AssetId) -> Vec<u8> {
+	pub fn create_claim_account_message(account: &T::AccountId, asset_id: T::AssetId) -> Vec<u8> {
 		(MESSAGE_PREFIX, account.clone(), asset_id).encode()
 	}
 
+	/// Validate a signature. Supports signatures on raw `data` or `data` wrapped in HTML `<Bytes>`.
+	pub fn validate_signature(data: &[u8], signature: &Signature, signer: &T::AccountId) -> DispatchResult {
+		// Happy path, user has signed the raw data.
+		if signature.verify(data, &signer.clone().into()) {
+			return Ok(());
+		}
+		// NOTE: for security reasons modern UIs implicitly wrap the data requested to sign into
+		// `<Bytes> + data + </Bytes>`.
+		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.clone().into()),
+			Error::<T>::InvalidSignature
+		);
+
+		Ok(())
+	}
+
 	fn _is_evm_account(account_id: &[u8; 32]) -> bool {
 		&account_id[0..4] == b"ETH\0" && account_id[24..32] == [0u8; 8]
 	}

pallets/evm-accounts/src/lib.rs

@@ -72,7 +72,7 @@ impl AccountFeeCurrency<AccountId> for FeeCurrencyMock {
 	type AssetId = AssetId;
 
 	fn get(a: &AccountId) -> Self::AssetId {
-		FEE_ASSET.with(|v| v.borrow().get(&a).copied()).unwrap_or_default()
+		FEE_ASSET.with(|v| v.borrow().get(a).copied()).unwrap_or_default()
 	}
 	fn set(who: &AccountId, asset_id: Self::AssetId) -> DispatchResult {
 		FEE_ASSET.with(|v| {

pallets/evm-accounts/src/mock.rs

@@ -23,7 +23,7 @@ use frame_support::sp_runtime::traits::ValidateUnsigned;
 use frame_support::sp_runtime::traits::Zero;
 use frame_support::sp_runtime::DispatchError;
 use frame_support::unsigned::TransactionValidityError;
-use frame_support::{assert_noop, assert_ok, assert_storage_noop};
+use frame_support::{assert_err, assert_noop, assert_ok, assert_storage_noop};
 use hex_literal::hex;
 use mock::*;
 use orml_traits::MultiCurrency;
@@ -221,7 +221,7 @@ fn renounce_contract_deployer_should_remove_address_from_the_storage() {
 }
 
 #[test]
-fn verify_signed_message_should_work() {
+fn verify_signed_message_should_work_if_message_not_wrapped() {
 	ExtBuilder::default().build().execute_with(|| {
 		// Arrange & Act
 		let pair = sp_core::sr25519::Pair::from_seed_slice([1; 64].as_slice()).unwrap();
@@ -231,7 +231,31 @@ fn verify_signed_message_should_work() {
 		let msg = EVMAccounts::create_claim_account_message(&account, HDX);
 
 		// Assert
-		assert!(signature.verify(msg.as_slice(), &account.clone().into()));
+		assert_ok!(EVMAccounts::validate_signature(msg.as_slice(), &signature, &account));
+	});
+}
+
+#[test]
+fn verify_signed_message_should_work_if_message_wrapped() {
+	ExtBuilder::default().build().execute_with(|| {
+		// Arrange & Act
+		let pair = sp_core::sr25519::Pair::from_seed_slice([1; 64].as_slice()).unwrap();
+		let account = frame_support::sp_runtime::MultiSigner::from(pair.public()).into_account();
+
+		let original_msg = EVMAccounts::create_claim_account_message(&account, HDX);
+		let mut wrapped_msg = Vec::new();
+		wrapped_msg.extend(b"<Bytes>");
+		wrapped_msg.extend(original_msg);
+		wrapped_msg.extend(b"</Bytes>");
+
+		let signature = Signature::Sr25519(pair.sign(wrapped_msg.as_slice()));
+
+		// Assert
+		assert_ok!(EVMAccounts::validate_signature(
+			wrapped_msg.as_slice(),
+			&signature,
+			&account
+		));
 	});
 }
 
@@ -246,7 +270,10 @@ fn verify_signed_message_should_fail_if_different_account() {
 		let msg = EVMAccounts::create_claim_account_message(&account, HDX);
 
 		// Assert
-		assert_eq!(signature.verify(msg.as_slice(), &ALICE.into()), false);
+		assert_err!(
+			EVMAccounts::validate_signature(msg.as_slice(), &signature, &ALICE),
+			Error::<Test>::InvalidSignature
+		);
 	});
 }
 
@@ -261,7 +288,10 @@ fn verify_signed_message_should_fail_if_different_asset() {
 		let msg = EVMAccounts::create_claim_account_message(&account, 1);
 
 		// Assert
-		assert_eq!(signature.verify(msg.as_slice(), &account.into()), false);
+		assert_err!(
+			EVMAccounts::validate_signature(msg.as_slice(), &signature, &account),
+			Error::<Test>::InvalidSignature
+		);
 	});
 }
 
@@ -275,7 +305,10 @@ fn verify_signed_message_should_fail_if_different_message() {
 		let signature = sign_message::<Test>(pair, &account, HDX);
 
 		// Assert
-		assert_eq!(signature.verify("wrong message".as_bytes(), &account.into()), false);
+		assert_err!(
+			EVMAccounts::validate_signature("wrong message".as_bytes(), &signature, &account),
+			Error::<Test>::InvalidSignature
+		);
 	});
 }
 
@@ -470,7 +503,7 @@ fn validate_unsigned_should_pass_if_correct_call() {
 
 		// Act & Assert
 		assert_storage_noop!({
-			let res = EVMAccounts::validate_unsigned(TransactionSource::Local, &call);
+			let res = EVMAccounts::validate_unsigned(TransactionSource::External, &call);
 			assert_ok!(res);
 		});
 	});