From f23c32cc00a169ef718f01004f858b3b69aa7729 Mon Sep 17 00:00:00 2001
From: RichardAH <richard.holland@starstone.co.nz>
Date: Wed, 12 Apr 2023 02:11:17 +0200
Subject: [PATCH] Prevent replay attacks with NetworkID field: (#4370)

Add a `NetworkID` field to help prevent replay attacks on and from
side-chains.

The new field must be used when the server is using a network id > 1024.

To preserve legacy behavior, all chains with a network ID less than 1025
retain the existing behavior. This includes Mainnet, Testnet, Devnet,
and hooks-testnet. If `sfNetworkID` is present in any transaction
submitted to any of the nodes on one of these chains, then
`telNETWORK_ID_MAKES_TX_NON_CANONICAL` is returned.

Since chains with a network ID less than 1025, including Mainnet, retain
the existing behavior, there is no need for an amendment.

The `NetworkID` helps to prevent replay attacks because users specify a
`NetworkID` field in every transaction for that chain.

This change introduces a new UINT32 field, `sfNetworkID` ("NetworkID").
There are also three new local error codes for transaction results:

- `telNETWORK_ID_MAKES_TX_NON_CANONICAL`
- `telREQUIRES_NETWORK_ID`
- `telWRONG_NETWORK`

To learn about the other transaction result codes, see:
https://xrpl.org/transaction-results.html

Local error codes were chosen because a transaction is not necessarily
malformed if it is submitted to a node running on the incorrect chain.
This is a local error specific to that node and could be corrected by
switching to a different node or by changing the `network_id` on that
node. See:
https://xrpl.org/connect-your-rippled-to-the-xrp-test-net.html

In addition to using `NetworkID`, it is still generally recommended to
use different accounts and keys on side-chains. However, people will
undoubtedly use the same keys on multiple chains; for example, this is
common practice on other blockchain networks. There are also some
legitimate use cases for this.

A `app.NetworkID` test suite has been added, and `core.Config` was
updated to include some network_id tests.
---
 src/ripple/protocol/impl/TER.cpp |  1 -
 src/test/app/NetworkID_test.cpp  | 31 ++++++++++---------------------
 2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp
index b7bc39186..9fee2d927 100644
--- a/src/ripple/protocol/impl/TER.cpp
+++ b/src/ripple/protocol/impl/TER.cpp
@@ -136,7 +136,6 @@ transResults()
         MAKE_ERROR(telNON_LOCAL_EMITTED_TXN, "Emitted transaction cannot be applied because it was not generated locally."),
         MAKE_ERROR(telIMPORT_VL_KEY_NOT_RECOGNISED, "Import vl key was not recognized."),
         MAKE_ERROR(telCAN_NOT_QUEUE_IMPORT,   "Import transaction was not able to be directly applied and cannot be queued."),
-
         MAKE_ERROR(temMALFORMED,                 "Malformed transaction."),
         MAKE_ERROR(temBAD_AMOUNT,                "Can only send positive amounts."),
         MAKE_ERROR(temBAD_CURRENCY,              "Malformed: Bad currency."),
diff --git a/src/test/app/NetworkID_test.cpp b/src/test/app/NetworkID_test.cpp
index c5488b331..e650667f8 100644
--- a/src/test/app/NetworkID_test.cpp
+++ b/src/test/app/NetworkID_test.cpp
@@ -48,7 +48,9 @@ public:
     void
     testNetworkID()
     {
-        testcase("network_id");
+        testcase(
+            "Require txn NetworkID to be specified (or not) depending on the "
+            "network ID of the node");
         using namespace jtx;
 
         auto const alice = Account{"alice"};
@@ -66,9 +68,14 @@ public:
                 jv[jss::Destination] = alice.human();
                 jv[jss::TransactionType] = "Payment";
                 jv[jss::Amount] = "10000000000";
+                if (env.app().config().NETWORK_ID > 1024)
+                    jv[jss::NetworkID] =
+                        std::to_string(env.app().config().NETWORK_ID);
+
                 env(jv, fee(1000), sig(env.master));
             }
 
+            // run tx
             env(jv, fee(1000), ter(expectedOutcome));
             env.close();
         };
@@ -121,29 +128,11 @@ public:
             test::jtx::Env env{*this, makeNetworkConfig(1025)};
             BEAST_EXPECT(env.app().config().NETWORK_ID == 1025);
 
-            {
-                env.fund(XRP(200), alice);
-                // try to submit a txn without network id, this should not work
-                Json::Value jvn;
-                jvn[jss::Account] = alice.human();
-                jvn[jss::TransactionType] = jss::AccountSet;
-                jvn[jss::Fee] = to_string(env.current()->fees().base);
-                jvn[jss::Sequence] = env.seq(alice);
-                jvn[jss::LastLedgerSequence] = env.current()->info().seq + 2;
-                auto jt = env.jtnofill(jvn, alice);
-                Serializer s;
-                jt.stx->add(s);
-                BEAST_EXPECT(
-                    env.rpc(
-                        "submit",
-                        strHex(s.slice()))[jss::result][jss::engine_result] ==
-                    "telREQUIRES_NETWORK_ID");
-                env.close();
-            }
-
+            // try to submit a txn without network id, this should not work
             Json::Value jv;
             jv[jss::Account] = alice.human();
             jv[jss::TransactionType] = jss::AccountSet;
+            runTx(env, jv, telREQUIRES_NETWORK_ID);
 
             // try to submit with wrong network id
             jv[jss::NetworkID] = 0;