diff --git a/include/xrpl/ledger/helpers/EscrowHelpers.h b/include/xrpl/ledger/helpers/EscrowHelpers.h index 9f54e537699..8e5050ef9f3 100644 --- a/include/xrpl/ledger/helpers/EscrowHelpers.h +++ b/include/xrpl/ledger/helpers/EscrowHelpers.h @@ -2,7 +2,9 @@ #include #include +#include #include +#include #include #include #include @@ -23,8 +25,52 @@ #include #include +#include + namespace xrpl { +// Deduct the transfer fee (if any) from an escrowed amount, returning the net +// amount delivered to the receiver. The transfer fee applies only when neither +// the sender nor the receiver is the issuer and the locked rate differs from +// parity. +// +// NOTE: Transfer fee in escrow works a bit differently from a normal payment. +// In escrow, the fee is deducted from the locked/sending amount, whereas in a +// normal payment, the transfer fee is taken on top of the sending amount. +// +// The net delivered amount is rounded down (fee rounded up in the issuer's +// favor). Rounding the net up (the legacy behavior) rounds the fee down, which +// for integral assets (e.g. MPT) collapses the fee to zero for small amounts +// and bypasses the transfer fee. Note that the legacy divideRound rounds to +// nearest, so the strict variant is used to floor the net. If the net rounds +// to zero the entire amount would be consumed by the fee, so tecPRECISION_LOSS +// is returned rather than silently delivering nothing to the receiver. +[[nodiscard]] inline std::expected +escrowDeductTransferFee( + ReadView const& view, + Rate const& lockedRate, + STAmount const& amount, + bool senderIssuer, + bool receiverIssuer) +{ + if ((senderIssuer || receiverIssuer) || lockedRate == kParityRate) + return amount; + + bool const strictRounding = view.rules().enabled(fixCleanup3_4_0); + auto const netAmt = strictRounding + ? divideRoundStrict(amount, lockedRate, amount.asset(), false) + : divideRound(amount, lockedRate, amount.asset(), true); + // compute transfer fee, if any + auto const xferFee = amount.value() - netAmt; + // compute balance to transfer + STAmount finalAmt = amount.value() - xferFee; + + if (strictRounding && finalAmt <= beast::kZero) + return std::unexpected(tecPRECISION_LOSS); + + return finalAmt; +} + template TER escrowUnlockApplyHelper( @@ -124,24 +170,12 @@ escrowUnlockApplyHelper( if (xferRate < lockedRate) lockedRate = xferRate; - // Transfer Rate only applies when: - // 1. Issuer is not involved in the transfer (senderIssuer or - // receiverIssuer) - // 2. The locked rate is different from the parity rate - - // NOTE: Transfer fee in escrow works a bit differently from a normal - // payment. In escrow, the fee is deducted from the locked/sending amount, - // whereas in a normal payment, the transfer fee is taken on top of the - // sending amount. - auto finalAmt = amount; - if ((!senderIssuer && !receiverIssuer) && lockedRate != kParityRate) - { - // compute transfer fee, if any - auto const xferFee = - amount.value() - divideRound(amount, lockedRate, amount.get(), true); - // compute balance to transfer - finalAmt = amount.value() - xferFee; - } + // Deduct the transfer fee (if any) from the escrowed amount. + auto const netResult = + escrowDeductTransferFee(ctx.view, lockedRate, amount, senderIssuer, receiverIssuer); + if (!netResult) + return netResult.error(); + auto const finalAmt = *netResult; // validate the line limit if the account submitting txn is not the receiver // of the funds @@ -229,23 +263,13 @@ escrowUnlockApplyHelper( if (xferRate < lockedRate) lockedRate = xferRate; - // Transfer Rate only applies when: - // 1. Issuer is not involved in the transfer (senderIssuer or - // receiverIssuer) - // 2. The locked rate is different from the parity rate - - // NOTE: Transfer fee in escrow works a bit differently from a normal - // payment. In escrow, the fee is deducted from the locked/sending amount, - // whereas in a normal payment, the transfer fee is taken on top of the - // sending amount. - auto finalAmt = amount; - if ((!senderIssuer && !receiverIssuer) && lockedRate != kParityRate) - { - // compute transfer fee, if any - auto const xferFee = amount.value() - divideRound(amount, lockedRate, amount.asset(), true); - // compute balance to transfer - finalAmt = amount.value() - xferFee; - } + // Deduct the transfer fee (if any) from the escrowed amount. + auto const netResult = + escrowDeductTransferFee(ctx.view, lockedRate, amount, senderIssuer, receiverIssuer); + if (!netResult) + return netResult.error(); + auto const finalAmt = *netResult; + return unlockEscrowMPT( ctx.view, sender, diff --git a/include/xrpl/protocol/Rate.h b/include/xrpl/protocol/Rate.h index 048787cab55..32290fdc7ee 100644 --- a/include/xrpl/protocol/Rate.h +++ b/include/xrpl/protocol/Rate.h @@ -65,6 +65,9 @@ divideRound(STAmount const& amount, Rate const& rate, bool roundUp); STAmount divideRound(STAmount const& amount, Rate const& rate, Asset const& asset, bool roundUp); +STAmount +divideRoundStrict(STAmount const& amount, Rate const& rate, Asset const& asset, bool roundUp); + namespace nft { /** * Given a transfer fee (in basis points) convert it to a transfer rate. diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index bfe03a63037..4f1fac82da9 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,6 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +XRPL_FIX (Cleanup3_4_0, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(Sponsor, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(BatchV1_1, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocolV1_1, Supported::No, VoteBehavior::DefaultNo) diff --git a/src/libxrpl/protocol/Rate2.cpp b/src/libxrpl/protocol/Rate2.cpp index fd56f83bf5c..b5354f646ed 100644 --- a/src/libxrpl/protocol/Rate2.cpp +++ b/src/libxrpl/protocol/Rate2.cpp @@ -98,4 +98,15 @@ divideRound(STAmount const& amount, Rate const& rate, Asset const& asset, bool r return divRound(amount, detail::asAmount(rate), asset, roundUp); } +STAmount +divideRoundStrict(STAmount const& amount, Rate const& rate, Asset const& asset, bool roundUp) +{ + XRPL_ASSERT(rate.value, "xrpl::nft::divideRoundStrict(Issue) : nonzero rate input"); + + if (rate == kParityRate) + return amount; + + return divRoundStrict(amount, detail::asAmount(rate), asset, roundUp); +} + } // namespace xrpl diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index 4015f5ddc8a..07c231b3054 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/src/test/app/EscrowToken_test.cpp @@ -1760,6 +1760,67 @@ struct EscrowToken_test : public beast::unit_test::Suite } } + void + testIOUTransferFeePrecisionLoss(FeatureBitset features) + { + testcase("IOU Transfer Fee Precision Loss"); + using namespace test::jtx; + using namespace std::literals; + + bool const withV2 = features[fixCleanup3_4_0]; + + // Escrow the smallest positive IOU amount with a non-zero transfer + // rate, then finish it. The net delivered amount underflows to zero. + // The legacy behavior rounds the net up to the smallest positive + // value, delivering the full amount with no fee. With fixCleanup3_4_0 + // the finish fails with tecPRECISION_LOSS instead. + Env env{*this, features}; + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account{"gateway"}; + auto const usd = gw["USD"]; + env.fund(XRP(10'000), alice, bob, gw); + env(fset(gw, asfAllowTrustLineLocking)); + env(rate(gw, 1.25)); + env.close(); + env.trust(usd(100), alice); + env.trust(usd(100), bob); + env.close(); + + // the smallest positive IOU amount + auto const tiny = usd(kEpsilon); + env(pay(gw, alice, tiny)); + env.close(); + + auto const preAlice = env.balance(alice, usd); + auto const preBob = env.balance(bob, usd); + auto const seq1 = env.seq(alice); + env(escrow::create(alice, bob, tiny), + escrow::kFinishTime(env.now() + 1s), + Fee(baseFee * 150)); + env.close(); + + if (withV2) + { + // The net delivered rounds to zero; the finish is rejected and + // the escrowed amount remains locked. + env(escrow::finish(bob, alice, seq1), Fee(baseFee * 150), Ter(tecPRECISION_LOSS)); + env.close(); + BEAST_EXPECT(env.balance(alice, usd) == preAlice - tiny); + BEAST_EXPECT(env.balance(bob, usd) == preBob); + } + else + { + // Legacy behavior: the net rounds up to the smallest positive + // value, so bob receives the full amount with no fee. + env(escrow::finish(bob, alice, seq1), Fee(baseFee * 150)); + env.close(); + BEAST_EXPECT(env.balance(alice, usd) == preAlice - tiny); + BEAST_EXPECT(env.balance(bob, usd) == preBob + tiny); + } + } + void testIOULimitAmount(FeatureBitset features) { @@ -3678,6 +3739,63 @@ struct EscrowToken_test : public beast::unit_test::Suite BEAST_EXPECT(issuerMPTEscrowed(env, mpt) == 0); BEAST_EXPECT(env.balance(gw, mpt) == mpt(-19'875)); } + + // test locked rate: issuer lowers the transfer fee after creation + if (features[featureDynamicMPT]) + { + Env env{*this, features}; + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account("gw"); + + MPTTester mptGw(env, gw, {.holders = {alice, bob}}); + mptGw.create( + {.transferFee = 25000, + .ownerCount = 1, + .holderCount = 0, + .flags = tfMPTCanEscrow | tfMPTCanTransfer, + .mutableFlags = tmfMPTCanMutateTransferFee}); + mptGw.authorize({.account = alice}); + mptGw.authorize({.account = bob}); + auto const mpt = mptGw["MPT"]; + env(pay(gw, alice, mpt(10'000))); + env(pay(gw, bob, mpt(10'000))); + env.close(); + + auto const preAlice = env.balance(alice, mpt); + auto const preBob = env.balance(bob, mpt); + auto const preOutstanding = env.balance(gw, mpt); + + // alice can create escrow w/ 25% xfer rate locked in + auto const seq1 = env.seq(alice); + env(escrow::create(alice, bob, mpt(110)), + escrow::kCondition(escrow::kCb1), + escrow::kFinishTime(env.now() + 1s), + Fee(baseFee * 150)); + env.close(); + auto const transferRate = escrow::rate(env, alice, seq1); + BEAST_EXPECT(transferRate.value == std::uint32_t(1'000'000'000 * 1.25)); + + // issuer lowers the transfer fee to 10% + mptGw.set({.transferFee = 10000}); + env.close(); + + // bob can finish escrow - the lower rate applies: + // net 110 / 1.10 = 100, fee 10 + env(escrow::finish(bob, alice, seq1), + escrow::kCondition(escrow::kCb1), + escrow::kFulfillment(escrow::kFb1), + Fee(baseFee * 150)); + env.close(); + + // The fee is only burned from OutstandingAmount when the gross vs. + // net accounting fix (fixTokenEscrowV1) is active. + std::uint64_t const burn = features[fixTokenEscrowV1] ? 10 : 0; + BEAST_EXPECT(env.balance(alice, mpt) == preAlice - mpt(110)); + BEAST_EXPECT(env.balance(bob, mpt) == preBob + mpt(100)); + BEAST_EXPECT(env.balance(gw, mpt) == preOutstanding + mpt(burn)); + } } void @@ -3723,6 +3841,138 @@ struct EscrowToken_test : public beast::unit_test::Suite env.close(); } + void + testMPTTransferFeeRoundingBypass(FeatureBitset features) + { + testcase("MPT Transfer Fee Rounding Bypass"); + using namespace test::jtx; + using namespace std::literals; + + bool const withV1 = features[fixTokenEscrowV1]; + bool const withV2 = features[fixCleanup3_4_0]; + + // Escrow a small MPT amount with a non-zero transfer fee, then finish + // it. Before fixCleanup3_4_0, the net delivered amount is rounded up + // which rounds the fee down to zero for small amounts, letting a sender + // deliver the full amount to the receiver while the issuer collects no + // fee. With the fix the fee is rounded up in the issuer's favor. + auto testChunk = [&](std::uint16_t transferFee, + std::uint64_t amount, + std::uint64_t feeWithoutV2, + std::uint64_t feeWithV2) { + Env env{*this, features}; + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account("gw"); + + MPTTester mptGw(env, gw, {.holders = {alice, bob}}); + mptGw.create( + {.transferFee = transferFee, + .ownerCount = 1, + .holderCount = 0, + .flags = tfMPTCanEscrow | tfMPTCanTransfer}); + mptGw.authorize({.account = alice}); + mptGw.authorize({.account = bob}); + auto const mpt = mptGw["MPT"]; + env(pay(gw, alice, mpt(10'000))); + env(pay(gw, bob, mpt(10'000))); + env.close(); + + auto const preAlice = env.balance(alice, mpt); + auto const preBob = env.balance(bob, mpt); + auto const preOutstanding = env.balance(gw, mpt); + + auto const seq1 = env.seq(alice); + env(escrow::create(alice, bob, mpt(amount)), + escrow::kFinishTime(env.now() + 1s), + Fee(baseFee * 150)); + env.close(); + + env(escrow::finish(bob, alice, seq1), Fee(baseFee * 150)); + env.close(); + + std::uint64_t const fee = withV2 ? feeWithV2 : feeWithoutV2; + std::uint64_t const net = amount - fee; + // The fee is only burned from OutstandingAmount when the gross vs. + // net accounting fix (fixTokenEscrowV1) is active. + std::uint64_t const burn = withV1 ? fee : 0; + + BEAST_EXPECT(env.balance(alice, mpt) == preAlice - mpt(amount)); + BEAST_EXPECT(env.balance(bob, mpt) == preBob + mpt(net)); + BEAST_EXPECT(env.balance(gw, mpt) == preOutstanding + mpt(burn)); + }; + + // 25% fee, MPT(2): net 2/1.25 = 1.6. The bug (round up) delivers 2 with + // zero fee; rounding to nearest would also deliver 2 (fee 0); only + // flooring the net charges a fee (net 1, fee 1). + testChunk(25000, 2, 0, 1); + // 25% fee, MPT(4): bug delivers 4 with zero fee; fix charges fee 1. + testChunk(25000, 4, 0, 1); + // 1% fee, MPT(90): max zero-fee chunk under the bug; fix charges fee 1. + testChunk(1000, 90, 0, 1); + // 1% fee, MPT(91): boundary just above the window; fee 1 either way. + testChunk(1000, 91, 1, 1); + + // When the escrowed amount is so small that the net delivered rounds to + // zero (the entire amount would be consumed by the fee), the legacy + // behavior silently delivers the full amount to the receiver with no + // fee. With fixCleanup3_4_0 the finish fails with tecPRECISION_LOSS + // instead of delivering nothing to the receiver. + { + std::uint16_t const transferFee = 25000; // 25% + std::uint64_t const amount = 1; + + Env env{*this, features}; + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account("gw"); + + MPTTester mptGw(env, gw, {.holders = {alice, bob}}); + mptGw.create( + {.transferFee = transferFee, + .ownerCount = 1, + .holderCount = 0, + .flags = tfMPTCanEscrow | tfMPTCanTransfer}); + mptGw.authorize({.account = alice}); + mptGw.authorize({.account = bob}); + auto const mpt = mptGw["MPT"]; + env(pay(gw, alice, mpt(10'000))); + env(pay(gw, bob, mpt(10'000))); + env.close(); + + auto const preAlice = env.balance(alice, mpt); + auto const preBob = env.balance(bob, mpt); + + auto const seq1 = env.seq(alice); + env(escrow::create(alice, bob, mpt(amount)), + escrow::kFinishTime(env.now() + 1s), + Fee(baseFee * 150)); + env.close(); + + if (withV2) + { + // The net delivered rounds to zero; the finish is rejected and + // the escrowed amount remains locked (alice down by amount, + // bob unchanged). + env(escrow::finish(bob, alice, seq1), Fee(baseFee * 150), Ter(tecPRECISION_LOSS)); + env.close(); + BEAST_EXPECT(env.balance(alice, mpt) == preAlice - mpt(amount)); + BEAST_EXPECT(env.balance(bob, mpt) == preBob); + } + else + { + // Legacy behavior: the net rounds up to the full amount with a + // zero fee, so bob receives everything. + env(escrow::finish(bob, alice, seq1), Fee(baseFee * 150)); + env.close(); + BEAST_EXPECT(env.balance(alice, mpt) == preAlice - mpt(amount)); + BEAST_EXPECT(env.balance(bob, mpt) == preBob + mpt(amount)); + } + } + } + void testMPTLock(FeatureBitset features) { @@ -3958,6 +4208,7 @@ struct EscrowToken_test : public beast::unit_test::Suite testIOURippleState(features); testIOUGateway(features); testIOULockedRate(features); + testIOUTransferFeePrecisionLoss(features); testIOULimitAmount(features); testIOURequireAuth(features); testIOUFreeze(features); @@ -3978,6 +4229,7 @@ struct EscrowToken_test : public beast::unit_test::Suite testMPTMetaAndOwnership(features); testMPTGateway(features); testMPTLockedRate(features); + testMPTTransferFeeRoundingBypass(features); testMPTRequireAuth(features); testMPTLock(features); testMPTCanTransfer(features); @@ -3997,6 +4249,8 @@ struct EscrowToken_test : public beast::unit_test::Suite testIOUWithFeats(feats - fixCleanup3_2_0); testMPTWithFeats(feats); testMPTWithFeats(feats - fixTokenEscrowV1); + testMPTTransferFeeRoundingBypass(feats - fixCleanup3_4_0); + testIOUTransferFeePrecisionLoss(feats - fixCleanup3_4_0); } } }; diff --git a/src/test/protocol/STAmount_test.cpp b/src/test/protocol/STAmount_test.cpp index f6c5a947524..b085b641e4f 100644 --- a/src/test/protocol/STAmount_test.cpp +++ b/src/test/protocol/STAmount_test.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -1252,6 +1253,68 @@ class STAmount_test : public beast::unit_test::Suite //-------------------------------------------------------------------------- + void + testDivideRoundStrict() + { + testcase("divideRoundStrict"); + + // Transfer-fee rates: 1.25 == 25% fee, 1.01 == 1% fee. + Rate const rate25{1'250'000'000}; + Rate const rate1{1'010'000'000}; + + MPTIssue const mpt{makeMptID(1, AccountID(0x4985601))}; + + // Parity rate is a no-op for both variants. + { + STAmount const amt{mpt, 8}; + BEAST_EXPECT(divideRound(amt, kParityRate, amt.asset(), true) == amt); + BEAST_EXPECT(divideRoundStrict(amt, kParityRate, amt.asset(), false) == amt); + } + + // For integral (MPT) amounts, the non-strict divideRound rounds the + // quotient to nearest, while divideRoundStrict floors it (toward zero) + // when roundUp is false. The escrow transfer-fee fix relies on this so + // the fee cannot collapse to zero for small amounts. + auto mptRound = [&](std::uint64_t v, Rate const& r, bool roundUp) { + STAmount const a{mpt, v}; + return divideRound(a, r, a.asset(), roundUp).mpt().value(); + }; + auto mptStrict = [&](std::uint64_t v, Rate const& r, bool roundUp) { + STAmount const a{mpt, v}; + return divideRoundStrict(a, r, a.asset(), roundUp).mpt().value(); + }; + + // 25% fee (rate 1.25): + // 1 / 1.25 = 0.8 -> nearest 1, floor 0 + // 2 / 1.25 = 1.6 -> nearest 2, floor 1 + // 4 / 1.25 = 3.2 -> nearest 3, floor 3 + // 6 / 1.25 = 4.8 -> nearest 5, floor 4 + BEAST_EXPECT(mptRound(1, rate25, false) == 1); + BEAST_EXPECT(mptStrict(1, rate25, false) == 0); + BEAST_EXPECT(mptRound(2, rate25, false) == 2); + BEAST_EXPECT(mptStrict(2, rate25, false) == 1); + BEAST_EXPECT(mptRound(4, rate25, false) == 3); + BEAST_EXPECT(mptStrict(4, rate25, false) == 3); + BEAST_EXPECT(mptRound(6, rate25, false) == 5); + BEAST_EXPECT(mptStrict(6, rate25, false) == 4); + + // roundUp=true rounds the quotient up: 3.2 -> 4. + BEAST_EXPECT(mptStrict(4, rate25, true) == 4); + + // 1% fee (rate 1.01): floor of a value just above an integer. + // 90 / 1.01 = 89.10 -> floor 89 + // 91 / 1.01 = 90.09 -> floor 90 + BEAST_EXPECT(mptStrict(90, rate1, false) == 89); + BEAST_EXPECT(mptStrict(91, rate1, false) == 90); + + // IOU amounts: an exact division is unaffected by the rounding mode. + { + STAmount const iou{noIssue(), 4}; + STAmount const expected{noIssue(), 32, -1}; // 3.2 + BEAST_EXPECT(divideRoundStrict(iou, rate25, iou.asset(), false) == expected); + } + } + void run() override { @@ -1261,6 +1324,7 @@ class STAmount_test : public beast::unit_test::Suite testArithmetic(); testUnderflow(); testRounding(); + testDivideRoundStrict(); testParseJson(); testConvertXRP(); testConvertIOU();