diff --git a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp index 653e8c6961..91165c345e 100644 --- a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include namespace xrpl { @@ -730,65 +731,86 @@ AMMDeposit::equalDepositLimit( std::optional const& lpTokensDepositMin, std::uint16_t tfee) { - auto frac = Number{amount} / amountBalance; - auto tokensAdj = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes); - if (tokensAdj == beast::kZero) + try { - if (!view.rules().enabled(fixAMMv1_3)) + auto frac = Number{amount} / amountBalance; + auto tokensAdj = getRoundedLPTokens(view.rules(), lptAMMBalance, frac, IsDeposit::Yes); + if (tokensAdj == beast::kZero) { - return {tecAMM_FAILED, STAmount{}}; // LCOV_EXCL_LINE - } + if (!view.rules().enabled(fixAMMv1_3)) + { + 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::overflow_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 Number's int64 range and the conversion to an integral + // STAmount throws. Guard it and fail cleanly with a tec instead of + // letting the exception escape doApply as tefEXCEPTION. + // + // Gated by featureMPTokensV2: 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(featureMPTokensV2)) + throw; // LCOV_EXCL_LINE - preserve legacy tefEXCEPTION + JLOG(j_.error()) << "AMMDeposit::equalDepositLimit overflow " << 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{}}; } /** Single asset deposit of the amount of asset specified by Asset1In. diff --git a/src/test/app/AMMMPT_test.cpp b/src/test/app/AMMMPT_test.cpp index 5b576b41e4..2693f75d5d 100644 --- a/src/test/app/AMMMPT_test.cpp +++ b/src/test/app/AMMMPT_test.cpp @@ -7079,6 +7079,154 @@ private: } } + void + testDepositIntegralOverflowMPT() + { + testcase("Deposit integral overflow (MPT)"); + + using namespace jtx; + + // MPT counterpart of AMM_test::testDepositIntegralOverflow. A two-asset + // deposit with a huge Amount against a tiny pool leg makes + // frac = Amount / balance enormous, so the computed deposit for the + // other (integral) leg exceeds Number's int64 range (kMaxRep ~= + // 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. + + // 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 + // overflows: XRP(10) is 1e7 drops, so getRoundedAsset(XRP, frac) is + // 1e7 * 1e13 = 1e20 drops, well past kMaxRep. + { + // The deposit intentionally overflows, which logs at error. + // Disable the log threshold to keep the test output clean. + Env env(*this, beast::Severity::Disabled); + env.fund(XRP(30'000), gw_, alice_); + env.close(); + + // kMptDexFlags (CanTrade | CanTransfer), which AMMs require, is + // the default. alice must hold enough MPT to fund the pool and the + // oversized deposit. + MPT const mpt = MPTTester( + {.env = env, + .issuer = gw_, + .holders = {alice_}, + .pay = 100'000'000'000'000, // 1e14 + .maxAmt = 1'000'000'000'000'000}); // 1e15 + env.close(); + + AMM amm(env, alice_, XRP(10), mpt(1)); + amm.deposit( + DepositArg{ + .account = alice_, + .asset1In = mpt(10'000'000'000'000), // 1e13 + .asset2In = XRP(1), + .err = Ter(tecAMM_INVALID_TOKENS)}); + } + + // IOU/MPT - the MPT leg is the one that overflows. A classic IOU + // trustline drives frac (huge USD Amount vs USD(1) balance); the + // MPT-side deposit is then mptBalance * frac = 10'000 * 1e16 = 1e20. + { + // The deposit intentionally overflows, which logs at error. + // Disable the log threshold to keep the test output clean. + Env env(*this, 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(); + + MPT const mpt = + MPTTester({.env = env, .issuer = gw_, .holders = {alice_}, .pay = 1'000'000}); + env.close(); + + AMM amm(env, alice_, mpt(10'000), USD(1)); + amm.deposit( + DepositArg{ + .account = alice_, + .asset1In = STAmount{USD, 1, 16}, + .asset2In = mpt(1), + .err = Ter(tecAMM_INVALID_TOKENS)}); + } + } + + void + testWithdrawIntegralNoOverflowMPT() + { + testcase("Withdraw integral no overflow (MPT)"); + + using namespace jtx; + + // MPT counterpart of AMM_test::testWithdrawIntegralNoOverflow and the + // sibling of testDepositIntegralOverflowMPT. AMMWithdraw:: + // equalWithdrawLimit has the same getRoundedAsset(integralBalance, + // frac) structure as the deposit path and is likewise not wrapped in a + // try/catch. It is safe only because withdraw preclaim (checkAmount) + // rejects a requested Amount greater than the pool balance with + // tecAMM_BALANCE *before* the math runs, so frac = Amount / balance + // stays <= 1 and the Number -> integral STAmount conversion cannot + // overflow. Deposit has no such bound, which is why only the deposit + // path was exposed. + // + // These mirror the deposit repros: the same oversized two-asset + // request is rejected cleanly. If the preclaim bound is ever weakened, + // equalWithdrawLimit would be reached with a huge frac and + // Number::operator rep() would escape as tefEXCEPTION, failing this. + + // XRP/MPT - the pool the report calls out. Requesting far more of the + // tiny MPT leg than the pool holds is rejected before the math. + { + Env env(*this); + env.fund(XRP(30'000), gw_, alice_); + env.close(); + + MPT const mpt = MPTTester( + {.env = env, + .issuer = gw_, + .holders = {alice_}, + .pay = 100'000'000'000'000, // 1e14 + .maxAmt = 1'000'000'000'000'000}); // 1e15 + env.close(); + + // alice holds all LPTokens of a tiny XRP/MPT pool. + AMM amm(env, alice_, XRP(10), mpt(1)); + amm.withdraw( + WithdrawArg{ + .account = alice_, + .asset1Out = mpt(10'000'000'000'000), // 1e13 > mpt(1) + .asset2Out = XRP(1), + .err = Ter(tecAMM_BALANCE)}); + } + + // IOU/MPT - requesting far more of the tiny IOU leg than the pool + // holds is likewise rejected. + { + Env env(*this); + env.fund(XRP(30'000), gw_, alice_); + env(trust(alice_, STAmount{USD, 1, 20})); + env(pay(gw_, alice_, STAmount{USD, 1, 18})); + env.close(); + + MPT const mpt = + MPTTester({.env = env, .issuer = gw_, .holders = {alice_}, .pay = 1'000'000}); + env.close(); + + AMM amm(env, alice_, mpt(10'000), USD(1)); + amm.withdraw( + WithdrawArg{ + .account = alice_, + .asset1Out = STAmount{USD, 1, 16}, // > USD(1) + .asset2Out = mpt(1), + .err = Ter(tecAMM_BALANCE)}); + } + } + void run() override { @@ -7114,6 +7262,8 @@ private: testLPTokenBalance(all - fixAMMv1_3); testAMMDepositWithFrozenAssets(); testAutoDelete(); + testDepositIntegralOverflowMPT(); + testWithdrawIntegralNoOverflowMPT(); } }; diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index f103fb5f2a..4be921a4fc 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -7186,6 +7186,96 @@ private: } } + void + testDepositIntegralOverflow() + { + testcase("Deposit integral overflow"); + + using namespace jtx; + auto const all = testableAmendments(); + + // Found by Antithesis: two-asset deposit with a huge Amount against a + // tiny pool leg makes frac = Amount/amountBalance enormous, so the + // computed XRP-side deposit exceeds Number's int64 range and + // Number::operator rep() throws out of doApply. + // + // Without featureMPTokensV2 the exception escapes and is converted to + // tefEXCEPTION by applySteps. With the amendment, equalDepositLimit + // guards the overflow and fails cleanly with a tec. + auto const test = [this](FeatureBitset features, 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)); + amm.deposit( + DepositArg{ + .account = alice_, + .asset1In = STAmount{USD, 1, 15}, + .asset2In = XRP(1), + .err = Ter(expected)}); + }; + + // Legacy behavior: overflow escapes as tefEXCEPTION. + test(all - featureMPTokensV2, tefEXCEPTION); + // Fixed behavior: overflow is guarded and returns a tec. + test(all, tecAMM_INVALID_TOKENS); + } + + void + testWithdrawIntegralNoOverflow() + { + testcase("Withdraw integral no overflow"); + + using namespace jtx; + auto const all = testableAmendments(); + + // Regression guard for the sibling of testDepositIntegralOverflow. + // AMMWithdraw::equalWithdrawLimit has the same + // getRoundedAsset(integralBalance, frac) structure as the deposit + // path and is likewise not wrapped in a try/catch. It is safe only + // because withdraw preclaim (checkAmount) rejects a requested Amount + // greater than the pool balance with tecAMM_BALANCE *before* the math + // runs, so frac = Amount / balance stays <= 1 and the Number -> + // integral STAmount conversion cannot overflow. Deposit has no such + // bound (depositing more than the pool holds is legal), which is why + // only the deposit path was exposed. + // + // This asserts the withdrawal analog of the deposit repro fails cleanly + // with a tec. If the preclaim bound is ever weakened, equalWithdrawLimit + // would be reached with a huge frac and Number::operator rep() would + // escape as tefEXCEPTION, failing this test. + auto const test = [this](FeatureBitset features) { + Env env(*this, features); + env.fund(XRP(30'000), gw_, alice_); + env(trust(alice_, STAmount{USD, 1, 20})); + env(pay(gw_, alice_, STAmount{USD, 1, 18})); + env.close(); + + // gw holds all LPTokens of a tiny XRP/USD pool. + AMM amm(env, gw_, XRP(10), USD(1)); + + // Two-asset limit withdraw (tfTwoAsset) requesting far more of the + // tiny USD leg than the pool holds - the mirror of the deposit + // repro. Rejected upstream, so no overflow is possible. + amm.withdraw( + WithdrawArg{ + .account = gw_, + .asset1Out = STAmount{USD, 1, 15}, + .asset2Out = XRP(1), + .err = Ter(tecAMM_BALANCE)}); + }; + + // Bound holds regardless of the deposit-side fix amendment. + test(all - featureMPTokensV2); + test(all); + } + void run() override { @@ -7258,6 +7348,8 @@ private: testFailedPseudoAccount(); testStaleAuthAccountsAfterReinit(all); testStaleAuthAccountsAfterReinit(all - fixCleanup3_2_0); + testDepositIntegralOverflow(); + testWithdrawIntegralNoOverflow(); } };