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 */