Skip to content

gate the transfer.allow limit parse so an oversized limit cannot grant MaxInt64 - #238

Open
lordbutterfly-hive wants to merge 1 commit into
mainfrom
fix/intent-limit-parse-gate
Open

gate the transfer.allow limit parse so an oversized limit cannot grant MaxInt64#238
lordbutterfly-hive wants to merge 1 commit into
mainfrom
fix/intent-limit-parse-gate

Conversation

@lordbutterfly-hive

Copy link
Copy Markdown
Contributor

fix(contract): gate the transfer.allow limit parse so an oversized limit cannot grant MaxInt64

Branch: fix/intent-limit-parse-gatemain
Base: cc069b3f (origin/main)
Files: 2 — modules/contract/execution-context/execution-context.go, modules/contract/execution-context/intent_limits_test.go (new)
Behaviour change on merge: none. The fix is gated and the gate defaults to off.


The bug

An intent is the permission slip that bounds what a contract may pull from the
caller. transfer.allow sets a per-token ceiling, and PullBalance is bounded by
it. Building those ceilings discarded the parse error:

val, _ := common.ParseDecimalsToBaseUnits(limit, decimals)
tokenLimits[token] = &val

ParseDecimalsToBaseUnits ends in strconv.ParseInt, which is asymmetric on
failure
: a syntax error returns 0, an out-of-range value returns
math.MaxInt64 — both alongside an error that was thrown away. So the two
malformed cases degraded in opposite directions:

signed limit recorded ceiling effect
"abc" 0 draw refused — fails closed
"1.2.3" 0 draw refused — fails closed
"99999999999999999999" math.MaxInt64 no ceiling in practice

The second row is a defence-in-depth failure on the only mechanism that limits
what a contract can take. It is not a way to steal from a stranger — the intent is
signed by the account whose funds are at risk — but it does mean a client that
displays 1.000 while signing an oversized limit obtains an allowance the user
never intended to grant, and the clamp is what makes a malformed value
permissive rather than refused.

Found while stress-testing intent handling for wallet-signed (did:pkh)
transactions. Everything else in that sweep failed closed; this was the only
fail-open.

Why it is gated instead of just fixed

This function feeds contract execution. What a malformed limit resolves to decides
whether a draw succeeds, which decides state. Change it unconditionally and an
upgraded node and a not-yet-upgraded node compute different state from the same
block, and a replay from genesis stops reproducing the chain.

That is the hazard ConsensusParams.EvmAddressChecksumHeight already documents at
length, and the reason this repo moved to consensus-version gating for
consensus-affecting changes.

So the strict rule rides strictIntentLimits, default false, which is
byte-identical to today including the MaxInt64 clamp. Nothing changes when this
merges.

Activation is a separate decision and is deliberately not made here. Resolve
it from the chain-active consensus version exactly as tryCatchActive is
resolved, on whichever line you want to batch it with. Until then the gate is
inert and the tests prove both that it is inert and that it works when switched
on.

What changed

  • The intent loop moved into buildTokenLimits(intents, strict)verbatim in
    the default path. It now runs after the option loop in New so it can read the
    gate; nothing between the struct literal and there reads tokenLimits, so that
    reorder is behaviour-preserving.
  • strictIntentLimits field + WithStrictIntentLimits(active bool), mirroring
    tryCatchActive / WithTryCatch.
  • The gate propagates into nested contract calls. A nested call builds its own
    tokenLimits from opts.Intents, so without this a single transaction would
    enforce two different ceilings depending on call depth. (This was missed in the
    first draft and caught by asking why the change was sitting on the wrong branch —
    worth stating plainly.)
  • Documented that for a duplicate (type, token) the first intent in array
    order wins
    — not the largest, not the last. Unchanged behaviour, just no
    longer a thing a reader has to guess.

Tests

intent_limits_test.go, 8 cases. Three of them exist specifically to catch the
ways this change could go wrong, and each was verified by mutation:

Mutation Caught by
ship the fix ungated (strict hard-coded true) — the fork NewDefaultsToLegacyBehaviour
gate wired but inert (option ignored) NewHonoursTheOption
propagation dropped from the nested call GatePropagatesIntoNestedContractCalls

Note the first mutant initially passed: the earlier tests called
buildTokenLimits directly and so pinned the function without pinning the wiring.
NewDefaultsToLegacyBehaviour goes through New and closes that.

The rest pin: legacy MaxInt64 on overflow with the gate off, refusal for every
unparseable limit with it on, valid limits identical either way, first-wins on
duplicates, and that unknown or incomplete intents contribute nothing.

Verification

Built and run in a container matching the repo's own Dockerfile (Go 1.25 +
WasmEdge 0.13.4, same version and checksum it pins):

  • go vet ./modules/contract/... ./modules/state-processing/ ./modules/common/... — clean
  • go test ./modules/contract/... ./modules/common/... — all green
    (execution-context, session, common, consensusversion, params, system-config)
  • gofmt clean; both files LF

Reviewer notes

  • Nothing to coordinate to merge this. The gate is off, so no witness needs to
    upgrade in step. The coordination question arrives only when you activate it.
  • If you would rather activate it in the same batch as an existing line, the only
    change needed is in the state engine where WithTryCatch is already resolved
    per-tx — pass WithStrictIntentLimits(<same style of resolver>) beside it.
  • The strict branch refuses by recording no entry for the token, so
    PullBalance's own missing-limit path does the refusing. That keeps the failure
    in one place rather than inventing a second "zero means refuse" convention.

Not in this PR

common.EncodeDagCbor discards three errors and always returns nil. Today a
malformed input panics; making it return an error would let any caller doing
bytes, _ := proceed with empty bytes and therefore a wrong CID, silently —
worse than the panic. Fixing it properly means auditing its ~24 call sites, which
belongs in its own change.

…mit cannot grant MaxInt64

The error from common.ParseDecimalsToBaseUnits was discarded when building a
transaction's per-token pull ceilings. strconv.ParseInt is asymmetric on failure:
a syntax error returns 0, but an OUT-OF-RANGE value returns math.MaxInt64. So the
two malformed cases degraded in opposite directions —

    limit "abc"                  -> 0        -> draw refused (safe)
    limit "99999999999999999999" -> MaxInt64 -> effectively unbounded (not safe)

— on the one mechanism that exists to bound what a contract may take from a
caller. A client that displays "1.000" while signing an oversized limit obtains
an allowance the user never intended to grant.

Fixed behind a gate, DEFAULT OFF, because it cannot be fixed unconditionally:
this feeds contract execution, so changing what a malformed limit resolves to
changes whether a draw succeeds, which changes state. An upgraded and a
not-yet-upgraded node would disagree and a replay from genesis would stop
reproducing the chain. Same hazard EvmAddressChecksumHeight documents.

With the gate off, behaviour is byte-identical to before, MaxInt64 clamp
included. Activation is a separate decision: resolve it from the chain-active
consensus version exactly as WithTryCatch is resolved.

- extract the intent loop into buildTokenLimits(intents, strict), unchanged in
  the default path
- add strictIntentLimits + WithStrictIntentLimits, mirroring WithTryCatch
- propagate the gate into nested contract calls; a nested call builds its own
  tokenLimits, so without this one transaction would enforce two different
  ceilings depending on call depth
- document that first-in-array wins for a duplicate (type, token) — not the
  largest, not the last

Tests pin both sides of the gate and the wiring: the default must stay
MaxInt64 (an ungated fix fails this), the option must actually reach the parse
(an inert gate fails this), and the nested call must propagate it. All three
verified by mutation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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