Skip to content

Commit e8a688c

Browse files
spylogssterspylogsster
authored
Add signing eip712 for hardware accounts (#12265)
1 parent 28e41a4 commit e8a688c

26 files changed

+419
-730
lines changed

‎browser/brave_wallet/brave_wallet_provider_impl_unittest.cc

+50-23
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,12 @@ void ValidateErrorCode(BraveWalletProviderImpl* provider,
8181
ASSERT_TRUE(callback_is_called);
8282
}
8383

84+
std::vector<uint8_t> DecodeHexHash(const std::string& hash_hex) {
85+
std::vector<uint8_t> hash;
86+
base::HexStringToBytes(hash_hex, &hash);
87+
return hash;
88+
}
89+
8490
} // namespace
8591

8692
class TestEventsListener : public brave_wallet::mojom::EventsListener {
@@ -424,7 +430,8 @@ class BraveWalletProviderImplUnitTest : public testing::Test {
424430
void SignTypedMessage(absl::optional<bool> user_approved,
425431
const std::string& address,
426432
const std::string& message,
427-
const std::string& message_to_sign,
433+
const std::vector<uint8_t>& domain_hash,
434+
const std::vector<uint8_t>& primary_hash,
428435
base::Value&& domain,
429436
std::string* signature_out,
430437
mojom::ProviderError* error_out,
@@ -434,7 +441,7 @@ class BraveWalletProviderImplUnitTest : public testing::Test {
434441

435442
base::RunLoop run_loop;
436443
provider()->SignTypedMessage(
437-
address, message, message_to_sign, std::move(domain),
444+
address, message, domain_hash, primary_hash, std::move(domain),
438445
base::BindLambdaForTesting([&](const std::string& signature,
439446
mojom::ProviderError error,
440447
const std::string& error_message) {
@@ -1216,47 +1223,50 @@ TEST_F(BraveWalletProviderImplUnitTest, SignTypedMessage) {
12161223
EXPECT_EQ(json_rpc_service()->GetChainId(), "0x1");
12171224
CreateWallet();
12181225
AddAccount();
1219-
const std::string valid_message_to_sign =
1220-
"be609aee343fb3c4b28e1df9e632fca64fcfaede20f02e86244efddf30957bd2";
12211226
std::string signature;
12221227
mojom::ProviderError error;
12231228
std::string error_message;
12241229
base::Value domain(base::Value::Type::DICTIONARY);
1230+
std::vector<uint8_t> domain_hash = DecodeHexHash(
1231+
"f2cee375fa42b42143804025fc449deafd50cc031ca257e0b194a650a912090f");
1232+
std::vector<uint8_t> primary_hash = DecodeHexHash(
1233+
"c52c0ee5d84264471806290a3f2c4cecfc5490626bf912d01f240d7a274b371e");
12251234
domain.SetIntKey("chainId", 1);
1226-
SignTypedMessage(absl::nullopt, "1234", "{...}", valid_message_to_sign,
1235+
SignTypedMessage(absl::nullopt, "1234", "{...}", domain_hash, primary_hash,
12271236
domain.Clone(), &signature, &error, &error_message);
12281237
EXPECT_TRUE(signature.empty());
12291238
EXPECT_EQ(error, mojom::ProviderError::kInvalidParams);
12301239
EXPECT_EQ(error_message,
12311240
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
12321241

1233-
SignTypedMessage(absl::nullopt, "0x12345678", "{...}", valid_message_to_sign,
1234-
domain.Clone(), &signature, &error, &error_message);
1242+
SignTypedMessage(absl::nullopt, "0x12345678", "{...}", domain_hash,
1243+
primary_hash, domain.Clone(), &signature, &error,
1244+
&error_message);
12351245
EXPECT_TRUE(signature.empty());
12361246
EXPECT_EQ(error, mojom::ProviderError::kInvalidParams);
12371247
EXPECT_EQ(error_message,
12381248
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
12391249

12401250
const std::string address = "0x1234567890123456789012345678901234567890";
12411251
// domain not dict
1242-
SignTypedMessage(absl::nullopt, address, "{...}", valid_message_to_sign,
1252+
SignTypedMessage(absl::nullopt, address, "{...}", domain_hash, primary_hash,
12431253
base::Value("not dict"), &signature, &error, &error_message);
12441254
EXPECT_TRUE(signature.empty());
12451255
EXPECT_EQ(error, mojom::ProviderError::kInvalidParams);
12461256
EXPECT_EQ(error_message,
12471257
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
12481258

1249-
// not valid hex
1250-
SignTypedMessage(absl::nullopt, address, "{...}", "brave", domain.Clone(),
1251-
&signature, &error, &error_message);
1259+
// not valid domain hash
1260+
SignTypedMessage(absl::nullopt, address, "{...}", {}, primary_hash,
1261+
domain.Clone(), &signature, &error, &error_message);
12521262
EXPECT_TRUE(signature.empty());
12531263
EXPECT_EQ(error, mojom::ProviderError::kInvalidParams);
12541264
EXPECT_EQ(error_message,
12551265
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
12561266

1257-
// not valid eip712 hash
1258-
SignTypedMessage(absl::nullopt, address, "{...}", "deadbeef", domain.Clone(),
1259-
&signature, &error, &error_message);
1267+
// not valid primary hash
1268+
SignTypedMessage(absl::nullopt, address, "{...}", domain_hash, {},
1269+
domain.Clone(), &signature, &error, &error_message);
12601270
EXPECT_TRUE(signature.empty());
12611271
EXPECT_EQ(error, mojom::ProviderError::kInvalidParams);
12621272
EXPECT_EQ(error_message,
@@ -1265,7 +1275,7 @@ TEST_F(BraveWalletProviderImplUnitTest, SignTypedMessage) {
12651275
domain.SetIntKey("chainId", 4);
12661276
std::string chain_id = "0x4";
12671277
// not active network
1268-
SignTypedMessage(absl::nullopt, address, "{...}", valid_message_to_sign,
1278+
SignTypedMessage(absl::nullopt, address, "{...}", domain_hash, primary_hash,
12691279
domain.Clone(), &signature, &error, &error_message);
12701280
EXPECT_TRUE(signature.empty());
12711281
EXPECT_EQ(error, mojom::ProviderError::kInternalError);
@@ -1275,7 +1285,7 @@ TEST_F(BraveWalletProviderImplUnitTest, SignTypedMessage) {
12751285
base::ASCIIToUTF16(chain_id)));
12761286
domain.SetIntKey("chainId", 1);
12771287

1278-
SignTypedMessage(absl::nullopt, address, "{...}", valid_message_to_sign,
1288+
SignTypedMessage(absl::nullopt, address, "{...}", domain_hash, primary_hash,
12791289
domain.Clone(), &signature, &error, &error_message);
12801290
EXPECT_TRUE(signature.empty());
12811291
EXPECT_EQ(error, mojom::ProviderError::kUnauthorized);
@@ -1286,8 +1296,9 @@ TEST_F(BraveWalletProviderImplUnitTest, SignTypedMessage) {
12861296
// No permission
12871297
const std::vector<std::string> addresses = GetAddresses();
12881298
ASSERT_FALSE(address.empty());
1289-
SignTypedMessage(absl::nullopt, addresses[0], "{...}", valid_message_to_sign,
1290-
domain.Clone(), &signature, &error, &error_message);
1299+
SignTypedMessage(absl::nullopt, addresses[0], "{...}", domain_hash,
1300+
primary_hash, domain.Clone(), &signature, &error,
1301+
&error_message);
12911302
EXPECT_TRUE(signature.empty());
12921303
EXPECT_EQ(error, mojom::ProviderError::kUnauthorized);
12931304
EXPECT_EQ(error_message,
@@ -1296,27 +1307,43 @@ TEST_F(BraveWalletProviderImplUnitTest, SignTypedMessage) {
12961307
GURL url("https://brave.com");
12971308
Navigate(url);
12981309
AddEthereumPermission(url);
1299-
SignTypedMessage(true, addresses[0], "{...}", valid_message_to_sign,
1310+
SignTypedMessage(true, addresses[0], "{...}", domain_hash, primary_hash,
13001311
domain.Clone(), &signature, &error, &error_message);
13011312

13021313
EXPECT_FALSE(signature.empty());
13031314
EXPECT_EQ(error, mojom::ProviderError::kSuccess);
13041315
EXPECT_TRUE(error_message.empty());
13051316

13061317
// User reject request
1307-
SignTypedMessage(false, addresses[0], "{...}", valid_message_to_sign,
1318+
SignTypedMessage(false, addresses[0], "{...}", domain_hash, primary_hash,
13081319
domain.Clone(), &signature, &error, &error_message);
13091320
EXPECT_TRUE(signature.empty());
13101321
EXPECT_EQ(error, mojom::ProviderError::kUserRejectedRequest);
13111322
EXPECT_EQ(error_message,
13121323
l10n_util::GetStringUTF8(IDS_WALLET_USER_REJECTED_REQUEST));
1313-
1324+
// not valid eip712 domain hash
1325+
SignTypedMessage(absl::nullopt, address, "{...}", DecodeHexHash("brave"),
1326+
primary_hash, domain.Clone(), &signature, &error,
1327+
&error_message);
1328+
EXPECT_TRUE(signature.empty());
1329+
EXPECT_EQ(error, mojom::ProviderError::kInvalidParams);
1330+
EXPECT_EQ(error_message,
1331+
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
1332+
// not valid eip712 primary hash
1333+
SignTypedMessage(absl::nullopt, address, "{...}", domain_hash,
1334+
DecodeHexHash("primary"), domain.Clone(), &signature, &error,
1335+
&error_message);
1336+
EXPECT_TRUE(signature.empty());
1337+
EXPECT_EQ(error, mojom::ProviderError::kInvalidParams);
1338+
EXPECT_EQ(error_message,
1339+
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
13141340
keyring_service()->Lock();
13151341

13161342
// nullopt for the first param here because we don't AddSignMessageRequest
13171343
// whent here are no accounts returned.
1318-
SignTypedMessage(absl::nullopt, addresses[0], "{...}", valid_message_to_sign,
1319-
domain.Clone(), &signature, &error, &error_message);
1344+
SignTypedMessage(absl::nullopt, addresses[0], "{...}", domain_hash,
1345+
primary_hash, domain.Clone(), &signature, &error,
1346+
&error_message);
13201347
EXPECT_TRUE(signature.empty());
13211348
EXPECT_EQ(error, mojom::ProviderError::kUnauthorized);
13221349
EXPECT_EQ(error_message,

‎browser/brave_wallet/brave_wallet_service_unittest.cc

+11-5
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
* You can obtain one at http://mozilla.org/MPL/2.0/. */
55

66
#include "brave/components/brave_wallet/browser/brave_wallet_service.h"
7+
#include <string>
78

89
#include "base/memory/raw_ptr.h"
910
#include "base/test/bind.h"
@@ -1315,7 +1316,8 @@ TEST_F(BraveWalletServiceUnitTest, SignMessageHardware) {
13151316
std::string address = "0xbe862ad9abfe6f22bcb087716c7d89a26051f74c";
13161317
std::string message = "0xAB";
13171318
auto request1 = mojom::SignMessageRequest::New(
1318-
1, address, std::string(message.begin(), message.end()));
1319+
1, address, std::string(message.begin(), message.end()), false,
1320+
absl::nullopt, absl::nullopt);
13191321
bool callback_is_called = false;
13201322
service_->AddSignMessageRequest(
13211323
std::move(request1), base::BindLambdaForTesting(
@@ -1337,7 +1339,8 @@ TEST_F(BraveWalletServiceUnitTest, SignMessageHardware) {
13371339
callback_is_called = false;
13381340
std::string expected_error = "error";
13391341
auto request2 = mojom::SignMessageRequest::New(
1340-
2, address, std::string(message.begin(), message.end()));
1342+
2, address, std::string(message.begin(), message.end()), false,
1343+
absl::nullopt, absl::nullopt);
13411344
service_->AddSignMessageRequest(
13421345
std::move(request2), base::BindLambdaForTesting(
13431346
[&](bool approved, const std::string& signature,
@@ -1359,7 +1362,8 @@ TEST_F(BraveWalletServiceUnitTest, SignMessage) {
13591362
std::string address = "0xbe862ad9abfe6f22bcb087716c7d89a26051f74c";
13601363
std::string message = "0xAB";
13611364
auto request1 = mojom::SignMessageRequest::New(
1362-
1, address, std::string(message.begin(), message.end()));
1365+
1, address, std::string(message.begin(), message.end()), false,
1366+
absl::nullopt, absl::nullopt);
13631367
bool callback_is_called = false;
13641368
service_->AddSignMessageRequest(
13651369
std::move(request1), base::BindLambdaForTesting(
@@ -1377,7 +1381,8 @@ TEST_F(BraveWalletServiceUnitTest, SignMessage) {
13771381
callback_is_called = false;
13781382
std::string expected_error = "error";
13791383
auto request2 = mojom::SignMessageRequest::New(
1380-
2, address, std::string(message.begin(), message.end()));
1384+
2, address, std::string(message.begin(), message.end()), false,
1385+
absl::nullopt, absl::nullopt);
13811386
service_->AddSignMessageRequest(
13821387
std::move(request2), base::BindLambdaForTesting(
13831388
[&](bool approved, const std::string& signature,
@@ -1569,7 +1574,8 @@ TEST_F(BraveWalletServiceUnitTest, Reset) {
15691574
std::string address = "0xbe862ad9abfe6f22bcb087716c7d89a26051f74c";
15701575
std::string message = "0xAB";
15711576
auto request1 = mojom::SignMessageRequest::New(
1572-
1, address, std::string(message.begin(), message.end()));
1577+
1, address, std::string(message.begin(), message.end()), false,
1578+
absl::nullopt, absl::nullopt);
15731579
service_->AddSignMessageRequest(
15741580
std::move(request1),
15751581
base::BindLambdaForTesting(

‎components/brave_wallet/browser/brave_wallet_constants.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,10 @@ constexpr webui::LocalizedString kLocalizedStrings[] = {
599599
{"braveWalletNFTDetailBlockchain", IDS_BRAVE_WALLET_NFT_DETAIL_BLOCKCHAIN},
600600
{"braveWalletNFTDetailTokenStandard",
601601
IDS_BRAVE_WALLET_NFT_DETAIL_TOKEN_STANDARD},
602-
{"braveWalletNFTDetailTokenID", IDS_BRAVE_WALLET_NFT_DETAIL_TOKEN_ID}};
602+
{"braveWalletNFTDetailTokenID", IDS_BRAVE_WALLET_NFT_DETAIL_TOKEN_ID},
603+
{"braveWalletTrezorSignTypedDataError",
604+
IDS_BRAVE_WALLET_TREZOR_SIGN_TYPED_DATA_ERROR},
605+
};
603606

604607
const char kRopstenSwapBaseAPIURL[] = "https://ropsten.api.0x.org/";
605608
const char kRopstenBuyTokenPercentageFee[] = "0.00875";

‎components/brave_wallet/browser/brave_wallet_provider_impl.cc

+38-17
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
#include "brave/components/brave_wallet/browser/brave_wallet_provider_impl.h"
77

8+
#include <string>
89
#include <utility>
910

1011
#include "base/json/json_reader.h"
@@ -21,6 +22,7 @@
2122
#include "brave/components/brave_wallet/browser/keyring_service.h"
2223
#include "brave/components/brave_wallet/browser/tx_service.h"
2324
#include "brave/components/brave_wallet/common/eth_address.h"
25+
#include "brave/components/brave_wallet/common/eth_sign_typed_data_helper.h"
2426
#include "brave/components/brave_wallet/common/hex_utils.h"
2527
#include "brave/components/brave_wallet/common/value_conversion_utils.h"
2628
#include "brave/components/brave_wallet/common/web3_provider_constants.h"
@@ -365,11 +367,11 @@ void BraveWalletProviderImpl::SignMessage(const std::string& address,
365367
// Convert to checksum address
366368
auto checksum_address = EthAddress::FromHex(address);
367369
GetAllowedAccounts(
368-
false,
369-
base::BindOnce(&BraveWalletProviderImpl::ContinueSignMessage,
370-
weak_factory_.GetWeakPtr(),
371-
checksum_address.ToChecksumAddress(), message_str,
372-
std::move(message_bytes), std::move(callback), false));
370+
false, base::BindOnce(&BraveWalletProviderImpl::ContinueSignMessage,
371+
weak_factory_.GetWeakPtr(),
372+
checksum_address.ToChecksumAddress(), message_str,
373+
std::move(message_bytes), absl::nullopt,
374+
absl::nullopt, false, std::move(callback)));
373375
}
374376

375377
void BraveWalletProviderImpl::RecoverAddress(const std::string& message,
@@ -414,19 +416,17 @@ void BraveWalletProviderImpl::RecoverAddress(const std::string& message,
414416
void BraveWalletProviderImpl::SignTypedMessage(
415417
const std::string& address,
416418
const std::string& message,
417-
const std::string& message_to_sign,
419+
const std::vector<uint8_t>& domain_hash,
420+
const std::vector<uint8_t>& primary_hash,
418421
base::Value domain,
419422
SignTypedMessageCallback callback) {
420-
std::vector<uint8_t> eip712_hash;
421-
if (!EthAddress::IsValidAddress(address) ||
422-
!base::HexStringToBytes(message_to_sign, &eip712_hash) ||
423-
eip712_hash.size() != 32 || !domain.is_dict()) {
423+
if (!EthAddress::IsValidAddress(address) || !domain.is_dict() ||
424+
domain_hash.empty() || primary_hash.empty()) {
424425
std::move(callback).Run(
425426
"", mojom::ProviderError::kInvalidParams,
426427
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
427428
return;
428429
}
429-
430430
auto chain_id = domain.FindDoubleKey("chainId");
431431
if (chain_id) {
432432
const std::string chain_id_hex =
@@ -441,21 +441,40 @@ void BraveWalletProviderImpl::SignTypedMessage(
441441
}
442442
}
443443

444+
if (domain_hash.empty() || primary_hash.empty()) {
445+
std::move(callback).Run(
446+
"", mojom::ProviderError::kInvalidParams,
447+
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
448+
return;
449+
}
450+
auto message_to_sign = EthSignTypedDataHelper::GetTypedDataMessageToSign(
451+
domain_hash, primary_hash);
452+
if (!message_to_sign || message_to_sign->size() != 32) {
453+
std::move(callback).Run(
454+
"", mojom::ProviderError::kInvalidParams,
455+
l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
456+
return;
457+
}
458+
444459
// Convert to checksum address
445460
auto checksum_address = EthAddress::FromHex(address);
446461
GetAllowedAccounts(
447-
false, base::BindOnce(&BraveWalletProviderImpl::ContinueSignMessage,
448-
weak_factory_.GetWeakPtr(),
449-
checksum_address.ToChecksumAddress(), message,
450-
std::move(eip712_hash), std::move(callback), true));
462+
false,
463+
base::BindOnce(&BraveWalletProviderImpl::ContinueSignMessage,
464+
weak_factory_.GetWeakPtr(),
465+
checksum_address.ToChecksumAddress(), message,
466+
std::move(*message_to_sign), base::HexEncode(domain_hash),
467+
base::HexEncode(primary_hash), true, std::move(callback)));
451468
}
452469

453470
void BraveWalletProviderImpl::ContinueSignMessage(
454471
const std::string& address,
455472
const std::string& message,
456473
std::vector<uint8_t>&& message_to_sign,
457-
SignMessageCallback callback,
474+
const absl::optional<std::string>& domain_hash,
475+
const absl::optional<std::string>& primary_hash,
458476
bool is_eip712,
477+
SignMessageCallback callback,
459478
const std::vector<std::string>& allowed_accounts,
460479
mojom::ProviderError error,
461480
const std::string& error_message) {
@@ -473,7 +492,9 @@ void BraveWalletProviderImpl::ContinueSignMessage(
473492
}
474493

475494
auto request =
476-
mojom::SignMessageRequest::New(sign_message_id_++, address, message);
495+
mojom::SignMessageRequest::New(sign_message_id_++, address, message,
496+
is_eip712, domain_hash, primary_hash);
497+
477498
if (keyring_service_->IsHardwareAccount(address)) {
478499
brave_wallet_service_->AddSignMessageRequest(
479500
std::move(request),

‎components/brave_wallet/browser/brave_wallet_provider_impl.h

+5-2
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,8 @@ class BraveWalletProviderImpl final
8181
RecoverAddressCallback callback) override;
8282
void SignTypedMessage(const std::string& address,
8383
const std::string& message,
84-
const std::string& message_to_sign,
84+
const std::vector<uint8_t>& domain_hash,
85+
const std::vector<uint8_t>& primary_hash,
8586
base::Value domain,
8687
SignTypedMessageCallback callback) override;
8788
void OnGetAllowedAccounts(GetAllowedAccountsCallback callback,
@@ -162,8 +163,10 @@ class BraveWalletProviderImpl final
162163
void ContinueSignMessage(const std::string& address,
163164
const std::string& message,
164165
std::vector<uint8_t>&& message_to_sign,
165-
SignMessageCallback callback,
166+
const absl::optional<std::string>& domain_hash,
167+
const absl::optional<std::string>& primary_hash,
166168
bool is_eip712,
169+
SignMessageCallback callback,
167170
const std::vector<std::string>& allowed_accounts,
168171
mojom::ProviderError error,
169172
const std::string& error_message);

0 commit comments

Comments
 (0)