Compare commits

...

1 Commits

Author SHA1 Message Date
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
3 changed files with 123 additions and 20 deletions

View File

@@ -16,6 +16,7 @@
// Add new amendments to the top of this list.
// Keep it sorted in reverse chronological order.
XRPL_FIX (AssortedFixes, Supported::yes, VoteBehavior::DefaultNo) // PLACEHOLDER
XRPL_FIX (PermissionedDomainInvariant, Supported::yes, VoteBehavior::DefaultNo)
XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo)
XRPL_FIX (BatchInnerSigs, Supported::no, VoteBehavior::DefaultNo)

View File

@@ -2507,57 +2507,74 @@ 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,
"rippler::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(fixAssortedFixes))
{
// Post-fixAssortedFixes: aggregate MaximumAmount check
// using a running total.
totalSendAmount += sendAmount;
if (sendAmount > maximumAmount ||
outstandingAmount + totalSendAmount > maximumAmount)
return tecPATH_DRY;
}
else
{
// Pre-fixAssortedFixes: 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 + sendAmount > maximumAmount)
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

@@ -3272,6 +3272,90 @@ 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-fixAssortedFixes,
// 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.
env.disableFeature(fixAssortedFixes);
env.close();
runTest(
R{{alice.id(), 100}, {bob.id(), 100}},
tesSUCCESS,
200,
"pre-amendment allows over-send");
}
public:
void
run() override
@@ -3279,6 +3363,7 @@ public:
using namespace test::jtx;
FeatureBitset const all{testable_amendments()};
testMultiSendMaximumAmount(all);
// MPTokenIssuanceCreate
testCreateValidation(all - featureSingleAssetVault);
testCreateValidation(all - featurePermissionedDomains);