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

fix: Fix MPT/DEX Audit/Attackathon reports (Phase 2) (#7537)

Browse Source 
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Sergey Kuznetsov <skuznetsov@ripple.com>
Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com>
Co-authored-by: Andrzej Budzanowski <andrzej.budzanowski@neti-soft.com>
Co-authored-by: Marek Foss <marek.foss@neti-soft.com>
Co-authored-by: Alex Kremer <akremer@ripple.com>
Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com>
Co-authored-by: Bart <bthomee@users.noreply.github.com>
Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com>
Co-authored-by: Mayukha Vadari <mvadari@ripple.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This commit is contained in:
Gregory Tsipenyuk
2026-08-07 17:53:54 -04:00
committed by GitHub
parent 0fb92c3194
commit 94bccb3a5a
14 changed files with 567 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -25,6 +25,7 @@
#include <xrpl/basics/chrono.h>
#include <xrpl/basics/safe_cast.h>
#include <xrpl/beast/unit_test/suite.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/json/json_value.h>
#include <xrpl/ledger/ApplyView.h>
#include <xrpl/ledger/helpers/AMMHelpers.h>
@@ -2292,8 +2293,9 @@ private:
// ePrice = lptAMMBalance(100) * f(0.001) / amountBalance(100) = 0.001
testAMM(
[&](AMM& ammAlice, Env& env) {
auto const err =
env.enabled(fixCleanup3_3_0) ? Ter(tecAMM_FAILED) : Ter(tefEXCEPTION);
auto const err = env.enabled(fixCleanup3_3_0) || env.enabled(fixCleanup3_4_0)
? Ter(tecAMM_FAILED)
: Ter(tefEXCEPTION);
ammAlice.withdraw(
WithdrawArg{
.account = alice_,
@@ -2304,7 +2306,7 @@ private:
{{USD(100), EUR(100)}},
1000,
std::nullopt,
{all - fixCleanup3_3_0, all});
{all - fixCleanup3_3_0 - fixCleanup3_4_0, all - fixCleanup3_4_0, all});
}
void
@@ -6024,7 +6026,7 @@ private:
void
// NOLINTNEXTLINE(readability-convert-member-functions-to-static)
testFixOverflowOffer(FeatureBitset featuresInitial)
testOverflowOffer(FeatureBitset featuresInitial)
{
using namespace jtx;
using namespace std::chrono;
@@ -6259,7 +6261,7 @@ private:
})
{
testcase(input.testCase);
for (auto const& features : {all - fixAMMOverflowOffer - fixAMMv1_1 - fixAMMv1_3, all})
for (auto const& features : {all - fixAMMv1_1 - fixAMMv1_3, all})
{
Env env(*this, features, std::make_unique<CaptureLogs>(&logs));
@@ -6308,11 +6310,6 @@ private:
return input.lpTokenBalanceAlt.value_or(input.lpTokenBalance);
}();
if (!features[fixAMMOverflowOffer])
{
BEAST_EXPECT(amm.expectBalances(failUsdGH, failUsdBIT, lpTokenBalance));
}
else
{
BEAST_EXPECT(amm.expectBalances(goodUsdGH, goodUsdBIT, lpTokenBalance));
@@ -7210,6 +7207,172 @@ 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 the integral asset's range and the
// conversion to an STAmount throws out of doApply.
//
// 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.
// XRP(10) is 1e7 drops, so the computed XRP leg is 1e7 * frac:
// asset1In 1e15 => frac ~1e15 => ~1e22 drops, past int64max; and
// asset1In 1e11 => frac ~1e11 => ~1e18 drops, in [kMaxNativeN=1e17,
// 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, 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
// 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 = asset1In,
.asset2In = XRP(1),
.err = Ter(expected)});
};
// 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_FAILED);
// canonicalize band (runtime_error): same behavior. Regression guard
// 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_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
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
{
@@ -7251,9 +7414,9 @@ private:
testSelection(all - fixAMMv1_1 - fixAMMv1_3);
testFixDefaultInnerObj();
testMalformed();
testFixOverflowOffer(all);
testFixOverflowOffer(all - fixAMMv1_3);
testFixOverflowOffer(all - fixAMMv1_1 - fixAMMv1_3);
testOverflowOffer(all);
testOverflowOffer(all - fixAMMv1_3);
testOverflowOffer(all - fixAMMv1_1 - fixAMMv1_3);
testSwapRounding();
testFixChangeSpotPriceQuality(all);
testFixChangeSpotPriceQuality(all - fixAMMv1_1 - fixAMMv1_3);
@@ -7282,6 +7445,9 @@ private:
testFailedPseudoAccount();
testStaleAuthAccountsAfterReinit(all);
testStaleAuthAccountsAfterReinit(all - fixCleanup3_2_0);
testDepositIntegralOverflow();
testDepositEPriceIntegralOverflow();
testWithdrawIntegralNoOverflow();
}
};