fix permission delegation vulnerability by yinyiqian1 · Pull Request #5825 · XRPLF/rippled

yinyiqian1 · 2025-09-29T19:22:20Z

New Amendment: Introduces the featurePermissionDelegationV1_1 amendment.
Amendment Replacement: This new amendment is designed to supersede both featurePermissionDelegation and fixDelegateV1_1, which should be considered deprecated.
Error Code Correction: The checkPermission function will now return terNO_DELEGATE_PERMISSION when a delegate transaction lacks the necessary permissions.

High Level Overview of Change

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

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)

codecov · 2025-09-29T21:07:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.3%. Comparing base (cbbb2b1) to head (0f9aedc).
⚠️ Report is 159 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5825     +/-   ##
=========================================
- Coverage     78.3%   78.3%   -0.0%     
=========================================
  Files          817     817             
  Lines        69016   69003     -13     
  Branches      8320    8322      +2     
=========================================
- Hits         54038   54023     -15     
- Misses       14978   14980      +2

Files with missing lines	Coverage Δ
include/xrpl/protocol/TER.h	`100.0% <ø> (ø)`
include/xrpl/protocol/detail/transactions.macro	`100.0% <ø> (ø)`
src/libxrpl/protocol/Permissions.cpp	`100.0% <100.0%> (ø)`
src/libxrpl/protocol/TER.cpp	`100.0% <ø> (ø)`
src/xrpld/app/misc/detail/DelegateUtils.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/DelegateSet.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/MPTokenIssuanceSet.h	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/Payment.cpp	`93.8% <100.0%> (+0.1%)`	⬆️
src/xrpld/app/tx/detail/Payment.h	`100.0% <ø> (ø)`
... and 7 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mvadari · 2025-09-30T16:12:29Z

@yinyiqian1 there are some merge conflicts here

mvadari · 2025-10-03T17:50:45Z

Blocked on waiting for the 3.0 release, will be merged after

bthomee · 2025-10-31T15:57:42Z

Since the 3.0.0 release branch has already been cut, we can go ahead and merge this one.

bthomee · 2026-03-20T17:21:46Z

/ai-review

xrplf-ai-reviewer

Multiple permission delegation security and correctness issues found in transaction handling.

Review by Claude Opus 4.6 · Prompt: V12

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

