feat: Enforce EVM-stringent validation on proposal action ABI inputs (#720)

Author: thekidnamedkd

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

@@ -0,0 +1,5 @@
+---
+"@aragon/gov-ui-kit": minor
+---
+
+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

@@ -69,6 +69,8 @@ export const modulesCopy = {
             address: (label: string) => `${label} is not a valid address.`,
             bytes: (label: string) => `${label} is not a valid bytes value.`,
             unsignedNumber: (label: string) => `${label} is not a valid uint value.`,
+            signedNumber: (label: string) => `${label} is not a valid int value.`,
+            numberRange: (label: string, type: string) => `${label} is out of range for ${type}.`,
         },
     },
     proposalActionChangeMembers: {

src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderBooleanField/proposalActionsDecoderBooleanField.test.tsx

@@ -61,6 +61,26 @@ describe('<ProposalActionsDecoderBooleanField /> component', () => {
         expect(useControllerSpy).toHaveBeenCalledWith(expect.objectContaining({ name: `${formPrefix}.${fieldName}` }));
     });
 
+    it('normalises string field values to booleans to support transaction data encoding', () => {
+        const onChange = jest.fn();
+        useControllerSpy.mockReturnValue({
+            fieldState: { error: undefined },
+            field: { value: 'true', onBlur: jest.fn(), onChange, ref: jest.fn() },
+        } as unknown as ReactHookForm.UseControllerReturn);
+        render(createTestComponent());
+        expect(onChange).toHaveBeenCalledWith(true);
+    });
+
+    it('does not update the field value when it is already a boolean', () => {
+        const onChange = jest.fn();
+        useControllerSpy.mockReturnValue({
+            fieldState: { error: undefined },
+            field: { value: false, onBlur: jest.fn(), onChange, ref: jest.fn() },
+        } as unknown as ReactHookForm.UseControllerReturn);
+        render(createTestComponent());
+        expect(onChange).not.toHaveBeenCalled();
+    });
+
     it('renders the parameter name, type and notice as field labels', () => {
         const parameter = { name: 'tryExecute', notice: 'description', type: 'bool', value: undefined };
         render(createTestComponent({ parameter }));

src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderBooleanField/proposalActionsDecoderBooleanField.tsx

@@ -1,3 +1,4 @@
+import { useEffect } from 'react';
 import { useController } from 'react-hook-form';
 import { Radio, RadioGroup } from '../../../../../../core';
 import { useGukModulesContext } from '../../../../gukModulesProvider';
@@ -39,13 +40,23 @@ export const ProposalActionsDecoderBooleanField: React.FC<IProposalActionsDecode
     const { error } = fieldState;
     const alert = error?.message == null ? undefined : { message: error.message, variant: 'critical' as const };
 
+    const { value, onChange } = field;
+
+    useEffect(() => {
+        // Normalise string values ("true" / "false") seeded by the application to booleans as the transaction data
+        // encoding only accepts boolean values for bool types.
+        if (typeof value === 'string' && proposalActionsDecoderUtils.validateBoolean(value)) {
+            onChange(value === 'true');
+        }
+    }, [value, onChange]);
+
     const label = hideLabels ? undefined : (
         <>
             {name} <span className="text-neutral-500">({type})</span>
         </>
     );
 
-    const handleValueChange = (value: string) => field.onChange(value === 'true');
+    const handleValueChange = (newValue: string) => onChange(newValue === 'true');
 
     return (
         <RadioGroup
@@ -56,7 +67,7 @@ export const ProposalActionsDecoderBooleanField: React.FC<IProposalActionsDecode
             onBlur={field.onBlur}
             onValueChange={handleValueChange}
             ref={field.ref}
-            value={field.value?.toString() ?? ''}
+            value={value?.toString() ?? ''}
         >
             <Radio label={copy.proposalActionsDecoder.booleanTrue} value="true" />
             <Radio label={copy.proposalActionsDecoder.booleanFalse} value="false" />

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

@@ -130,7 +130,7 @@ describe('<ProposalActionsDecoderTextFieldEdit /> component', () => {
         expect(onChange).toHaveBeenCalledWith('tru');
     });
 
-    it('triggers the onChange callback removing the non-numberic values when type is a number', async () => {
+    it('triggers the onChange callback removing the non-numeric values and negative signs when type is an unsigned number', async () => {
         const onChange = jest.fn();
         const parameter = { name: 'uintParam', type: 'uint32', value: undefined };
         const controllerReturn = {
@@ -143,6 +143,22 @@ describe('<ProposalActionsDecoderTextFieldEdit /> component', () => {
         } as useFormContext.UseFormContextReturn);
         render(createTestComponent({ parameter }));
         await userEvent.type(screen.getByRole('textbox'), '1');
+        expect(onChange).toHaveBeenCalledWith('321');
+    });
+
+    it('triggers the onChange callback keeping one leading negative sign when type is a signed number', async () => {
+        const onChange = jest.fn();
+        const parameter = { name: 'intParam', type: 'int32', value: undefined };
+        const controllerReturn = {
+            fieldState: { error: undefined },
+            field: { value: 'ab--32.', onChange },
+        } as unknown as ReactHookForm.UseControllerReturn;
+        useControllerSpy.mockReturnValue(controllerReturn);
+        useFormContextSpy.mockReturnValue({
+            watch: () => controllerReturn.field.value as unknown,
+        } as useFormContext.UseFormContextReturn);
+        render(createTestComponent({ parameter }));
+        await userEvent.type(screen.getByRole('textbox'), '1');
         expect(onChange).toHaveBeenCalledWith('-321');
     });
 

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

@@ -55,8 +55,9 @@ export const ProposalActionsDecoderTextFieldEdit: React.FC<IProposalActionsDecod
                 : newValue.toLocaleLowerCase();
             onChange(parsedValue);
         } else if (proposalActionsDecoderUtils.isNumberType(type)) {
-            // Allow only numbers and one "-" negative sign
-            const newValue = event.target.value.replace(/[^0-9-]/g, '').replace(/(?!^-)-/g, '');
+            // Allow only numbers and, for signed types only, one leading "-" negative sign
+            const negativeSignRegex = proposalActionsDecoderUtils.isSignedNumberType(type) ? /(?!^-)-/g : /-/g;
+            const newValue = event.target.value.replace(/[^0-9-]/g, '').replace(negativeSignRegex, '');
             onChange(newValue);
         } else {
             onChange(event);

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

@@ -23,13 +23,17 @@ describe('ProposalActionsDecoder utils', () => {
         const validateAddressSpy = jest.spyOn(addressUtils, 'isAddress');
         const validateBytesSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateBytes');
         const validateUnsignedNumberSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateUnsignedNumber');
+        const validateSignedNumberSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateSignedNumber');
+        const validateNumberRangeSpy = jest.spyOn(proposalActionsDecoderUtils, 'validateNumberRange');
 
         afterEach(() => {
             validateRequiredSpy.mockReset();
             validateBooleanSpy.mockReset();
             validateAddressSpy.mockReset();
             validateBytesSpy.mockReset();
             validateUnsignedNumberSpy.mockReset();
+            validateSignedNumberSpy.mockReset();
+            validateNumberRangeSpy.mockReset();
         });
 
         afterAll(() => {
@@ -38,6 +42,8 @@ describe('ProposalActionsDecoder utils', () => {
             validateAddressSpy.mockRestore();
             validateBytesSpy.mockRestore();
             validateUnsignedNumberSpy.mockRestore();
+            validateSignedNumberSpy.mockRestore();
+            validateNumberRangeSpy.mockRestore();
         });
 
         const buildValidateValueParams = (params?: Partial<IGetValidationRulesParams>): IGetValidationRulesParams => ({
@@ -49,6 +55,8 @@ describe('ProposalActionsDecoder utils', () => {
                 address: () => 'address-error',
                 bytes: () => 'bytes-error',
                 unsignedNumber: () => 'uint-error',
+                signedNumber: () => 'int-error',
+                numberRange: () => 'range-error',
             },
             ...params,
         });
@@ -119,6 +127,38 @@ describe('ProposalActionsDecoder utils', () => {
             const params = buildValidateValueParams({ type: 'uint16' });
             expect(proposalActionsDecoderUtils.validateValue('10', params)).toBeTruthy();
         });
+
+        it('returns range error message when value has uint type and does not fit its bit-width', () => {
+            validateUnsignedNumberSpy.mockReturnValue(true);
+            validateNumberRangeSpy.mockReturnValue(false);
+            const params = buildValidateValueParams({ type: 'uint8' });
+            expect(proposalActionsDecoderUtils.validateValue('300', params)).toEqual('range-error');
+        });
+
+        it('returns int error message when value has int type and is not valid', () => {
+            validateSignedNumberSpy.mockReturnValue(false);
+            const params = buildValidateValueParams({ type: 'int256' });
+            expect(proposalActionsDecoderUtils.validateValue('-', params)).toEqual('int-error');
+        });
+
+        it('returns range error message when value has int type and does not fit its bit-width', () => {
+            validateSignedNumberSpy.mockReturnValue(true);
+            validateNumberRangeSpy.mockReturnValue(false);
+            const params = buildValidateValueParams({ type: 'int8' });
+            expect(proposalActionsDecoderUtils.validateValue('-129', params)).toEqual('range-error');
+        });
+
+        it('returns true when value has int type and is valid', () => {
+            const params = buildValidateValueParams({ type: 'int32' });
+            expect(proposalActionsDecoderUtils.validateValue('-5', params)).toBeTruthy();
+        });
+
+        it('validates addresses with strict checksum validation', () => {
+            const value = '0x0B2a45c2bCb56dA84920585f985087973c715364';
+            const params = buildValidateValueParams({ type: 'address' });
+            proposalActionsDecoderUtils.validateValue(value, params);
+            expect(validateAddressSpy).toHaveBeenCalledWith(value, { strict: true });
+        });
     });
 
     describe('validateRequired', () => {
@@ -179,6 +219,7 @@ describe('ProposalActionsDecoder utils', () => {
     describe('validateUnsignedNumber', () => {
         it('returns false when value is not a valid uint value', () => {
             expect(proposalActionsDecoderUtils.validateUnsignedNumber(undefined)).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateUnsignedNumber('')).toBeFalsy();
             expect(proposalActionsDecoderUtils.validateUnsignedNumber('-1')).toBeFalsy();
             expect(proposalActionsDecoderUtils.validateUnsignedNumber('79.11')).toBeFalsy();
         });
@@ -189,6 +230,52 @@ describe('ProposalActionsDecoder utils', () => {
         });
     });
 
+    describe('validateSignedNumber', () => {
+        it('returns false when value is not a valid int value', () => {
+            expect(proposalActionsDecoderUtils.validateSignedNumber(undefined)).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateSignedNumber('')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateSignedNumber('-')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateSignedNumber('79.11')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateSignedNumber('1-2')).toBeFalsy();
+        });
+
+        it('returns true when value is a valid int value', () => {
+            expect(proposalActionsDecoderUtils.validateSignedNumber('-1')).toBeTruthy();
+            expect(proposalActionsDecoderUtils.validateSignedNumber('0')).toBeTruthy();
+            expect(proposalActionsDecoderUtils.validateSignedNumber('8645312')).toBeTruthy();
+        });
+    });
+
+    describe('validateNumberRange', () => {
+        it('returns false when value does not fit the bit-width of the type', () => {
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint8', '256')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint16', '65536')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('int8', '128')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('int8', '-129')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint', '-1')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint256', (2n ** 256n).toString())).toBeFalsy();
+        });
+
+        it('returns true when value fits the bit-width of the type', () => {
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint8', '255')).toBeTruthy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('int8', '-128')).toBeTruthy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('int8', '127')).toBeTruthy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint', (2n ** 256n - 1n).toString())).toBeTruthy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('int', (-(2n ** 255n)).toString())).toBeTruthy();
+        });
+
+        it('returns false when value cannot be parsed as an integer', () => {
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint8', 'abc')).toBeFalsy();
+        });
+
+        it('returns false when the type has an invalid bit-width', () => {
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint0', '0')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint12', '0')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('uint264', '0')).toBeFalsy();
+            expect(proposalActionsDecoderUtils.validateNumberRange('int9999', '0')).toBeFalsy();
+        });
+    });
+
     describe('isArrayType', () => {
         it('returns false when type is not an array type', () => {
             expect(proposalActionsDecoderUtils.isArrayType('uint')).toBeFalsy();

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

@@ -28,7 +28,8 @@ export type NestedProposalActionFormValues = DeepPartial<IProposalAction> | Reco
 
 class ProposalActionsDecoderUtils {
     private bytesRegex = /^0x[0-9a-fA-F]*$/;
-    private unsignedNumberRegex = /^[0-9]*$/;
+    private unsignedNumberRegex = /^[0-9]+$/;
+    private signedNumberRegex = /^-?[0-9]+$/;
 
     getFieldName = (name: string, prefix?: string) => (prefix == null ? name : `${prefix}.${name}`);
 
@@ -42,13 +43,22 @@ class ProposalActionsDecoderUtils {
             return this.validateBoolean(value) || errorMessages.boolean(label);
         }
         if (type === 'address') {
-            return addressUtils.isAddress(value?.toString()) || errorMessages.address(label);
+            return addressUtils.isAddress(value?.toString(), { strict: true }) || errorMessages.address(label);
         }
         if (type.startsWith('bytes')) {
             return this.validateBytes(type, value) || errorMessages.bytes(label);
         }
         if (this.isUnsignedNumberType(type)) {
-            return this.validateUnsignedNumber(value) || errorMessages.unsignedNumber(label);
+            if (!this.validateUnsignedNumber(value)) {
+                return errorMessages.unsignedNumber(label);
+            }
+            return this.validateNumberRange(type, value) || errorMessages.numberRange(label, type);
+        }
+        if (this.isSignedNumberType(type)) {
+            if (!this.validateSignedNumber(value)) {
+                return errorMessages.signedNumber(label);
+            }
+            return this.validateNumberRange(type, value) || errorMessages.numberRange(label, type);
         }
 
         return undefined;
@@ -75,6 +85,35 @@ class ProposalActionsDecoderUtils {
     validateUnsignedNumber = (value?: ProposalActionsFieldValue): boolean =>
         value != null && this.unsignedNumberRegex.test(value.toString());
 
+    validateSignedNumber = (value?: ProposalActionsFieldValue): boolean =>
+        value != null && this.signedNumberRegex.test(value.toString());
+
+    // 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).
+    // Only call after the value passed the unsigned / signed number format validation.
+    validateNumberRange = (type: string, value?: ProposalActionsFieldValue): boolean => {
+        const isSigned = this.isSignedNumberType(type);
+        const sizeString = type.slice(isSigned ? 3 : 4);
+        const size = sizeString.length === 0 ? 256 : Number.parseInt(sizeString, 10);
+
+        // ABI integer sizes are limited to 8..256 bits in steps of 8 (bare uint / int alias to 256 bits).
+        if (Number.isNaN(size) || size < 8 || size > 256 || size % 8 !== 0) {
+            return false;
+        }
+
+        let parsedValue: bigint;
+        try {
+            parsedValue = BigInt(value?.toString() ?? '');
+        } catch {
+            return false;
+        }
+
+        const bits = BigInt(size);
+        const minValue = isSigned ? -(2n ** (bits - 1n)) : 0n;
+        const maxValue = isSigned ? 2n ** (bits - 1n) - 1n : 2n ** bits - 1n;
+
+        return parsedValue >= minValue && parsedValue <= maxValue;
+    };
+
     isArrayType = (type: string) => type.endsWith('[]');
 
     isTupleType = (type: string) => type === 'tuple';