Skip to content

Commit 7ee3181

Browse files
jeffsmale90jeffsmale90
authored
Validate terms is Hex, rather than validating each erc20 token address individually (#8127)
## Explanation Previously we used the `isHexAddress` function to validate the erc20 token address. This function expects the input value to be in lowercase hex. Because we are splitting the address from the `terms`, now we just validate that the entire terms is valid Hex (using case-insensitive function) in the `makePermissionRule` function. `no-changelog` because this is a fix to an unreleased change. ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes shared permission decoding/validation by rejecting any non-hex `terms` upfront and removing per-rule `tokenAddress` hex checks; this can affect which permissions are accepted/rejected across all rules. > > **Overview** > Moves hex validation to `makePermissionRule` by rejecting any caveat whose `terms` are not a strict hex string (while still allowing empty `0x`), preventing downstream decoders from operating on malformed data. > > Removes `isHexAddress` checks from the ERC20 stream/periodic decoders so mixed-case token addresses in `terms` are accepted, and updates tests to cover mixed-case addresses while dropping rule-specific “invalid token address characters” cases. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8e8f420. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 1a9dd3a commit 7ee3181

File tree

6 files changed

+251
-74
lines changed

6 files changed

+251
-74
lines changed

‎packages/gator-permissions-controller/src/decodePermission/rules/erc20TokenPeriodic.test.ts‎

Lines changed: 33 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,39 @@ describe('erc20-token-periodic rule', () => {
170170
expect(result.data.startTime).toBe(1715664);
171171
});
172172

173+
it('decodes mixed-case token address', () => {
174+
const mixedCaseAddress = contracts.ERC20PeriodTransferEnforcer;
175+
const caveats = [
176+
expiryCaveat,
177+
valueLteCaveat,
178+
{
179+
enforcer: ERC20PeriodTransferEnforcer,
180+
terms: createERC20TokenPeriodTransferTerms(
181+
{
182+
tokenAddress: mixedCaseAddress,
183+
periodAmount: 200n,
184+
periodDuration: 86400,
185+
startDate: 1715664,
186+
},
187+
{ out: 'hex' },
188+
),
189+
args: '0x' as const,
190+
},
191+
];
192+
const result = rule.validateAndDecodePermission(caveats);
193+
expect(result.isValid).toBe(true);
194+
195+
// this is here as a type guard
196+
if (!result.isValid) {
197+
throw new Error('Expected valid result');
198+
}
199+
200+
expect(result.expiry).toBe(1720000);
201+
expect(result.data.tokenAddress.toLowerCase()).toBe(
202+
mixedCaseAddress.toLowerCase(),
203+
);
204+
});
205+
173206
it('rejects when periodDuration is 0', () => {
174207
const tokenAddress = '0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' as Hex;
175208
const periodAmountHex = 100n.toString(16).padStart(64, '0');
@@ -294,35 +327,4 @@ describe('erc20-token-periodic rule', () => {
294327
'Invalid erc20-token-periodic terms: periodAmount must be a positive number',
295328
);
296329
});
297-
298-
it('rejects when tokenAddress is not valid hex (invalid characters)', () => {
299-
const invalidTokenAddress = 'gg';
300-
const periodAmountHex = 100n.toString(16).padStart(64, '0');
301-
const periodDurationHex = (86400).toString(16).padStart(64, '0');
302-
const startDateHex = (1715664).toString(16).padStart(64, '0');
303-
const terms =
304-
`0x${invalidTokenAddress}${'0'.repeat(38)}${periodAmountHex}${periodDurationHex}${startDateHex}` as Hex;
305-
306-
const caveats = [
307-
expiryCaveat,
308-
valueLteCaveat,
309-
{
310-
enforcer: ERC20PeriodTransferEnforcer,
311-
terms,
312-
args: '0x' as const,
313-
},
314-
];
315-
316-
const result = rule.validateAndDecodePermission(caveats);
317-
expect(result.isValid).toBe(false);
318-
319-
// this is here as a type guard
320-
if (result.isValid) {
321-
throw new Error('Expected invalid result');
322-
}
323-
324-
expect(result.error.message).toContain(
325-
'Invalid erc20-token-periodic terms: tokenAddress must be a valid hex string',
326-
);
327-
});
328330
});

