Skip to content

Commit

Permalink
fix: Fix eth_signTypedData signatures containing 0x
Browse files Browse the repository at this point in the history 
In v7.10.0, signatures including a `bytes` field with the value `0x`
were being encoded differently than before. Previously the string `0x`
was interpreted as an "empty" hex number, but as of v7.10.0 they were
encoded as the string "0x". This change was not intentional, and it
resulted in invalid signatures.

This problem was introduced in `@metamask/eth-sig-util@6.0.1` (see here
for details: MetaMask/eth-sig-util#340). This
package was introduced to mobile when `@metamask/keyring-controller`
was updated to v6 (#7267).
The keyrings themselves still use `@metamask/eth-sig-util@5.0.1`, but
the `KeyringController` is signing these messages directly rather than
delegating to the keyrings, which is why the newer
`@metamask/eth-sig-util` package is used here.

This has been resovled by patching `@metamask/keyring-controller` to
delegate signing typed messages to the keyrings. The very next release
of that package includes that change anyway. We will wait until this
bug is fixed before updating `@metamask/eth-sig-util` in the keyrings.

Fixes #7792
  • Loading branch information
@Gudahtt
Gudahtt committed Nov 20, 2023
1 parent f63c1dd commit 57b42a8
Showing 1 changed file with 69 additions and 3 deletions.
72 changes: 69 additions & 3 deletions patches/@metamask+keyring-controller+6.1.0.patch
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,81 @@
diff --git a/node_modules/@metamask/keyring-controller/dist/KeyringController.js b/node_modules/@metamask/keyring-controller/dist/KeyringController.js
index c905cc0..f670fd3 100644
index c905cc0..82c7489 100644
--- a/node_modules/@metamask/keyring-controller/dist/KeyringController.js
+++ b/node_modules/@metamask/keyring-controller/dist/KeyringController.js
@@ -678,13 +678,21 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
@@ -436,45 +436,27 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
signTypedMessage(messageParams, version) {
return __awaiter(this, void 0, void 0, function* () {
try {
- const address = (0, eth_sig_util_1.normalize)(messageParams.from);
- if (!address || !(0, controller_utils_1.isValidHexAddress)(address)) {
- throw new Error(`Missing or invalid address ${JSON.stringify(messageParams.from)}`);
- }
- const qrKeyring = yield this.getOrAddQRKeyring();
- const qrAccounts = yield qrKeyring.getAccounts();
- if (qrAccounts.findIndex((qrAddress) => qrAddress.toLowerCase() === address.toLowerCase()) !== -1) {
- const messageParamsClone = Object.assign({}, messageParams);
- if (version !== SignTypedDataVersion.V1 &&
- typeof messageParamsClone.data === 'string') {
- messageParamsClone.data = JSON.parse(messageParamsClone.data);
- }
- return __classPrivateFieldGet(this, _KeyringController_keyring, "f").signTypedMessage(messageParamsClone, { version });
- }
- const { password } = __classPrivateFieldGet(this, _KeyringController_keyring, "f");
- const privateKey = yield this.exportAccount(password, address);
- const privateKeyBuffer = (0, ethereumjs_util_1.toBuffer)((0, ethereumjs_util_1.addHexPrefix)(privateKey));
- switch (version) {
- case SignTypedDataVersion.V1:
- return (0, eth_sig_util_1.signTypedData)({
- privateKey: privateKeyBuffer,
- data: messageParams.data,
- version: SignTypedDataVersion.V1,
- });
- case SignTypedDataVersion.V3:
- return (0, eth_sig_util_1.signTypedData)({
- privateKey: privateKeyBuffer,
- data: JSON.parse(messageParams.data),
- version: SignTypedDataVersion.V3,
- });
- case SignTypedDataVersion.V4:
- return (0, eth_sig_util_1.signTypedData)({
- privateKey: privateKeyBuffer,
- data: JSON.parse(messageParams.data),
- version: SignTypedDataVersion.V4,
- });
- default:
- throw new Error(`Unexpected signTypedMessage version: '${version}'`);
+ // PATCH NOTES: This change has been backported from @metamask/keyring-controller@7.0.0
+ // We can remove this after we update to v7
+ // This patch ensures that we delegate signing typed messages to the keyrings, rather
+ // than signing directly in the keyring controller.
+ // This patch was introduced as a workaround for a signing bug in
+ // @metamask/eth-sig-util@6.0.1 (and all v6.x/v7.x versions at time of writing). The
+ // keyrings still use @metamask/eth-sig-util@5, which is not affected.
+ if (![
+ SignTypedDataVersion.V1,
+ SignTypedDataVersion.V3,
+ SignTypedDataVersion.V4,
+ ].includes(version)) {
+ throw new Error(`Unexpected signTypedMessage version: '${version}'`);
}
+ return yield __classPrivateFieldGet(this, _KeyringController_keyring, "f").signTypedMessage({
+ from: messageParams.from,
+ data: version !== SignTypedDataVersion.V1 &&
+ typeof messageParams.data === 'string'
+ ? JSON.parse(messageParams.data)
+ : messageParams.data,
+ }, { version });
}
catch (error) {
throw new Error(`Keyring Controller signTypedMessage: ${error}`);
@@ -678,13 +660,21 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
}
forgetQRDevice() {
return __awaiter(this, void 0, void 0, function* () {
+ /**
+ * ============================== PATCH INFORMATION ==============================
+ * This patch addresses an issue regarding the forget device functionality. It
+ * improves the logic to correctly remove the QR accounts and update the
+ * improves the logic to correctly remove the QR accounts and update the
+ * identities as needed.
+ * ===============================================================================
+ */
Expand Down

0 comments on commit 57b42a8

Please sign in to comment.