diff --git a/include/xrpl/tx/invariants/PermissionedDEXInvariant.h b/include/xrpl/tx/invariants/PermissionedDEXInvariant.h index b4e06cd212..da187779b2 100644 --- a/include/xrpl/tx/invariants/PermissionedDEXInvariant.h +++ b/include/xrpl/tx/invariants/PermissionedDEXInvariant.h @@ -11,7 +11,8 @@ namespace xrpl { class ValidPermissionedDEX { bool regularOffers_ = false; - bool badHybrids_ = false; + bool badHybridsOld_ = false; // pre-fixSecurity3_1_3: missing field/domain or size > 1 + bool badHybrids_ = false; // post-fixSecurity3_1_3: also catches size == 0 (size != 1) hash_set domains_; public: diff --git a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp index a49e8c86d0..2dfdbc29b2 100644 --- a/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp +++ b/src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -61,10 +62,26 @@ offerInDomain( if (sleOffer->getFieldH256(sfDomainID) != domainID) return false; // LCOV_EXCL_LINE - if (sleOffer->isFlag(lsfHybrid) && !sleOffer->isFieldPresent(sfAdditionalBooks)) + if (view.rules().enabled(fixSecurity3_1_3)) { - JLOG(j.error()) << "Hybrid offer " << offerID << " missing AdditionalBooks field"; - return false; // LCOV_EXCL_LINE + // post-fixSecurity3_1_3: also catches empty sfAdditionalBooks (size == 0) + if (sleOffer->isFlag(lsfHybrid) && + (!sleOffer->isFieldPresent(sfAdditionalBooks) || + sleOffer->getFieldArray(sfAdditionalBooks).size() != 1)) + { + JLOG(j.error()) << "Hybrid offer " << offerID + << " missing or malformed AdditionalBooks field"; + return false; // LCOV_EXCL_LINE + } + } + else + { + // pre-fixSecurity3_1_3: only check for missing sfAdditionalBooks + if (sleOffer->isFlag(lsfHybrid) && !sleOffer->isFieldPresent(sfAdditionalBooks)) + { + JLOG(j.error()) << "Hybrid offer " << offerID << " missing AdditionalBooks field"; + return false; // LCOV_EXCL_LINE + } } return accountInDomain(view, sleOffer->getAccountID(sfAccount), domainID); diff --git a/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp b/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp index 8405a3c48d..6466812743 100644 --- a/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp +++ b/src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -40,11 +41,17 @@ ValidPermissionedDEX::visitEntry( regularOffers_ = true; } - // if a hybrid offer is missing domain or additional book, there's - // something wrong + // pre-fixSecurity3_1_3: hybrid offer missing domain, missing + // sfAdditionalBooks, or sfAdditionalBooks has more than one entry if (after->isFlag(lsfHybrid) && (!after->isFieldPresent(sfDomainID) || !after->isFieldPresent(sfAdditionalBooks) || after->getFieldArray(sfAdditionalBooks).size() > 1)) + badHybridsOld_ = true; + + // post-fixSecurity3_1_3: same as above but also catches size == 0 + if (after->isFlag(lsfHybrid) && + (!after->isFieldPresent(sfDomainID) || !after->isFieldPresent(sfAdditionalBooks) || + after->getFieldArray(sfAdditionalBooks).size() != 1)) badHybrids_ = true; } } @@ -63,7 +70,8 @@ ValidPermissionedDEX::finalize( // For each offercreate transaction, check if // permissioned offers are valid - if (txType == ttOFFER_CREATE && badHybrids_) + bool const isMalformed = view.rules().enabled(fixSecurity3_1_3) ? badHybrids_ : badHybridsOld_; + if (txType == ttOFFER_CREATE && isMalformed) { JLOG(j.fatal()) << "Invariant failed: hybrid offer is malformed"; return false; diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 188be33d2d..35182a2db0 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -1774,8 +1774,10 @@ class Invariants_test : public beast::unit_test::suite using namespace test::jtx; bool const fixPDEnabled = features[fixPermissionedDomainInvariant]; + bool const fixS313Enabled = features[fixSecurity3_1_3]; - testcase << "PermissionedDEX" + std::string(fixPDEnabled ? " fix" : ""); + testcase << "PermissionedDEX" + std::string(fixPDEnabled ? " fixPD" : "") + + std::string(fixS313Enabled ? " fixS313" : ""); doInvariantCheck( Env(*this, features), @@ -1863,6 +1865,45 @@ class Invariants_test : public beast::unit_test::suite {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); } + // empty sfAdditionalBooks (size 0) + { + Env env1(*this, features); + + Account const A1{"A1"}; + Account const A2{"A2"}; + env1.fund(XRP(1000), A1, A2); + env1.close(); + + [[maybe_unused]] auto [seq1, pd1] = createPermissionedDomainEnv(env1, A1, A2); + env1.close(); + + doInvariantCheck( + std::move(env1), + A1, + A2, + fixS313Enabled ? std::vector{{"hybrid offer is malformed"}} + : std::vector{}, + [&pd1](Account const& A1, Account const& A2, ApplyContext& ac) { + Keylet const offerKey = keylet::offer(A2.id(), 10); + auto sleOffer = std::make_shared(offerKey); + sleOffer->setAccountID(sfAccount, A2); + sleOffer->setFieldAmount(sfTakerPays, A1["USD"](10)); + sleOffer->setFieldAmount(sfTakerGets, XRP(1)); + sleOffer->setFlag(lsfHybrid); + sleOffer->setFieldH256(sfDomainID, pd1); + + STArray const bookArr; // empty array, size 0 + sleOffer->setFieldArray(sfAdditionalBooks, bookArr); + ac.view().insert(sleOffer); + return true; + }, + XRPAmount{}, + STTx{ttOFFER_CREATE, [&](STObject&) {}}, + fixS313Enabled + ? std::initializer_list{tecINVARIANT_FAILED, tecINVARIANT_FAILED} + : std::initializer_list{tesSUCCESS, tesSUCCESS}); + } + // hybrid offer missing sfAdditionalBooks { Env env1(*this, features); @@ -4061,6 +4102,10 @@ public: testPermissionedDomainInvariants(defaultAmendments() - fixPermissionedDomainInvariant); testPermissionedDEX(defaultAmendments() | fixPermissionedDomainInvariant); testPermissionedDEX(defaultAmendments() - fixPermissionedDomainInvariant); + testPermissionedDEX( + (defaultAmendments() | fixPermissionedDomainInvariant) - fixSecurity3_1_3); + testPermissionedDEX( + defaultAmendments() - fixPermissionedDomainInvariant - fixSecurity3_1_3); testNoModifiedUnmodifiableFields(); testValidPseudoAccounts(); testValidLoanBroker(); diff --git a/src/test/app/PermissionedDEX_test.cpp b/src/test/app/PermissionedDEX_test.cpp index b116f25058..e2c567ec7f 100644 --- a/src/test/app/PermissionedDEX_test.cpp +++ b/src/test/app/PermissionedDEX_test.cpp @@ -20,6 +20,8 @@ #include #include +#include +#include #include #include #include @@ -28,6 +30,8 @@ #include #include #include +#include +#include #include #include @@ -35,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -1385,6 +1390,73 @@ class PermissionedDEX_test : public beast::unit_test::suite BEAST_EXPECT(!offerExists(env, bob, carolOfferSeq)); } + void + testHybridMalformedOffer(FeatureBitset features) + { + bool const fixS313Enabled = features[fixSecurity3_1_3]; + + testcase << "Hybrid offer with empty AdditionalBooks" + << (fixS313Enabled ? " (fixSecurity3_1_3 enabled)" + : " (fixSecurity3_1_3 disabled)"); + + // offerInDomain has two code paths gated by fixSecurity3_1_3: + // + // pre-fix: only rejects a hybrid offer when sfAdditionalBooks is + // entirely absent — an empty array (size 0) passes through. + // post-fix: also rejects a hybrid offer whose sfAdditionalBooks array + // has size != 1 (i.e. 0 or >1 entries). + // + // We create a valid hybrid offer, then directly manipulate its SLE to + // produce the size==0 case that cannot occur via normal transactions, + // and verify that the two code paths produce the expected outcomes. + // + // Note: the PermissionedDEX invariant checker (ValidPermissionedDEX) + // does not flag this malformation for ttPAYMENT — only for + // ttOFFER_CREATE — so the without-fix payment completes as tesSUCCESS. + + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + // Create a valid hybrid offer (sfAdditionalBooks has exactly 1 entry) + auto const bobOfferSeq{env.seq(bob)}; + env(offer(bob, XRP(10), USD(10)), txflags(tfHybrid), domain(domainID)); + env.close(); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + + // Directly manipulate the offer SLE in the open ledger so that + // sfAdditionalBooks is present but empty (size 0). This is the + // malformed state that fixSecurity3_1_3 is designed to catch. + auto const offerKey = keylet::offer(bob.id(), bobOfferSeq); + env.app().getOpenLedger().modify([&offerKey](OpenView& view, beast::Journal) { + auto const sle = view.read(offerKey); + if (!sle) + return false; + auto replacement = std::make_shared(*sle, sle->key()); + replacement->setFieldArray(sfAdditionalBooks, STArray{}); + view.rawReplace(replacement); + return true; + }); + + if (fixS313Enabled) + { + // post-fixSecurity3_1_3: offerInDomain rejects the malformed + // offer (size == 0), so no valid domain offer is found. + env(pay(alice, carol, USD(10)), + path(~USD), + sendmax(XRP(10)), + domain(domainID), + ter(tecPATH_PARTIAL)); + } + else + { + // pre-fixSecurity3_1_3: offerInDomain only checks for a missing + // sfAdditionalBooks field; size == 0 passes through, so the + // malformed offer is crossed and the payment succeeds. + env(pay(alice, carol, USD(10)), path(~USD), sendmax(XRP(10)), domain(domainID)); + } + } + public: void run() override @@ -1406,6 +1478,8 @@ public: testHybridBookStep(all); testHybridInvalidOffer(all); testHybridOfferDirectories(all); + testHybridMalformedOffer(all); + testHybridMalformedOffer(all - fixSecurity3_1_3); } };