Compare commits

...

3 Commits

Author SHA1 Message Date
Vito
5b3ae8c9d5 fix: Harden MaximumAmount overflow checks in rippleSendMultiMPT
Use subtraction-based guards instead of addition to prevent uint64_t
overflow in both the post-amendment aggregate check and the
pre-amendment per-iteration check. Each condition in the cascade
protects the subtraction in the next from underflow.

Move totalSendAmount accumulation after the check so the guard
operates on the pre-addition value.
2026-03-26 15:21:40 +01:00
Vito
e5b2d9a682 Merge remote-tracking branch 'origin/develop' into tapanito/fix-multi-send-max-amount
# Conflicts:
#	include/xrpl/protocol/detail/features.macro
#	src/libxrpl/ledger/View.cpp
2026-03-26 10:58:12 +01:00
Vito
bd544acb9e fix: Enforce aggregate MaximumAmount in multi-send MPT
rippleSendMultiMPT used a read-only SLE snapshot (view.read) to check
MaximumAmount per iteration. Since rippleCreditMPT updates a separate
mutable copy (view.peek), the snapshot's sfOutstandingAmount was stale
after the first iteration, allowing the aggregate to exceed
MaximumAmount.

Replace the per-iteration check with a running total that validates
the aggregate against MaximumAmount within the send loop. The old
per-iteration check is retained behind a !fixAssortedFixes gate for
ledger replay compatibility.
2026-03-25 12:22:33 +01:00
2 changed files with 134 additions and 20 deletions

View File

