diff --git a/include/xrpl/protocol/Permissions.h b/include/xrpl/protocol/Permissions.h index eb161ef7ad..c6f464082d 100644 --- a/include/xrpl/protocol/Permissions.h +++ b/include/xrpl/protocol/Permissions.h @@ -106,7 +106,7 @@ public: txToPermissionType(TxType type); // tx type value is permission value minus one - [[nodiscard]] static TxType + [[nodiscard]] static std::optional permissionToTxType(std::uint32_t value); /** diff --git a/src/libxrpl/protocol/Permissions.cpp b/src/libxrpl/protocol/Permissions.cpp index 3aa9705b03..80aa1b1f4e 100644 --- a/src/libxrpl/protocol/Permissions.cpp +++ b/src/libxrpl/protocol/Permissions.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -152,9 +153,11 @@ Permission::getPermissionName(std::uint32_t value) const return granular; // not a granular permission, check if it maps to a transaction type - auto const txType = permissionToTxType(value); - if (auto const* item = TxFormats::getInstance().findByType(txType); item != nullptr) - return item->getName(); + if (auto const txType = permissionToTxType(value)) + { + if (auto const* item = TxFormats::getInstance().findByType(*txType); item != nullptr) + return item->getName(); + } return std::nullopt; } @@ -231,7 +234,10 @@ Permission::isDelegable(std::uint32_t permissionValue, Rules const& rules) const } auto const txType = permissionToTxType(permissionValue); - auto const txIt = txDelegationMap_.find(txType); + if (!txType) + return false; + + auto const txIt = txDelegationMap_.find(*txType); // Tx-level permissions require the transaction type itself to be delegable, and // the corresponding amendment enabled. @@ -245,10 +251,14 @@ Permission::txToPermissionType(TxType const type) return static_cast(type) + 1; } -TxType +std::optional Permission::permissionToTxType(uint32_t value) { - XRPL_ASSERT(value > 0, "xrpl::Permission::permissionToTxType : value is greater than 0"); + // Values outside this range [1, 65536] would silently truncate when cast to + // uint16_t, for example, 65537 would become 1, mapping to the Payment transaction. + if (value == 0 || value > std::numeric_limits::max() + 1u) + return std::nullopt; + return static_cast(value - 1); } diff --git a/src/libxrpl/tx/transactors/payment/Payment.cpp b/src/libxrpl/tx/transactors/payment/Payment.cpp index 9a9a01ec19..7f1e4d8079 100644 --- a/src/libxrpl/tx/transactors/payment/Payment.cpp +++ b/src/libxrpl/tx/transactors/payment/Payment.cpp @@ -283,16 +283,59 @@ Payment::checkGranularSemantics( if (tx.isFieldPresent(sfSendMax) && tx[sfSendMax].asset() != amountAsset) return terNO_DELEGATE_PERMISSION; - // PaymentMint and PaymentBurn apply to both IOU and MPT direct payments. - if (heldGranularPermissions.contains(PaymentMint) && !isXRP(amountAsset) && - amountAsset.getIssuer() == tx[sfAccount]) - return tesSUCCESS; + if (isXRP(amountAsset)) + return terNO_DELEGATE_PERMISSION; - if (heldGranularPermissions.contains(PaymentBurn) && !isXRP(amountAsset) && - amountAsset.getIssuer() == tx[sfDestination]) - return tesSUCCESS; + return amountAsset.visit( + [&](MPTIssue const& mptIssue) -> NotTEC { + // For MPT payments, the MPTokenIssuanceID encodes the issuer unambiguously, + // unlike IOU, there is no endpoint aliasing where either side of the + // trustline can appear as the issuer. + if (heldGranularPermissions.contains(PaymentMint) && + mptIssue.getIssuer() == tx[sfAccount]) + return tesSUCCESS; + if (heldGranularPermissions.contains(PaymentBurn) && + mptIssue.getIssuer() == tx[sfDestination]) + return tesSUCCESS; + return terNO_DELEGATE_PERMISSION; + }, + [&](Issue const& issue) -> NotTEC { + // For IOU payments, either endpoint may be encoded as the issuer in + // sfAmount. PaySteps normalizes those endpoint aliases, so sfAmount.issuer + // alone does not reliably identify whether the transaction issues or redeems + // IOUs. We determine PaymentMint vs PaymentBurn from the trustline balance + // direction instead. + auto const account = tx[sfAccount]; + auto const destination = tx[sfDestination]; - return terNO_DELEGATE_PERMISSION; + // Reject if neither endpoint is the issuer. + if (issue.getIssuer() != account && issue.getIssuer() != destination) + return terNO_DELEGATE_PERMISSION; + + auto const sle = view.read(keylet::trustLine(account, destination, issue.currency)); + if (!sle) + return terNO_DELEGATE_PERMISSION; + + bool const accountIsLow = (account < destination); + auto const destLimit = sle->getFieldAmount(accountIsLow ? sfHighLimit : sfLowLimit); + auto const rawBalance = sle->getFieldAmount(sfBalance); + bool const accountIsHolder = + accountIsLow ? rawBalance > beast::kZero : rawBalance < beast::kZero; + + // PaymentMint requires the destination to be the holder and the account to be the + // issuer. destLimit > 0: destination is willing to hold account's IOUs (account is the + // issuer). !accountIsHolder: DirectStepI will issue, not redeem. + if (heldGranularPermissions.contains(PaymentMint) && destLimit > beast::kZero && + !accountIsHolder) + return tesSUCCESS; + + // PaymentBurn requires the source account to be the holder and the destination to be + // the issuer. accountIsHolder: DirectStepI will redeem, not issue. + if (heldGranularPermissions.contains(PaymentBurn) && accountIsHolder) + return tesSUCCESS; + + return terNO_DELEGATE_PERMISSION; + }); } TER diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index 6e80577797..f68f813853 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #include #include @@ -1105,10 +1106,43 @@ class Delegate_test : public beast::unit_test::Suite env(delegate::set(alice, bob, {"PaymentBurn"})); env.close(); - env(pay(alice, gw, usd(30)), Sendmax(usd(30)), delegate::As(bob)); + env(pay(alice, gw, usd(30)), delegate::As(bob)); env.require(Balance(alice, usd(20))); } + // PaymentBurn is authorized by balance direction, not trust limit. + // holder is allowed to burn even if trust limit is 0. + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + auto const gwUSD = gw["USD"]; + auto const aliceUSD = alice["USD"]; + + env.fund(XRP(10000), alice, bob, gw); + env.trust(gwUSD(200), alice); + env.close(); + + env(pay(gw, alice, gwUSD(50))); + env.require(Balance(alice, gwUSD(50))); + env.close(); + + env(delegate::set(alice, bob, {"PaymentBurn"})); + env.close(); + + env.trust(gwUSD(0), alice); + env.close(); + BEAST_EXPECT(env.limit(alice, gwUSD.issue()) == gwUSD(0)); + + env(trust(gw, aliceUSD(200))); + env.close(); + + env(pay(alice, gw, gwUSD(30)), delegate::As(bob)); + env.require(Balance(alice, gwUSD(20))); + env.require(Balance(gw, aliceUSD(-20))); + } + // Test invalid fields or flags not allowed in granular permission template { Env env(*this, features); @@ -1184,22 +1218,22 @@ class Delegate_test : public beast::unit_test::Suite mpt.authorize({.account = alice}); mpt.authorize({.account = bob}); - auto const MPT = mpt["MPT"]; // NOLINT(readability-identifier-naming) - env(pay(gw, alice, MPT(500))); - env(pay(gw, bob, MPT(500))); + auto const gwMPT = mpt["MPT"]; + env(pay(gw, alice, gwMPT(500))); + env(pay(gw, bob, gwMPT(500))); env.close(); - auto aliceMPT = env.balance(alice, MPT); - auto bobMPT = env.balance(bob, MPT); + auto aliceMPT = env.balance(alice, gwMPT); + auto bobMPT = env.balance(bob, gwMPT); // PaymentMint { env(delegate::set(gw, bob, {"PaymentMint"})); env.close(); - env(pay(gw, alice, MPT(50)), delegate::As(bob)); - BEAST_EXPECT(env.balance(alice, MPT) == aliceMPT + MPT(50)); - BEAST_EXPECT(env.balance(bob, MPT) == bobMPT); - aliceMPT = env.balance(alice, MPT); + env(pay(gw, alice, gwMPT(50)), delegate::As(bob)); + BEAST_EXPECT(env.balance(alice, gwMPT) == aliceMPT + gwMPT(50)); + BEAST_EXPECT(env.balance(bob, gwMPT) == bobMPT); + aliceMPT = env.balance(alice, gwMPT); } // PaymentBurn @@ -1207,26 +1241,235 @@ class Delegate_test : public beast::unit_test::Suite env(delegate::set(alice, bob, {"PaymentBurn"})); env.close(); - env(pay(alice, gw, MPT(50)), delegate::As(bob)); - BEAST_EXPECT(env.balance(alice, MPT) == aliceMPT - MPT(50)); - BEAST_EXPECT(env.balance(bob, MPT) == bobMPT); - aliceMPT = env.balance(alice, MPT); + env(pay(alice, gw, gwMPT(50)), delegate::As(bob)); + BEAST_EXPECT(env.balance(alice, gwMPT) == aliceMPT - gwMPT(50)); + BEAST_EXPECT(env.balance(bob, gwMPT) == bobMPT); + aliceMPT = env.balance(alice, gwMPT); } // Grant both granular permissions and tx level permission. { env(delegate::set(alice, bob, {"PaymentBurn", "PaymentMint", "Payment"})); env.close(); - env(pay(alice, gw, MPT(50)), delegate::As(bob)); - BEAST_EXPECT(env.balance(alice, MPT) == aliceMPT - MPT(50)); - BEAST_EXPECT(env.balance(bob, MPT) == bobMPT); - aliceMPT = env.balance(alice, MPT); - env(pay(alice, bob, MPT(100)), delegate::As(bob)); - BEAST_EXPECT(env.balance(alice, MPT) == aliceMPT - MPT(100)); - BEAST_EXPECT(env.balance(bob, MPT) == bobMPT + MPT(100)); + env(pay(alice, gw, gwMPT(50)), delegate::As(bob)); + BEAST_EXPECT(env.balance(alice, gwMPT) == aliceMPT - gwMPT(50)); + BEAST_EXPECT(env.balance(bob, gwMPT) == bobMPT); + aliceMPT = env.balance(alice, gwMPT); + env(pay(alice, bob, gwMPT(100)), delegate::As(bob)); + BEAST_EXPECT(env.balance(alice, gwMPT) == aliceMPT - gwMPT(100)); + BEAST_EXPECT(env.balance(bob, gwMPT) == bobMPT + gwMPT(100)); } } + // PaymentMint/PaymentBurn must not trust IOU issuer aliases. + // In a direct IOU payment, sfAmount.issuer may be encoded as either + // endpoint, and PaySteps normalizes those aliases to the same execution. + // These cases ensure a delegate cannot flip the encoded issuer to turn a + // mint into an apparent burn, or a burn into an apparent mint. + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + auto const gwUSD = gw["USD"]; + auto const aliceUSD = alice["USD"]; + + env.fund(XRP(10000), alice, bob, gw); + env.trust(gwUSD(200), alice); + env.close(); + + // Alice holds 100 USD issued by gw. + env(pay(gw, alice, gwUSD(100))); + env.close(); + env.require(Balance(alice, gwUSD(100))); + + // Delegate with only PaymentBurn tries to mint by encoding + // Amount.issuer as the destination alias, alice. The actual issuer + // is gw, so this requires PaymentMint and must be rejected. + { + env(delegate::set(gw, bob, {"PaymentBurn"})); + env.close(); + + // Amount.issuer = alice (destination), rejected because gw is + // the actual issuer and PaymentMint is required. + env(pay(gw, alice, aliceUSD(50)), + delegate::As(bob), + Ter(terNO_DELEGATE_PERMISSION)); + env.require(Balance(alice, gwUSD(100))); + + // Fails because bob holds PaymentBurn, not PaymentMint. + env(pay(gw, alice, gwUSD(50)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + env.require(Balance(alice, gwUSD(100))); + } + + // Delegate with only PaymentMint tries to burn by encoding + // Amount.issuer as the source alias, alice. The actual issuer is + // gw, so this requires PaymentBurn and must be rejected. + { + env(delegate::set(alice, bob, {"PaymentMint"})); + env.close(); + + // Amount.issuer = alice (account), rejected because gw is the + // actual issuer and PaymentBurn is required. + env(pay(alice, gw, aliceUSD(50)), + delegate::As(bob), + Ter(terNO_DELEGATE_PERMISSION)); + env.require(Balance(alice, gwUSD(100))); + + // Fails because bob holds PaymentMint, not PaymentBurn. + env(pay(alice, gw, gwUSD(50)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + env.require(Balance(alice, gwUSD(100))); + } + } + + // Neither account nor destination is issuer. + // PaymentMint and PaymentBurn do not authorize these payments. + { + // IOU + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + Account const gw2{"gateway2"}; + auto const gwUSD = gw["USD"]; + + env.fund(XRP(10000), alice, bob, gw, gw2); + env.close(); + + env.trust(gwUSD(200), alice); + env.close(); + + env(pay(gw, alice, gwUSD(100))); + env.close(); + + env(delegate::set(alice, bob, {"PaymentMint", "PaymentBurn"})); + env.close(); + + env(pay(alice, gw, gw2["USD"](50)), + delegate::As(bob), + Ter(terNO_DELEGATE_PERMISSION)); + } + + // MPT + { + Env env(*this, features); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + Account const gw2{"gateway2"}; + + env.fund(XRP(10000), gw2); + env.close(); + + MPTTester mpt(env, gw, {.holders = {alice, bob}}); + mpt.create({.ownerCount = 1, .flags = tfMPTCanTransfer}); + + mpt.authorize({.account = alice}); + mpt.authorize({.account = bob}); + + auto const gwMPT = mpt["MPT"]; + env(pay(gw, alice, gwMPT(500))); + env(pay(gw, bob, gwMPT(500))); + env.close(); + + env(delegate::set(alice, bob, {"PaymentMint", "PaymentBurn"})); + env.close(); + + env(pay(alice, gw2, gwMPT(50)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + } + } + + // IOU issuer is an endpoint, but no trustline exists. + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + + env.fund(XRP(10000), alice, bob, gw); + env.close(); + + env(delegate::set(alice, bob, {"PaymentMint", "PaymentBurn"})); + env.close(); + + env(pay(alice, gw, alice["USD"](50)), + delegate::As(bob), + Ter(terNO_DELEGATE_PERMISSION)); + } + + // Both trust limits (who is the designated issuer) and balance direction + // (which way DirectStepI executes) must be checked. Neither alone is sufficient. + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + auto const gwUSD = gw["USD"]; + auto const aliceUSD = alice["USD"]; + + env.fund(XRP(10000), alice, bob, gw); + + // Alice trusts gw but holds zero gw-issued USD. With balance == 0, + // DirectStepI would issue rather than redeem, so PaymentBurn must + // be rejected even though Alice's trust limit to gw is positive. + { + env.trust(gwUSD(200), alice); + env.close(); + + // Alice has nothing to burn. + env(delegate::set(alice, bob, {"PaymentBurn"})); + env.close(); + + env(pay(alice, gw, gwUSD(50)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + env(pay(alice, gw, aliceUSD(50)), + delegate::As(bob), + Ter(terNO_DELEGATE_PERMISSION)); + } + + // Set up a trust line where gw holds alice-issued USD. DirectStepI + // would redeem rather than issue, so PaymentMint must be rejected + // even though the endpoint identity matches. + { + // Gw sets trust to accept alice-issued USD. + env(trust(gw, aliceUSD(200))); + env.close(); + + // Alice issues her own USD to gw; now gw holds alice's IOUs. + env(pay(alice, gw, aliceUSD(100))); + env.close(); + + // In gw's view, accountHolds(gw, USD, alice) > 0, so DirectStepI redeems. + // PaymentMint must be rejected because the step would redeem, not issue. + env(delegate::set(gw, bob, {"PaymentMint"})); + env.close(); + + env(pay(gw, alice, gwUSD(50)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + env(pay(gw, alice, aliceUSD(50)), + delegate::As(bob), + Ter(terNO_DELEGATE_PERMISSION)); + } + } + + // Alice trusts gw but gw is not willing to hold alice's IOU (destLimit == 0). + { + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gateway"}; + env.fund(XRP(10000), alice, bob, gw); + env.trust(gw["USD"](200), alice); + env.close(); + + env(delegate::set(alice, bob, {"PaymentMint"})); + env.close(); + + env(pay(alice, gw, gw["USD"](50)), delegate::As(bob), Ter(terNO_DELEGATE_PERMISSION)); + env(pay(alice, gw, alice["USD"](50)), + delegate::As(bob), + Ter(terNO_DELEGATE_PERMISSION)); + } + // Verify granular permissions of different tx types in the same SLE are scoped // correctly. AccountSet permissions don't apply to Payment and vice versa { @@ -2617,6 +2860,38 @@ class Delegate_test : public beast::unit_test::Suite BEAST_EXPECT(granularPermissions.empty()); } + void + testPermissionToTxType() + { + testcase("test Permission to Tx type"); + + // 0 is not a valid permission value + BEAST_EXPECT(!Permission::permissionToTxType(0)); + + // 1 maps to Payment transaction + BEAST_EXPECT(Permission::permissionToTxType(1) == ttPAYMENT); + + // UINT16_MAX+1 is the maximum possible tx-level permission value + constexpr uint32_t maxTxPermission = std::numeric_limits::max() + 1u; + BEAST_EXPECT(Permission::permissionToTxType(maxTxPermission).has_value()); + + // exceeding maximum value should return nullopt + BEAST_EXPECT(!Permission::permissionToTxType(maxTxPermission + 1)); + + // All granular permission values should return nullopt since they do not map to a TxType. + for (auto const gp : { +#pragma push_macro("GRANULAR_PERMISSION") +#undef GRANULAR_PERMISSION +#define GRANULAR_PERMISSION(type, txType, value, ...) GranularPermissionType::type, +#include +#undef GRANULAR_PERMISSION +#pragma pop_macro("GRANULAR_PERMISSION") + }) + { + BEAST_EXPECT(!Permission::permissionToTxType(static_cast(gp))); + } + } + void run() override { @@ -2647,6 +2922,7 @@ class Delegate_test : public beast::unit_test::Suite testTxDelegableCount(); testNonDelegableTxWithDelegate(all); testDelegateUtilsNullptrCheck(); + testPermissionToTxType(); } }; BEAST_DEFINE_TESTSUITE(Delegate, app, xrpl);