From c0487d66aaadac1a8a0b3a0a7f574e76f261e9d3 Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 17:16:30 +0800
Subject: [PATCH 1/9] fix: hide Spender if no "spender" is found
---
.../components/confirm/info/typed-sign/typed-sign.tsx | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx b/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
index 86ea63470c37..6460d0ee0ab7 100644
--- a/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
+++ b/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
@@ -60,7 +60,7 @@ const TypedSignInfo: React.FC = () => {
<>
{isPermit && useTransactionSimulations && }
- {isPermit && (
+ {isPermit && spender && (
<>
From e732e665208725de05b9c94e750d067597d361d7 Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 17:33:44 +0800
Subject: [PATCH 2/9] chore: add permit spender comments
---
.../components/confirm/info/typed-sign/typed-sign.tsx | 4 ++++
ui/pages/confirmations/utils/confirm.ts | 4 +++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx b/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
index 6460d0ee0ab7..8efb93aef49d 100644
--- a/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
+++ b/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
@@ -60,6 +60,10 @@ const TypedSignInfo: React.FC = () => {
<>
{isPermit && useTransactionSimulations && }
+ {/*
+ It's possible the primaryType matches our Permit primaryType list,
+ but it may not be, so check spender exists before displaying.
+ */}
{isPermit && spender && (
<>
diff --git a/ui/pages/confirmations/utils/confirm.ts b/ui/pages/confirmations/utils/confirm.ts
index 1fa7ff36085e..9cc1990639af 100644
--- a/ui/pages/confirmations/utils/confirm.ts
+++ b/ui/pages/confirmations/utils/confirm.ts
@@ -77,7 +77,9 @@ export const isOrderSignatureRequest = (request: SignatureRequestType) => {
};
/**
- * Returns true if the request is a Permit Typed Sign signature request
+ * Returns true if the request appears to be a Permit Typed Sign signature request
+ * based on the primaryType. This *does not* do an in-depth check to verify the message matches
+ * the EIP-2612 standard.
*
* @param request - The confirmation request to check
*/
From d19d8caba8d645cc98a2733ec5ff4720b779d8af Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 19:33:23 +0800
Subject: [PATCH 3/9] refactor: isPermitSignatureRequest logic
---
ui/pages/confirmations/utils/confirm.ts | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/ui/pages/confirmations/utils/confirm.ts b/ui/pages/confirmations/utils/confirm.ts
index 9cc1990639af..5b4159f0bc6f 100644
--- a/ui/pages/confirmations/utils/confirm.ts
+++ b/ui/pages/confirmations/utils/confirm.ts
@@ -1,7 +1,7 @@
import { ApprovalRequest } from '@metamask/approval-controller';
import { ApprovalType } from '@metamask/controller-utils';
import { TransactionType } from '@metamask/transaction-controller';
-import { Json } from '@metamask/utils';
+import { isValidHexAddress, Json } from '@metamask/utils';
import {
PRIMARY_TYPES_ORDER,
PRIMARY_TYPES_PERMIT,
@@ -78,8 +78,9 @@ export const isOrderSignatureRequest = (request: SignatureRequestType) => {
/**
* Returns true if the request appears to be a Permit Typed Sign signature request
- * based on the primaryType. This *does not* do an in-depth check to verify the message matches
- * the EIP-2612 standard.
+ * based on EIP-2612 spec and the primaryType. This may exclude EIP-2612 types if
+ * the primaryType does not match our list. Ideally, we will remove the strict
+ * primaryType specification.
*
* @param request - The confirmation request to check
*/
@@ -93,10 +94,25 @@ export const isPermitSignatureRequest = (request?: Confirmation) => {
) {
return false;
}
- const { primaryType } = parseTypedDataMessage(
+ const parsedData = parseTypedDataMessage(
(request as SignatureRequestType).msgParams?.data as string,
);
+ const {
+ message: { owner, spender, value },
+ primaryType,
+ } = parsedData;
+
+ const hasPermitFields =
+ isValidHexAddress(owner) &&
+ isValidHexAddress(spender) &&
+ value !== undefined &&
+ value !== null;
+
+ if (!hasPermitFields) {
+ return false;
+ }
+
return PRIMARY_TYPES_PERMIT.includes(primaryType);
};
From 7cd71ee066ebc5848f9f6f601228c3d6bb144764 Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 19:34:00 +0800
Subject: [PATCH 4/9] Revert "fix: hide Spender if no "spender" is found"
This reverts commit c0487d66aaadac1a8a0b3a0a7f574e76f261e9d3.
---
.../components/confirm/info/typed-sign/typed-sign.tsx | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx b/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
index 8efb93aef49d..86ea63470c37 100644
--- a/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
+++ b/ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign.tsx
@@ -60,11 +60,7 @@ const TypedSignInfo: React.FC = () => {
<>
{isPermit && useTransactionSimulations && }
- {/*
- It's possible the primaryType matches our Permit primaryType list,
- but it may not be, so check spender exists before displaying.
- */}
- {isPermit && spender && (
+ {isPermit && (
<>
From 1a5a7845ebaef76109cc67cc028410eb8d7f0c93 Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 19:54:47 +0800
Subject: [PATCH 5/9] test: add isPermitSignatureRequest test cases
---
test/data/confirmations/typed_sign.ts | 61 ++++++++++++++++++-
ui/pages/confirmations/utils/confirm.test.ts | 62 ++++++++++++++++++++
2 files changed, 122 insertions(+), 1 deletion(-)
diff --git a/test/data/confirmations/typed_sign.ts b/test/data/confirmations/typed_sign.ts
index f02705a2540b..723ffcf7c2e0 100644
--- a/test/data/confirmations/typed_sign.ts
+++ b/test/data/confirmations/typed_sign.ts
@@ -163,6 +163,65 @@ export const orderSignatureMsg = {
},
};
+export const permitMessageDataJson = {
+ types: {
+ EIP712Domain: [
+ {
+ name: 'name',
+ type: 'string',
+ },
+ {
+ name: 'version',
+ type: 'string',
+ },
+ {
+ name: 'chainId',
+ type: 'uint256',
+ },
+ {
+ name: 'verifyingContract',
+ type: 'address',
+ },
+ ],
+ Permit: [
+ {
+ name: 'owner',
+ type: 'address',
+ },
+ {
+ name: 'spender',
+ type: 'address',
+ },
+ {
+ name: 'value',
+ type: 'uint256',
+ },
+ {
+ name: 'nonce',
+ type: 'uint256',
+ },
+ {
+ name: 'deadline',
+ type: 'uint256',
+ },
+ ],
+ },
+ primaryType: 'Permit',
+ domain: {
+ name: 'MyToken',
+ version: '1',
+ verifyingContract: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC',
+ chainId: 1,
+ },
+ message: {
+ owner: '0x935e73edb9ff52e23bac7f7e043a1ecd06d05477',
+ spender: '0x5B38Da6a701c568545dCfcB03FcB875f56beddC4',
+ value: 3000,
+ nonce: 0,
+ deadline: 50000000000,
+ },
+};
+
export const permitSignatureMsg = {
id: '0b1787a0-1c44-11ef-b70d-e7064bd7b659',
securityAlertResponse: {
@@ -174,7 +233,7 @@ export const permitSignatureMsg = {
time: 1716826404122,
type: TransactionType.signTypedData,
msgParams: {
- data: '{"types":{"EIP712Domain":[{"name":"name","type":"string"},{"name":"version","type":"string"},{"name":"chainId","type":"uint256"},{"name":"verifyingContract","type":"address"}],"Permit":[{"name":"owner","type":"address"},{"name":"spender","type":"address"},{"name":"value","type":"uint256"},{"name":"nonce","type":"uint256"},{"name":"deadline","type":"uint256"}]},"primaryType":"Permit","domain":{"name":"MyToken","version":"1","verifyingContract":"0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC","chainId":1},"message":{"owner":"0x935e73edb9ff52e23bac7f7e043a1ecd06d05477","spender":"0x5B38Da6a701c568545dCfcB03FcB875f56beddC4","value":3000,"nonce":0,"deadline":50000000000}}',
+ data: JSON.stringify(permitMessageDataJson),
from: '0x935e73edb9ff52e23bac7f7e043a1ecd06d05477',
version: 'V4',
requestId: 14,
diff --git a/ui/pages/confirmations/utils/confirm.test.ts b/ui/pages/confirmations/utils/confirm.test.ts
index b1f6494ca627..55d746fea647 100644
--- a/ui/pages/confirmations/utils/confirm.test.ts
+++ b/ui/pages/confirmations/utils/confirm.test.ts
@@ -1,9 +1,11 @@
import { ApprovalRequest } from '@metamask/approval-controller';
import { ApprovalType } from '@metamask/controller-utils';
import { TransactionType } from '@metamask/transaction-controller';
+import { cloneDeep } from 'lodash';
import {
orderSignatureMsg,
+ permitMessageDataJson,
permitSignatureMsg,
unapprovedTypedSignMsgV4,
} from '../../../../test/data/confirmations/typed_sign';
@@ -76,6 +78,66 @@ describe('confirm util', () => {
);
expect(result).toStrictEqual(true);
});
+
+ it('returns true for permit signature requests including when value is 0', () => {
+ const data = cloneDeep(permitMessageDataJson);
+ data.message.value = 0;
+
+ const mockMessage = {
+ ...permitSignatureMsg,
+ msgParams: {
+ data: JSON.stringify(data),
+ },
+ } as SignatureRequestType;
+
+ const result = isPermitSignatureRequest(mockMessage);
+ expect(result).toStrictEqual(true);
+ });
+
+ it('returns false if it is missing a value', () => {
+ const data = cloneDeep(permitMessageDataJson);
+ data.message.value = NaN;
+
+ const mockMessage = {
+ ...permitSignatureMsg,
+ msgParams: {
+ data: JSON.stringify(data),
+ },
+ } as SignatureRequestType;
+
+ const result = isPermitSignatureRequest(mockMessage);
+ expect(result).toStrictEqual(false);
+ });
+
+ it('returns false if it is missing a valid owner address', () => {
+ const data = cloneDeep(permitMessageDataJson);
+ data.message.owner = '0x';
+ const mockMessage = {
+ ...permitSignatureMsg,
+ msgParams: {
+ data: JSON.stringify(data),
+ },
+ } as SignatureRequestType;
+
+ const result = isPermitSignatureRequest(mockMessage);
+ expect(result).toStrictEqual(false);
+ });
+
+ it('returns false if it is missing a valid spender address', () => {
+ const data = cloneDeep(permitMessageDataJson);
+ data.message.spender = '';
+
+ const mockMessage = {
+ ...permitSignatureMsg,
+ msgParams: {
+ data: JSON.stringify(data),
+ },
+ } as SignatureRequestType;
+
+ const result = isPermitSignatureRequest(mockMessage);
+ expect(result).toStrictEqual(false);
+ });
+
it('returns false for request not of type permit signature', () => {
const result = isPermitSignatureRequest(
unapprovedTypedSignMsgV4 as SignatureRequestType,
From 53c99237f0d7811d51da766f077366820d52e74c Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 20:52:47 +0800
Subject: [PATCH 6/9] fix: reduce isPermitSignatureRequest checks e.g. owner
and value will not exist if it is a Permit2 request.
---
ui/pages/confirmations/utils/confirm.ts | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/ui/pages/confirmations/utils/confirm.ts b/ui/pages/confirmations/utils/confirm.ts
index 5b4159f0bc6f..635f8336a522 100644
--- a/ui/pages/confirmations/utils/confirm.ts
+++ b/ui/pages/confirmations/utils/confirm.ts
@@ -1,7 +1,7 @@
import { ApprovalRequest } from '@metamask/approval-controller';
import { ApprovalType } from '@metamask/controller-utils';
import { TransactionType } from '@metamask/transaction-controller';
-import { isValidHexAddress, Json } from '@metamask/utils';
+import { isHexString, Json } from '@metamask/utils';
import {
PRIMARY_TYPES_ORDER,
PRIMARY_TYPES_PERMIT,
@@ -77,10 +77,11 @@ export const isOrderSignatureRequest = (request: SignatureRequestType) => {
};
/**
- * Returns true if the request appears to be a Permit Typed Sign signature request
- * based on EIP-2612 spec and the primaryType. This may exclude EIP-2612 types if
- * the primaryType does not match our list. Ideally, we will remove the strict
- * primaryType specification.
+ * Returns true if the request appears to be a Permit or Permit2 Typed Sign signature request
+ * Caution: This is a limited check. It may exclude valid types if the primaryType does not
+ * match our list.
+ *
+ * TODO: Improve detection of fields and remove explicit primaryType check.
*
* @param request - The confirmation request to check
*/
@@ -99,17 +100,11 @@ export const isPermitSignatureRequest = (request?: Confirmation) => {
);
const {
- message: { owner, spender, value },
+ message: { spender },
primaryType,
} = parsedData;
- const hasPermitFields =
- isValidHexAddress(owner) &&
- isValidHexAddress(spender) &&
- value !== undefined &&
- value !== null;
-
- if (!hasPermitFields) {
+ if (!isHexString(spender)) {
return false;
}
From 016e7b297a5a5ec0470263e9c600b40efc833470 Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 20:54:00 +0800
Subject: [PATCH 7/9] test: rm irrelevant tests
---
ui/pages/confirmations/utils/confirm.test.ts | 44 --------------------
1 file changed, 44 deletions(-)
diff --git a/ui/pages/confirmations/utils/confirm.test.ts b/ui/pages/confirmations/utils/confirm.test.ts
index 55d746fea647..41a4992fa11f 100644
--- a/ui/pages/confirmations/utils/confirm.test.ts
+++ b/ui/pages/confirmations/utils/confirm.test.ts
@@ -79,50 +79,6 @@ describe('confirm util', () => {
expect(result).toStrictEqual(true);
});
- it('returns true for permit signature requests including when value is 0', () => {
- const data = cloneDeep(permitMessageDataJson);
- data.message.value = 0;
-
- const mockMessage = {
- ...permitSignatureMsg,
- msgParams: {
- data: JSON.stringify(data),
- },
- } as SignatureRequestType;
-
- const result = isPermitSignatureRequest(mockMessage);
- expect(result).toStrictEqual(true);
- });
-
- it('returns false if it is missing a value', () => {
- const data = cloneDeep(permitMessageDataJson);
- data.message.value = NaN;
-
- const mockMessage = {
- ...permitSignatureMsg,
- msgParams: {
- data: JSON.stringify(data),
- },
- } as SignatureRequestType;
-
- const result = isPermitSignatureRequest(mockMessage);
- expect(result).toStrictEqual(false);
- });
-
- it('returns false if it is missing a valid owner address', () => {
- const data = cloneDeep(permitMessageDataJson);
- data.message.owner = '0x';
- const mockMessage = {
- ...permitSignatureMsg,
- msgParams: {
- data: JSON.stringify(data),
- },
- } as SignatureRequestType;
-
- const result = isPermitSignatureRequest(mockMessage);
- expect(result).toStrictEqual(false);
- });
-
it('returns false if it is missing a valid spender address', () => {
const data = cloneDeep(permitMessageDataJson);
data.message.spender = '';
From 876f27d3fe7b46598b9f6902cc8f285f4846d7ad Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Wed, 25 Sep 2024 21:00:15 +0800
Subject: [PATCH 8/9] style: destruct parseTypedDataMessage
---
ui/pages/confirmations/utils/confirm.ts | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/ui/pages/confirmations/utils/confirm.ts b/ui/pages/confirmations/utils/confirm.ts
index 635f8336a522..bca469cabbed 100644
--- a/ui/pages/confirmations/utils/confirm.ts
+++ b/ui/pages/confirmations/utils/confirm.ts
@@ -95,14 +95,13 @@ export const isPermitSignatureRequest = (request?: Confirmation) => {
) {
return false;
}
- const parsedData = parseTypedDataMessage(
- (request as SignatureRequestType).msgParams?.data as string,
- );
const {
message: { spender },
primaryType,
- } = parsedData;
+ } = parseTypedDataMessage(
+ (request as SignatureRequestType).msgParams?.data as string,
+ );
if (!isHexString(spender)) {
return false;
From 14be86e3e0b7e0e61b3e42fc558f745313e4ccfc Mon Sep 17 00:00:00 2001
From: digiwand <20778143+digiwand@users.noreply.github.com>
Date: Thu, 26 Sep 2024 21:50:53 +0800
Subject: [PATCH 9/9] chore: update comment
---
ui/pages/confirmations/utils/confirm.ts | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/ui/pages/confirmations/utils/confirm.ts b/ui/pages/confirmations/utils/confirm.ts
index bca469cabbed..e23108e7e5f2 100644
--- a/ui/pages/confirmations/utils/confirm.ts
+++ b/ui/pages/confirmations/utils/confirm.ts
@@ -77,11 +77,11 @@ export const isOrderSignatureRequest = (request: SignatureRequestType) => {
};
/**
- * Returns true if the request appears to be a Permit or Permit2 Typed Sign signature request
- * Caution: This is a limited check. It may exclude valid types if the primaryType does not
- * match our list.
+ * Returns true if the request appears to be an EIP-2612 Permit or Permit2 signTypedData signature request.
+ * Caution: This is a limited check. It may include other Permits which are not the specific types mentioned
+ * if they match the primaryType check and have a spender field.
*
- * TODO: Improve detection of fields and remove explicit primaryType check.
+ * TODO: Improve detection of fields.
*
* @param request - The confirmation request to check
*/