From 730e526080f9cac0430c52ec7d752e1975a5a9a6 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 1 Jul 2026 11:37:45 -0400 Subject: [PATCH 1/2] fix: Better handling of txs that send XRP + charge reserve (#7671) --- .../tx/transactors/escrow/EscrowCreate.cpp | 26 +++++- .../payment_channel/PaymentChannelCreate.cpp | 32 +++++-- src/test/app/Sponsor_test.cpp | 86 +++++++++++++++++++ 3 files changed, 133 insertions(+), 11 deletions(-) diff --git a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp index bdb4bcddae..89142baf8a 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp @@ -33,6 +33,7 @@ #include #include +#include #include #include #include @@ -439,16 +440,37 @@ EscrowCreate::doApply() auto const sponsorSle = getTxReserveSponsor(view(), ctx_.tx); if (!sponsorSle) return sponsorSle.error(); // LCOV_EXCL_LINE + // First check: whoever is on the hook for the new owner increment + // can cover it. When sponsored this hits the sponsor branch and + // validates the sponsor's reserve + remaining credit. When + // unsponsored this hits the source branch and validates the + // source's pre-lock balance against base + (currentOC+1)*increment. if (auto const ret = checkInsufficientReserve(ctx_.view(), ctx_.tx, sle, balance, *sponsorSle, 1, 0, j_); !isTesSuccess(ret)) return ret; - // Check reserve and funds availability if (isXRP(amount)) { + // Second check (XRP escrow only): after locking the escrowed + // amount, the source must still meet its own reserve floor. + // Always passes `{}` so the source branch runs (the sponsor's + // reserve was already validated above; here we're verifying the + // source can fund the lock without dipping below its own + // reserve). ownerCountAdj differs by case: + // - sponsored: adj=0 — sponsor covers the new owner increment, + // so the source only owes its base reserve. + // - unsponsored: adj=1 — source owes base + the new increment. + std::int32_t const ownerCountAdj = *sponsorSle ? 0 : 1; if (auto const ret = checkInsufficientReserve( - ctx_.view(), ctx_.tx, sle, balance - STAmount(amount).xrp(), {}, 1, 0, j_); + ctx_.view(), + ctx_.tx, + sle, + balance - STAmount(amount).xrp(), + {}, + ownerCountAdj, + 0, + j_); !isTesSuccess(ret)) return tecUNFUNDED; } diff --git a/src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp b/src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp index a6c1d176bb..f65bc33056 100644 --- a/src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp +++ b/src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp @@ -22,6 +22,7 @@ #include #include +#include #include namespace xrpl { @@ -134,22 +135,38 @@ PaymentChannelCreate::doApply() return tecEXPIRED; } + auto const sponsorSle = getTxReserveSponsor(view(), ctx_.tx); + if (!sponsorSle) + return sponsorSle.error(); // LCOV_EXCL_LINE + if (ctx_.view().rules().enabled(featureSponsor)) { - auto const sponsorSle = getTxReserveSponsor(ctx_.view(), ctx_.tx); - if (!sponsorSle) - return sponsorSle.error(); + // First check: whoever is on the hook for the new owner increment + // can cover it. When sponsored this hits the sponsor branch and + // validates the sponsor's reserve + remaining credit. When + // unsponsored this hits the source branch and validates the + // source's pre-lock balance against base + (currentOC+1)*increment. if (auto const ret = checkInsufficientReserve( - ctx_.view(), ctx_.tx, sle, STAmount{preFeeBalance_}, *sponsorSle, 1, 0, j_); + ctx_.view(), ctx_.tx, sle, preFeeBalance_, *sponsorSle, 1, 0, j_); !isTesSuccess(ret)) return ret; + + // Second check: after locking sfAmount in the channel, the source + // must still meet its own reserve floor. Always passes `{}` so the + // source branch runs (the sponsor's reserve was already validated + // above; here we're verifying the source can fund the lock without + // dipping below its own reserve). ownerCountAdj differs by case: + // - sponsored: adj=0 — sponsor covers the new owner increment, + // so the source only owes its base reserve. + // - unsponsored: adj=1 — source owes base + the new increment. + std::int32_t const ownerCountAdj = *sponsorSle ? 0 : 1; if (auto const ret = checkInsufficientReserve( ctx_.view(), ctx_.tx, sle, - STAmount{preFeeBalance_ - ctx_.tx[sfAmount].xrp()}, + preFeeBalance_ - ctx_.tx[sfAmount].xrp(), {}, - 1, + ownerCountAdj, 0, j_); !isTesSuccess(ret)) @@ -203,9 +220,6 @@ PaymentChannelCreate::doApply() // Deduct owner's balance, increment owner count (*sle)[sfBalance] = (*sle)[sfBalance] - ctx_.tx[sfAmount]; - auto const sponsorSle = getTxReserveSponsor(view(), ctx_.tx); - if (!sponsorSle) - return sponsorSle.error(); // LCOV_EXCL_LINE adjustOwnerCount(ctx_.view(), sle, *sponsorSle, 1, ctx_.journal); addSponsorToLedgerEntry(slep, *sponsorSle); ctx_.view().update(sle); diff --git a/src/test/app/Sponsor_test.cpp b/src/test/app/Sponsor_test.cpp index d0f8a60654..74847ea40b 100644 --- a/src/test/app/Sponsor_test.cpp +++ b/src/test/app/Sponsor_test.cpp @@ -2985,6 +2985,50 @@ public: BEAST_EXPECT(sponsoredOwnerCount(env, bob) == 1); BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 1); } + + // A sponsored EscrowCreate must still verify that the source + // can fund the escrow amount and stay above its own base + // reserve. The sponsor covers the new object's owner + // increment, but cannot cover the source's base reserve. + { + Env env{*this, testableAmendments()}; + env.fund(XRP(10000), alice, bob, sponsor); + env.close(); + + // alice's balance is just above the base reserve. After + // locking escrowAmount she would dip below it. + adjustAccountXRPBalance(env, alice, accountReserve(env, 1) + XRP(1)); + + auto const escrowAmount = XRP(2); + auto const seq = env.seq(alice); + + if (cosigning) + { + env(escrow::create(alice, bob, escrowAmount), + escrow::kCondition(escrow::kCb1), + escrow::kCancelTime(env.now() + 100s), + sponsor::As(sponsor, spfSponsorReserve), + Sig(sfSponsorSignature, sponsor), + Ter(tecUNFUNDED)); + } + else + { + env(sponsor::set(sponsor, 0, 1, XRP(1)), sponsor::SponseeAcc(alice)); + env.close(); + + env(escrow::create(alice, bob, escrowAmount), + escrow::kCondition(escrow::kCb1), + escrow::kCancelTime(env.now() + 100s), + sponsor::As(sponsor, spfSponsorReserve), + Ter(tecUNFUNDED)); + } + env.close(); + + BEAST_EXPECT(!env.le(keylet::escrow(alice, seq))); + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(sponsoredOwnerCount(env, alice) == 0); + BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 0); + } } void @@ -3251,6 +3295,48 @@ public: BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 0); BEAST_EXPECT(sponsoringOwnerCount(env, sponsor2) == 0); } + + // A sponsored PaymentChannelCreate must still verify that the + // source can fund the channel amount and stay above its own + // base reserve. The sponsor covers the new object's owner + // increment, but cannot cover the source's base reserve. + { + Env env{*this, testableAmendments()}; + env.fund(XRP(10000), alice, bob, sponsor); + env.close(); + + // alice's balance is just above the base reserve. After + // locking channelAmount she would dip below it. + adjustAccountXRPBalance(env, alice, accountReserve(env, 1) + XRP(1)); + + auto const pk = alice.pk(); + auto const settleDelay = 10s; + auto const channelAmount = XRP(2); + auto const chan = paychan::channel(alice, bob, env.seq(alice)); + + if (cosigning) + { + env(paychan::create(alice, bob, channelAmount, settleDelay, pk), + sponsor::As(sponsor, spfSponsorReserve), + Sig(sfSponsorSignature, sponsor), + Ter(tecUNFUNDED)); + } + else + { + env(sponsor::set(sponsor, 0, 1, XRP(1)), sponsor::SponseeAcc(alice)); + env.close(); + + env(paychan::create(alice, bob, channelAmount, settleDelay, pk), + sponsor::As(sponsor, spfSponsorReserve), + Ter(tecUNFUNDED)); + } + env.close(); + + BEAST_EXPECT(!paychan::channelExists(*env.current(), chan)); + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(sponsoredOwnerCount(env, alice) == 0); + BEAST_EXPECT(sponsoringOwnerCount(env, sponsor) == 0); + } } void From 7817245314a1a6bf8854e1bfcd3add16050b08d6 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 1 Jul 2026 13:18:20 -0400 Subject: [PATCH 2/2] refactor: Switch `checkInsufficientReserve` from `ReadView` to `ApplyView` (#7667) --- .../xrpl/ledger/helpers/AccountRootHelpers.h | 2 +- include/xrpl/ledger/helpers/SponsorHelpers.h | 22 +++ .../ledger/helpers/AccountRootHelpers.cpp | 2 +- .../Sponsor/SponsorshipTransfer.cpp | 163 +++++++++++------- 4 files changed, 123 insertions(+), 66 deletions(-) diff --git a/include/xrpl/ledger/helpers/AccountRootHelpers.h b/include/xrpl/ledger/helpers/AccountRootHelpers.h index 403626d06f..6c8233fa61 100644 --- a/include/xrpl/ledger/helpers/AccountRootHelpers.h +++ b/include/xrpl/ledger/helpers/AccountRootHelpers.h @@ -109,7 +109,7 @@ baseAccountReserve(ReadView const& view, std::int32_t ownerCount, std::int32_t a */ [[nodiscard]] TER checkInsufficientReserve( - ReadView const& view, + ApplyView const& view, STTx const& tx, SLE::const_ref accSle, STAmount const& accBalance, diff --git a/include/xrpl/ledger/helpers/SponsorHelpers.h b/include/xrpl/ledger/helpers/SponsorHelpers.h index 93614e5e17..4b35e155b4 100644 --- a/include/xrpl/ledger/helpers/SponsorHelpers.h +++ b/include/xrpl/ledger/helpers/SponsorHelpers.h @@ -173,6 +173,28 @@ getLedgerEntryOwner(ReadView const& view, T const& sle, AccountID const& account }; } +template +inline bool +isLedgerEntrySupportedBySponsorship(T const& sle) +{ + switch (sle->getType()) + { + case ltCHECK: + case ltESCROW: + case ltPAYCHAN: + case ltMPTOKEN: + case ltDELEGATE: + case ltDEPOSIT_PREAUTH: + case ltMPTOKEN_ISSUANCE: + case ltSIGNER_LIST: + case ltCREDENTIAL: + case ltRIPPLE_STATE: + return true; + default: + return false; + }; +} + template inline std::uint32_t getLedgerEntryOwnerCount(T const& sle) diff --git a/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp b/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp index 3264fa1a45..26a3a0fabf 100644 --- a/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AccountRootHelpers.cpp @@ -302,7 +302,7 @@ baseAccountReserve(ReadView const& view, std::int32_t ownerCount, std::int32_t a TER checkInsufficientReserve( - ReadView const& view, + ApplyView const& view, STTx const& tx, SLE::const_ref accSle, STAmount const& accBalance, diff --git a/src/libxrpl/tx/transactors/Sponsor/SponsorshipTransfer.cpp b/src/libxrpl/tx/transactors/Sponsor/SponsorshipTransfer.cpp index deea6edc84..3840c390b1 100644 --- a/src/libxrpl/tx/transactors/Sponsor/SponsorshipTransfer.cpp +++ b/src/libxrpl/tx/transactors/Sponsor/SponsorshipTransfer.cpp @@ -10,7 +10,6 @@ #include #include #include -#include #include #include #include @@ -147,30 +146,8 @@ SponsorshipTransfer::preclaim(PreclaimContext const& ctx) if (!sle) return tecNO_ENTRY; - // v1 scope: an object is only sponsorable via SponsorshipTransfer if - // its creating transaction type is itself permitted to set - // spfSponsorReserve (the allow-list in preflight1Sponsor). Otherwise - // an Oracle / Ticket / DID / etc. could be retroactively sponsored - // even though its creating tx cannot be, leaving downstream - // transactors with no path to maintain the sponsorship invariants. - switch (sle->getType()) - { - case ltDELEGATE: - case ltDEPOSIT_PREAUTH: - case ltMPTOKEN: - case ltMPTOKEN_ISSUANCE: - case ltCREDENTIAL: - case ltRIPPLE_STATE: - case ltSIGNER_LIST: - case ltCHECK: - case ltESCROW: - case ltPAYCHAN: - break; - default: - return tecNO_PERMISSION; - } - - std::uint32_t const ownerCountDelta = 1; + if (!isLedgerEntrySupportedBySponsorship(sle)) + return tecNO_PERMISSION; auto const owner = getLedgerEntryOwner(ctx.view, sle, sponseeID); if (!owner.has_value() || owner.value() != sponseeID) @@ -210,20 +187,6 @@ SponsorshipTransfer::preclaim(PreclaimContext const& ctx) if (account != sponsor && account != sponseeID) return tecNO_PERMISSION; } - - // check new sponsor have sufficient balance - // NOLINTNEXTLINE(readability-suspicious-call-argument) - if (auto const ter = checkInsufficientReserve( - ctx.view, - ctx.tx, - sponseeSle, - sponseeSle->getFieldAmount(sfBalance), - newSponsorSle, - ownerCountDelta, - 0, - ctx.j); - !isTesSuccess(ter)) - return ter; } else { @@ -259,23 +222,6 @@ SponsorshipTransfer::preclaim(PreclaimContext const& ctx) if (account != sponsor && account != sponseeID) return tecNO_PERMISSION; } - - // check account have sufficient balance - // In the case of removing an account sponsor, accSle should have no sfSponsor set - // (AccountReserve = 0). However, by setting accountCountDelta = 1 here, we are able to - // calculate the actual required Account Reserve. - // NOLINTNEXTLINE(readability-suspicious-call-argument) - if (auto const ter = checkInsufficientReserve( - ctx.view, - ctx.tx, - sponseeSle, - sponseeSle->getFieldAmount(sfBalance), - newSponsorSle, - 0, - 1, - ctx.j); - !isTesSuccess(ter)) - return ter; } return tesSUCCESS; @@ -338,6 +284,12 @@ SponsorshipTransfer::doApply() return tesSUCCESS; }; + auto const balanceBeforeFee = [&](SLE::const_ref sle) -> STAmount { + if (sle->getAccountID(sfAccount) == accountID_) + return STAmount{preFeeBalance_}; + return sle->getFieldAmount(sfBalance); + }; + if (isObjectSponsor) { auto const hasSignature = tx.isFieldPresent(sfSponsorSignature); @@ -363,6 +315,23 @@ SponsorshipTransfer::doApply() { auto const newSponsorID = tx.getAccountID(sfSponsor); XRPL_ASSERT(!!newSponsorID, "New sponsor is required when creating sponsorship"); + auto const newSponsorSle = view().peek(keylet::account(newSponsorID)); + if (!newSponsorSle) + return tefINTERNAL; // LCOV_EXCL_LINE + + // check new sponsor have sufficient balance + // NOLINTNEXTLINE(readability-suspicious-call-argument) + if (auto const ter = checkInsufficientReserve( + ctx_.view(), + ctx_.tx, + sponseeSle, + sponseeSle->getFieldAmount(sfBalance), + newSponsorSle, + ownerCountDelta, + 0, + ctx_.journal); + !isTesSuccess(ter)) + return ter; // update owner's sponsored count if (auto const ter = @@ -372,9 +341,6 @@ SponsorshipTransfer::doApply() view().update(ownerSle); // increment new sponsor's sponsoring count - auto const newSponsorSle = view().peek(keylet::account(newSponsorID)); - if (!newSponsorSle) - return tefINTERNAL; // LCOV_EXCL_LINE if (auto const ter = setSponsorFieldU32(newSponsorSle, sfSponsoringOwnerCount, ownerCountDelta); !isTesSuccess(ter)) @@ -398,14 +364,31 @@ SponsorshipTransfer::doApply() { auto const newSponsorID = tx.getAccountID(sfSponsor); XRPL_ASSERT(!!newSponsorID, "New sponsor is required when reassigning sponsorship"); + auto const newSponsorSle = view().peek(keylet::account(newSponsorID)); + if (!newSponsorSle) + return tefINTERNAL; // LCOV_EXCL_LINE auto const oldSponsorID = objSle->getAccountID(sponsorField); XRPL_ASSERT(!!oldSponsorID, "Old sponsor is required when reassigning sponsorship"); - - // decrement old sponsor's sponsoring count auto const oldSponsorSle = view().peek(keylet::account(oldSponsorID)); if (!oldSponsorSle) return tefINTERNAL; // LCOV_EXCL_LINE + + // check new sponsor have sufficient balance + // NOLINTNEXTLINE(readability-suspicious-call-argument) + if (auto const ter = checkInsufficientReserve( + ctx_.view(), + ctx_.tx, + sponseeSle, + sponseeSle->getFieldAmount(sfBalance), + newSponsorSle, + ownerCountDelta, + 0, + ctx_.journal); + !isTesSuccess(ter)) + return ter; + + // decrement old sponsor's sponsoring count if (auto const ter = setSponsorFieldU32(oldSponsorSle, sfSponsoringOwnerCount, -ownerCountDelta); !isTesSuccess(ter)) @@ -413,9 +396,6 @@ SponsorshipTransfer::doApply() view().update(oldSponsorSle); // increment new sponsor's sponsoring count - auto const newSponsorSle = view().peek(keylet::account(newSponsorID)); - if (!newSponsorSle) - return tefINTERNAL; // LCOV_EXCL_LINE if (auto const ter = setSponsorFieldU32(newSponsorSle, sfSponsoringOwnerCount, ownerCountDelta); !isTesSuccess(ter)) @@ -444,6 +424,20 @@ SponsorshipTransfer::doApply() if (!oldSponsorSle) return tefINTERNAL; // LCOV_EXCL_LINE + // The owner takes the reserve burden back when the object is + // no longer sponsored. + if (auto const ter = checkInsufficientReserve( + ctx_.view(), + ctx_.tx, + ownerSle, + balanceBeforeFee(ownerSle), + SLE::pointer(), + ownerCountDelta, + 0, + ctx_.journal); + !isTesSuccess(ter)) + return ter; + // decrement sponsored count if (auto const ter = setSponsorFieldU32(sponseeSle, sfSponsoredOwnerCount, -ownerCountDelta); @@ -473,6 +467,19 @@ SponsorshipTransfer::doApply() auto const newSponsorSle = view().peek(keylet::account(newSponsorID)); if (!newSponsorSle) return tefINTERNAL; // LCOV_EXCL_LINE + + if (auto const ter = checkInsufficientReserve( + ctx_.view(), + ctx_.tx, + sponseeSle, + sponseeSle->getFieldAmount(sfBalance), + newSponsorSle, + 0, + 1, + ctx_.journal); + !isTesSuccess(ter)) + return ter; + if (auto const ter = setSponsorFieldU32(newSponsorSle, sfSponsoringAccountCount, 1); !isTesSuccess(ter)) return ter; @@ -490,6 +497,19 @@ SponsorshipTransfer::doApply() auto const newSponsorSle = view().peek(keylet::account(newSponsorID)); if (!newSponsorSle) return tefINTERNAL; // LCOV_EXCL_LINE + + if (auto const ter = checkInsufficientReserve( + ctx_.view(), + ctx_.tx, + sponseeSle, + sponseeSle->getFieldAmount(sfBalance), + newSponsorSle, + 0, + 1, + ctx_.journal); + !isTesSuccess(ter)) + return ter; + if (auto const ter = setSponsorFieldU32(newSponsorSle, sfSponsoringAccountCount, 1); !isTesSuccess(ter)) return ter; @@ -513,6 +533,21 @@ SponsorshipTransfer::doApply() { // dissolve account sponsor auto const oldSponsorID = sponseeSle->getAccountID(sfSponsor); + + // The sponsee must be able to hold its own account reserve after + // the sponsorship is removed. + if (auto const ter = checkInsufficientReserve( + ctx_.view(), + ctx_.tx, + sponseeSle, + balanceBeforeFee(sponseeSle), + SLE::pointer(), + 0, + 1, + ctx_.journal); + !isTesSuccess(ter)) + return ter; + sponseeSle->makeFieldAbsent(sfSponsor); view().update(sponseeSle);