include/xrpl/protocol/detail/transactions.macro

 TRANSACTION(ttACCOUNT_DELETE, 21, AccountDelete,
    Delegation::notDelegatable,
-    uint256{},
+    featureDeletableAccounts,


AccountDelete feature guard changed - verify delegation behavior across feature boundary:

In the latest develop branch, featureDeletableAccounts has been removed, so this is no longer a concern.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

src/xrpld/app/tx/detail/SetTrust.cpp


    if (!sle)
-        return tecNO_DELEGATE_PERMISSION;
+        return terNO_DELEGATE_PERMISSION;


Permission-denied transactions now return ter instead of tec - prevents fee-free spam:

We intended to use NotTEC, because this is called before checkSign

Good point — since this is called before checkSign, using NotTEC (a ter code) is correct here. ter results are retried rather than applied to the ledger, which makes sense for a pre-signing permission check. Thanks for the clarification; I'll update my understanding. This is the right approach.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

include/xrpl/protocol/TER.h

    tecLIMIT_EXCEEDED = 195,
    tecPSEUDO_ACCOUNT = 196,
    tecPRECISION_LOSS = 197,
+    // DEPRECATED: This error code tecNO_DELEGATE_PERMISSION is reserved for


Semantic mismatch: ter error suggests retry when permission failures are permanent. Add client documentation:

This is a duplicate comment(the same as the next one), #6675

Thanks for pointing that out! You're right that this is a duplicate of the same concern raised in the linked comment. I'll defer to the discussion in that thread to avoid redundancy.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

include/xrpl/protocol/TER.h

    tecLIMIT_EXCEEDED = 195,
    tecPSEUDO_ACCOUNT = 196,
    tecPRECISION_LOSS = 197,
+    // DEPRECATED: This error code tecNO_DELEGATE_PERMISSION is reserved for


Deprecated error code lacks clear reclamation path. Add tracking mechanism:

The comment is sufficient

A comment alone doesn't provide a structured tracking mechanism for deprecation. In a production blockchain protocol like rippled, deprecated error codes need careful lifecycle management to avoid breakage in client integrations. Consider at minimum adding a // TODO(deprecation): Remove after featurePermissionDelegationV1_1 is in majority style comment with a concrete removal condition, or tracking it in the project's issue tracker. A plain "deprecated" comment doesn't signal when or how the code will be removed.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

src/xrpld/app/tx/detail/Payment.cpp


    if (!sle)
-        return tecNO_DELEGATE_PERMISSION;
+        return terNO_DELEGATE_PERMISSION;


Permission-denied payment transactions return ter, enabling potential fee-free transaction spam:

We intended to return NotTEC, because this is returned before the checkSign.
pre-sign checks must be NotTEC to prevent fee extraction without key ownership.

Good point — returning NotTEC before checkSign is the correct behavior to prevent fee extraction without key ownership. Thanks for the explanation. My concern was misplaced here since the normal spam protection applies: without a valid signature, the transaction won't get through the network in the first place. I'll withdraw my original concern.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

src/xrpld/app/tx/detail/Payment.cpp

+    if ((tx.isFieldPresent(sfSendMax) &&
+         tx[sfSendMax].asset() != amountAsset) ||
+        tx.isFieldPresent(sfPaths))
+        return terNO_DELEGATE_PERMISSION;


MPT payment authorization logic expanded without explicit guard:

false alarm: PaymentMint and PaymentBurn apply to both IOU and MPT direct payments.
Authorization logic is in doApply.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

src/xrpld/app/tx/detail/applySteps.cpp


-                result = T::checkFee(ctx, calculateBaseFee(ctx.view, ctx.tx));
+                        if (NotTEC const result =
+                                T::checkPermission(ctx.view, ctx.tx))


Fee check moved after permission validation - potential DoS vector with fee-free invalid submissions:

We intended to group the NotTEC functions together.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

src/xrpld/app/misc/detail/DelegateUtils.cpp

+NotTEC
 checkTxPermission(std::shared_ptr<SLE const> const& delegate, STTx const& tx)
 {
    if (!delegate)


LCOV_EXCL_LINE suppresses coverage on security-critical null check. Add explicit unit tests:

Thanks for the reference! That PR #6675 appears to add the unit tests for this security-critical path. Once those tests are in place, the LCOV_EXCL_LINE suppression would be justified. Could you confirm that PR #6675 covers the null check at line 28 specifically, and that it will be merged alongside or before this change?

The test case in #6675 is used to cover the if (!delegate) null check path in DelegateUtils.cpp:

void testDelegateUtilsNullptrCheck() { testcase("DelegateUtils nullptr check"); // checkTxPermission nullptr check STTx const tx{ttPAYMENT, [](STObject&) {}}; BEAST_EXPECT(checkTxPermission(nullptr, tx) == terNO_DELEGATE_PERMISSION); // loadGranularPermission nullptr check std::unordered_set<GranularPermissionType> granularPermissions; loadGranularPermission(nullptr, ttPAYMENT, granularPermissions); BEAST_EXPECT(granularPermissions.empty()); }

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp


    if (!sle)
-        return tecNO_DELEGATE_PERMISSION;
+        return terNO_DELEGATE_PERMISSION;


Permission-denied MPTokenIssuanceSet transactions return ter, enabling potential fee-free transaction spam:

We intended to use NotTEC, because this is called before checkSign

Thanks for the context! You're right that NotTEC (a TER result) is appropriate here when called before checkSign, since the transaction hasn't been signed yet and fee charging hasn't been determined. Returning a TER at this stage is the correct pattern to avoid charging fees for transactions that fail early validation. I'll withdraw my concern.

xrplf-ai-reviewer · 2026-03-20T17:47:12Z

src/test/app/Batch_test.cpp

@@ -3946,14 +3946,13 @@ class Batch_test : public beast::unit_test::suite
                tesSUCCESS,


Test case silently drops validation of a failed inner Batch transaction

The test previously validated that the second inner transaction in a Batch (jv2, seq+2) was recorded in the ledger with tecNO_DELEGATE_PERMISSION. After changing to terNO_DELEGATE_PERMISSION, the validation entry is simply removed. The absence of any assertion means the test no longer explicitly verifies what happens to the second inner transaction.

Suggested fix: Add an explicit assertion that the second inner transaction is NOT present in the closed ledger to preserve test coverage for the 'permission denied inside a Batch' path.

#6675 added:

std::vector<TestLedgerData> testCases = { {0, "Batch", "tesSUCCESS", batchID, std::nullopt}, {1, "TrustSet", "tesSUCCESS", txIDs[0], batchID}, // jv2 fails with terNO_DELEGATE_PERMISSION. }; validateClosedLedger(env, testCases); // verify jv2 is not present in the closed ledger. BEAST_EXPECT( env.rpc("tx", txIDs[1])[jss::result][jss::error] == "txnNotFound");

yinyiqian1 · 2026-03-27T00:40:55Z

The AI comments are addressed in a separate PR: #6675
The remaining comments appear to be false alarms.

yinyiqian1 requested a review from a team September 29, 2025 19:22

mvadari approved these changes Sep 29, 2025

View reviewed changes

mvadari requested a review from ximinez September 29, 2025 19:31

yinyiqian1 marked this pull request as draft September 29, 2025 19:40

yinyiqian1 added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Sep 29, 2025

yinyiqian1 marked this pull request as ready for review September 29, 2025 20:20

yinyiqian1 added Bug and removed DraftRunCI Normally CI does not run on draft PRs. This opts in. labels Sep 29, 2025

mvadari added the Amendment label Sep 29, 2025

fix permission delegation vulnerability

3db5026

yinyiqian1 force-pushed the fix_permission_delegation branch from da6ec15 to 3db5026 Compare September 29, 2025 20:40

ximinez approved these changes Sep 29, 2025

View reviewed changes

mvadari 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 Sep 29, 2025

Bronek 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 Sep 30, 2025

Bronek added 2 commits September 30, 2025 19:09

Merge branch 'develop' into fix_permission_delegation

2c188e8

Merge branch 'develop' into fix_permission_delegation

eb46664

bthomee added this to the 3.1.0 milestone Oct 3, 2025

mvadari added the Blocked label Oct 3, 2025

yinyiqian1 added 2 commits October 31, 2025 11:35

Merge branch 'develop' into fix_permission_delegation

6dd1961

change feature to Supported::no

ee86c32

bthomee removed the Blocked label Oct 31, 2025

bthomee 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 Oct 31, 2025

Merge branch 'develop' into fix_permission_delegation

0f9aedc

bthomee merged commit fa69918 into XRPLF:develop Oct 31, 2025
37 checks passed

yinyiqian1 deleted the fix_permission_delegation branch January 30, 2026 19:05

bthomee added the QE test required RippleX QE Team must look at this PR. label Feb 11, 2026

xrplf-ai-reviewer bot reviewed Mar 20, 2026

View reviewed changes

kennyzlei requested a review from shawnxie999 March 23, 2026 21:08

yinyiqian1 mentioned this pull request Mar 27, 2026

fix: Address ai reviewer comments for delegation #6675

Open

4 tasks

		@@ -3946,14 +3946,13 @@ class Batch_test : public beast::unit_test::suite
		tesSUCCESS,

Conversation

yinyiqian1 commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

codecov bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mvadari commented Sep 30, 2025

Uh oh!

mvadari commented Oct 3, 2025

Uh oh!

bthomee commented Oct 31, 2025

Uh oh!

Uh oh!

bthomee commented Mar 20, 2026

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

yinyiqian1 Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 20, 2026

yinyiqian1 commented Sep 29, 2025 •

edited

Loading

codecov bot commented Sep 29, 2025 •

edited

Loading

yinyiqian1 Mar 26, 2026 •

edited

Loading