diff --git a/src/libxrpl/tx/paths/BookStep.cpp b/src/libxrpl/tx/paths/BookStep.cpp index e7220aae68..e3f1b39f5b 100644 --- a/src/libxrpl/tx/paths/BookStep.cpp +++ b/src/libxrpl/tx/paths/BookStep.cpp @@ -44,7 +44,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -750,6 +752,21 @@ BookStep::forEachOffer( } } + auto removeOffer = [&](std::string_view logMessage = {}) { + auto const key = offer.key(); + if (!logMessage.empty()) + { + JLOG(j_.trace()) << logMessage << (key ? " " + to_string(*key) : ""); + } + if (key) + offers.permRmOffer(*key); + if (!offerAttempted) + { + // Change quality only if no previous offers were tried. + ofrQ = std::nullopt; + } + }; + // It shouldn't matter from auth point of view whether it's sb // or afView. Amendment guard this change just in case. auto& applyView = sb.rules().enabled(featureMPTokensV2) ? sb : afView; @@ -760,13 +777,7 @@ BookStep::forEachOffer( { // Offer owner not authorized to hold IOU/MPT from issuer. // Remove this offer even if no crossing occurs. - if (auto const key = offer.key()) - offers.permRmOffer(*key); - if (!offerAttempted) - { - // Change quality only if no previous offers were tried. - ofrQ = std::nullopt; - } + removeOffer(); // Returning true causes offers.step() to delete the offer. return true; } @@ -779,56 +790,75 @@ BookStep::forEachOffer( static_cast(this)->getOfrOutRate(prevStep_, owner, strandDst_, trOut)); auto ofrAmt = offer.amount(); - TAmounts stpAmt{mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true), ofrAmt.out}; - - // owner pays the transfer fee. - auto ownerGives = mulRatio( - ofrAmt.out, - ofrOutRate, - QUALITY_ONE, - /*roundUp*/ std::is_same_v); - - auto const funds = offer.isFunded() - ? ownerGives // Offer owner is issuer; they have unlimited funds - : offers.ownerFunds(); - - // Only if CLOB offer - if (funds < ownerGives) + TAmounts stpAmt{ofrAmt.in, ofrAmt.out}; + auto ownerGives = ofrAmt.out; + try { - // We already know offer.owner()!=offer.issueOut().account - ownerGives = funds; - stpAmt.out = mulRatio(ownerGives, QUALITY_ONE, ofrOutRate, /*roundUp*/ false); - - // It turns out we can prevent order book blocking by (strictly) - // rounding down the ceil_out() result. This adjustment changes - // transaction outcomes, so it must be made under an amendment. - ofrAmt = offer.limitOut(ofrAmt, stpAmt.out, /*roundUp*/ false); - + // All arithmetic in this block runs before the offer is consumed. + // A crafted MPTokensV2 offer can overflow while transfer rates or + // crossing limits are applied; remove that unusable offer instead + // of letting it persist as a tecINTERNAL source. stpAmt.in = mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true); - } - // Limit offer's input if MPT, BookStep is the first step (an issuer - // is making a cross-currency payment), and this offer is not owned - // by the issuer. Otherwise, OutstandingAmount may overflow. - auto const& issuer = assetIn.getIssuer(); - if (isAssetInMPT && !prevStep_ && offer.owner() != issuer) - { - // Funds available to issue - auto const available = toAmount(accountFunds( - sb, - issuer, - assetIn, // STAmount{0}, but the default is not used - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_)); - if (stpAmt.in > available) + // owner pays the transfer fee. + ownerGives = mulRatio( + ofrAmt.out, + ofrOutRate, + QUALITY_ONE, + /*roundUp*/ std::is_same_v); + + auto const funds = offer.isFunded() + ? ownerGives // Offer owner is issuer; they have unlimited funds + : offers.ownerFunds(); + + // Only if CLOB offer + if (funds < ownerGives) { - limitStepIn(offer, ofrAmt, stpAmt, ownerGives, ofrInRate, ofrOutRate, available); - } - } + // We already know offer.owner()!=offer.issueOut().account + ownerGives = funds; + stpAmt.out = mulRatio(ownerGives, QUALITY_ONE, ofrOutRate, /*roundUp*/ false); - offerAttempted = true; - return callback(offer, ofrAmt, stpAmt, ownerGives, ofrInRate, ofrOutRate); + // It turns out we can prevent order book blocking by (strictly) + // rounding down the ceil_out() result. This adjustment changes + // transaction outcomes, so it must be made under an amendment. + ofrAmt = offer.limitOut(ofrAmt, stpAmt.out, /*roundUp*/ false); + + stpAmt.in = mulRatio(ofrAmt.in, ofrInRate, QUALITY_ONE, /*roundUp*/ true); + } + + // Limit offer's input if MPT, BookStep is the first step (an issuer + // is making a cross-currency payment), and this offer is not owned + // by the issuer. Otherwise, OutstandingAmount may overflow. + auto const& issuer = assetIn.getIssuer(); + if (isAssetInMPT && !prevStep_ && offer.owner() != issuer) + { + // Funds available to issue + auto const available = toAmount(accountFunds( + sb, + issuer, + assetIn, // STAmount{0}, but the default is not used + FreezeHandling::IgnoreFreeze, + AuthHandling::IgnoreAuth, + j_)); + if (stpAmt.in > available) + { + limitStepIn( + offer, ofrAmt, stpAmt, ownerGives, ofrInRate, ofrOutRate, available); + } + } + + offerAttempted = true; + return callback(offer, ofrAmt, stpAmt, ownerGives, ofrInRate, ofrOutRate); + } + catch (std::overflow_error const&) + { + if (sb.rules().enabled(featureMPTokensV2)) + { + removeOffer("Removing offer with overflowing amount calculation"); + return true; + } + throw; + } }; // At any payment engine iteration, AMM offer can only be consumed once. @@ -1056,6 +1086,9 @@ BookStep::revImp( auto ofrAdjAmt = ofrAmt; auto stpAdjAmt = stpAmt; auto ownerGivesAdj = ownerGives; + // This reduction can overflow, but savedIns/savedOuts are not updated + // until after it succeeds. The outer execOffer() catch can therefore + // remove the offer without rolling back local state. limitStepOut( offer, ofrAdjAmt, @@ -1157,12 +1190,21 @@ BookStep::fwdImp( auto stpAdjAmt = stpAmt; auto ownerGivesAdj = ownerGives; + // limitStepIn()/limitStepOut() can throw std::overflow_error, which is + // handled by execOffer() by removing the offer. Keep candidate + // accumulator changes local until those calls succeed so the catch + // path does not observe partially updated state. Re-sum the staged + // sets to preserve historical flat_multiset summing behavior. + auto savedInsAdj = savedIns; + auto savedOutsAdj = savedOuts; + auto resultAdj = result; typename boost::container::flat_multiset::const_iterator lastOut; + if (stpAmt.in <= remainingIn) { - savedIns.insert(stpAmt.in); - lastOut = savedOuts.insert(stpAmt.out); - result = TAmounts(sum(savedIns), sum(savedOuts)); + savedInsAdj.insert(stpAmt.in); + lastOut = savedOutsAdj.insert(stpAmt.out); + resultAdj = TAmounts(sum(savedInsAdj), sum(savedOutsAdj)); // consume the offer even if stepAmt.in == remainingIn processMore = true; } @@ -1176,15 +1218,15 @@ BookStep::fwdImp( transferRateIn, transferRateOut, remainingIn); - savedIns.insert(remainingIn); - lastOut = savedOuts.insert(stpAdjAmt.out); - result.out = sum(savedOuts); - result.in = in; + savedInsAdj.insert(remainingIn); + lastOut = savedOutsAdj.insert(stpAdjAmt.out); + resultAdj.out = sum(savedOutsAdj); + resultAdj.in = in; processMore = false; } - if (result.out > cache_->out && result.in <= cache_->in) + if (resultAdj.out > cache_->out && resultAdj.in <= cache_->in) { // The step produced more output in the forward pass than the // reverse pass while consuming the same input (or less). If we @@ -1194,8 +1236,8 @@ BookStep::fwdImp( // input provided in the forward step and produce the output // requested from the reverse step. auto const lastOutAmt = *lastOut; - savedOuts.erase(lastOut); - auto const remainingOut = cache_->out - sum(savedOuts); + savedOutsAdj.erase(lastOut); + auto const remainingOut = cache_->out - sum(savedOutsAdj); auto ofrAdjAmtRev = ofrAmt; auto stpAdjAmtRev = stpAmt; auto ownerGivesAdjRev = ownerGives; @@ -1210,13 +1252,13 @@ BookStep::fwdImp( if (stpAdjAmtRev.in == remainingIn) { - result.in = in; - result.out = cache_->out; + resultAdj.in = in; + resultAdj.out = cache_->out; - savedIns.clear(); - savedIns.insert(result.in); - savedOuts.clear(); - savedOuts.insert(result.out); + savedInsAdj.clear(); + savedInsAdj.insert(resultAdj.in); + savedOutsAdj.clear(); + savedOutsAdj.insert(resultAdj.out); ofrAdjAmt = ofrAdjAmtRev; stpAdjAmt.in = remainingIn; @@ -1227,10 +1269,15 @@ BookStep::fwdImp( { // This is (likely) a problem case, and will be caught // with later checks - savedOuts.insert(lastOutAmt); + savedOutsAdj.insert(lastOutAmt); } } + // Commit the staged accounting only after limitStepIn()/limitStepOut() + // have succeeded. + savedIns = std::move(savedInsAdj); + savedOuts = std::move(savedOutsAdj); + result = resultAdj; remainingIn = in - result.in; this->consumeOffer(sb, offer, ofrAdjAmt, stpAdjAmt, ownerGivesAdj); diff --git a/src/libxrpl/tx/paths/OfferStream.cpp b/src/libxrpl/tx/paths/OfferStream.cpp index b5ac45503a..53a4223b43 100644 --- a/src/libxrpl/tx/paths/OfferStream.cpp +++ b/src/libxrpl/tx/paths/OfferStream.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -29,6 +30,8 @@ #include #include #include +#include +#include namespace xrpl { @@ -299,7 +302,28 @@ TOfferStreamBase::step() continue; } - if (shouldRmSmallIncreasedQOffer()) + // Partially funded offers can be reduced before BookStep sees them. + // If that strict reduction overflows under MPTokensV2, remove the + // unusable offer instead of leaving it at the book tip. + bool shouldRemoveSmallIncreasedQOffer = false; + try + { + shouldRemoveSmallIncreasedQOffer = shouldRmSmallIncreasedQOffer(); + } + catch (std::overflow_error const&) + { + if (view_.rules().enabled(featureMPTokensV2)) + { + permRmOffer(entry->key()); + JLOG(j_.trace()) << "Removing offer with overflowing reduced quality " + << entry->key(); + offer_ = TOffer{}; + continue; + } + throw; + } + + if (shouldRemoveSmallIncreasedQOffer) { auto const originalFunds = accountFundsHelper( cancelView_, diff --git a/src/test/app/OfferMPT_test.cpp b/src/test/app/OfferMPT_test.cpp index 11c73b0c83..4e7cdfdf34 100644 --- a/src/test/app/OfferMPT_test.cpp +++ b/src/test/app/OfferMPT_test.cpp @@ -3058,6 +3058,158 @@ public: testHelper2TokensMix(test); } + void + testTransferRateOverflowOffer(FeatureBitset features) + { + testcase("Transfer Rate Overflow Offer"); + + using namespace jtx; + + auto const issuer = Account("issuer"); + auto const taker = Account("taker"); + + // Each scenario below targets a specific overflow path. The expected + // behavior is the same in all cases: remove the unusable book tip offer + // and let the taker's crossing offer remain rather than returning + // tecINTERNAL with the poison offer still on-ledger. + { + Env env{*this, features}; + env.fund(XRP(10'000), issuer, taker); + env.close(); + + MPTTester const token{ + {.env = env, .issuer = issuer, .holders = {taker}, .transferFee = 10'000}}; + + // Covers BookStep::forEachOffer() offer preparation, where + // ownerGives = mulRatio(ofrAmt.out, transferRateOut) overflowed + // for an oversized MPT output with a transfer fee. + std::int64_t const poisonAmount = 8'500'000'000'000'000'000LL; + auto const poisonSeq = env.seq(issuer); + env(offer(issuer, XRP(1), token(poisonAmount))); + env.close(); + + auto const poisonKeylet = keylet::offer(issuer.id(), poisonSeq); + BEAST_EXPECT(env.le(poisonKeylet) != nullptr); + + auto const takerSeq = env.seq(taker); + env(offer(taker, token(100), XRP(100))); + env.close(); + + BEAST_EXPECT(env.le(poisonKeylet) == nullptr); + BEAST_EXPECT(env.le(keylet::offer(taker.id(), takerSeq)) != nullptr); + } + + { + auto const gwA = Account("gatewayA"); + auto const gwB = Account("gatewayB"); + auto const alice = Account("alice"); + auto const mallory = Account("mallory"); + + Env env{*this, features}; + env.fund(XRP(10'000), gwA, gwB, alice, mallory); + env.close(); + + MPTTester const tokenA{ + {.env = env, .issuer = gwA, .holders = {alice, mallory}, .transferFee = 50'000}}; + + MPTTester const tokenB{{.env = env, .issuer = gwB, .holders = {alice, mallory}}}; + + env(pay(gwA, alice, tokenA(1'000))); + + // Covers BookStep::forEachOffer() offer preparation, where + // stpAmt.in = mulRatio(ofrAmt.in, transferRateIn) overflowed. + // The MPT/MPT amounts keep the offer quality reachable while + // applying tokenA's transfer rate overflows the input side. + std::int64_t const poisonPays = 6'148'914'691'236'517'205LL; + std::int64_t const poisonGets = 34'000'000'000'000'000LL; + env(pay(gwB, mallory, tokenB(poisonGets))); + + auto const poisonSeq = env.seq(mallory); + env(offer(mallory, tokenA(poisonPays), tokenB(poisonGets))); + env.close(); + + auto const poisonKeylet = keylet::offer(mallory.id(), poisonSeq); + BEAST_EXPECT(env.le(poisonKeylet) != nullptr); + + auto const aliceSeq = env.seq(alice); + env(offer(alice, tokenB(1), tokenA(100))); + env.close(); + + BEAST_EXPECT(env.le(poisonKeylet) == nullptr); + BEAST_EXPECT(env.le(keylet::offer(alice.id(), aliceSeq)) != nullptr); + } + + { + Env env{*this, features}; + env.fund(XRP(10'000), issuer, taker); + env.close(); + + MPTTester const token{ + {.env = env, .issuer = issuer, .holders = {taker}, .maxAmt = kMAX_MP_TOKEN_AMOUNT}}; + + // Covers BookStep::revImp() output reduction. The issuer's offer + // is fully funded and has no transfer fee, so offer preparation + // succeeds. The taker asks for slightly less output, forcing + // limitStepOut() to reduce the offer; that strict reduction used + // to overflow and leave the poison offer on the book. + auto const funded = 1'844'674'407'370'955'162LL; + auto const offerOut = funded + 1; + + auto const poisonSeq = env.seq(issuer); + env(offer(issuer, XRP(1), token(offerOut))); + env.close(); + + auto const poisonKeylet = keylet::offer(issuer.id(), poisonSeq); + BEAST_EXPECT(env.le(poisonKeylet) != nullptr); + + auto const takerSeq = env.seq(taker); + env(offer(taker, token(funded), XRP(1))); + env.close(); + + BEAST_EXPECT(env.le(poisonKeylet) == nullptr); + BEAST_EXPECT(env.le(keylet::offer(taker.id(), takerSeq)) != nullptr); + BEAST_EXPECT(env.balance(taker, token) == token(0)); + } + + { + auto const poisonMaker = Account("poisonMaker"); + + Env env{*this, features}; + env.fund(XRP(10'000), issuer, poisonMaker, taker); + env.close(); + + MPTTester const token{ + {.env = env, + .issuer = issuer, + .holders = {poisonMaker, taker}, + .maxAmt = kMAX_MP_TOKEN_AMOUNT}}; + + // Covers OfferStream::step() filtering. The offer is mostly + // funded, but reducing it to the actual owner funds inside + // shouldRmSmallIncreasedQOffer() used to overflow before BookStep + // saw the offer. + auto const funded = 1'844'674'407'370'955'162LL; + auto const offerOut = funded + 1; + env(pay(issuer, poisonMaker, token(funded))); + + auto const poisonSeq = env.seq(poisonMaker); + env(offer(poisonMaker, XRP(1), token(offerOut))); + env.close(); + + auto const poisonKeylet = keylet::offer(poisonMaker.id(), poisonSeq); + BEAST_EXPECT(env.le(poisonKeylet) != nullptr); + + auto const takerSeq = env.seq(taker); + env(offer(taker, token(1), XRP(1))); + env.close(); + + BEAST_EXPECT(env.le(poisonKeylet) == nullptr); + BEAST_EXPECT(env.le(keylet::offer(taker.id(), takerSeq)) != nullptr); + BEAST_EXPECT(env.balance(poisonMaker, token) == token(funded)); + BEAST_EXPECT(env.balance(taker, token) == token(0)); + } + } + void testSelfCrossOffer1(FeatureBitset features) { @@ -4789,6 +4941,7 @@ public: testSellOffer(features); testSellWithFillOrKill(features); testTransferRateOffer(features); + testTransferRateOverflowOffer(features); testSelfCrossOffer(features); testSelfIssueOffer(features); testDirectToDirectPath(features);