@@ -1155,57 +1155,82 @@ rippleSendMultiMPT(
beast::Journal j,
WaiveTransferFee waiveFee)
{
// Safe to get MPT since rippleSendMultiMPT is only called by
// accountSendMultiMPT
auto const& issuer = mptIssue.getIssuer();
auto const sle = view.read(keylet::mptIssuance(mptIssue.getMptID()));
if (!sle)
return tecOBJECT_NOT_FOUND;
// These may diverge
// For the issuer-as-sender case, track the running total to validate
// against MaximumAmount. The read-only SLE (view.read) is not updated
// by rippleCreditMPT, so a per-iteration SLE read would be stale.
Number totalSendAmount;
auto const maximumAmount = sle->at(~sfMaximumAmount).value_or(maxMPTokenAmount);
auto const outstandingAmount = sle->getFieldU64(sfOutstandingAmount);
// actual accumulates the total cost to the sender (includes transfer
// fees for third-party transit sends). takeFromSender accumulates only
// the transit portion that is debited to the issuer in bulk after the
// loop. They diverge when there are transfer fees.
STAmount takeFromSender{mptIssue};
actual = takeFromSender;
for (auto const& r : receivers)
for (auto const& [receiverID, amt] : receivers)
{
auto const& receiverID = r.first;
STAmount amount{mptIssue, r.second};
STAmount const amount{mptIssue, amt};
if (amount < beast::zero)
{
return tecINTERNAL; // LCOV_EXCL_LINE
}
/* If we aren't sending anything or if the sender is the same as the
* receiver then we don't need to do anything.
*/
if (!amount || (senderID == receiverID))
if (!amount || senderID == receiverID)
continue;
if (senderID == issuer || receiverID == issuer)
{
// if sender is issuer, check that the new OutstandingAmount will
// not exceed MaximumAmount
if (senderID == issuer)
{
XRPL_ASSERT_PARTS(
takeFromSender == beast::zero,
"xrpl::rippleSendMultiMPT",
"sender == issuer, takeFromSender == zero");
auto const sendAmount = amount.mpt().value();
auto const maximumAmount = sle->at(~sfMaximumAmount).value_or(maxMPTokenAmount);
if (sendAmount > maximumAmount ||
sle->getFieldU64(sfOutstandingAmount) > maximumAmount - sendAmount)
return tecPATH_DRY;
if (view.rules().enabled(fixSecurity3_1_3))
{
// Post-fixSecurity3_1_3: aggregate MaximumAmount
// check. Each condition guards the subtraction
// in the next to prevent underflow.
auto const exceedsMaximumAmount =
// This send alone exceeds the max cap
sendAmount > maximumAmount ||
// The aggregate of all sends exceeds the max cap
totalSendAmount > maximumAmount - sendAmount ||
// Outstanding + aggregate exceeds the max cap
outstandingAmount > maximumAmount - sendAmount - totalSendAmount;
if (exceedsMaximumAmount)
return tecPATH_DRY;
totalSendAmount += sendAmount;
}
else
{
// Pre-fixSecurity3_1_3: per-iteration MaximumAmount
// check. Reads sfOutstandingAmount from a stale
// view.read() snapshot — incorrect for multi-destination
// sends but retained for ledger replay compatibility.
if (sendAmount > maximumAmount ||
outstandingAmount > maximumAmount - sendAmount)
return tecPATH_DRY;
}
}
// Direct send: redeeming MPTs and/or sending own MPTs.
if (auto const ter = rippleCreditMPT(view, senderID, receiverID, amount, j))
return ter;
actual += amount;
// Do not add amount to takeFromSender, because rippleCreditMPT took
// it
// Do not add amount to takeFromSender, because rippleCreditMPT
// took it.
continue;
}

View File

@@ -6,6 +6,7 @@
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/TER.h>
#include <xrpl/protocol/TxFlags.h>
@@ -3272,6 +3273,93 @@ class MPToken_test : public beast::unit_test::suite
mptAlice.claw(alice, bob, 1, tecNO_PERMISSION);
}
void
testMultiSendMaximumAmount(FeatureBitset features)
{
// Verify that rippleSendMultiMPT correctly enforces MaximumAmount
// when the issuer sends to multiple receivers. Pre-fixSecurity3_1_3,
// a stale view.read() snapshot caused per-iteration checks to miss
// aggregate overflows. Post-fix, a running total is used instead.
testcase("Multi-send MaximumAmount enforcement");
using namespace test::jtx;
Account const issuer("issuer");
Account const alice("alice");
Account const bob("bob");
std::uint64_t constexpr maxAmt = 150;
Env env{*this, features};
MPTTester mptt(env, issuer, {.holders = {alice, bob}});
mptt.create({.maxAmt = maxAmt, .ownerCount = 1, .flags = tfMPTCanTransfer});
mptt.authorize({.account = alice});
mptt.authorize({.account = bob});
Asset const asset{MPTIssue{mptt.issuanceID()}};
// Each test case creates a fresh ApplyView and calls
// accountSendMulti from the issuer to the given receivers.
auto const runTest = [&](MultiplePaymentDestinations const& receivers,
TER expectedTer,
std::optional<std::uint64_t> expectedOutstanding,
std::string const& label) {
ApplyViewImpl av(&*env.current(), tapNONE);
auto const ter =
accountSendMulti(av, issuer.id(), asset, receivers, env.app().journal("View"));
BEAST_EXPECTS(ter == expectedTer, label);
// Only verify OutstandingAmount on success — on error the
// view may contain partial state and must be discarded.
if (expectedOutstanding)
{
auto const sle = av.peek(keylet::mptIssuance(mptt.issuanceID()));
if (!BEAST_EXPECT(sle))
return;
BEAST_EXPECTS(sle->getFieldU64(sfOutstandingAmount) == *expectedOutstanding, label);
}
};
using R = MultiplePaymentDestinations;
// Post-amendment: aggregate check with running total
runTest(
R{{alice.id(), 100}, {bob.id(), 100}},
tecPATH_DRY,
std::nullopt,
"aggregate exceeds max");
runTest(R{{alice.id(), 75}, {bob.id(), 75}}, tesSUCCESS, maxAmt, "aggregate at boundary");
runTest(R{{alice.id(), 50}, {bob.id(), 50}}, tesSUCCESS, 100, "aggregate within limit");
runTest(
R{{alice.id(), 150}, {bob.id(), 0}},
tesSUCCESS,
maxAmt,
"one receiver at max, other zero");
runTest(
R{{alice.id(), 151}, {bob.id(), 0}},
tecPATH_DRY,
std::nullopt,
"one receiver exceeds max, other zero");
// Pre-amendment: the stale per-iteration check allows each
// individual send (100 <= 150) even though the aggregate (200)
// exceeds MaximumAmount. Preserved for ledger replay.
{
// KNOWN BUG (pre-fixSecurity3_1_3): preserved for ledger replay only
env.disableFeature(fixSecurity3_1_3);
env.close();
runTest(
R{{alice.id(), 100}, {bob.id(), 100}},
tesSUCCESS,
200,
"pre-amendment allows over-send");
}
}
public:
void
run() override
@@ -3279,6 +3367,7 @@ public:
using namespace test::jtx;
FeatureBitset const all{testable_amendments()};
testMultiSendMaximumAmount(all);
// MPTokenIssuanceCreate
testCreateValidation(all - featureSingleAssetVault);
testCreateValidation(all - featurePermissionedDomains);