Sponsored Fees and Reserves for Attackathon#6842
Sponsored Fees and Reserves for Attackathon#6842kennyzlei merged 245 commits intoripple/attackathon-april-2026from
Conversation
- not fully tested - need to modify: Reserve check where using sfOwnerCount
|
Tests related to delegation seem to be failing. cc @yinyiqian1 @tequdev |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (602,283 chars). Complex issues deep in the diff may receive less attention.
Multiple critical and high-severity bugs flagged inline: missing view().update() causing silent balance data loss in SponsorshipSet, sponsor-after-erase use-after-free in XChainBridge and MPTokenIssuanceDestroy, NFTokenAcceptOffer sponsor applied to third-party buyer, reserve bypass for drained co-signing sponsors in View.cpp, wrong lsf/asf flag namespaces in tests, and several other correctness issues.
Review by Claude Opus 4.6 · Prompt: V14
| JLOG(ctx.j.debug()) << "AMM Instance: insufficient reserves"; | ||
| return tecINSUF_RESERVE_LINE; | ||
| } | ||
| JLOG(ctx.j.fatal()) << "AMM Instance: insufficient reserves"; |
There was a problem hiding this comment.
Misplaced fatal log fires on the success path, not the failure path — remove it.
| JLOG(ctx.j.fatal()) << "AMM Instance: insufficient reserves"; | |
| } |
| auto const trustlineExists = | ||
| view.exists(keylet::line(account_, lpIssue.account, lpIssue.currency)); | ||
| auto const ownerCountAdj = trustlineExists ? 0 : 1; | ||
| if (xrpLiquid(view, sponsor.value_or(account_), sponsor ? ownerCountAdj : 0, j_) >= |
There was a problem hiding this comment.
Reserve bug: when no sponsor, sponsor ? ownerCountAdj : 0 is always 0, skipping the trustline reserve. Use xrpLiquid with the account and ownerCountAdj for the non-sponsored case.
| if (xrpLiquid(view, sponsor.value_or(account_), sponsor ? ownerCountAdj : 0, j_) >= | |
| if (xrpLiquid(view, account_, ownerCountAdj, j_) >= |
| // transfer feeAmount to ledger entry | ||
| if (feeAmountDelta != beast::zero) | ||
| { | ||
| (*sponsorAccSle)[sfBalance] -= feeAmountDelta; |
There was a problem hiding this comment.
Balance change to sponsorAccSle is never persisted — view().update(sponsorAccSle) is missing in the update path. The invariant checker reads stale state and won't catch this.
| if (flags != (*sponsorObjSle)[sfFlags]) | ||
| (*sponsorObjSle)[sfFlags] = flags; | ||
|
|
||
| view().update(sponsorObjSle); |
There was a problem hiding this comment.
Missing view().update(sponsorAccSle) — balance modified at line 247 is never written to the ledger.
view().update(sponsorObjSle);
view().update(sponsorAccSle);
| { | ||
| if (balance < reserve + STAmount(amount).xrp()) | ||
| if (auto const ret = checkInsufficientReserve( | ||
| ctx_.view(), ctx_.tx, sle, balance - STAmount(amount).xrp(), {}, 1); |
There was a problem hiding this comment.
Second reserve check ignores sponsorship ({}), rejecting valid sponsored XRP escrows. Pass sponsor to match the first check.
| ctx_.view(), ctx_.tx, sle, balance - STAmount(amount).xrp(), {}, 1); | |
| ctx_.view(), ctx_.tx, sle, balance - STAmount(amount).xrp(), sponsor, 1); |
| Account const alie{"alie", KeyType::secp256k1}; | ||
| env(regkey(alice, alie)); | ||
| env(fset(alice, asfDisableMaster), sig(alice)); | ||
| env(fset(alice, lsfDisableMaster), sig(alice)); |
There was a problem hiding this comment.
fset() expects asf* constants; lsfDisableMaster (0x00100000) won't match any case and throws "unknown flag" at runtime.
| env(fset(alice, lsfDisableMaster), sig(alice)); | |
| env(fset(alice, asfDisableMaster), sig(alice)); |
See: std::runtime_error
| if (reserveCountAllowed < ownerCountDelta) | ||
| return tecINSUFFICIENT_RESERVE; | ||
|
|
||
| return tesSUCCESS; |
There was a problem hiding this comment.
Co-signing + existing sponsorship entry returns tesSUCCESS after checking only sfReserveCount, without verifying the sponsor's actual XRP balance. A drained sponsor can bypass reserve requirements indefinitely.
| auto const hasSponsorSignature = tx.isFieldPresent(sfSponsorSignature); | ||
|
|
||
| if (hasSponsorSignature) | ||
| return tesSUCCESS; |
There was a problem hiding this comment.
Co-signed sponsorship returns tesSUCCESS before checking lsfSponsorshipRequireSignForFee / lsfSponsorshipRequireSignForReserve flags. Sponsors who set these flags get no protection against co-signed transactions.
| outerSb.erase(sleClaimID); | ||
|
|
||
| adjustOwnerCount(outerSb, sleOwner, -1, j); | ||
| auto const sponsor = getLedgerEntryReserveSponsor(outerSb, sleClaimID); |
There was a problem hiding this comment.
Sponsor lookup after outerSb.erase(sleClaimID) — peek() on an erased entry returns nullptr, silently failing to credit freed reserve to the sponsor. Move the lookup before erase.
auto const sponsor = getLedgerEntryReserveSponsor(outerSb, sleClaimID);
// Remove the claim id from the ledger
outerSb.erase(sleClaimID);
adjustOwnerCount(outerSb, sleOwner, sponsor, -1, j);
|
|
||
| // Invalid flags | ||
| oracle.remove({.flags = tfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); | ||
| oracle.remove({.flags = lsfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); |
There was a problem hiding this comment.
lsfSellNFToken is a ledger flag, not a transaction flag. Both are 0x00000001 so the test passes, but the wrong namespace is misleading.
| oracle.remove({.flags = lsfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); | |
| oracle.remove({.flags = tfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); |
…linked to l`tSponsorship`
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (602,790 chars). Complex issues deep in the diff may receive less attention.
Multiple high/critical bugs flagged inline: auth bypass in checkSponsor, use-after-erase in XChainBridge and MPTokenIssuanceDestroy, UNFUNDED check bypass in PaymentChannelCreate, unpersisted balance in SponsorshipSet, missing isXRP() guard in InvariantCheck, wrong sponsor in brokered NFT trades, stale sponsor on Oracle reserve-count change, and several medium-severity issues. See inline comments.
Review by Claude Opus 4.6 · Prompt: V14
|
|
||
| // Invalid flags | ||
| oracle.remove({.flags = tfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); | ||
| oracle.remove({.flags = lsfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); |
There was a problem hiding this comment.
Wrong flag type — lsfSellNFToken is a ledger state flag; use the transaction flag tfSellNFToken:
| oracle.remove({.flags = lsfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); | |
| oracle.remove({.flags = tfSellNFToken, .fee = baseFee, .err = ter(temINVALID_FLAG)}); |
| Account const alie{"alie", KeyType::secp256k1}; | ||
| env(regkey(alice, alie)); | ||
| env(fset(alice, asfDisableMaster), sig(alice)); | ||
| env(fset(alice, lsfDisableMaster), sig(alice)); |
There was a problem hiding this comment.
fset() sets sfSetFlag which expects asf* values — lsfDisableMaster is a bitmask (0x00100000), not the AccountSet flag value. This won't disable the master key:
| env(fset(alice, lsfDisableMaster), sig(alice)); | |
| env(fset(alice, asfDisableMaster), sig(alice)); |
| if (!objSle) | ||
| return tefINTERNAL; // LCOV_EXCL_LINE | ||
|
|
||
| auto const ownerAccountID = getLedgerEntryOwner(view(), objSle, account_); |
There was a problem hiding this comment.
Bug: account_ should be sponseeAccountID. When sfSponsee is set (allowed for tfSponsorshipEnd), these differ, causing tefINTERNAL for ltSIGNER_LIST and ltRIPPLE_STATE:
| auto const ownerAccountID = getLedgerEntryOwner(view(), objSle, account_); | |
| auto const ownerAccountID = getLedgerEntryOwner(view(), objSle, sponseeAccountID); |
| outerSb.erase(sleClaimID); | ||
|
|
||
| adjustOwnerCount(outerSb, sleOwner, -1, j); | ||
| auto const sponsor = getLedgerEntryReserveSponsor(outerSb, sleClaimID); |
There was a problem hiding this comment.
Use-after-erase: getLedgerEntryReserveSponsor is called on sleClaimID after it was erased on line 708. Every other call site retrieves the sponsor before erasing. Move this line above the erase call.
| drops_ -= (*before)[sfAmount].xrp().drops(); | ||
| break; | ||
| case ltSPONSORSHIP: | ||
| if (before->isFieldPresent(sfFeeAmount)) |
There was a problem hiding this comment.
Missing isXRP() guard before calling .xrp() on sfFeeAmount — compare the sfAmount pattern at lines 117-118. If sfFeeAmount ever holds an IOU amount, this will assert or throw.
| auto const trustlineExists = | ||
| view.exists(keylet::line(account_, lpIssue.account, lpIssue.currency)); | ||
| auto const ownerCountAdj = trustlineExists ? 0 : 1; | ||
| if (xrpLiquid(view, sponsor.value_or(account_), sponsor ? ownerCountAdj : 0, j_) >= |
There was a problem hiding this comment.
Reserve check regression: when there's no sponsor the third arg becomes 0 instead of ownerCountAdj, skipping the trustline reserve check for the account:
| if (xrpLiquid(view, sponsor.value_or(account_), sponsor ? ownerCountAdj : 0, j_) >= | |
| if (xrpLiquid(view, sponsor.value_or(account_), ownerCountAdj, j_) >= |
src/libxrpl/tx/Transactor.cpp
Outdated
| // then the ledger is corrupted. Rather than make things worse we | ||
| // reject the transaction. | ||
| payerSle->setFieldAmount(sfBalance, balance - fee); | ||
| payerSle->setFieldAmount(payer.balanceField, balance - fee); |
There was a problem hiding this comment.
reset() sets sfFeeAmount to zero rather than making it absent. payFee() explicitly calls makeFieldAbsent when sfFeeAmount reaches zero (since it's soeOptional), but reset() doesn't — creating inconsistent state for callers that check isFieldPresent(sfFeeAmount).
|
|
||
| auto const hasSponsorSignature = tx.isFieldPresent(sfSponsorSignature); | ||
|
|
||
| if (hasSponsorSignature) |
There was a problem hiding this comment.
Auth bypass: checkSponsor returns tesSUCCESS unconditionally when sfSponsorSignature is present, before the sponsorship-object flag checks. A co-signed tx passes regardless of lsfSponsorshipRequireSignForFee/lsfSponsorshipRequireSignForReserve.
| auto const adjust = newCount - oldCount; | ||
| if (adjust != 0 && !adjustOwnerCount(ctx_, adjust)) | ||
| auto const newCount = calculateOracleReserve(pairs.size()); | ||
| int32_t const adjust = newCount - oldCount; |
There was a problem hiding this comment.
When adjust == 0 but the sponsor changed, neither branch executes and the sponsor on the ledger entry is never updated. preclaim handles sponsor changes by setting adjustReserve = newCount when sponsors differ, but doApply computes adjust = newCount - oldCount without that logic.
|
|
||
| if (maxFee) | ||
| { | ||
| if (*maxFee == XRPAmount(0)) |
There was a problem hiding this comment.
In the Update path, (*sponsorAccSle)[sfBalance] -= feeAmountDelta modifies the SLE but view().update(sponsorAccSle) is never called — only view().update(sponsorObjSle) is. The sponsor's XRP balance change is silently dropped. Add view().update(sponsorAccSle) alongside the existing view().update(sponsorObjSle) at line 289.
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (606,982 chars). Complex issues deep in the diff may receive less attention.
Multiple correctness and security issues flagged inline — highlights include: auth bypass in checkSponsor (co-sign path skips flag validation), critical reserve check regression in AMMDeposit, PaymentChannelCreate/EscrowCreate using opposite incorrect patterns for the two-check checkInsufficientReserve contract, missing isXRP() guards in the invariant checker, wrong flag constants in tests, stray FATAL log on the success path, missing view().update() in SponsorshipSet, and getLedgerEntryReserveSponsor called after erase in two transactors.
Review by Claude Opus 4.6 · Prompt: V14
| view().erase(mpt); | ||
|
|
||
| adjustOwnerCount(view(), view().peek(keylet::account(account_)), -1, j_); | ||
| auto const sponsor = getLedgerEntryReserveSponsor(view(), mpt); |
There was a problem hiding this comment.
getLedgerEntryReserveSponsor called after view().erase(mpt) — read sponsor before erasing:
auto const sponsor = getLedgerEntryReserveSponsor(view(), mpt);
view().erase(mpt);
auto const sponsor = getLedgerEntryReserveSponsor(view(), mpt);
Replace lines 47-49 — move the sponsor read before the erase call to match all other call sites in this PR.
| view().erase(mpt); | ||
|
|
||
| adjustOwnerCount(view(), view().peek(keylet::account(account_)), -1, j_); | ||
| auto const sponsor = getLedgerEntryReserveSponsor(view(), mpt); |
There was a problem hiding this comment.
getLedgerEntryReserveSponsor is called after view().erase(mpt). Every other call site reads the sponsor before erasing. Move the sponsor read before the erase to be consistent and safe.
Suggested fix: move line 49 above line 47 so the result is:
auto const sponsor = getLedgerEntryReserveSponsor(view(), mpt);
view().erase(mpt);
adjustOwnerCount(...);
| if (sle) | ||
| { | ||
| auto const reserveCountAllowed = sle->getFieldU32(sfReserveCount); | ||
| if (reserveCountAllowed < ownerCountDelta) |
There was a problem hiding this comment.
Signed/unsigned comparison: reserveCountAllowed is uint32_t, ownerCountDelta is int32_t. A negative delta promotes to ~4B unsigned, making this check almost always true and returning spurious tecINSUFFICIENT_RESERVE. Same issue at line ~357.
| if (reserveCountAllowed < ownerCountDelta) | |
| if (static_cast<std::int64_t>(reserveCountAllowed) < ownerCountDelta) |
| STAmount const sponsorReserve{view.fees().accountReserve( | ||
| sponsorSle->getFieldU32(sfOwnerCount), | ||
| sponsorSle->getFieldU32(sfSponsoredOwnerCount), | ||
| sponsorSle->getFieldU32(sfSponsoringOwnerCount) + ownerCountDelta, |
There was a problem hiding this comment.
Unsigned wrap: getFieldU32() + negative int32_t ownerCountDelta promotes to uint32_t, wrapping to ~4B on underflow. This produces an astronomically wrong reserve value. Same pattern on line 345 with accountCountDelta, and on lines ~364/368 in the unsponsored path. Cast to a wider signed type before arithmetic.
| JLOG(ctx.j.debug()) << "AMM Instance: insufficient reserves"; | ||
| return tecINSUF_RESERVE_LINE; | ||
| } | ||
| JLOG(ctx.j.fatal()) << "AMM Instance: insufficient reserves"; |
There was a problem hiding this comment.
Stray fatal log on the success path — logs "insufficient reserves" at FATAL for every valid non-sponsored IOU deposit by a new LP. Remove this line.
| JLOG(ctx.j.fatal()) << "AMM Instance: insufficient reserves"; |
| return ret; | ||
|
|
||
| if (auto const ret = checkInsufficientReserve( | ||
| ctx.view, ctx.tx, sle, balance - ctx.tx[sfAmount], sponsor, 1); |
There was a problem hiding this comment.
Second checkInsufficientReserve passes sponsor instead of {}. When a sponsor exists, only the sponsor's reserve is checked and the account's own balance after deduction is never validated. Compare EscrowCreate which correctly passes {} on the second check.
if (auto const ret = checkInsufficientReserve(
ctx.view, ctx.tx, sle, balance - ctx.tx[sfAmount], {}, 1);
| { | ||
| if (balance < reserve + STAmount(amount).xrp()) | ||
| if (auto const ret = checkInsufficientReserve( | ||
| ctx_.view(), ctx_.tx, sle, balance - STAmount(amount).xrp(), {}, 1); |
There was a problem hiding this comment.
Opposite error to PaymentChannelCreate: here the second check passes {} (no sponsor) when a sponsor is present, over-restricting and causing spurious tecUNFUNDED. Together these two files reveal a systematic misunderstanding of the two-check checkInsufficientReserve API contract — one under-restricts (PaymentChannel), one over-restricts (Escrow). The correct pattern should be agreed upon and applied consistently.
| auto const trustlineExists = | ||
| view.exists(keylet::line(account_, lpIssue.account, lpIssue.currency)); | ||
| auto const ownerCountAdj = trustlineExists ? 0 : 1; | ||
| if (xrpLiquid(view, sponsor.value_or(account_), sponsor ? ownerCountAdj : 0, j_) >= |
There was a problem hiding this comment.
Reserve check regression: sponsor ? ownerCountAdj : 0 passes 0 when there is no sponsor, but the correct value is ownerCountAdj (1 when the trustline doesn't exist). Non-sponsored XRP deposits skip the new-trustline reserve, allowing under-reserved deposits.
if (xrpLiquid(view, sponsor.value_or(account_), ownerCountAdj, j_) >=
depositAmount)
|
|
||
| auto const setSponsorFieldU32 = [](auto const& sle, auto const& field, auto const& delta) { | ||
| int32_t const newValue = static_cast<int32_t>(sle->getFieldU32(field)) + delta; | ||
| if (newValue <= 0) |
There was a problem hiding this comment.
newValue <= 0 silently masks underflow — a negative count (state corruption) is treated the same as zero and cleared without error. adjustReserveCount in this same file correctly returns tefINTERNAL for negative results. Add an assert or explicit negative check.
auto const setSponsorFieldU32 = [](auto const& sle, auto const& field, int32_t const delta) {
int32_t const newValue = static_cast<int32_t>(sle->getFieldU32(field)) + delta;
XRPL_ASSERT(newValue >= 0, "setSponsorFieldU32 : result must not be negative");
if (newValue == 0)
| !typeMatchesFilter(typeFilter.value(), sleNode->getType())) | ||
| canAppend = false; | ||
|
|
||
| auto const getSponsor = [&sleNode]() -> std::optional<AccountID> { |
There was a problem hiding this comment.
getSponsor doesn't check which side of the RippleState the requesting account is on. It returns sfHighSponsor first regardless, so accounts on the LOW side will see the counterparty's sponsor, producing false positives in the sponsored filter. Capture account and select the correct sponsor field based on high/low side.
High Level Overview of Change
Adding Sponsored Fees and Reserves to attackathon branch, mirroring #5887 plus resolving merge conflicts
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)