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

Fix AMMDeposit returns tefEXCEPTION on integral-asset overflow

Browse Source
This commit is contained in:
Gregory Tsipenyuk
2026-07-06 13:59:57 -04:00
parent 7686d16cb7
commit 25289c26c9
3 changed files with 318 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -28,6 +28,7 @@
#include <cstdint>
#include <exception>
#include <optional>
#include <stdexcept>
#include <utility>
namespace xrpl {
@@ -730,65 +731,86 @@ AMMDeposit::equalDepositLimit(
std::optional<STAmount> 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.

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

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

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

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