fix: Enforce aggregate MaximumAmount in multi-send MPT#6644
fix: Enforce aggregate MaximumAmount in multi-send MPT#6644
Conversation
rippleSendMultiMPT used a read-only SLE snapshot (view.read) to check MaximumAmount per iteration. Since rippleCreditMPT updates a separate mutable copy (view.peek), the snapshot's sfOutstandingAmount was stale after the first iteration, allowing the aggregate to exceed MaximumAmount. Replace the per-iteration check with a running total that validates the aggregate against MaximumAmount within the send loop. The old per-iteration check is retained behind a !fixAssortedFixes gate for ledger replay compatibility.
Tapanito
added
Amendment
AI Triage
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…-send-max-amount # Conflicts: # include/xrpl/protocol/detail/features.macro # src/libxrpl/ledger/View.cpp
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Went through the changes
The core fix is correct, but four issues need attention: a uint64_t wrap-around risk in the pre-amendment arithmetic (line 1215), a semantic divergence from the original subtraction-underflow behavior that may break ledger replay fidelity (line 1215), missing // KNOWN BUG comments at the broken-behavior assertion sites in the test (line 3351), and a reminder to queue fixSecurity3_1_3 for prompt activation and audit other multi-send paths for the same stale-snapshot pattern. See inline comments.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6644 +/- ##
=======================================
Coverage 81.4% 81.5%
=======================================
Files 998 998
Lines 74443 74450 +7
Branches 7563 7558 -5
=======================================
+ Hits 60632 60640 +8
+ Misses 13811 13810 -1
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Use subtraction-based guards instead of addition to prevent uint64_t overflow in both the post-amendment aggregate check and the pre-amendment per-iteration check. Each condition in the cascade protects the subtraction in the next from underflow. Move totalSendAmount accumulation after the check so the guard operates on the pre-addition value.
There was a problem hiding this comment.
Looked through this one
One high-severity security note flagged inline: the pre-fixSecurity3_1_3 path allows issuer to bypass MaximumAmount via stale snapshot — activate the amendment promptly.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
2 participants
rippleSendMultiMPT used a read-only SLE snapshot (view.read) to check MaximumAmount per iteration. Since rippleCreditMPT updates a separate mutable copy (view.peek), the snapshot's sfOutstandingAmount was stale after the first iteration, allowing the aggregate to exceed MaximumAmount.
Replace the per-iteration check with a running total that validates the aggregate against MaximumAmount within the send loop. The old per-iteration check is retained behind a !fixAssortedFixes gate for ledger replay compatibility.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)