Skip to content

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

Merged
thekidnamedkd merged 3 commits into
mainfrom
fix/evm-stringent-abi-validation
Jul 14, 2026
Merged

feat: Enforce EVM-stringent validation on proposal action ABI inputs#720
thekidnamedkd merged 3 commits into
mainfrom
fix/evm-stringent-abi-validation

Conversation

@thekidnamedkd

@thekidnamedkd thekidnamedkd commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Description

In relation to #719 -- this hardens ProposalActionsDecoder form validation based on findings from the review of #719. The principle applied: validate the shape of ABI types exactly as strictly as the ABI spec defines them — no stricter, no looser (#719 fixed a too-strict case; this fixes the too-loose ones).

Why

The decoder's live encoding wraps encodeFunctionData in a silent try/catch (proposalActionsDecoder.tsx). When form validation passes but viem throws, the data field silently stays 0x — and a submitted action with data: 0x isn't a failed action, it's a different action (a bare call / ETH transfer to the target). In a governance context that output gets voted on, not double-checked. Three validation gaps could reach that state (all verified empirically against viem 2.52.2):

  1. Signed int* types had no validation branch at all — only the required check. A lone - passed validation; viem throws Cannot convert - to a BigInt; the error was swallowed. (Also: BigInt('') === 0n, so only the required check stood between an empty int and silently encoding as zero.)
  2. No bit-width range validationuint8 with value 300 passed the ^[0-9]*$ regex; viem throws not in safe 8-bit unsigned integer range; swallowed.
  3. Address checksums were not verifiedaddressUtils.isAddress defaulted to strict: false, so a mixed-case address with a wrong EIP-55 checksum validated. That discards the checksum's entire purpose: catching typos/corruption on the one field where a mistake is irrecoverable. All-lowercase addresses still pass (verified — viem's strict mode only enforces the checksum when the input is mixed-case).

Changes

  • proposalActionsDecoderUtils: new validateSignedNumber (format) and validateNumberRange (BigInt bounds per bit-width, signed and unsigned, default 256); validateValue now covers int* and checks ranges after format; address validation uses { strict: true }.
  • ProposalActionsDecoderTextFieldEdit: the numeric input sanitizer no longer allows a leading - for unsigned types (it previously did, and only the regex validator caught it afterwards).
  • New copy messages signedNumber and numberRange under proposalActionsDecoder.validation (minor bump — additive copy API, same as previous copy additions e.g. feat(APP-823): Decoded action form input UX improvements #701).

With these in place, no value that passes validation can make encodeFunctionData throw, which closes the silent-0x channel from the input side. The try/catch remains as intended for transient keystroke states.

Not included (deliberately)

  • Surfacing encode failures in the UI: with spec-exact validators the catch should now only see transient states; if we ever want a visible error there it needs a small design decision and its own PR.
  • bytes fields still require an explicit 0x for empty bytes rather than accepting a blank field — that's the canonical representation (viem's Hex contract; JSON-RPC DATA requires the prefix) and blank-vs-empty ambiguity on calldata is worth keeping explicit.

How to test: Storybook → ProposalActions.DecoderArrayType story with mode: EDIT: paste a mixed-case address with one letter's case flipped → checksum error appears; all-lowercase or correctly-checksummed → accepted. Unit tests cover the int/range branches (uint8 255/256, int8 ±127/128/−129, uint256 boundary, lone -, unparsable values).

Type of change

  • New feature (non-breaking change which adds functionality)

Developer Checklist:

  • Manually smoke tested the functionality locally
  • Confirmed there are no new warnings or errors in the browser console
  • Made the corresponding changes to the documentation
  • Added tests that prove my fix is effective or that my feature works
  • Confirmed there are no new warnings on automated tests
  • Selected the correct base branch
  • Commented the code in hard-to-understand areas
  • Followed the code style guidelines of this project
  • Reviewed that the files changed in GitHub's UI reflect my intended changes
  • Confirmed the pipeline checks are not failing

Review Checklist:

  • Tested locally that all Acceptance Criteria or Expected Outcomes are satisfied
  • Confirmed that changes follow the code style guidelines of this project

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the ProposalActionsDecoder edit-mode form validation so ABI inputs are validated to EVM/ABI constraints (signed ints, bit-width bounds, and strict EIP-55 checksum checking) to prevent cases where invalid inputs silently fall back to data: 0x due to encodeFunctionData errors being swallowed.

Changes:

  • Add signed int* validation and numeric bit-width range checks (signed/unsigned), and enable strict checksum validation for address.
  • Update numeric input sanitization to disallow - for unsigned number types while preserving it for signed types.
  • Add new validation copy strings and a changeset entry for the minor release.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderUtils.ts Adds signed int validation, range checking, and strict address validation.
src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderUtils.test.ts Adds/extends unit tests for new numeric validation branches.
src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderTextField/proposalActionsDecoderTextFieldEdit.tsx Updates numeric sanitization to treat signed/unsigned differently for -.
src/modules/components/proposal/proposalActions/proposalActionsDecoder/proposalActionsDecoderTextField/proposalActionsDecoderTextFieldEdit.test.tsx Updates component tests for the sanitizer behavior.
src/modules/assets/copy/modulesCopy.ts Adds signedNumber and numberRange validation copy strings.
.changeset/evm-stringent-abi-validation.md Declares a minor version bump and documents the validation hardening.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@thekidnamedkd
thekidnamedkd force-pushed the fix/evm-stringent-abi-validation branch from 538684e to 112a64d Compare July 13, 2026 13:02
harryburger
harryburger previously approved these changes Jul 13, 2026

@milosh86 milosh86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@thekidnamedkd
thekidnamedkd merged commit 576b552 into main Jul 14, 2026
6 checks passed
@thekidnamedkd
thekidnamedkd deleted the fix/evm-stringent-abi-validation branch July 14, 2026 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants