fix: Fix MPT/DEX Audit/Attackathon reports (Phase 1) - #7334
Conversation
gregtatcam
changed the title
24f3b17 to
77d5cea
Compare
d4431dd to
b2a0a2b
Compare
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall this is a well-scoped, carefully tested batch of audit fixes (MPT liquidity accounting, book-hash collisions, overflow-safe arithmetic, pathfinding fixes, reserve check, and a zero-quality offer guard). Most changes are accompanied by targeted regression tests and detailed rationale comments, and I did not find clear correctness bugs in the added lines. The one area worth a second pair of eyes is the removal of the explicit MPT lock/freeze check in BookStep::checkMPTDEX, since it is a security-sensitive control and its safety now depends on an implicit invariant (that every liquidity source feeding this method already zeroes/filters locked holdings) that isn't enforced by this function itself.
There was a problem hiding this comment.
This is a large, well-scoped audit-fix MR (MPT/DEX Phase 1) touching pathfinding, book indexing, BookStep/MPTEndpointStep flow logic, AMMWithdraw reserve checks, and several RPC surfaces (book_offers, ledger, book_changes). The changes are consistently paired with explanatory comments and targeted regression tests (freeze bypass, overflow handling, srcAmount plumbing, missing-else bug, book hash collisions, MPT funding in book_offers/ledger). I traced the core logic changes (getBookBase tagging, Pathfinder srcAmount fix, PathRequest missing-else fix, canSend semantics in MPT.h/AccountAssets.cpp, AMMWithdraw balanceAdj fix, OfferCreate zero-rate rejection, MPTEndpointStep overflow-safe mulRatio) against their stated rationale and did not find correctness problems in the added lines. The one area worth a second set of eyes is the removal of the explicit isFrozen() check in BookStep::checkMPTDEX, which relies on OfferStream/AMMLiquidity already filtering locked holdings before reaching this method — the reasoning and the new testLockedMidPathHolder test are consistent with that claim, but it's a freeze-bypass-relevant code path worth a deliberate re-confirmation given the audit context.
Add UNREACHABLE probes to MPTEndpointStep
There was a problem hiding this comment.
This is a well-scoped, heavily-documented audit-fix PR covering 11 distinct MPT/DEX issues (inflated MPT book liquidity, book hash collisions, pathfinding constraints, overflow handling in MPTEndpointStep/BookStep, AMMWithdraw reserve checks, owner_funds for MPT offers, PathRequest's missing else, and zero-quality offer placement). I traced each production-code change against its corresponding fix description and, where present, the paired regression test (testBookBaseMixedAssetCollision, testMPTEndpointTransferRateOverflow, testWithdrawReserveUsesLiveBalance, testLockedMidPathHolder, testMPTOfferZeroRate, testBookOffersMPTFunding, testSkipsOverflowingRate/Volume, testLedgerOwnerFundsMPTOffer, convertAllSendMaxRanking, maxedOutMPTPathfinding, sourceCurrencyIssuerSelection). Each change is consistent with its stated intent, the new type-tagging in getBookBase avoids preimage collisions correctly, the tryMulRatio/UNREACHABLE guards in MPTEndpointStep follow the existing codebase convention for asserted-unreachable error paths, the AMMWithdraw reserve fix now uses max(priorBalance, balance) uniformly for both Issue and MPT, and the PathRequest::parseJson else-branch fix correctly scopes the issuer-reconciliation check to the Currency/IOU case only. No high-confidence correctness, security, or resource-leak issues were found on the added lines beyond what the authors already reasoned through and covered with dedicated tests.
vlntb
left a comment
vlntb
left a comment
There was a problem hiding this comment.
Looks good.
Left two last comments for you to decide.
Approving.
There was a problem hiding this comment.
This is a large, well-scoped audit/attackathon fix PR touching MPT/DEX pathfinding, book indexing, arithmetic overflow handling, and RPC funding calculations. Each fix in the description is paired with targeted regression tests (e.g., testWithdrawReserveUsesLiveBalance, testMPTEndpointTransferRateOverflow, testLockedMidPathHolder, testBookBaseMixedAssetCollision, convertAllSendMaxRanking), and I traced the logic for each change against its stated intent without finding a clear correctness or security regression in the added lines. The riskiest-looking change (removing the isFrozen check in BookStep::checkMPTDEX) is explicitly justified by the new top-of-function comment and is directly exercised by testLockedMidPathHolder, which confirms OfferStream's isDeepFrozen gating makes the removed check redundant. The overflow-guard additions (tryMulRatio, checkedAdd in BookChanges.h, the temBAD_OFFER preflight check) are internally consistent with their call sites and covered by dedicated tests. No high-confidence bugs were found in the newly added (+) lines.
There was a problem hiding this comment.
This is a large, well-documented audit-fix PR touching MPT/DEX pathfinding, book-hashing, transfer-rate arithmetic, and overflow handling. Each change is accompanied by an explanatory comment and dedicated regression test (e.g., getBookBase collision tags, MPTEndpointStep overflow capping, AMMWithdraw live-balance reserve check, OfferCreate zero-rate rejection, PathRequest missing-else fix, Pathfinder srcAmount fix, book_offers MPT funding). I traced the changed logic in TokenHelpers, Indexes.cpp, BookStep.cpp, MPTEndpointStep.cpp, AMMWithdraw.cpp, OfferCreate.cpp, NetworkOPs.cpp, BookChanges.h, AccountAssets.cpp/MPT.h, and Pathfinder.cpp/PathRequest.cpp against their stated intent and did not find correctness or security defects in the added (+) lines — the overflow-avoidance math in MPTEndpointStep::revImp/fwdImp checks out numerically (ceil(floor(M*Q/r)*r/Q) <= M), the checkedAdd/rate-guard logic in BookChanges.h correctly skips only the offending crossing without corrupting other entries, and the refactored canSend()/foundBalance() helpers preserve prior semantics while fixing the described bugs. No issues met the confidence bar for flagging.
| // Reject an offer whose quality is not representable. getRate() returns 0 | ||
| // when the rate overflows (a large MPT amount -- XLS-0082 allows up to | ||
| // 2^63-1 -- over a small IOU) and when it underflows ("too good", reachable | ||
| // for XRP/IOU and IOU/IOU at extreme magnitude ratios). A rate-0 offer | ||
| // cannot function: it would rest in the quality-0 book directory, whose | ||
| // index equals getBookBase(book). BookTip starts its scan at getBookBase | ||
| // and asks succ() for keys strictly greater (an open interval), so the | ||
| // quality-0 directory is never returned -- the offer can never be crossed, | ||
| // yet it still consumes the owner's reserve. Rejecting it also prevents the | ||
| // tick-size rounding path in applyGuts from dividing by a zero rate (which | ||
| // would throw and surface as tefEXCEPTION). Applies to all asset types, | ||
| // gated on featureMPTokensV2 so that pre-amendment book behavior (such | ||
| // offers placed but inert) is preserved for consensus. | ||
| if (ctx.rules.enabled(featureMPTokensV2) && getRate(saTakerGets, saTakerPays) == 0) | ||
| { | ||
| JLOG(j.debug()) << "Malformed offer: unrepresentable quality"; | ||
| return temBAD_OFFER; | ||
| } |
There was a problem hiding this comment.
This fix solves one problem but creates another. The residual case it protects against is real — a remainder resting at quality 0 lands in the directory whose index equals getBookBase(book), which BookTip's strict-successor scan never returns, so it can never be crossed while still holding the owner's reserve. But rejecting in preflight also rejects every offer that would have filled completely and never rested anything, which is a much larger class: any bid whose raw TakerPays exceeds ~1.8447e17 (2^64 / 1e17 — the point where divide()'s muldiv(numVal, kTenTO17, denVal) overflows). So the fix protects a narrow victim by penalising a wider one.
preflight is the structural problem: it never sees the ledger, so it can't know whether the offer will cross, rest, or partially do both — and crossing happens in applyGuts, before any residual is placed. "Can never be crossed" is true of an offer that rests, and preflight can't tell which ones are.
There was a problem hiding this comment.
This test demonstrates the problem:
void
testMPTOfferZeroRateCrossable(FeatureBitset features)
{
// An unrepresentable quality does not imply an offer that cannot
// function. "Can never be crossed" describes an offer that RESTS:
// crossing happens in applyGuts, before any residual is placed in the
// book, so an offer whose rate is unrepresentable can still consume a
// resting offer in full and never reach the quality-0 directory.
//
// The two sides of one trade do not have the same rate
// representability: getRate(TakerGets, TakerPays) overflows to 0 for
// the side paying a large MPT, but not for the side paying XRP. So a
// preflight rejection keyed on getRate() == 0 admits the resting half
// of a trade and rejects the crossing half.
testcase("MPT Offer Zero Rate - crossable quality");
using namespace jtx;
// Above the rate-overflow threshold: divide() scales the XRP
// denominator up to a 1e15 mantissa and then evaluates
// muldiv(mptMantissa, 1e17, denMantissa), which exceeds 2^64 -- so
// getRate() takes its catch-all and returns 0.
auto const kBigMpt = 200'000'000'000'000'000LL;
// Both scenarios are the same trade against the same resting offer,
// and both execute identically (at bob's price). They differ only in
// the price alice quotes, and therefore only in whether the rate on
// HER side of the book is representable.
auto runScenario = [&](STAmount const& aliceQuote, bool rateRepresentable) {
Env env{*this, features};
auto const gw = Account{"gateway"};
auto const alice = Account{"alice"};
auto const bob = Account{"bob"};
env.fund(XRP(10'000), gw, alice, bob);
env.close();
MPTTester mt{
{.env = env, .issuer = gw, .holders = {alice, bob}, .maxAmt = kMaxMpTokenAmount}};
MPT const mpt = mt;
mt.pay(gw, bob, kBigMpt);
env.close();
// Bob rests the sell side: TakerPays = XRP(1), TakerGets =
// kBigMpt. getRate(TakerGets, TakerPays) is representable in this
// direction, so preflight admits it and it rests at a normal
// quality.
BEAST_EXPECT(getRate(mpt(kBigMpt), XRP(1)) != 0);
auto const bobSeq = env.seq(bob);
env(offer(bob, XRP(1), mpt(kBigMpt)), Ter(tesSUCCESS));
env.close();
BEAST_EXPECT(env.le(keylet::offer(bob.id(), SeqProxy::rawSequence(bobSeq))) != nullptr);
// Alice takes it from the other side: TakerPays = kBigMpt,
// TakerGets = her quote.
BEAST_EXPECT((getRate(aliceQuote, mpt(kBigMpt)) != 0) == rateRepresentable);
auto const aliceMptBefore = mt.getBalance(alice);
auto const bobXrpBefore = env.balance(bob).value().xrp();
env(offer(alice, mpt(kBigMpt), aliceQuote), Ter(tesSUCCESS));
env.close();
// Alice's offer crosses bob's in full, so it never rests: nothing
// ends up in the quality-0 directory, no reserve is stranded, and
// the tick-rounding divide is never reached with a zero rate.
BEAST_EXPECT(mt.getBalance(alice) == aliceMptBefore + kBigMpt);
BEAST_EXPECT(env.le(keylet::offer(bob.id(), SeqProxy::rawSequence(bobSeq))) == nullptr);
BEAST_EXPECT(offersOnAccount(env, alice).empty());
// And it executes at bob's 1 XRP, whatever alice quoted.
BEAST_EXPECT(env.balance(bob).value().xrp() == bobXrpBefore + XRP(1).value().xrp());
};
// Alice quotes bob's exact price. getRate() overflows to 0 on her
// side, so the preflight check rejects an offer that would have
// crossed in full.
runScenario(XRP(1), /*rateRepresentable=*/false);
// Alice quotes a price worse for herself, which halves the rate into
// representable range and is admitted. Same execution as above: she
// pays 1 XRP for kBigMpt. The rejection above is therefore numeric,
// not economic.
runScenario(XRP(2), /*rateRepresentable=*/true);
}
There was a problem hiding this comment.
xrpl.tx.OfferMPT MPT Offer Zero Rate - crossable quality
#55 failed: apply (OfferMPT_test.cpp:5648): Got temBAD_OFFER (Malformed: Bad offer.);
Expected tesSUCCESS
#56 failed: OfferMPT_test.cpp(5654) <- alice gains no MPT
#57 failed: OfferMPT_test.cpp(5655) <- bob's offer still resting
#59 failed: OfferMPT_test.cpp(5658) <- bob receives no XRP
failed: xrpl.tx.OfferMPT had 4 failures.
| auto const rate = transferRate(view, book.out.getIssuer()); | ||
| auto const rate = transferRate(view, book.out); |
There was a problem hiding this comment.
This overload switch improves the existing behaviour, but still can be improved further. It opens the fee-adjustment gate for MPT books, and the divide behind it is unguarded:
offerRate = rate;
saOwnerFundsLimit = divide(saOwnerFunds, offerRate); // 4928
divide() multiplies the numerator by 1e17 for precision (STAmount.cpp:1266), which overflows uint64 once the numerator mantissa passes ~184.467 × the normalized denominator. saOwnerFunds is an MPT amount, so its mantissa is the raw int64 balance — the boundary is ~1.8447e17 at a near-parity rate, rising to ~2.77e17 at the maximum transfer fee. The throw escapes as error: internal from book_offers.
Two things make this worth considering:
- It's a public RPC, and one offer owner poisons the whole response. Every wallet, UI, and market maker querying that book gets internal — not a degraded answer, no answer.
- The correct result always fits.
offerRate ≥ 1, sosaOwnerFunds / offerRate ≤ saOwnerFunds. Only the intermediate overflows, which means no clamping or policy decision is needed — just 128-bit arithmetic.MPTAmount::mulRatio / tryMulRatioalready do this, and this PR uses them inMPTEndpointStep.
… update unit-tests
There was a problem hiding this comment.
This is a large, well-scoped security/correctness fix set (11 distinct audit/attackathon issues) with unusually thorough inline reasoning and matching regression tests for nearly every change (freeze bypass, overflow guards, reserve check, pathfinding constraints, book hashing collisions, zero-quality offers, etc.). I went through each production file and traced the logic against the accompanying tests; I did not find a clear, high-confidence new bug introduced by this diff — the changes are internally consistent with their comments and the added tests exercise the exact scenarios described in the MR (e.g. testLockedMidPathHolder for the BookStep freeze-check removal, testWithdrawReserveUsesLiveBalance for the AMMWithdraw reserve fix, testBookBaseMixedAssetCollision for the Indexes.cpp hash-tag fix, convertAllSendMaxRanking for the Pathfinder srcAmount fix). One item worth a second pair of eyes is called out below, but it's a maintainability/robustness note rather than a confirmed defect.
There was a problem hiding this comment.
This is a large, carefully constructed audit-fix PR (11 distinct MPT/DEX issues) with extensive accompanying unit tests (AMMMPT_test, FlowMPT_test, OfferMPT_test, Path_test, PathMPT_test, BookChanges_test, LedgerRPC_test) that specifically target each described defect. I traced each production-code hunk (getBookBase type tags, BookStep freeze-check removal, MPTEndpointStep overflow-safe revImp/fwdImp, AMMWithdraw reserve check, OfferCreate zero-rate handling, NetworkOPs::getBookPage MPT funding/transfer-rate math, BookChanges overflow guards, PathRequest/Pathfinder/AccountAssets/MPT.h pathfinding constraint fixes) against the bug it claims to fix and against the new regression tests, and did not find a clear, high-confidence correctness or security defect in the changed (+) lines. The MPTEndpointStep.cpp refactor introduces several UNREACHABLE()-guarded branches replacing previously live error-handling paths (e.g. dropped inline resetCache() calls in checkCreateMPT), but on closer inspection the resetCache() call is still performed by the outer caller (revImp/fwdImp) whenever sendWithMPTCreate() returns a failure TER, so the safety net is preserved rather than removed. The arithmetic changes (mulRatio-based transfer-rate math replacing divide() for MPT, tryMulRatio for the endpoint step) are consistent with avoiding the std::overflow_error paths described in the MR and are exercised by dedicated overflow-focused tests. No SQL/command injection, hardcoded secrets, resource leaks, or K8s/Terraform issues apply to this diff. Given the scope and quality of test coverage, I'm not confident enough in any specific line to flag it as a defect rather than noise.
3 participants
High Level Overview of Change
Fix audit and attackathon issues:
Test Plan
Extend:
AMMMPT_test, FlowMPT_test, OfferMPT_test, Path_test, PathMPT_test