From 6fbeb04d9e231aaa16b04503ae389885b81f6e31 Mon Sep 17 00:00:00 2001 From: yinyiqian1 Date: Wed, 18 Mar 2026 09:39:31 -0400 Subject: [PATCH] fix: Disallow empty permission list when Delegate object is absent (#6542) This change fixes delegation: * If the Delegate object is not present, we should disallow empty permission list in DelegateSet preclaim. * Empty permission list is only allowed to delete the existing Delegate object. * In `doApply`, permission list being empty returns `tecINTERNAL`, which should not happen. --- .../tx/transactors/delegate/DelegateSet.cpp | 37 ++++++++------ src/test/app/Delegate_test.cpp | 49 +++++++++++++------ 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp b/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp index 87e6c076b5..58f68b628a 100644 --- a/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp +++ b/src/libxrpl/tx/transactors/delegate/DelegateSet.cpp @@ -41,6 +41,13 @@ DelegateSet::preclaim(PreclaimContext const& ctx) if (!ctx.view.exists(keylet::account(ctx.tx[sfAuthorize]))) return tecNO_TARGET; + // Deleting the delegate object is invalid if it doesn’t exist. + if (ctx.tx.getFieldArray(sfPermissions).empty() && + !ctx.view.exists(keylet::delegate(ctx.tx[sfAccount], ctx.tx[sfAuthorize]))) + { + return tecNO_ENTRY; + } + return tesSUCCESS; } @@ -67,30 +74,30 @@ DelegateSet::doApply() return tesSUCCESS; } + auto const& permissions = ctx_.tx.getFieldArray(sfPermissions); + if (permissions.empty()) + return tecINTERNAL; // LCOV_EXCL_LINE + STAmount const reserve{ ctx_.view().fees().accountReserve(sleOwner->getFieldU32(sfOwnerCount) + 1)}; if (preFeeBalance_ < reserve) return tecINSUFFICIENT_RESERVE; - auto const& permissions = ctx_.tx.getFieldArray(sfPermissions); - if (!permissions.empty()) - { - sle = std::make_shared(delegateKey); - sle->setAccountID(sfAccount, account_); - sle->setAccountID(sfAuthorize, authAccount); + sle = std::make_shared(delegateKey); + sle->setAccountID(sfAccount, account_); + sle->setAccountID(sfAuthorize, authAccount); - sle->setFieldArray(sfPermissions, permissions); - auto const page = ctx_.view().dirInsert( - keylet::ownerDir(account_), delegateKey, describeOwnerDir(account_)); + sle->setFieldArray(sfPermissions, permissions); + auto const page = + ctx_.view().dirInsert(keylet::ownerDir(account_), delegateKey, describeOwnerDir(account_)); - if (!page) - return tecDIR_FULL; // LCOV_EXCL_LINE + if (!page) + return tecDIR_FULL; // LCOV_EXCL_LINE - (*sle)[sfOwnerNode] = *page; - ctx_.view().insert(sle); - adjustOwnerCount(ctx_.view(), sleOwner, 1, ctx_.journal); - } + (*sle)[sfOwnerNode] = *page; + ctx_.view().insert(sle); + adjustOwnerCount(ctx_.view(), sleOwner, 1, ctx_.journal); return tesSUCCESS; } diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index cbfa29c215..2618a95d0d 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -45,11 +45,9 @@ class Delegate_test : public beast::unit_test::suite env.close(); // delegating an empty permission list when the delegate ledger object - // does not exist will not create the ledger object - env(delegate::set(gw, alice, std::vector{})); + // does not exist is not allowed + env(delegate::set(gw, alice, {}), ter(tecNO_ENTRY)); env.close(); - auto const entry = delegate::entry(env, gw, alice); - BEAST_EXPECT(entry[jss::result][jss::error] == "entryNotFound"); auto const permissions = std::vector{ "Payment", "EscrowCreate", "EscrowFinish", "TrustlineAuthorize", "CheckCreate"}; @@ -92,9 +90,7 @@ class Delegate_test : public beast::unit_test::suite // newPermissions comparePermissions(delegate::entry(env, gw, alice), newPermissions, gw, alice); - // gw deletes all permissions delegated to alice, this will delete - // the - // ledger entry + // gw deletes all permissions delegated to alice, this will delete the ledger entry env(delegate::set(gw, alice, {})); env.close(); auto const jle = delegate::entry(env, gw, alice); @@ -204,27 +200,48 @@ class Delegate_test : public beast::unit_test::suite testcase("test reserve"); using namespace jtx; - // test reserve for DelegateSet + // reserve requirement not met { Env env(*this); Account alice{"alice"}; Account bob{"bob"}; - Account carol{"carol"}; - env.fund(drops(env.current()->fees().accountReserve(0)), alice); - env.fund(drops(env.current()->fees().accountReserve(1)), bob, carol); + auto const txFee = env.current()->fees().base; + env.fund(env.current()->fees().accountReserve(0) + txFee, alice); + env.fund(XRP(100000), bob); env.close(); // alice does not have enough reserve to create Delegate env(delegate::set(alice, bob, {"Payment"}), ter(tecINSUFFICIENT_RESERVE)); + } - // bob has enough reserve - env(delegate::set(bob, alice, {"Payment"})); + // reserve recovered after deleting delegation object + { + Env env(*this); + Account bob{"bob"}; + Account alice{"alice"}; + Account carol{"carol"}; + + auto const txFee = env.current()->fees().base; + + env.fund(env.current()->fees().accountReserve(1) + (txFee * 4), alice); + env.fund(XRP(100000), bob, carol); env.close(); - // now bob create another Delegate, he does not have - // enough reserve - env(delegate::set(bob, carol, {"Payment"}), ter(tecINSUFFICIENT_RESERVE)); + // alice consumes 1 txFee and requires 1 object reserve + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + // alice does not have enough reserve to create another delegation object + env(delegate::set(alice, carol, {"Payment"}), ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // deleting delegation object recovers 1 reserve + env(delegate::set(alice, bob, {})); + env.close(); + + // now alice can delegate again + env(delegate::set(alice, carol, {"Payment"})); } // test reserve when sending transaction on behalf of other account