From abdb205de7814df62130241aa2df035b62006241 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Wed, 22 Jul 2026 12:14:19 -0400 Subject: [PATCH] Centralize AMMDeposit overflow guard in applyGuts 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. --- src/libxrpl/tx/transactors/dex/AMMDeposit.cpp | 169 ++++++++---------- src/test/app/AMMMPT_test.cpp | 12 +- src/test/app/AMM_test.cpp | 76 +++++++- 3 files changed, 153 insertions(+), 104 deletions(-) diff --git a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp index 51b746e631..91213a5549 100644 --- a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp @@ -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 { + auto dispatchToDeposit = [&, + &amountBalance = amountBalance, + &amount2Balance = amount2Balance, + &lptAMMBalance = lptAMMBalance]() -> std::pair { 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 { + 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 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{}}; } /** diff --git a/src/test/app/AMMMPT_test.cpp b/src/test/app/AMMMPT_test.cpp index 9c70e1873c..17d7375e17 100644 --- a/src/test/app/AMMMPT_test.cpp +++ b/src/test/app/AMMMPT_test.cpp @@ -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)}); } } diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 3b17277b29..a6e3899ffc 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -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(); } };