ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-04-21 03:12:29 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Check for empty sfAdditionalBooks array in hybrid offer invariant (#6716)

Browse Source
This commit is contained in:
Zhiyuan Wang
2026-04-20 13:10:28 -04:00
committed by GitHub
parent e83818241a
commit 96643bb0fa
5 changed files with 153 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
include/xrpl/tx/invariants/PermissionedDEXInvariant.h
View File

@@ -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<uint256> domains_;
public:

23
src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp
View File

@@ -6,6 +6,7 @@
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/SField.h>
@@ -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);

14
src/libxrpl/tx/invariants/PermissionedDEXInvariant.cpp
View File

@@ -3,6 +3,7 @@
#include <xrpl/basics/Log.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/SField.h>
@@ -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;

47
src/test/app/Invariants_test.cpp
View File

@@ -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<std::string>{{"hybrid offer is malformed"}}
: std::vector<std::string>{},
[&pd1](Account const& A1, Account const& A2, ApplyContext& ac) {
Keylet const offerKey = keylet::offer(A2.id(), 10);
auto sleOffer = std::make_shared<SLE>(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<TER>{tecINVARIANT_FAILED, tecINVARIANT_FAILED}
: std::initializer_list<TER>{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();

74
src/test/app/PermissionedDEX_test.cpp
View File

@@ -20,6 +20,8 @@
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/unit_test/suite.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/ledger/OpenView.h>
#include <xrpl/protocol/Book.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
@@ -28,6 +30,8 @@
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/STArray.h>
#include <xrpl/protocol/STLedgerEntry.h>
#include <xrpl/protocol/TER.h>
#include <xrpl/protocol/TxFlags.h>
@@ -35,6 +39,7 @@
#include <cstddef>
#include <cstdint>
#include <map>
#include <memory>
#include <optional>
#include <string>
#include <utility>
@@ -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, 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);
}
};