From 925aca764b46a8d2bc882eccb5ba3685a7a24e1d Mon Sep 17 00:00:00 2001
From: Scott Determan <scott.determan@yahoo.com>
Date: Mon, 2 Oct 2023 10:49:33 -0400
Subject: [PATCH] fix(XLS-38): disallow the same bridge on one chain: (#4720)

Modify the `XChainBridge` amendment.

Before this patch, two door accounts on the same chain could could own
the same bridge spec (of course, one would have to be the issuer and one
would have to be the locker). While this is silly, it does not violate
any bridge invariants. However, on further review, if we allow this then
the `claim` transactions would need to change. Since it's hard to see a
use case for two doors to own the same bridge, this patch disallows
it. (The transaction will return tecDUPLICATE).
---
 src/ripple/app/tx/impl/XChainBridge.cpp | 12 ++++++++---
 src/test/app/XChain_test.cpp            | 28 ++++++++++++++++++-------
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/src/ripple/app/tx/impl/XChainBridge.cpp b/src/ripple/app/tx/impl/XChainBridge.cpp
index 7b924ec1636..6ef10b0ebfa 100644
--- a/src/ripple/app/tx/impl/XChainBridge.cpp
+++ b/src/ripple/app/tx/impl/XChainBridge.cpp
@@ -1452,13 +1452,19 @@ XChainCreateBridge::preclaim(PreclaimContext const& ctx)
 {
     auto const account = ctx.tx[sfAccount];
     auto const bridgeSpec = ctx.tx[sfXChainBridge];
-
     STXChainBridge::ChainType const chainType =
         STXChainBridge::srcChain(account == bridgeSpec.lockingChainDoor());
 
-    if (ctx.view.read(keylet::bridge(bridgeSpec, chainType)))
     {
-        return tecDUPLICATE;
+        auto hasBridge = [&](STXChainBridge::ChainType ct) -> bool {
+            return ctx.view.exists(keylet::bridge(bridgeSpec, ct));
+        };
+
+        if (hasBridge(STXChainBridge::ChainType::issuing) ||
+            hasBridge(STXChainBridge::ChainType::locking))
+        {
+            return tecDUPLICATE;
+        }
     }
 
     if (!isXRP(bridgeSpec.issue(chainType)))
diff --git a/src/test/app/XChain_test.cpp b/src/test/app/XChain_test.cpp
index a27370dbf02..9b8aaf397dc 100644
--- a/src/test/app/XChain_test.cpp
+++ b/src/test/app/XChain_test.cpp
@@ -472,15 +472,16 @@ struct XChain_test : public beast::unit_test::suite,
                 mcBob, bridge(mcBob, mcGw["USD"], mcBob, mcGw["USD"])),
             ter(temXCHAIN_EQUAL_DOOR_ACCOUNTS));
 
-        // Both door accounts are on the same chain is allowed (they likely
-        // belong to different chains. If they do belong to the same chain, that
-        // is silly, but doesn't violate any invariants).
+        // Both door accounts are on the same chain. This is not allowed.
+        // Although it doesn't violate any invariants, it's not a useful thing
+        // to do and it complicates the "add claim" transactions.
         XEnv(*this)
             .tx(create_bridge(
                 mcAlice, bridge(mcAlice, mcGw["USD"], mcBob, mcBob["USD"])))
             .close()
             .tx(create_bridge(
-                mcBob, bridge(mcAlice, mcGw["USD"], mcBob, mcBob["USD"])))
+                    mcBob, bridge(mcAlice, mcGw["USD"], mcBob, mcBob["USD"])),
+                ter(tecDUPLICATE))
             .close();
 
         // Create a bridge on an account with exactly enough balance to
@@ -622,8 +623,10 @@ struct XChain_test : public beast::unit_test::suite,
         Account const a2("a2");
         Account const a3("a3");
         Account const a4("a4");
+        Account const a5("a5");
+        Account const a6("a6");
 
-        env.fund(XRP(10000), a1, a2, a3, a4);
+        env.fund(XRP(10000), a1, a2, a3, a4, a5, a6);
         env.close();
 
         // Add a bridge on two different accounts with the same locking and
@@ -631,10 +634,19 @@ struct XChain_test : public beast::unit_test::suite,
         env.tx(create_bridge(a1, bridge(a1, GUSD, B, BUSD))).close();
         env.tx(create_bridge(a2, bridge(a2, GUSD, B, BUSD))).close();
 
-        // Add the exact same bridge to two different accoutns (one must locking
-        // account and one must be issuing)
+        // Add the exact same bridge to two different accounts (one locking
+        // account and one issuing)
         env.tx(create_bridge(a3, bridge(a3, GUSD, a4, a4["USD"]))).close();
-        env.tx(create_bridge(a4, bridge(a3, GUSD, a4, a4["USD"]))).close();
+        env.tx(create_bridge(a4, bridge(a3, GUSD, a4, a4["USD"])),
+               ter(tecDUPLICATE))
+            .close();
+
+        // Add the exact same bridge to two different accounts (one issuing
+        // account and one locking - opposite order from the test above)
+        env.tx(create_bridge(a5, bridge(a6, GUSD, a5, a5["USD"]))).close();
+        env.tx(create_bridge(a6, bridge(a6, GUSD, a5, a5["USD"])),
+               ter(tecDUPLICATE))
+            .close();
 
         // Test case 1 ~ 5, create bridges
         auto const goodBridge1 = bridge(A, GUSD, B, BUSD);