Fix Invalid Tx flags (#514)

src/ripple/app/tx/impl/SetHook.cpp

@@ -33,6 +33,7 @@
 #include <ripple/protocol/STArray.h>
 #include <ripple/protocol/STObject.h>
 #include <ripple/protocol/STTx.h>
+#include <ripple/protocol/TxFlags.h>
 #include <algorithm>
 #include <cstdint>
 #include <exception>
@@ -665,6 +666,13 @@ SetHook::preflight(PreflightContext const& ctx)
     if (!isTesSuccess(ret))
         return ret;
 
+    if (ctx.rules.enabled(fixInvalidTxFlags) &&
+        ctx.tx.getFlags() & tfUniversalMask)
+    {
+        JLOG(ctx.j.trace()) << "SetHook: Invalid flags set.";
+        return temINVALID_FLAG;
+    }
+
     if (!ctx.tx.isFieldPresent(sfHooks))
     {
         JLOG(ctx.j.trace())

src/ripple/app/tx/impl/SetSignerList.cpp

@@ -27,6 +27,7 @@
 #include <ripple/protocol/STArray.h>
 #include <ripple/protocol/STObject.h>
 #include <ripple/protocol/STTx.h>
+#include <ripple/protocol/TxFlags.h>
 #include <algorithm>
 #include <cstdint>
 
@@ -81,6 +82,13 @@ SetSignerList::preflight(PreflightContext const& ctx)
     if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
         return ret;
 
+    if (ctx.rules.enabled(fixInvalidTxFlags) &&
+        (ctx.tx.getFlags() & tfUniversalMask))
+    {
+        JLOG(ctx.j.trace()) << "SetSignerList: invalid flags.";
+        return temINVALID_FLAG;
+    }
+
     auto const result = determineOperation(ctx.tx, ctx.flags, ctx.j);
 
     if (!isTesSuccess(std::get<0>(result)))

src/ripple/protocol/Feature.h

@@ -74,7 +74,7 @@ namespace detail {
 // Feature.cpp. Because it's only used to reserve storage, and determine how
 // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
 // the actual number of amendments. A LogicError on startup will verify this.
-static constexpr std::size_t numFeatures = 85;
+static constexpr std::size_t numFeatures = 86;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -373,6 +373,7 @@ extern uint256 const fixProvisionalDoubleThreading;
 extern uint256 const featureClawback;
 extern uint256 const featureDeepFreeze;
 extern uint256 const featureIOUIssuerWeakTSH;
+extern uint256 const fixInvalidTxFlags;
 
 }  // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -479,6 +479,7 @@ REGISTER_FEATURE(Clawback,                      Supported::yes, VoteBehavior::De
 REGISTER_FIX    (fixProvisionalDoubleThreading, Supported::yes, VoteBehavior::DefaultYes);
 REGISTER_FEATURE(DeepFreeze,                    Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FEATURE(IOUIssuerWeakTSH,              Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixInvalidTxFlags,             Supported::yes, VoteBehavior::DefaultYes);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.

src/test/app/Import_test.cpp

@@ -5203,8 +5203,8 @@ class Import_test : public beast::unit_test::suite
             std::string ns_str =
                 "CAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECA"
                 "FE";
-            Json::Value jv = ripple::test::jtx::hook(
-                issuer, {{hso(createCodeHex)}}, hsfOVERRIDE | hsfCOLLECT);
+            Json::Value jv =
+                ripple::test::jtx::hook(issuer, {{hso(createCodeHex)}}, 0);
             jv[jss::Hooks][0U][jss::Hook][jss::HookNamespace] = ns_str;
             jv[jss::Hooks][0U][jss::Hook][jss::HookOn] =
                 "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFDFFFFFFFFFFFFFFFFFFBFFF"

src/test/app/MultiSign_test.cpp

@@ -1659,6 +1659,36 @@ public:
         BEAST_EXPECT(env.seq(alice) == aliceSeq + 1);
     }
 
+    void
+    test_signerListSetFlags(FeatureBitset features)
+    {
+        using namespace test::jtx;
+
+        for (bool const withFixInvalidTxFlags : {false, true})
+        {
+            Env env{
+                *this,
+                withFixInvalidTxFlags ? features
+                                      : features - fixInvalidTxFlags};
+            Account const alice{"alice"};
+
+            env.fund(XRP(1000), alice);
+            env.close();
+
+            bool const enabled = features[fixInvalidTxFlags];
+            testcase(
+                std::string("SignerListSet flag, fix ") +
+                (withFixInvalidTxFlags ? "enabled" : "disabled"));
+
+            ter const expected(
+                withFixInvalidTxFlags ? TER(temINVALID_FLAG) : TER(tesSUCCESS));
+            env(signers(alice, 2, {{bogie, 1}, {ghost, 1}}),
+                expected,
+                txflags(tfPassive));
+            env.close();
+        }
+    }
+
     void
     testAll(FeatureBitset features)
     {
@@ -1695,6 +1725,9 @@ public:
         testAll(all - featureMultiSignReserve - featureExpandedSignerList);
         testAll(all - featureExpandedSignerList);
         testAll(all);
+
+        test_signerListSetFlags(all);
+
         test_amendmentTransition();
     }
 };

src/test/app/SetHook_test.cpp

@@ -364,6 +364,35 @@ public:
         }
     }
 
