ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-23 15:10:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

Centralize AMMDeposit overflow guard in applyGuts

Browse Source 
Wrap the deposit-math dispatch in a single try/catch (gated by
fixCleanup3_4_0) instead of guarding equalDepositLimit and
singleDepositEPrice individually. This covers all deposit sub-types
uniformly and returns tecAMM_FAILED on an out-of-range solved amount.
Add a singleDepositEPrice (tfLimitLPToken) regression test.
This commit is contained in:
Gregory Tsipenyuk
2026-07-22 12:14:19 -04:00
parent 303724c108
commit abdb205de7
3 changed files with 153 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files

169
src/libxrpl/tx/transactors/dex/AMMDeposit.cpp
View File

@@ -438,11 +438,10 @@ AMMDeposit::applyGuts(Sandbox& sb)
auto const subTxType = ctx_.tx.getFlags() & tfDepositSubTx;
auto const [result, newLPTokenBalance] = [&,
&amountBalance = amountBalance,
&amount2Balance = amount2Balance,
&lptAMMBalance =
lptAMMBalance]() -> std::pair<TER, STAmount> {
auto dispatchToDeposit = [&,
&amountBalance = amountBalance,
&amount2Balance = amount2Balance,
&lptAMMBalance = lptAMMBalance]() -> std::pair<TER, STAmount> {
if (subTxType & tfTwoAsset)
{
return equalDepositLimit(
@@ -494,6 +493,27 @@ AMMDeposit::applyGuts(Sandbox& sb)
JLOG(j_.error()) << "AMM Deposit: invalid options.";
return std::make_pair(tecINTERNAL, STAmount{});
// LCOV_EXCL_STOP
};
auto const [result, newLPTokenBalance] = [&]() -> std::pair<TER, STAmount> {
try
{
return dispatchToDeposit();
}
catch (std::runtime_error const& e)
{
// A deposit whose solved amount exceeds the integral asset's range
// throws while converting to STAmount: past int64max
// Number::operator rep() throws std::overflow_error; above the asset
// maximum STAmount::canonicalize throws std::runtime_error. Fail
// cleanly with a tec rather than letting it escape doApply as
// tefEXCEPTION. Any other exception is left to propagate.
// Gated by fixCleanup3_4_0 to preserve the legacy result pre-amendment.
if (!sb.rules().enabled(fixCleanup3_4_0))
throw; // LCOV_EXCL_LINE - preserve legacy tefEXCEPTION
JLOG(j_.error()) << "AMMDeposit: deposit amount out of range " << e.what();
return std::make_pair(tecAMM_FAILED, STAmount{});
}
}();
if (isTesSuccess(result))
@@ -771,96 +791,65 @@ AMMDeposit::equalDepositLimit(
std::optional<STAmount> const& lpTokensDepositMin,
std::uint16_t tfee)
{
try
auto frac = Number{amount} / amountBalance;
auto tokensAdj = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes);
if (tokensAdj == beast::kZero)
{
auto frac = Number{amount} / amountBalance;
auto tokensAdj = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes);
if (tokensAdj == beast::kZero)
if (!view.rules().enabled(fixAMMv1_3))
{
if (!view.rules().enabled(fixAMMv1_3))
{
return {tecAMM_FAILED, STAmount{}}; // LCOV_EXCL_LINE
}
return {tecAMM_FAILED, STAmount{}}; // LCOV_EXCL_LINE
}
return {tecAMM_INVALID_TOKENS, STAmount{}};
}
// factor in the adjusted tokens
frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac);
auto const amount2Deposit =
getRoundedAsset(view.rules(), amount2Balance, frac, IsDeposit::Yes);
if (amount2Deposit <= amount2)
{
return deposit(
view,
ammAccount,
amountBalance,
amount,
amount2Deposit,
lptAMMBalance,
tokensAdj,
std::nullopt,
std::nullopt,
lpTokensDepositMin,
tfee);
}
frac = Number{amount2} / amount2Balance;
tokensAdj = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes);
if (tokensAdj == beast::kZero)
{
if (!view.rules().enabled(fixAMMv1_3))
{
return {tecAMM_FAILED, STAmount{}}; // LCOV_EXCL_LINE
}
return {tecAMM_INVALID_TOKENS, STAmount{}}; // LCOV_EXCL_LINE
}
// factor in the adjusted tokens
frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac);
auto const amountDeposit =
getRoundedAsset(view.rules(), amountBalance, frac, IsDeposit::Yes);
if (amountDeposit <= amount)
{
return deposit(
view,
ammAccount,
amountBalance,
amountDeposit,
amount2,
lptAMMBalance,
tokensAdj,
std::nullopt,
std::nullopt,
lpTokensDepositMin,
tfee);
}
return {tecAMM_FAILED, STAmount{}};
}
catch (std::runtime_error const& e)
{
// A huge Amount against a tiny integral (XRP/MPT) pool leg makes
// frac = Amount / balance enormous, so the computed pool-side deposit
// exceeds the integral asset's range and converting it to an STAmount
// throws. This happens two ways, both handled here since
// std::overflow_error derives from std::runtime_error:
// - value beyond int64 range: the Number -> int64 conversion throws
// std::overflow_error (Number::operator rep()); and
// - value within int64 but above the asset maximum (kMaxNativeN /
// kMaxMpTokenAmount): STAmount::canonicalize throws
// std::runtime_error.
// Both are user-triggered out-of-range conditions, so fail cleanly with
// a tec instead of letting the exception escape doApply as
// tefEXCEPTION. Any other (non-runtime_error) exception is left to
// propagate and is surfaced as tefEXCEPTION by the outer applySteps
// layer, consistent with treating it as an unexpected/internal error.
//
// Gated by fixCleanup3_4_0: without the amendment we must preserve
// the pre-existing (tefEXCEPTION) behavior so that upgraded and
// non-upgraded nodes agree on the transaction result.
if (!view.rules().enabled(fixCleanup3_4_0))
throw; // LCOV_EXCL_LINE - preserve legacy tefEXCEPTION
JLOG(j_.error()) << "AMMDeposit::equalDepositLimit out of range " << e.what();
return {tecAMM_INVALID_TOKENS, STAmount{}};
}
// factor in the adjusted tokens
frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac);
auto const amount2Deposit = getRoundedAsset(view.rules(), amount2Balance, frac, IsDeposit::Yes);
if (amount2Deposit <= amount2)
{
return deposit(
view,
ammAccount,
amountBalance,
amount,
amount2Deposit,
lptAMMBalance,
tokensAdj,
std::nullopt,
std::nullopt,
lpTokensDepositMin,
tfee);
}
frac = Number{amount2} / amount2Balance;
tokensAdj = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes);
if (tokensAdj == beast::kZero)
{
if (!view.rules().enabled(fixAMMv1_3))
{
return {tecAMM_FAILED, STAmount{}}; // LCOV_EXCL_LINE
}
return {tecAMM_INVALID_TOKENS, STAmount{}}; // LCOV_EXCL_LINE
}
// factor in the adjusted tokens
frac = adjustFracByTokens(view.rules(), lptAMMBalance, tokensAdj, frac);
auto const amountDeposit = getRoundedAsset(view.rules(), amountBalance, frac, IsDeposit::Yes);
if (amountDeposit <= amount)
{
return deposit(
view,
ammAccount,
amountBalance,
amountDeposit,
amount2,
lptAMMBalance,
tokensAdj,
std::nullopt,
std::nullopt,
lpTokensDepositMin,
tfee);
}
return {tecAMM_FAILED, STAmount{}};
}
/**

12
src/test/app/AMMMPT_test.cpp
View File

@@ -7156,11 +7156,9 @@ private:
// 9.22e18) and the conversion to an integral STAmount throws out of
// doApply - which applySteps would surface as tefEXCEPTION.
//
// featureMPTokensV2 is required for MPT AMMs to exist and also gates
// the equalDepositLimit overflow guard, so there is no pre-amendment
// case to compare against here - the pool cannot be created without
// the amendment. This verifies the guarded path: the deposit fails
// cleanly with a tec and no overflow escapes.
// The default amendments include fixCleanup3_4_0, under which applyGuts
// guards the overflow and fails cleanly with tecAMM_FAILED. This
// verifies the guarded path: no overflow escapes.
// XRP/MPT - the exact pool the report (Antithesis) calls out. A tiny
// mpt(1) balance and a huge MPT Amount drive frac; the XRP leg is what
@@ -7190,7 +7188,7 @@ private:
.account = alice_,
.asset1In = mpt(10'000'000'000'000), // 1e13
.asset2In = XRP(1),
.err = Ter(tecAMM_INVALID_TOKENS)});
.err = Ter(tecAMM_FAILED)});
}
// IOU/MPT - the MPT leg is the one that overflows. A classic IOU
@@ -7215,7 +7213,7 @@ private:
.account = alice_,
.asset1In = STAmount{USD, 1, 16},
.asset2In = mpt(1),
.err = Ter(tecAMM_INVALID_TOKENS)});
.err = Ter(tecAMM_FAILED)});
}
}

76
src/test/app/AMM_test.cpp
View File

@@ -7219,8 +7219,8 @@ private:
// computed XRP-side deposit exceeds the integral asset's range and the
// conversion to an STAmount throws out of doApply.
//
// equalDepositLimit catches std::runtime_error, which covers both ways
// the conversion can throw:
// applyGuts catches std::runtime_error around the deposit math, which
// covers both ways the conversion can throw:
// - value beyond int64 range: Number::operator rep() throws
// std::overflow_error (a std::runtime_error); and
// - value within int64 but above the asset maximum (kMaxNativeN):
@@ -7231,8 +7231,8 @@ private:
// int64max) - the canonicalize band, which would otherwise escape.
//
// Without fixCleanup3_4_0 the exception escapes and is converted to
// tefEXCEPTION by applySteps. With the amendment, equalDepositLimit
// guards it and fails cleanly with a tec.
// tefEXCEPTION by applySteps. With the amendment, applyGuts guards it
// and fails cleanly with tecAMM_FAILED.
auto const test = [this](FeatureBitset features, STAmount const& asset1In, TER expected) {
// These deposits intentionally trigger the overflow, which logs
// at error (guarded) or fatal (legacy tefEXCEPTION). Disable the
@@ -7255,11 +7255,72 @@ private:
// int64-range band (overflow_error): legacy escapes as tefEXCEPTION,
// fixed returns a tec.
test(all - fixCleanup3_4_0, STAmount{USD, 1, 15}, tefEXCEPTION);
test(all, STAmount{USD, 1, 15}, tecAMM_INVALID_TOKENS);
test(all, STAmount{USD, 1, 15}, tecAMM_FAILED);
// canonicalize band (runtime_error): same behavior. Regression guard
// for the band the plain overflow_error catch used to miss.
// for the band a plain overflow_error catch would miss.
test(all - fixCleanup3_4_0, STAmount{USD, 1, 11}, tefEXCEPTION);
test(all, STAmount{USD, 1, 11}, tecAMM_INVALID_TOKENS);
test(all, STAmount{USD, 1, 11}, tecAMM_FAILED);
}
void
testDepositEPriceIntegralOverflow()
{
testcase("Deposit EPrice integral overflow");
using namespace jtx;
auto const all = testableAmendments();
// Found by Antithesis: a one-sided tfLimitLPToken deposit (Amount and
// EPrice) with Amount = 0 and a large EPrice makes the solved pool-side
// deposit enormous, so it exceeds the integral asset's range and the
// conversion to an STAmount throws out of doApply. This is the
// singleDepositEPrice sibling of testDepositIntegralOverflow.
//
// applyGuts catches std::runtime_error around the deposit math, which
// covers both ways the conversion can throw:
// - value beyond int64 range: Number::operator rep() throws
// std::overflow_error (a std::runtime_error); and
// - value within int64 but above the asset maximum (kMaxNativeN):
// STAmount::canonicalize throws std::runtime_error.
//
// Without fixCleanup3_4_0 the exception escapes and is converted to
// tefEXCEPTION by applySteps. With the amendment, applyGuts guards it
// and fails cleanly with tecAMM_FAILED.
auto const test = [this](FeatureBitset features, STAmount const& ePrice, TER expected) {
// These deposits intentionally trigger the overflow, which logs
// at error (guarded) or fatal (legacy tefEXCEPTION). Disable the
// log threshold to keep the test output clean.
Env env(*this, envconfig(), features, nullptr, beast::Severity::Disabled);
env.fund(XRP(30'000), gw_, alice_);
env(trust(alice_, STAmount{USD, 1, 20}));
env(pay(gw_, alice_, STAmount{USD, 1, 18}));
env.close();
AMM amm(env, gw_, XRP(10), USD(1));
// Amount = 0 (XRP), EPrice large => tfLimitLPToken. The solved XRP
// leg blows past the integral range.
amm.deposit(
DepositArg{
.account = alice_, .asset1In = XRP(0), .maxEP = ePrice, .err = Ter(expected)});
};
// For this XRP(10)/USD(1) pool the LPToken balance is
// sqrt(1e7 drops * 1) = 3162, so T^2/B = 1e7/1e7 = 1 and the solved
// XRP-side deposit is ~EPrice^2 drops.
//
// int64-range band (overflow_error): legacy escapes as tefEXCEPTION,
// fixed returns a tec. EPrice ~1e17 drops => solved deposit ~1e34 drops,
// past int64max, so Number::operator rep() throws.
auto const bigEP = STAmount{XRPAmount{99'999'999'999'999'999}};
test(all - fixCleanup3_4_0, bigEP, tefEXCEPTION);
test(all, bigEP, tecAMM_FAILED);
// canonicalize band (runtime_error): same behavior. Regression guard
// for the band a plain overflow_error catch would miss. EPrice 1e9 drops
// => solved deposit ~1e18 drops, in [kMaxNativeN=1e17, int64max), so
// STAmount::canonicalize throws.
auto const midEP = STAmount{XRPAmount{1'000'000'000}};
test(all - fixCleanup3_4_0, midEP, tefEXCEPTION);
test(all, midEP, tecAMM_FAILED);
}
void
@@ -7384,6 +7445,7 @@ private:
testStaleAuthAccountsAfterReinit(all);
testStaleAuthAccountsAfterReinit(all - fixCleanup3_2_0);
testDepositIntegralOverflow();
testDepositEPriceIntegralOverflow();
testWithdrawIntegralNoOverflow();
}
};