Fix empty/zero broker ID in LoanBrokerSet and LoanSet (#5977)

- Add checks for empty/zero brokerID where needed
- Extend unit-tests.

src/test/app/LoanBroker_test.cpp

@@ -904,9 +904,26 @@ class LoanBroker_test : public beast::unit_test::suite
         if (!BEAST_EXPECT(broker))
             return;
 
+        auto testZeroBrokerID = [&](auto&& getTxJv) {
+            auto jv = getTxJv();
+            // empty broker ID
+            jv[sfLoanBrokerID] = "";
+            env(jv, ter(temINVALID));
+            // zero broker ID
+            jv[sfLoanBrokerID] = to_string(uint256{});
+            // needs a flag to distinguish the parsed STTx from the prior
+            // test
+            env(jv, txflags(tfFullyCanonicalSig), ter(temINVALID));
+        };
+
         if (brokerTest == CoverDeposit)
         {
-            // preclaim: tecWRONG_ASET
+            // preflight: temINVALID (empty/zero broker id)
+            testZeroBrokerID([&]() {
+                return coverDeposit(alice, brokerKeylet.key, asset(10));
+            });
+
+            // preclaim: tecWRONG_ASSET
             env(coverDeposit(alice, brokerKeylet.key, issuer["BAD"](10)),
                 ter(tecWRONG_ASSET));
 
@@ -929,6 +946,11 @@ class LoanBroker_test : public beast::unit_test::suite
 
         if (brokerTest == CoverWithdraw)
         {
+            // preflight: temINVALID (empty/zero broker id)
+            testZeroBrokerID([&]() {
+                return coverWithdraw(alice, brokerKeylet.key, asset(10));
+            });
+
             // preclaim: tecWRONG_ASSSET
             env(coverWithdraw(alice, brokerKeylet.key, issuer["BAD"](10)),
                 ter(tecWRONG_ASSET));
@@ -976,6 +998,14 @@ class LoanBroker_test : public beast::unit_test::suite
 
         if (brokerTest == CoverClawback)
         {
+            // preflight: temINVALID (empty/zero broker id)
+            testZeroBrokerID([&]() {
+                return env.json(
+                    coverClawback(alice),
+                    loanBrokerID(brokerKeylet.key),
+                    amount(vaultInfo.asset(2)));
+            });
+
             if (asset.holds<Issue>())
             {
                 // preclaim: AllowTrustLineClaback is not set
@@ -1011,23 +1041,8 @@ class LoanBroker_test : public beast::unit_test::suite
                 sig(sfCounterpartySignature, alice),
                 fee(env.current()->fees().base * 2));
 
-            // preflight: temINVALID (empty broker id)
-            {
-                auto jv = del(alice, brokerKeylet.key);
-                jv[sfLoanBrokerID] = "";
-                env(jv, ter(temINVALID));
-            }
-            // preflight: temINVALID (zero broker id)
-            {
-                // needs a flag to distinguish the parsed STTx from the prior
-                // test
-                auto jv = del(alice, uint256{}, tfFullyCanonicalSig);
-                BEAST_EXPECT(
-                    jv[sfLoanBrokerID] ==
-                    "0000000000000000000000000000000000000000000000000000000000"
-                    "000000");
-                env(jv, ter(temINVALID));
-            }
+            // preflight: temINVALID (empty/zero broker id)
+            testZeroBrokerID([&]() { return del(alice, brokerKeylet.key); });
 
             // preclaim: tecHAS_OBLIGATIONS
             env(del(alice, brokerKeylet.key), ter(tecHAS_OBLIGATIONS));
@@ -1037,6 +1052,13 @@ class LoanBroker_test : public beast::unit_test::suite
 
         if (brokerTest == Set)
         {
+            // preflight: temINVALID (empty/zero broker id)
+            testZeroBrokerID([&]() {
+                return env.json(
+                    set(alice, vaultInfo.vaultID),
+                    loanBrokerID(brokerKeylet.key));
+            });
+
             if (asset.holds<Issue>())
             {
                 env(fclear(issuer, asfDefaultRipple));

src/test/app/Loan_test.cpp

@@ -4271,6 +4271,28 @@ protected:
                         Number const& debtMaximumRequest) {
             // first temBAD_SIGNER: TODO
 
+            // empty/zero broker ID
+            {
+                auto jv = set(borrower, uint256{}, debtMaximumRequest);
+
+                auto testZeroBrokerID = [&](std::string const& id,
+                                            std::uint32_t flags = 0) {
+                    // empty broker ID
+                    jv[sfLoanBrokerID] = id;
+                    env(jv,
+                        sig(sfCounterpartySignature, lender),
+                        loanSetFee,
+                        txflags(flags),
+                        ter(temINVALID));
+                };
+                // empty broker ID
+                testZeroBrokerID(std::string(""));
+                // zero broker ID
+                // needs a flag to distinguish the parsed STTx from the prior
+                // test
+                testZeroBrokerID(to_string(uint256{}), tfFullyCanonicalSig);
+            }
+
             // preflightCheckSigningKey() failure:
             // can it happen? the signature is checked before transactor
             // executes

src/xrpld/app/tx/detail/LoanBrokerSet.cpp

@@ -35,6 +35,9 @@ LoanBrokerSet::preflight(PreflightContext const& ctx)
             tx.isFieldPresent(sfCoverRateMinimum) ||
             tx.isFieldPresent(sfCoverRateLiquidation))
             return temINVALID;
+
+        if (tx[sfLoanBrokerID] == beast::zero)
+            return temINVALID;
     }
 
     {

src/xrpld/app/tx/detail/LoanSet.cpp

@@ -100,6 +100,10 @@ LoanSet::preflight(PreflightContext const& ctx)
             return *ret;
     }
 
+    if (auto const brokerID = ctx.tx[~sfLoanBrokerID];
+        brokerID && *brokerID == beast::zero)
+        return temINVALID;
+
     return tesSUCCESS;
 }