+    void
+    testInvalidTxFlags(FeatureBitset features)
+    {
+        testcase("Checks invalid tx flags");
+        using namespace jtx;
+
+        for (bool const withFixInvalidTxFlags : {false, true})
+        {
+            Env env{
+                *this,
+                withFixInvalidTxFlags ? features
+                                      : features - fixInvalidTxFlags};
+
+            auto const alice = Account{"alice"};
+            env.fund(XRP(10000), alice);
+            env.close();
+
+            Json::Value jv =
+                ripple::test::jtx::hook(alice, {{{hso_delete()}}}, 0);
+            jv[jss::Flags] = tfUniversalMask;
+
+            env(jv,
+                M("Invalid SetHook flags"),
+                HSFEE,
+                withFixInvalidTxFlags ? ter(temINVALID_FLAG) : ter(tesSUCCESS));
+            env.close();
+        }
+    }
+
     void
     testGrants(FeatureBitset features)
     {
@@ -12737,6 +12766,7 @@ public:
         testHooksOwnerDir(features);
         testHooksDisabled(features);
         testTxStructure(features);
+        testInvalidTxFlags(features);
         testInferHookSetOperation();
         testParams(features);
         testGrants(features);

src/test/rpc/AccountTx_test.cpp

@@ -523,8 +523,7 @@ class AccountTx_test : public beast::unit_test::suite
                     "0B";
                 Json::Value jhv = hso(updateHookHex);
                 jhv[jss::Flags] = hsfOVERRIDE;
-                Json::Value jv =
-                    ripple::test::jtx::hook(account, {{jhv}}, hsfOVERRIDE);
+                Json::Value jv = ripple::test::jtx::hook(account, {{jhv}}, 0);
                 return jv;
             };
             env(updateHook(alice), HSFEE, sig(alie));
@@ -553,8 +552,7 @@ class AccountTx_test : public beast::unit_test::suite
                     "000000";
                 jhv[jss::HookNamespace] = to_string(uint256{beast::zero});
                 jhv[jss::HookHash] = to_string(hookHash);
-                Json::Value jv =
-                    ripple::test::jtx::hook(account, {{jhv}}, hsfOVERRIDE);
+                Json::Value jv = ripple::test::jtx::hook(account, {{jhv}}, 0);
                 return jv;
             };
             uint256 const hid = hh(env, alice);
@@ -563,8 +561,8 @@ class AccountTx_test : public beast::unit_test::suite
 
             // Delete Hook
             auto deleteHook = [](test::jtx::Account const& account) {
-                Json::Value jv = ripple::test::jtx::hook(
-                    account, {{hso_delete()}}, hsfOVERRIDE);
+                Json::Value jv =
+                    ripple::test::jtx::hook(account, {{hso_delete()}}, 0);
                 return jv;
             };
             env(deleteHook(alice), HSFEE, sig(alie));