Skip to content

fix(binarycodec): verify IOU not-XRP bit before sign parsing#311

Open
e-desouza wants to merge 1 commit into
mainfrom
fix/iou-sign-not-xrp-bit-260
Open

fix(binarycodec): verify IOU not-XRP bit before sign parsing#311
e-desouza wants to merge 1 commit into
mainfrom
fix/iou-sign-not-xrp-bit-260

Conversation

@e-desouza

Copy link
Copy Markdown
Collaborator

Why

The IOU amount deserialization function (_deserialize_issued_currency_amount) reads the sign from bit 6 of byte 0 (the 0x40 mask) but did not first verify that byte 0 has the 0x80 "not XRP" bit set, which is required by the XRPL IOU wire format. A malformed 8-byte buffer where 0x80 is absent but the buffer still reaches the IOU branch — because it is neither native (0x80 clear, 0x20 clear is native) nor MPT (0x20 set) — could trigger incorrect sign parsing. The routing invariant that currently prevents this from occurring in the Amount::serialize path is not enforced at the deserialization layer itself, leaving a gap if the routing logic is ever extended or the function is called directly.

Closes #260.

What changed

  • src/core/binarycodec/types/amount.rs: added an early guard at the top of _deserialize_issued_currency_amount that returns InvalidReadFromBytesValue when bytes[0] & 0x80 == 0. The guard uses the existing _NOT_XRP_BIT_MASK constant and the same error variant already used throughout the binary codec.
  • Four new unit tests in the same file:
    • test_iou_positive_sign_round_trips — a valid positive IOU (byte 0 = 0xC0) serializes without error.
    • test_iou_negative_sign_round_trips — a valid negative IOU (byte 0 = 0x80, sign bit clear) serializes with a negative value string.
    • test_iou_missing_not_xrp_bit_returns_error — bytes with 0x80 absent return an error from _deserialize_issued_currency_amount.
    • test_iou_all_zero_bytes_returns_error — all-zero bytes (also lacking 0x80) are rejected.

How to validate

cargo test --release
cargo test --release --no-default-features --features embassy-rt,core,utils,wallet,models,helpers,websocket,json-rpc
cargo clippy --features std,tokio-rt,embassy-rt,actix-rt,futures-rt,smol-rt,core,wallet,models,utils,helpers,json-rpc,websocket,cli -- -D warnings

All tests pass and clippy reports zero warnings.

Add a guard in `_deserialize_issued_currency_amount` that rejects any
8-byte IOU value buffer where byte 0 has the 0x80 "not XRP" bit clear.
Without the check a malformed buffer reaching the IOU branch could
produce a sign-inverted result because the sign bit (0x40) would be
read against an unexpected bit pattern.

Adds four unit tests: positive IOU round-trip, negative IOU round-trip
with sign preservation, and two rejection tests confirming that buffers
with 0x80 missing return an error. Closes #260.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@c80d932). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/core/binarycodec/types/amount.rs 95.83% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #311   +/-   ##
=======================================
  Coverage        ?   84.27%           
=======================================
  Files           ?      200           
  Lines           ?    20802           
  Branches        ?        0           
=======================================
  Hits            ?    17531           
  Misses          ?     3271           
  Partials        ?        0           
Flag Coverage Δ
unit 84.27% <95.83%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/core/binarycodec/types/amount.rs 87.62% <95.83%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

IOU deserialization can invert sign under parser confusion

2 participants