Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 51 additions & 21 deletions src/libxrpl/ledger/helpers/TokenHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1155,57 +1155,87 @@
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.
// Use uint64_t, not STAmount, to keep MaximumAmount comparisons in exact
// integer arithmetic. STAmount implicitly converts to Number, whose
// small-scale mantissa (~16 digits) can lose precision for values near
// maxMPTokenAmount (19 digits).
std::uint64_t totalSendAmount{0};
std::uint64_t const maximumAmount = sle->at(~sfMaximumAmount).value_or(maxMPTokenAmount);
std::uint64_t 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 const amount{mptIssue, r.second};
STAmount const amount{mptIssue, amt};
Comment thread
shawnxie999 marked this conversation as resolved.

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,
Comment thread
Tapanito marked this conversation as resolved.
Comment thread
Tapanito marked this conversation as resolved.
"xrpl::rippleSendMultiMPT",
Comment thread
Tapanito marked this conversation as resolved.
"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;

std::uint64_t const sendAmount = amount.mpt().value();

if (view.rules().enabled(fixSecurity3_1_3))
{
// Post-fixSecurity3_1_3: aggregate MaximumAmount
// check. WARNING: the order of conditions is
// critical — each guards the subtraction in the
// next against unsigned underflow. Do not reorder.
bool const exceedsMaximumAmount =
// This send alone exceeds the max cap
sendAmount > maximumAmount ||
Comment thread
Tapanito marked this conversation as resolved.
// The aggregate of all sends exceeds the max cap
totalSendAmount > maximumAmount - sendAmount ||
Comment thread
Tapanito marked this conversation as resolved.
// Outstanding + aggregate exceeds the max cap
outstandingAmount > maximumAmount - sendAmount - totalSendAmount;
Comment thread
Tapanito marked this conversation as resolved.

if (exceedsMaximumAmount)
return tecPATH_DRY;
totalSendAmount += sendAmount;
Comment thread
Tapanito marked this conversation as resolved.
}
else
Comment thread
Tapanito marked this conversation as resolved.
{
// 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;

Check warning on line 1229 in src/libxrpl/ledger/helpers/TokenHelpers.cpp

View check run for this annotation

Codecov / codecov/patch

src/libxrpl/ledger/helpers/TokenHelpers.cpp#L1229

Added line #L1229 was not covered by tests
Comment thread
Tapanito marked this conversation as resolved.
}
}

// 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;
}
Expand Down
119 changes: 119 additions & 0 deletions src/test/app/MPToken_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -3272,13 +3273,131 @@ 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().getJournal("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");

// Issue 50 tokens so outstandingAmount is nonzero, then verify
// the third condition: outstandingAmount > maximumAmount - sendAmount - totalSendAmount
mptt.pay(issuer, alice, 50);
env.close();

// maxAmt=150, outstanding=50, so 100 more available
runTest(
R{{alice.id(), 50}, {bob.id(), 50}},
tesSUCCESS,
maxAmt,
"nonzero outstanding, aggregate at boundary");

runTest(
R{{alice.id(), 50}, {bob.id(), 51}},
tecPATH_DRY,
std::nullopt,
"nonzero outstanding, aggregate exceeds max");

runTest(
R{{alice.id(), 100}, {bob.id(), 0}},
tesSUCCESS,
maxAmt,
"nonzero outstanding, single send at remaining capacity");

runTest(
R{{alice.id(), 101}, {bob.id(), 0}},
tecPATH_DRY,
std::nullopt,
"nonzero outstanding, single send exceeds remaining capacity");

// 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);
Comment thread
Tapanito marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutating the shared env feature set is fragile — isolate the pre-amendment test in its own Env to prevent amendment-disabled state from potentially affecting later tests:

Comment thread
Tapanito marked this conversation as resolved.
runTest(
Comment thread
Tapanito marked this conversation as resolved.
R{{alice.id(), 100}, {bob.id(), 100}},
tesSUCCESS,
250,
"pre-amendment allows over-send");
env.enableFeature(fixSecurity3_1_3);
}
}

public:
void
run() override
{
using namespace test::jtx;
FeatureBitset const all{testable_amendments()};

testMultiSendMaximumAmount(all);
// MPTokenIssuanceCreate
testCreateValidation(all - featureSingleAssetVault);
testCreateValidation(all - featurePermissionedDomains);
Expand Down
Loading