Sponsored Content
Skip to content

fix: Use trustline balance direction to validate IOU PaymentMint/PaymentBurn - #7584

Merged
bthomee merged 9 commits into
XRPLF:developfrom
yinyiqian1:fix-granular-payment-new
Jun 26, 2026
Merged

fix: Use trustline balance direction to validate IOU PaymentMint/PaymentBurn#7584
bthomee merged 9 commits into
XRPLF:developfrom
yinyiqian1:fix-granular-payment-new

Conversation

@yinyiqian1

@yinyiqian1 yinyiqian1 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Change 1:

PaymentMint/PaymentBurn granular permission check for IOU relies on:

  • Trust limit (destLimit > 0 / accountLimit > 0): identifies which side is willing to hold another side's IOU
  • Balance direction (accountIsHolder): confirms which direction DirectStepI will actually execute (issue vs redeem)

MPT does not have this issue because MPTokenIssuanceID encodes the issuer unambiguously, there is no endpoint aliasing where either side of the trustline can appear as the issuer.

Change 2:
in Permission::permissionToTxType, force the permission value within range [1, 65536]

Context of Change

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@yinyiqian1
yinyiqian1 force-pushed the fix-granular-payment-new branch 2 times, most recently from 4afe188 to ecc4b22 Compare June 22, 2026 05:22
@yinyiqian1 yinyiqian1 changed the title fix PaymentMint and PaymentBurn endpoint exploit fix: Use trustline balance direction to validate IOU PaymentMint/PaymentBurn Jun 22, 2026
@yinyiqian1
yinyiqian1 force-pushed the fix-granular-payment-new branch 2 times, most recently from c8ee8d9 to c0aebbd Compare June 22, 2026 05:32
@yinyiqian1
yinyiqian1 marked this pull request as ready for review June 22, 2026 05:34
@yinyiqian1
yinyiqian1 marked this pull request as draft June 22, 2026 05:50
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.0%. Comparing base (50fdb38) to head (ab21948).

Files with missing lines Patch % Lines
src/libxrpl/tx/transactors/payment/Payment.cpp 96.7% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #7584     +/-   ##
=========================================
- Coverage     82.0%   82.0%   -0.0%     
=========================================
  Files         1007    1007             
  Lines        76888   76914     +26     
  Branches      8971    8971             
=========================================
+ Hits         63042   63061     +19     
- Misses       13837   13844      +7     
  Partials         9       9             
Files with missing lines Coverage Δ
src/libxrpl/protocol/Permissions.cpp 100.0% <100.0%> (ø)
src/libxrpl/tx/transactors/payment/Payment.cpp 92.2% <96.7%> (+0.3%) ⬆️

... and 5 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yinyiqian1
yinyiqian1 marked this pull request as ready for review June 22, 2026 15:53
Comment thread src/libxrpl/tx/transactors/payment/Payment.cpp Outdated
Comment thread src/libxrpl/tx/transactors/payment/Payment.cpp Outdated
Comment thread src/libxrpl/tx/transactors/payment/Payment.cpp Outdated
Comment thread src/libxrpl/tx/transactors/payment/Payment.cpp Outdated
Comment thread src/libxrpl/tx/transactors/payment/Payment.cpp Outdated
@yinyiqian1
yinyiqian1 force-pushed the fix-granular-payment-new branch from 9664d02 to 852abb7 Compare June 22, 2026 22:22
Comment thread src/libxrpl/tx/transactors/payment/Payment.cpp Outdated

@gregtatcam gregtatcam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yinyiqian1 yinyiqian1 added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jun 23, 2026
@yinyiqian1 yinyiqian1 added this to the 3.3.0 milestone Jun 23, 2026
@yinyiqian1 yinyiqian1 removed the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jun 23, 2026
shawnxie999
shawnxie999 previously approved these changes Jun 24, 2026
@yinyiqian1 yinyiqian1 added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jun 24, 2026
gregtatcam
gregtatcam previously approved these changes Jun 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes delegate granular permission validation for direct IOU payments so PaymentMint/PaymentBurn are determined by the trustline balance direction (execution direction) rather than trusting sfAmount.issuer, which can be encoded as either endpoint. It also hardens Permission::permissionToTxType against out-of-range values that previously could truncate into a different tx type.

Changes:

  • Update Payment::checkGranularSemantics to validate IOU PaymentMint/PaymentBurn using trustline limit + balance direction, while keeping MPT behavior based on the unambiguous MPToken issuer.
  • Change Permission::permissionToTxType to return std::optional<TxType> and reject values outside [1, 65536] to prevent truncation/mis-mapping.
  • Extend Delegate_test coverage for issuer-alias scenarios, trust-limit-vs-balance-direction cases, and permissionToTxType range handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/test/app/Delegate_test.cpp Adds targeted tests for IOU issuer aliasing and permission range behavior; adjusts MPT test naming/usage.
src/libxrpl/tx/transactors/payment/Payment.cpp Implements trustline-balance-direction based authorization for IOU PaymentMint/PaymentBurn.
src/libxrpl/protocol/Permissions.cpp Makes permissionToTxType range-safe and updates callers to handle std::optional.
include/xrpl/protocol/Permissions.h Updates the public signature of permissionToTxType to return std::optional<TxType>.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread include/xrpl/protocol/Permissions.h
@bthomee
bthomee added this pull request to the merge queue Jun 26, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 26, 2026
@bthomee

bthomee commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

@yinyiqian1 it seems one of the tests fails to build when I tried to merge the PR - can you please take a look?

@yinyiqian1

yinyiqian1 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@yinyiqian1 it seems one of the tests fails to build when I tried to merge the PR - can you please take a look?

I just rebased and it is currently re-running the CI

@yinyiqian1
yinyiqian1 force-pushed the fix-granular-payment-new branch from ef8785a to a999fd7 Compare June 26, 2026 16:05
@yinyiqian1
yinyiqian1 dismissed stale reviews from gregtatcam and shawnxie999 via ab21948 June 26, 2026 17:38
@kennyzlei
kennyzlei requested a review from PeterChen13579 June 26, 2026 22:22
@bthomee
bthomee added this pull request to the merge queue Jun 26, 2026
Merged via the queue into XRPLF:develop with commit fd8a915 Jun 26, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants