From a80a8cd1dec3b473056ff52f2fbdb65e61bbc123 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Mon, 5 Aug 2024 19:39:06 -0400 Subject: [PATCH] Add rules guards in doSubmit() if transaction is not signed. Fix Amount parsing if transaction is not signed. Extend unit-tests. Consolidate errors. Remove redundant checks. --- src/libxrpl/protocol/STEitherAmount.cpp | 12 ++- src/libxrpl/protocol/STObject.cpp | 6 +- src/test/app/MPToken_test.cpp | 97 ++++++++++++++++++++++++ src/xrpld/app/tx/detail/Transactor.cpp | 2 +- src/xrpld/rpc/detail/TransactionSign.cpp | 9 ++- src/xrpld/rpc/handlers/Submit.cpp | 2 + 6 files changed, 119 insertions(+), 9 deletions(-) diff --git a/src/libxrpl/protocol/STEitherAmount.cpp b/src/libxrpl/protocol/STEitherAmount.cpp index eac706a8729..a060d434ff6 100644 --- a/src/libxrpl/protocol/STEitherAmount.cpp +++ b/src/libxrpl/protocol/STEitherAmount.cpp @@ -18,6 +18,8 @@ //============================================================================== #include +#include +#include #include #include #include @@ -244,10 +246,14 @@ amountFromJson(SField const& name, Json::Value const& v) value = v[jss::value]; if (v.isMember(jss::mpt_issuance_id)) { - isMPT = true; - currencyOrMPTID = v[jss::mpt_issuance_id]; + std::optional const& rules = getCurrentTransactionRules(); + if (!rules || (rules && rules->enabled(featureMPTokensV1))) + { + isMPT = true; + currencyOrMPTID = v[jss::mpt_issuance_id]; + } } - else + if (!isMPT) { currencyOrMPTID = v[jss::currency]; issuer = v[jss::issuer]; diff --git a/src/libxrpl/protocol/STObject.cpp b/src/libxrpl/protocol/STObject.cpp index 6542424e372..a5eb0e6f45b 100644 --- a/src/libxrpl/protocol/STObject.cpp +++ b/src/libxrpl/protocol/STObject.cpp @@ -170,7 +170,11 @@ STObject::applyTemplate(const SOTemplate& type) if (auto const v = dynamic_cast(&iter->get()); v && v->isMPT()) { - throwFieldErr(e.sField().fieldName, "doesn't support MPT"); + std::optional const& rules = + getCurrentTransactionRules(); + if (!rules || rules->enabled(featureMPTokensV1)) + throwFieldErr( + e.sField().fieldName, "doesn't support MPT"); } } v.emplace_back(std::move(*iter)); diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 94cbe5a616b..163ce83f897 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -897,6 +897,54 @@ class MPToken_test : public beast::unit_test::suite delivermin(mptAlice.mpt(100)), ter(temMALFORMED)); } + + // Unsigned request + { + Env env{*this, features}; + Account const alice("alice"); + Account const carol("carol"); + + MPTTester mptAlice(env, alice, {.holders = {&bob, &carol}}); + + mptAlice.create({.ownerCount = 1, .holderCount = 0}); + + mptAlice.authorize({.account = &carol}); + + Json::Value payment; + payment[jss::secret] = alice.name(); + payment[jss::tx_json] = pay(alice, carol, mptAlice.mpt(100)); + payment[jss::tx_json][jss::SendMax] = + mptAlice.mpt(100).getJson(JsonOptions::none); + payment[jss::tx_json][jss::Fee] = + to_string(env.current()->fees().base); + auto jrr = env.rpc("json", "submit", to_string(payment)); + BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::result][jss::error_message] == + "Field 'SendMax' doesn't support MPT"); + + payment[jss::tx_json].removeMember(jss::SendMax); + payment[jss::tx_json][jss::DeliverMin] = + mptAlice.mpt(100).getJson(JsonOptions::none); + jrr = env.rpc("json", "submit", to_string(payment)); + BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::result][jss::error_message] == + "Field 'DeliverMin' doesn't support MPT"); + + payment[jss::tx_json].removeMember(jss::DeliverMin); + payment[jss::build_path] = true; + jrr = env.rpc("json", "submit", to_string(payment)); + BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::result][jss::error_message] == + "Field 'build_path' not allowed in this context."); + + payment.removeMember(jss::build_path); + jrr = env.rpc("json", "submit", to_string(payment)); + BEAST_EXPECT(jrr[jss::result][jss::status] == "success"); + BEAST_EXPECT(jrr[jss::result][jss::engine_result] == "tesSUCCESS"); + } } void @@ -919,6 +967,34 @@ class MPToken_test : public beast::unit_test::suite BEAST_EXPECT(expectOffers(env, alice, 0)); } + // Unsigned request + { + Env env{*this, features}; + Account const alice("alice"); // issuer + auto const USD = alice["USD"]; + + MPTTester mptAlice(env, alice); + + mptAlice.create(); + + Json::Value jv; + jv[jss::secret] = alice.name(); + jv[jss::tx_json] = offer(alice, USD(100), mptAlice.mpt(100)); + jv[jss::tx_json][jss::Fee] = to_string(env.current()->fees().base); + auto jrr = env.rpc("json", "submit", to_string(jv)); + BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::result][jss::error_message] == + "Field 'TakerGets' doesn't support MPT"); + + jv[jss::tx_json] = offer(alice, mptAlice.mpt(100), USD(100)); + jrr = env.rpc("json", "submit", to_string(jv)); + BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::result][jss::error_message] == + "Field 'TakerPays' doesn't support MPT"); + } + { Env env{*this, features - featureMPTokensV1}; Account const alice("alice"); @@ -932,6 +1008,27 @@ class MPToken_test : public beast::unit_test::suite BEAST_EXPECT(expectOffers(env, alice, 0)); } + + // Unsigned request, MPT amendment not enabled + { + Env env{*this, features - featureMPTokensV1}; + Account const alice("alice"); // issuer + auto const USD = alice["USD"]; + + env.fund(XRP(1'000), alice); + STMPTAmount mpt{ + MPTIssue{std::make_pair(1, alice.id())}, UINT64_C(100)}; + + Json::Value jv; + jv[jss::secret] = alice.name(); + jv[jss::tx_json] = offer(alice, USD(100), mpt); + jv[jss::tx_json][jss::Fee] = to_string(env.current()->fees().base); + auto jrr = env.rpc("json", "submit", to_string(jv)); + BEAST_EXPECT(jrr[jss::result][jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::result][jss::error_message] == + "Field 'tx_json.TakerGets' has invalid data."); + } } void diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 42e9f0677ab..bf52a66debe 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -835,7 +835,7 @@ Transactor::operator()() // fixSTAmountCanonicalize predate the rulesGuard and should be replaced. STAmountSO stAmountSO{view().rules().enabled(fixSTAmountCanonicalize)}; NumberSO stNumberSO{view().rules().enabled(fixUniversalNumber)}; - CurrentTransactionRulesGuard currentTransctionRulesGuard(view().rules()); + CurrentTransactionRulesGuard currentTransactionRulesGuard(view().rules()); #ifdef DEBUG { diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 1fee84c683b..77938b4924a 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -200,7 +200,7 @@ checkPayment( if (!tx_json.isMember(jss::Amount)) return RPC::missing_field_error("tx_json.Amount"); - STAmount amount; + STEitherAmount amount; if (!amountFromJsonNoThrow(amount, tx_json[jss::Amount])) return RPC::invalid_field_error("tx_json.Amount"); @@ -213,7 +213,8 @@ checkPayment( if (!dstAccountID) return RPC::invalid_field_error("tx_json.Destination"); - if ((doPath == false) && params.isMember(jss::build_path)) + if (((doPath == false) && params.isMember(jss::build_path)) || + (params.isMember(jss::build_path) && !amount.isIssue())) return RPC::make_error( rpcINVALID_PARAMS, "Field 'build_path' not allowed in this context."); @@ -235,7 +236,7 @@ checkPayment( else { // If no SendMax, default to Amount with sender as issuer. - sendMax = amount; + sendMax = get(amount); sendMax.setIssuer(srcAddressID); } @@ -259,7 +260,7 @@ checkPayment( *dstAccountID, sendMax.issue().currency, sendMax.issue().account, - amount, + get(amount), std::nullopt, app); if (pf.findPaths(app.config().PATH_SEARCH_OLD)) diff --git a/src/xrpld/rpc/handlers/Submit.cpp b/src/xrpld/rpc/handlers/Submit.cpp index 73fdc3822c2..b323f592ab1 100644 --- a/src/xrpld/rpc/handlers/Submit.cpp +++ b/src/xrpld/rpc/handlers/Submit.cpp @@ -50,6 +50,8 @@ doSubmit(RPC::JsonContext& context) if (!context.params.isMember(jss::tx_blob)) { + CurrentTransactionRulesGuard currentTransactionRulesGuard( + context.ledgerMaster.getCurrentLedger()->rules()); auto const failType = getFailHard(context); if (context.role != Role::ADMIN && !context.app.config().canSign())