Skip to content

fix(transaction): reject malformed field 11 instead of dropping it#68

Closed
toddfil wants to merge 2 commits into
tempoxyz:mainfrom
toddfil:fix/reject-malformed-field11
Closed

fix(transaction): reject malformed field 11 instead of dropping it#68
toddfil wants to merge 2 commits into
tempoxyz:mainfrom
toddfil:fix/reject-malformed-field11

Conversation

@toddfil

@toddfil toddfil commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Don't silently swallow a malformed field 11

So there's this branch in Deserialize that always bugged me:

} else if len(feePayerSigRaw) > 0 {
    // Non-empty bytes that aren't 0x00 - unusual case
}

That's it. The "unusual case" is just... an empty block. If field 11
(feePayerSignatureOrSender) decodes to a non-empty byte string that isn't the
single 0x00 marker, we shrug and move on. The data is gone, no error, nothing.

Why does that matter? Field 11 is security-relevant — it's where the fee-payer
signature or the await-fee-payer marker lives. For a 0x76 transaction the only
legal shapes are:

  • empty (0x80) — no fee payer,
  • a single 0x00 byte — awaiting fee payer,
  • a [yParity, r, s] tuple — actual fee-payer signature.

A bare byte string (say, a 20-byte address — which only belongs in the 0x78
signing form, not on the wire) is malformed. Accepting it means we happily parse
a transaction that nobody should consider valid, and we throw away whatever was
there without telling the caller. A parser for a security-critical format should
reject what it doesn't understand, not guess.

So now we return an error:

return nil, fmt.Errorf("%w: invalid feePayerSignatureOrSender (field 11): unexpected %d-byte value",
    ErrInvalidTransaction, len(feePayerSigRaw))

Added TestDeserializeRejectsMalformedField11, which hand-builds a 13-field tx with
a 20-byte value in slot 11 and checks we now error out. Rest of the suite is still
green.

@brendanjryan

Copy link
Copy Markdown
Collaborator

Thanks for the PR! Rolled fwd with your commits in the linked PR

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.

2 participants