Skip to content

Commit 112a64d

Browse files
thekidnamedkdthekidnamedkd
committed
feat: Enforce EVM-stringent validation on proposal action ABI inputs
1 parent 942d83f commit 112a64d

6 files changed

Lines changed: 139 additions & 5 deletions

File tree

‎.changeset/evm-stringent-abi-validation.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@aragon/gov-ui-kit": minor
3+
---
4+
5+
Enforce EVM-stringent validation on `ProposalActionsDecoder` inputs: validate signed `int*` values (previously unvalidated), check that numeric values fit the bit-width of their type (e.g. `uint8` must be 0-255), verify EIP-55 address checksums (all-lowercase addresses are still accepted), and strip negative signs from unsigned number inputs. Adds `signedNumber` and `numberRange` messages to the `proposalActionsDecoder.validation` copy.

‎src/modules/assets/copy/modulesCopy.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ export const modulesCopy = {
6969
address: (label: string) => `${label} is not a valid address.`,
7070
bytes: (label: string) => `${label} is not a valid bytes value.`,
7171
unsignedNumber: (label: string) => `${label} is not a valid uint value.`,
72+
signedNumber: (label: string) => `${label} is not a valid int value.`,
73+
numberRange: (label: string, type: string) => `${label} is out of range for ${type}.`,
7274
},
7375
},
7476
proposalActionChangeMembers: {

‎src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderTextField/proposalActionsDecoderTextFieldEdit.test.tsx‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ describe('<ProposalActionsDecoderTextFieldEdit /> component', () => {
130130
expect(onChange).toHaveBeenCalledWith('tru');
131131
});
132132

133-
it('triggers the onChange callback removing the non-numberic values when type is a number', async () => {
133+
it('triggers the onChange callback removing the non-numeric values and negative signs when type is an unsigned number', async () => {
134134
const onChange = jest.fn();
135135
const parameter = { name: 'uintParam', type: 'uint32', value: undefined };
136136
const controllerReturn = {
@@ -143,6 +143,22 @@ describe('<ProposalActionsDecoderTextFieldEdit /> component', () => {
143143
} as useFormContext.UseFormContextReturn);
144144
render(createTestComponent({ parameter }));
145145
await userEvent.type(screen.getByRole('textbox'), '1');
146+
expect(onChange).toHaveBeenCalledWith('321');
147+
});
148+
149+
it('triggers the onChange callback keeping one leading negative sign when type is a signed number', async () => {
150+
const onChange = jest.fn();
151+
const parameter = { name: 'intParam', type: 'int32', value: undefined };
152+
const controllerReturn = {
153+
fieldState: { error: undefined },
154+
field: { value: 'ab--32.', onChange },
155+
} as unknown as ReactHookForm.UseControllerReturn;
156+
useControllerSpy.mockReturnValue(controllerReturn);
157+
useFormContextSpy.mockReturnValue({
158+
watch: () => controllerReturn.field.value as unknown,
159+
} as useFormContext.UseFormContextReturn);
160+
render(createTestComponent({ parameter }));
161+
await userEvent.type(screen.getByRole('textbox'), '1');
146162
expect(onChange).toHaveBeenCalledWith('-321');
147163
});
148164

‎src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderTextField/proposalActionsDecoderTextFieldEdit.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,9 @@ export const ProposalActionsDecoderTextFieldEdit: React.FC<IProposalActionsDecod
5555
: newValue.toLocaleLowerCase();
5656
onChange(parsedValue);
5757
} else if (proposalActionsDecoderUtils.isNumberType(type)) {
58-
// Allow only numbers and one "-" negative sign
59-
const newValue = event.target.value.replace(/[^0-9-]/g, '').replace(/(?!^-)-/g, '');
58+
// Allow only numbers and, for signed types only, one leading "-" negative sign
59+
const negativeSignRegex = proposalActionsDecoderUtils.isSignedNumberType(type) ? /(?!^-)-/g : /-/g;
60+
const newValue = event.target.value.replace(/[^0-9-]/g, '').replace(negativeSignRegex, '');
6061
onChange(newValue);
6162
} else {
6263
onChange(event);

‎src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderUtils.test.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,17 @@ describe('ProposalActionsDecoder utils', () => {
2323
const validateAddressSpy = jest.spyOn(addressUtils, 'isAddress');
2424
const validateBytesSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateBytes');
2525
const validateUnsignedNumberSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateUnsignedNumber');
26+
const validateSignedNumberSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateSignedNumber');
27+
const validateNumberRangeSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateNumberRange');
2628

2729
afterEach(() => {
2830
validateRequiredSpy.mockReset();
2931
validateBooleanSpy.mockReset();
3032
validateAddressSpy.mockReset();
3133
validateBytesSpy.mockReset();
3234
validateUnsignedNumberSpy.mockReset();
35+
validateSignedNumberSpy.mockReset();
36+
validateNumberRangeSpy.mockReset();
3337
});
3438

3539
afterAll(() => {
@@ -38,6 +42,8 @@ describe('ProposalActionsDecoder utils', () => {
3842
validateAddressSpy.mockRestore();
3943
validateBytesSpy.mockRestore();
4044
validateUnsignedNumberSpy.mockRestore();
45+
validateSignedNumberSpy.mockRestore();
46+
validateNumberRangeSpy.mockRestore();
4147
});
4248

4349
const buildValidateValueParams = (params?: Partial<IGetValidationRulesParams>): IGetValidationRulesParams => ({
@@ -49,6 +55,8 @@ describe('ProposalActionsDecoder utils', () => {
4955
address: () => 'address-error',
5056
bytes: () => 'bytes-error',
5157
unsignedNumber: () => 'uint-error',
58+
signedNumber: () => 'int-error',
59+
numberRange: () => 'range-error',
5260
},
5361
...params,
5462
});
@@ -119,6 +127,38 @@ describe('ProposalActionsDecoder utils', () => {
119127
const params = buildValidateValueParams({ type: 'uint16' });
120128
expect(proposalActionsDecoderUtils.validateValue('10', params)).toBeTruthy();
121129
});
130+
131+
it('returns range error message when value has uint type and does not fit its bit-width', () => {
132+
validateUnsignedNumberSpy.mockReturnValue(true);
133+
validateNumberRangeSpy.mockReturnValue(false);
134+
const params = buildValidateValueParams({ type: 'uint8' });
135+
expect(proposalActionsDecoderUtils.validateValue('300', params)).toEqual('range-error');
136+
});
137+
138+
it('returns int error message when value has int type and is not valid', () => {
139+
validateSignedNumberSpy.mockReturnValue(false);
140+
const params = buildValidateValueParams({ type: 'int256' });
141+
expect(proposalActionsDecoderUtils.validateValue('-', params)).toEqual('int-error');
142+
});
143+
144+
it('returns range error message when value has int type and does not fit its bit-width', () => {
145+
validateSignedNumberSpy.mockReturnValue(true);
146+
validateNumberRangeSpy.mockReturnValue(false);
147+
const params = buildValidateValueParams({ type: 'int8' });
148+
expect(proposalActionsDecoderUtils.validateValue('-129', params)).toEqual('range-error');
149+
});
150+
151+
it('returns true when value has int type and is valid', () => {
152+
const params = buildValidateValueParams({ type: 'int32' });
153+
expect(proposalActionsDecoderUtils.validateValue('-5', params)).toBeTruthy();
154+
});
155+
156+
it('validates addresses with strict checksum validation', () => {
157+
const value = '0x0B2a45c2bCb56dA84920585f985087973c715364';
158+
const params = buildValidateValueParams({ type: 'address' });
159+
proposalActionsDecoderUtils.validateValue(value, params);
160+
expect(validateAddressSpy).toHaveBeenCalledWith(value, { strict: true });
161+
});
122162
});
123163

124164
describe('validateRequired', () => {
@@ -189,6 +229,44 @@ describe('ProposalActionsDecoder utils', () => {
189229
});
190230
});
191231

232+
describe('validateSignedNumber', () => {
233+
it('returns false when value is not a valid int value', () => {
234+
expect(proposalActionsDecoderUtils.validateSignedNumber(undefined)).toBeFalsy();
235+
expect(proposalActionsDecoderUtils.validateSignedNumber('-')).toBeFalsy();
236+
expect(proposalActionsDecoderUtils.validateSignedNumber('79.11')).toBeFalsy();
237+
expect(proposalActionsDecoderUtils.validateSignedNumber('1-2')).toBeFalsy();
238+
});
239+
240+
it('returns true when value is a valid int value', () => {
241+
expect(proposalActionsDecoderUtils.validateSignedNumber('-1')).toBeTruthy();
242+
expect(proposalActionsDecoderUtils.validateSignedNumber('0')).toBeTruthy();
243+
expect(proposalActionsDecoderUtils.validateSignedNumber('8645312')).toBeTruthy();
244+
});
245+
});
246+
247+
describe('validateNumberRange', () => {
248+
it('returns false when value does not fit the bit-width of the type', () => {
249+
expect(proposalActionsDecoderUtils.validateNumberRange('uint8', '256')).toBeFalsy();
250+
expect(proposalActionsDecoderUtils.validateNumberRange('uint16', '65536')).toBeFalsy();
251+
expect(proposalActionsDecoderUtils.validateNumberRange('int8', '128')).toBeFalsy();
252+
expect(proposalActionsDecoderUtils.validateNumberRange('int8', '-129')).toBeFalsy();
253+
expect(proposalActionsDecoderUtils.validateNumberRange('uint', '-1')).toBeFalsy();
254+
expect(proposalActionsDecoderUtils.validateNumberRange('uint256', (2n ** 256n).toString())).toBeFalsy();
255+
});
256+
257+
it('returns true when value fits the bit-width of the type', () => {
258+
expect(proposalActionsDecoderUtils.validateNumberRange('uint8', '255')).toBeTruthy();
259+
expect(proposalActionsDecoderUtils.validateNumberRange('int8', '-128')).toBeTruthy();
260+
expect(proposalActionsDecoderUtils.validateNumberRange('int8', '127')).toBeTruthy();
261+
expect(proposalActionsDecoderUtils.validateNumberRange('uint', (2n ** 256n - 1n).toString())).toBeTruthy();
262+
expect(proposalActionsDecoderUtils.validateNumberRange('int', (-(2n ** 255n)).toString())).toBeTruthy();
263+
});
264+
265+
it('returns false when value cannot be parsed as an integer', () => {
266+
expect(proposalActionsDecoderUtils.validateNumberRange('uint8', 'abc')).toBeFalsy();
267+
});
268+
});
269+
192270
describe('isArrayType', () => {
193271
it('returns false when type is not an array type', () => {
194272
expect(proposalActionsDecoderUtils.isArrayType('uint')).toBeFalsy();

‎src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderUtils.ts‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export type NestedProposalActionFormValues = DeepPartial<IProposalAction> | Reco
2929
class ProposalActionsDecoderUtils {
3030
private bytesRegex = /^0x[0-9a-fA-F]*$/;
3131
private unsignedNumberRegex = /^[0-9]*$/;
32+
private signedNumberRegex = /^(-?[0-9]+)?$/;
3233

3334
getFieldName = (name: string, prefix?: string) => (prefix == null ? name : `${prefix}.${name}`);
3435

@@ -42,13 +43,22 @@ class ProposalActionsDecoderUtils {
4243
return this.validateBoolean(value) || errorMessages.boolean(label);
4344
}
4445
if (type === 'address') {
45-
return addressUtils.isAddress(value?.toString()) || errorMessages.address(label);
46+
return addressUtils.isAddress(value?.toString(), { strict: true }) || errorMessages.address(label);
4647
}
4748
if (type.startsWith('bytes')) {
4849
return this.validateBytes(type, value) || errorMessages.bytes(label);
4950
}
5051
if (this.isUnsignedNumberType(type)) {
51-
return this.validateUnsignedNumber(value) || errorMessages.unsignedNumber(label);
52+
if (!this.validateUnsignedNumber(value)) {
53+
return errorMessages.unsignedNumber(label);
54+
}
55+
return this.validateNumberRange(type, value) || errorMessages.numberRange(label, type);
56+
}
57+
if (this.isSignedNumberType(type)) {
58+
if (!this.validateSignedNumber(value)) {
59+
return errorMessages.signedNumber(label);
60+
}
61+
return this.validateNumberRange(type, value) || errorMessages.numberRange(label, type);
5262
}
5363

5464
return undefined;
@@ -75,6 +85,28 @@ class ProposalActionsDecoderUtils {
7585
validateUnsignedNumber = (value?: ProposalActionsFieldValue): boolean =>
7686
value != null && this.unsignedNumberRegex.test(value.toString());
7787

88+
validateSignedNumber = (value?: ProposalActionsFieldValue): boolean =>
89+
value != null && this.signedNumberRegex.test(value.toString());
90+
91+
// Checks that the value fits the bit-width of the number type (e.g. 0 to 2^8-1 for uint8, -2^7 to 2^7-1 for int8).
92+
// Only call after the value passed the unsigned / signed number format validation.
93+
validateNumberRange = (type: string, value?: ProposalActionsFieldValue): boolean => {
94+
const isSigned = this.isSignedNumberType(type);
95+
const bits = BigInt(Number.parseInt(type.slice(isSigned ? 3 : 4), 10) || 256);
96+
97+
let parsedValue: bigint;
98+
try {
99+
parsedValue = BigInt(value?.toString() ?? '');
100+
} catch {
101+
return false;
102+
}
103+
104+
const minValue = isSigned ? -(2n ** (bits - 1n)) : 0n;
105+
const maxValue = isSigned ? 2n ** (bits - 1n) - 1n : 2n ** bits - 1n;
106+
107+
return parsedValue >= minValue && parsedValue <= maxValue;
108+
};
109+
78110
isArrayType = (type: string) => type.endsWith('[]');
79111

80112
isTupleType = (type: string) => type === 'tuple';

0 commit comments

Comments
 (0)