‎packages/gator-permissions-controller/src/decodePermission/rules/erc20TokenPeriodic.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { hexToBigInt, hexToNumber, isHexAddress } from '@metamask/utils';
1+
import { hexToBigInt, hexToNumber } from '@metamask/utils';
22

33
import { makePermissionRule } from './makePermissionRule';
44
import type {
@@ -89,12 +89,6 @@ function validateAndDecodeData(
8989
const periodAmountBigInt = hexToBigInt(periodAmount);
9090
const startTime = hexToNumber(startTimeRaw);
9191

92-
if (!isHexAddress(tokenAddress)) {
93-
throw new Error(
94-
'Invalid erc20-token-periodic terms: tokenAddress must be a valid hex string',
95-
);
96-
}
97-
9892
if (periodAmountBigInt === 0n) {
9993
throw new Error(
10094
'Invalid erc20-token-periodic terms: periodAmount must be a positive number',

‎packages/gator-permissions-controller/src/decodePermission/rules/erc20TokenStream.test.ts‎

Lines changed: 34 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,40 @@ describe('erc20-token-stream rule', () => {
130130
expect(result.error.message).toContain('must be');
131131
});
132132

133+
it('decodes mixed-case token address', () => {
134+
const mixedCaseAddress = contracts.ERC20StreamingEnforcer;
135+
const caveats = [
136+
expiryCaveat,
137+
valueLteCaveat,
138+
{
139+
enforcer: ERC20StreamingEnforcer,
140+
terms: createERC20StreamingTerms(
141+
{
142+
tokenAddress: mixedCaseAddress,
143+
initialAmount: 1n,
144+
maxAmount: 2n,
145+
amountPerSecond: 1n,
146+
startTime: 1715664,
147+
},
148+
{ out: 'hex' },
149+
),
150+
args: '0x' as const,
151+
},
152+
];
153+
const result = rule.validateAndDecodePermission(caveats);
154+
expect(result.isValid).toBe(true);
155+
156+
// this is here as a type guard
157+
if (!result.isValid) {
158+
throw new Error('Expected valid result');
159+
}
160+
161+
expect(result.expiry).toBe(1720000);
162+
expect(result.data?.tokenAddress.toLowerCase()).toBe(
163+
mixedCaseAddress.toLowerCase(),
164+
);
165+
});
166+
133167
it('decodes zero token address', () => {
134168
const zeroAddress = '0x0000000000000000000000000000000000000000' as Hex;
135169
const caveats = [
@@ -299,32 +333,4 @@ describe('erc20-token-stream rule', () => {
299333
'Invalid erc20-token-stream terms: startTime must be a positive number',
300334
);
301335
});
302-
303-
it('rejects when tokenAddress is not valid hex (invalid characters)', () => {
304-
const invalidTokenAddress = 'gg';
305-
const initialAmountHex = 1n.toString(16).padStart(64, '0');
306-
const maxAmountHex = 2n.toString(16).padStart(64, '0');
307-
const amountPerSecondHex = 1n.toString(16).padStart(64, '0');
308-
const startTimeHex = (1715664).toString(16).padStart(64, '0');
309-
const terms =
310-
`0x${invalidTokenAddress}${'0'.repeat(38)}${initialAmountHex}${maxAmountHex}${amountPerSecondHex}${startTimeHex}` as Hex;
311-
312-
const caveats = [
313-
expiryCaveat,
314-
valueLteCaveat,
315-
{ enforcer: ERC20StreamingEnforcer, terms, args: '0x' as const },
316-
];
317-
318-
const result = rule.validateAndDecodePermission(caveats);
319-
expect(result.isValid).toBe(false);
320-
321-
// this is here as a type guard
322-
if (result.isValid) {
323-
throw new Error('Expected invalid result');
324-
}
325-
326-
expect(result.error.message).toContain(
327-
'Invalid erc20-token-stream terms: tokenAddress must be a valid hex string',
328-
);
329-
});
330336
});

‎packages/gator-permissions-controller/src/decodePermission/rules/erc20TokenStream.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { hexToBigInt, hexToNumber, isHexAddress } from '@metamask/utils';
1+
import { hexToBigInt, hexToNumber } from '@metamask/utils';
22

33
import { makePermissionRule } from './makePermissionRule';
44
import type {
@@ -96,12 +96,6 @@ function validateAndDecodeData(
9696
const maxAmountBigInt = hexToBigInt(maxAmount);
9797
const amountPerSecondBigInt = hexToBigInt(amountPerSecond);
9898

99-
if (!isHexAddress(tokenAddress)) {
100-
throw new Error(
101-
'Invalid erc20-token-stream terms: tokenAddress must be a valid hex string',
102-
);
103-
}
104-
10599
if (maxAmountBigInt <= initialAmountBigInt) {
106100
throw new Error(
107101
'Invalid erc20-token-stream terms: maxAmount must be greater than initialAmount',

‎packages/gator-permissions-controller/src/decodePermission/rules/makePermissionRule.test.ts‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,176 @@ describe('makePermissionRule', () => {
4949
expect(result.data).toStrictEqual({});
5050
expect(validateAndDecodeData).toHaveBeenCalled();
5151
});
52+
53+
it('rejects when any caveat terms are not valid hex (invalid characters)', () => {
54+
const validateAndDecodeData = jest.fn().mockReturnValue({});
55+
56+
const rule = makePermissionRule({
57+
permissionType: 'native-token-stream',
58+
timestampEnforcer,
59+
optionalEnforcers: [],
60+
requiredEnforcers: { [requiredEnforcer]: 1 },
61+
validateAndDecodeData,
62+
});
63+
64+
const caveats = [
65+
{
66+
enforcer: timestampEnforcer,
67+
terms: '0xgg' as Hex,
68+
args: '0x' as Hex,
69+
},
70+
{
71+
enforcer: requiredEnforcer,
72+
terms: '0x' as Hex,
73+
args: '0x' as Hex,
74+
},
75+
];
76+
77+
const result = rule.validateAndDecodePermission(caveats);
78+
79+
expect(result.isValid).toBe(false);
80+
if (result.isValid) {
81+
throw new Error('Expected invalid result');
82+
}
83+
expect(result.error.message).toBe('Invalid terms: must be a hex string');
84+
expect(validateAndDecodeData).not.toHaveBeenCalled();
85+
});
86+
87+
it('rejects when any caveat terms contain non-hex characters after 0x prefix', () => {
88+
const validateAndDecodeData = jest.fn().mockReturnValue({});
89+
90+
const rule = makePermissionRule({
91+
permissionType: 'native-token-stream',
92+
timestampEnforcer,
93+
optionalEnforcers: [],
94+
requiredEnforcers: { [requiredEnforcer]: 1 },
95+
validateAndDecodeData,
96+
});
97+
98+
const caveats = [
99+
{
100+
enforcer: timestampEnforcer,
101+
terms:
102+
'0x000000000000000000000000000000000000000000000000000000000000000z' as Hex,
103+
args: '0x' as Hex,
104+
},
105+
{
106+
enforcer: requiredEnforcer,
107+
terms: '0x' as Hex,
108+
args: '0x' as Hex,
109+
},
110+
];
111+
112+
const result = rule.validateAndDecodePermission(caveats);
113+
114+
expect(result.isValid).toBe(false);
115+
if (result.isValid) {
116+
throw new Error('Expected invalid result');
117+
}
118+
expect(result.error.message).toBe('Invalid terms: must be a hex string');
119+
expect(validateAndDecodeData).not.toHaveBeenCalled();
120+
});
121+
122+
it('rejects when required enforcer terms are not valid hex', () => {
123+
const validateAndDecodeData = jest.fn().mockReturnValue({});
124+
125+
const rule = makePermissionRule({
126+
permissionType: 'native-token-stream',
127+
timestampEnforcer,
128+
optionalEnforcers: [],
129+
requiredEnforcers: { [requiredEnforcer]: 1 },
130+
validateAndDecodeData,
131+
});
132+
133+
const caveats = [
134+
{
135+
enforcer: timestampEnforcer,
136+
terms: createTimestampTerms({
137+
timestampAfterThreshold: 0,
138+
timestampBeforeThreshold: 1720000,
139+
}),
140+
args: '0x' as Hex,
141+
},
142+
{
143+
enforcer: requiredEnforcer,
144+
terms: '0xNOTHEX' as Hex,
145+
args: '0x' as Hex,
146+
},
147+
];
148+
149+
const result = rule.validateAndDecodePermission(caveats);
150+
151+
expect(result.isValid).toBe(false);
152+
if (result.isValid) {
153+
throw new Error('Expected invalid result');
154+
}
155+
expect(result.error.message).toBe('Invalid terms: must be a hex string');
156+
expect(validateAndDecodeData).not.toHaveBeenCalled();
157+
});
158+
159+
it('accepts caveat terms with mixed-case hex', () => {
160+
const validateAndDecodeData = jest.fn().mockReturnValue({});
161+
162+
const rule = makePermissionRule({
163+
permissionType: 'native-token-stream',
164+
timestampEnforcer,
165+
optionalEnforcers: [],
166+
requiredEnforcers: { [requiredEnforcer]: 1 },
167+
validateAndDecodeData,
168+
});
169+
170+
const caveats = [
171+
{
172+
enforcer: timestampEnforcer,
173+
terms: createTimestampTerms({
174+
timestampAfterThreshold: 0,
175+
timestampBeforeThreshold: 1720000,
176+
}),
177+
args: '0x' as Hex,
178+
},
179+
{
180+
enforcer: requiredEnforcer,
181+
terms:
182+
'0x000000000000000000000000000000000000000000000000000000000000abAB' as Hex,
183+
args: '0x' as Hex,
184+
},
185+
];
186+
187+
const result = rule.validateAndDecodePermission(caveats);
188+
189+
expect(result.isValid).toBe(true);
190+
if (!result.isValid) {
191+
throw new Error('Expected valid result');
192+
}
193+
expect(result.expiry).toBe(1720000);
194+
expect(validateAndDecodeData).toHaveBeenCalled();
195+
});
196+
197+
it('accepts caveat terms with empty hex', () => {
198+
const validateAndDecodeData = jest.fn().mockReturnValue({});
199+
200+
const rule = makePermissionRule({
201+
permissionType: 'native-token-stream',
202+
timestampEnforcer,
203+
optionalEnforcers: [],
204+
requiredEnforcers: { [requiredEnforcer]: 1 },
205+
validateAndDecodeData,
206+
});
207+
208+
const caveats = [
209+
{
210+
enforcer: requiredEnforcer,
211+
terms: '0x' as Hex,
212+
args: '0x' as Hex,
213+
},
214+
];
215+
216+
const result = rule.validateAndDecodePermission(caveats);
217+
218+
expect(result.isValid).toBe(true);
219+
if (!result.isValid) {
220+
throw new Error('Expected valid result');
221+
}
222+
expect(validateAndDecodeData).toHaveBeenCalled();
223+
});
52224
});

‎packages/gator-permissions-controller/src/decodePermission/rules/makePermissionRule.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { Caveat } from '@metamask/delegation-core';
2-
import { getChecksumAddress } from '@metamask/utils';
2+
import { getChecksumAddress, isStrictHexString } from '@metamask/utils';
33
import type { Hex } from '@metamask/utils';
44

55
import type {
@@ -71,6 +71,15 @@ export function makePermissionRule({
7171
enforcer: getChecksumAddress(caveat.enforcer),
7272
}));
7373
try {
74+
const invalidTerms = checksumCaveats.filter(
75+
// isStrictHexString rejects '0x' which is a valid terms value
76+
({ terms }) => terms !== '0x' && !isStrictHexString(terms),
77+
);
78+
79+
if (invalidTerms.length > 0) {
80+
throw new Error('Invalid terms: must be a hex string');
81+
}
82+
7483
let expiry: number | null = null;
7584

7685
const expiryTerms = getTermsByEnforcer({

0 commit comments

Comments
 (0)