diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index d797d17fea..7750bf2248 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2669,51 +2669,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. + // 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 amount{mptIssue, r.second}; + STAmount 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; + + 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 || + // 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. @@ -2721,8 +2752,8 @@ rippleSendMultiMPT( 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; } diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 2b42e76645..2870248af8 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -3625,6 +3625,139 @@ 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 + 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"); + + // 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); + runTest( + R{{alice.id(), 100}, {bob.id(), 100}}, + tesSUCCESS, + 250, + "pre-amendment allows over-send"); + env.enableFeature(fixSecurity3_1_3); + } + } + public: void run() override @@ -3632,6 +3765,7 @@ public: using namespace test::jtx; FeatureBitset const all{testable_amendments()}; + testMultiSendMaximumAmount(all); // MPTokenIssuanceCreate testCreateValidation(all - featureSingleAssetVault); testCreateValidation(all - featurePermissionedDomains);