ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-06-04 17:27:00 +00:00
Code Issues Packages Projects Releases Wiki Activity

Remove overflowing MPT DEX offers during crossing.

Browse Source
This commit is contained in:
Gregory Tsipenyuk
2026-05-15 16:56:55 -04:00
parent 305d784b26
commit 210b6e08ba
3 changed files with 294 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

185
src/libxrpl/tx/paths/BookStep.cpp
View File

@@ -44,7 +44,9 @@
#include <numeric>
#include <optional>
#include <sstream>
#include <stdexcept>
#include <string>
#include <string_view>
#include <type_traits>
#include <utility>
#include <variant>
@@ -750,6 +752,21 @@ BookStep<TIn, TOut, TDerived>::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<TIn, TOut, TDerived>::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<TIn, TOut, TDerived>::forEachOffer(
static_cast<TDerived const*>(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<TOut, MPTAmount>);
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<TIn>(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<TOut, MPTAmount>);
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<TIn>(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<TIn, TOut, TDerived>::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<TIn, TOut, TDerived>::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<TOut>::const_iterator lastOut;
if (stpAmt.in <= remainingIn)
{
savedIns.insert(stpAmt.in);
lastOut = savedOuts.insert(stpAmt.out);
result = TAmounts<TIn, TOut>(sum(savedIns), sum(savedOuts));
savedInsAdj.insert(stpAmt.in);
lastOut = savedOutsAdj.insert(stpAmt.out);
resultAdj = TAmounts<TIn, TOut>(sum(savedInsAdj), sum(savedOutsAdj));
// consume the offer even if stepAmt.in == remainingIn
processMore = true;
}
@@ -1176,15 +1218,15 @@ BookStep<TIn, TOut, TDerived>::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<TIn, TOut, TDerived>::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<TIn, TOut, TDerived>::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<TIn, TOut, TDerived>::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);

26
src/libxrpl/tx/paths/OfferStream.cpp
View File

@@ -17,6 +17,7 @@
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/Book.h>
#include <xrpl/protocol/Concepts.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/IOUAmount.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/MPTAmount.h>
@@ -29,6 +30,8 @@
#include <algorithm>
#include <memory>
#include <optional>
#include <stdexcept>
#include <type_traits>
namespace xrpl {
@@ -299,7 +302,28 @@ TOfferStreamBase<TIn, TOut>::step()
continue;
}
if (shouldRmSmallIncreasedQOffer<TIn, TOut>())
// 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<TIn, TOut>();
}
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<TIn, TOut>{};
continue;
}
throw;
}
if (shouldRemoveSmallIncreasedQOffer)
{
auto const originalFunds = accountFundsHelper(
cancelView_,

153
src/test/app/OfferMPT_test.cpp
View File

@@ -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);