Skip to content

fix: F-2026-18798 | [Dual Defense] UExecutor ValidateBasic Parses Unbounded Decimals Before Ante - #335

Open
0xNilesh wants to merge 1 commit into
audit-fixesfrom
F-2026-18798
Open

fix: F-2026-18798 | [Dual Defense] UExecutor ValidateBasic Parses Unbounded Decimals Before Ante#335
0xNilesh wants to merge 1 commit into
audit-fixesfrom
F-2026-18798

Conversation

@0xNilesh

Copy link
Copy Markdown
Member

F-2026-18798 — UExecutor ValidateBasic Parses Unbounded Decimals Before Ante

UniversalPayload.ValidateBasic validated six numeric string fields with only parse-success plus non-negativity:

// Validate all numeric string fields as uint256
bi, ok := new(big.Int).SetString(value, 10)
if !ok || bi.Sign() < 0 { return ... }

The comment promised uint256; the check delivered "parses as a non-negative integer". That gap is two distinct defects.

Defect 1 — unmetered DoS

big.Int decimal parsing is superlinear in digit count. Measured:

digits parse time
78 18µs
10k 456µs
100k 24.2ms
400k 486.6ms
900k 3.353s

This is reached from MsgExecutePayload.ValidateBasic, which is a gasless message type, and BaseApp.runTx runs validateBasicTxMsgs before the ante handler. So the work is free and unmetered to the submitter — no gas is charged, and no fee is deducted, before the parse happens.

The cost is also per field: there are six numeric fields on a single UniversalPayload, so one message multiplies the figure above by six. (decode_payload.go re-parses all six downstream, paying it a second time.) Hacken estimated ~0.4s at 900k digits; we measured 3.353s.

Defect 2 — silent truncation (not in the original finding)

All six fields are declared uint256 in the contract (src/libraries/Types.sol). We probed go-ethereum's ABI encoder on the pinned dependency:

input BitLen packs? round-trips?
2^256 - 1 (max uint256) 256 OK equal = true
2^256 (max + 1) 257 OK equal = false
78 nines 260 OK equal = false

The encoder truncates mod 2^256 without erroring. An over-range value was therefore not rejected anywhere — the UEA would execute a different amount than the user signed over. That is a correctness bug, not only a DoS.

The fix

A shared helper, types.ValidateUint256String, with three checks in this order:

  1. Length cap (80 chars) BEFORE SetString. This is the whole point of the ordering: it turns rejecting a 900k-digit field from an O(n²) parse into an O(1) length comparison. 80 = the 78 digits of max uint256 plus slack for a zero-padded client value.
  2. Parse — reject non-numeric and negative (pre-existing behaviour, and pre-existing error text preserved at each call site).
  3. BitLen() <= 256 — authoritative range check; closes the truncation gap.

Hacken's recommendation 1 (a length cap) is not sufficient on its own. 78 nines is only 78 characters — comfortably inside any 78- or 80-char cap — but has BitLen 260 and still overflows uint256. BitLen() is the load-bearing check; the length cap is the cheap pre-filter that keeps the expensive parse off the unmetered path. Neither replaces the other, which is why both are present and why the order is fixed.

We did not implement Hacken's recommendation 3 (reject in ante with gas charging). On gasless messages, charging gas is not the defence — Hacken themselves prefer failing cheaply in ValidateBasic. Same reasoning as F-2026-18821 / F-2026-18816.

Sites covered

  • the six UniversalPayload fields: value, gas_limit, max_fee_per_gas, max_priority_fee_per_gas, nonce, deadline
  • Inbound.Amount (ValidateForExecution)
  • OutboundTx.Amount

The two Amount sites keep their existing extra semantics — OutboundTx still requires strictly positive, Inbound still enforces its zero-amount-by-tx-type rule. Bounds were added; nothing was relaxed.

Out of scope

MigrationPayload.ValidateBasic has the identical defect, and is deliberately untouched here. It is deprecated on both sides — the contract marks it "(Deprecated) Legacy migration payload", and the latest UEA has no migrateUEA at all. It is being handled by the separate MsgMigrateUEA removal, so changing it here would only create a conflict.

Tests

x/uexecutor/types/uint256_test.go, covering both defects at all three call sites:

  • DoS: a multi-million-digit field is rejected, and rejected within a 1s wall-clock budget, on every one of the six payload fields plus both Amount sites. The timing assertion is deliberately made first, because it is the only assertion that fails if the length cap is removed (BitLen would still reject the value — just after paying for the parse).
  • Truncation boundary: 2^256 - 1 (BitLen 256) is accepted; 2^256 (BitLen 257) and 78 nines (BitLen 260) are rejected — with an explicit assertion that 78 nines fits inside the length cap, so only BitLen can catch it.
  • Normal values still accepted, empty strings still skipped, negatives and non-numerics still rejected with their original messages.

Both halves were mutation-verified: removing the length cap fails the timing assertions (6.3s vs the 1s budget), and removing the BitLen() check fails the 2^256 / 78-nines cases at all three sites.

Full suite green: ./x/... ./app/... ./test/integration/... — 20 packages ok, 0 failures.

Length-cap before parse, then BitLen<=256, via a shared helper applied to the six UniversalPayload numeric fields, Inbound.Amount and OutboundTx.Amount.
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.

1 participant