IntersectMBO / IntersectMBO/evolution-sdk

Signing: signTx hashes a re-encoded body when given a Transaction object

Open
#531 0 comments 0 reactions 0 assignees View on GitHub
bug external-review
Dominant language
TypeScript
Stars
22
Forks
30
Avg merge
5h 29m
Merged PRs (30d)
12

Description

## Summary
`signTx` accepts `Transaction | string` and computes the message to sign two different ways. Given a
hex string it hashes the original body bytes; given a decoded `Transaction` it calls
`TransactionBody.toHash(tx.body)`, which re-encodes the body canonically. Since #236, submission
replays the original bytes through the `formatCache`, so for a body decoded from a valid but
non-canonical encoding (indefinite-length arrays, non-minimal integers) the object path signs a
transaction id the node will not compute, and the witness does not validate. The cause is the lookup:
`formatCache` is keyed on `Transaction` (Transaction.ts L130), so passing `tx.body` cannot find the
cached format, while `tx` itself is in scope on the line above.

## Affected
packages/evolution/src/sdk/client/internal/Signing.ts
- signTx message selection (L328-331): string branch hashes original bytes via `toHashFromBytes`;
object branch re-encodes via `TransactionBody.toHash(tx.body)` (L331), while `tx` is in scope (L323)

packages/evolution/src/Transaction.ts
- formatCache (L130) keyed on `Transaction`; written only by `fromCBORBytes` (L141) and
`fromCBORHex` (L150), transferred through `addVKeyWitnesses` (L363-364)
- contrast: the object call sites in sdk/builders/SignBuilderImpl.ts (L119, L281) are fed only by
sdk/builders/internal/build.ts (L41) with freshly built transactions, which carry no cached format,
so the two paths agree there

## Fix
Make the object overload hash the bytes that will actually be submitted, so both overloads agree:

: TransactionBody.toHashFromBytes(Transaction.extractBodyBytes(Transaction.toCBORBytes(tx)))

One caveat before applying it. Hashing original bytes means signing entries the decoded body dropped,
including duplicate CBOR map keys, which `decodeMapAt` currently accepts with last-writer-wins. That
exposure exists on the string path today and this change extends it to both. Pair it with rejecting
duplicate keys on decode, the same fail-closed approach taken for Data in #397, or the fix trades one
inconsistency for a wider one.

## Regression test
- given: a transaction decoded from a body with a non-minimal fee (`1a0000000a` where canonical is `0a`)
that round-trips exactly through `Transaction.toCBORBytes`
- before fix: the object overload signs `233fb742...` while the node txid over the submitted bytes is
`1d043700...`; the string overload signs `1d043700...`
- after fix: object overload, string overload, and node txid all agree
- control: a fully canonical body must match on all three before and after

Must FAIL on main today and PASS after the fix.

## Reference
Residual inconsistency after #236 made submission byte-preserving while the object signing path kept
canonicalizing. Adjacent to #235, the same class in redeemer PlutusData, and to #480, the same
re-encode-instead-of-preserve pattern in COSE.

Contributor guide

Open the contributing guide

Research direction

Start in packages/evolution/src/sdk/client/internal/Signing.ts at signTx lines 323-331, then inspect the formatCache handling in packages/evolution/src/Transaction.ts lines 130-150 and 363-364. Add the described regression test using a non-minimal fee encoding, plus the canonical control, and verify that object signing, string signing, and the node transaction id agree after the fix.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
blockchain, cryptography
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.