Check XRP endpoints for circular paths (RIPD-1781):

The payment engine restricts payment paths so two steps do not input the same Currency/Issuer or output the same Currency/Issuer. This check was skipped when the path started or ended with XRP. An example of a path that was incorrectly accepted was: XRP -> //USD -> //XRP -> EUR This patch enables the path loop check for paths that start or end with XRP.
XRPLF · Feb 24, 2020 · 2b6293b · 2b6293b
1 parent 0c6d380
commit 2b6293b
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 1 deletion.
diff --git a/src/ripple/app/paths/impl/XRPEndpointStep.cpp b/src/ripple/app/paths/impl/XRPEndpointStep.cpp
@@ -25,6 +25,7 @@
 #include <ripple/basics/Log.h>
 #include <ripple/basics/XRPAmount.h>
 #include <ripple/ledger/PaymentSandbox.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/Quality.h>
 
 #include <boost/container/flat_set.hpp>
@@ -359,6 +360,18 @@ XRPEndpointStep<TDerived>::check (StrandContext const& ctx) const
     if (ter != tesSUCCESS)
         return ter;
 
+    if (ctx.view.rules().enabled(fix1781))
+    {
+        auto const issuesIndex = isLast_ ? 0 : 1;
+        if (!ctx.seenDirectIssues[issuesIndex].insert(xrpIssue()).second)
+        {
+            JLOG(j_.debug())
+                << "XRPEndpointStep: loop detected: Index: " << ctx.strandSize
+                << ' ' << *this;
+            return temBAD_PATH_LOOP;
+        }
+    }
+
     return tesSUCCESS;
 }
 

diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h
@@ -110,6 +110,7 @@ class FeatureCollections
         "DeletableAccounts",
         // fixQualityUpperBound should be activated before FlowCross
         "fixQualityUpperBound",
+        "fix1781", // XRPEndpointSteps should be included in the circular payment check
     };
 
     std::vector<uint256> features;
@@ -397,6 +398,7 @@ extern uint256 const fixCheckThreading;
 extern uint256 const fixPayChanRecipientOwnerDir;
 extern uint256 const featureDeletableAccounts;
 extern uint256 const fixQualityUpperBound;
+extern uint256 const fix1781;
 
 } // ripple
 

diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp
@@ -129,6 +129,7 @@ detail::supportedAmendments ()
         "fixPayChanRecipientOwnerDir",
         "DeletableAccounts",
         "fixQualityUpperBound",
+        "fix1781",
     };
     return supported;
 }
@@ -187,5 +188,6 @@ uint256 const fixCheckThreading = *getRegisteredFeature("fixCheckThreading");
 uint256 const fixPayChanRecipientOwnerDir = *getRegisteredFeature("fixPayChanRecipientOwnerDir");
 uint256 const featureDeletableAccounts = *getRegisteredFeature("DeletableAccounts");
 uint256 const fixQualityUpperBound = *getRegisteredFeature("fixQualityUpperBound");
+uint256 const fix1781 = *getRegisteredFeature("fix1781");
 
 } // ripple
diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp
@@ -1180,6 +1180,51 @@ struct Flow_test : public beast::unit_test::suite
             ter(temBAD_PATH));
     }
 
+    void
+    testXRPPathLoop()
+    {
+        testcase("Circular XRP");
+
+        using namespace jtx;
+        auto const alice = Account("alice");
+        auto const bob = Account("bob");
+        auto const gw = Account("gw");
+        auto const USD = gw["USD"];
+        auto const EUR = gw["EUR"];
+
+        for (auto const withFix : {true, false})
+        {
+            auto const feats = [&withFix]() -> FeatureBitset {
+                if (withFix)
+                    return supported_amendments();
+                return supported_amendments() - FeatureBitset{fix1781};
+            }();
+            Env env(*this, feats);
+            env.fund(XRP(10000), alice, bob, gw);
+            env.trust(USD(1000), alice, bob);
+            env.trust(EUR(1000), alice, bob);
+            env(pay(gw, alice, USD(100)));
+            env(pay(gw, alice, EUR(100)));
+            env.close();
+
+            env(offer(alice, XRP(100), USD(100)), txflags(tfPassive));
+            env(offer(alice, USD(100), XRP(100)), txflags(tfPassive));
+            env(offer(alice, XRP(100), EUR(100)), txflags(tfPassive));
+            env.close();
+
+            TER const expectedTer =
+                withFix ? TER{temBAD_PATH_LOOP} : TER{tesSUCCESS};
+            env(pay(alice, bob, EUR(1)),
+                path(~USD, ~XRP, ~EUR),
+                sendmax(XRP(1)),
+                txflags(tfNoRippleDirect),
+                ter(expectedTer));
+            // Need at least one "pass" in any beast test (the test will fail if
+            // the `ter` result in pay tx doesn't match)
+            pass();
+        }
+    }
+
     void testWithFeats(FeatureBitset features)
     {
         using namespace jtx;
@@ -1204,6 +1249,7 @@ struct Flow_test : public beast::unit_test::suite
     void run() override
     {
         testLimitQuality();
+        testXRPPathLoop();
         testRIPD1443();
         testRIPD1449();
 

diff --git a/src/test/app/PayStrand_test.cpp b/src/test/app/PayStrand_test.cpp
@@ -703,7 +703,7 @@ struct PayStrand_test : public beast::unit_test::suite
                     alice,
                     /*deliver*/ xrpIssue(),
                     /*limitQuality*/ boost::none,
-                    /*sendMaxIssue*/ xrpIssue(),
+                    /*sendMaxIssue*/ EUR.issue(),
                     path,
                     true,
                     false,