Fix PoolPair value overflow undefined behavior bug (#2077)

* Fixes to check for max PoolPair value to prevent value overflow * Revert validation and legacy const expr * Fixes to poolprice max and compatibility with legacy max poolprice * Use setMaxPoolPrice in customtx rpc * Fix max poolpair value check * Refactor for more clarity * Fix new unit test value * Refactor items * Switch to getMaxValid, cleanup headers * Simplfy actions checkout * Remove unused code after re-abstractions --------- Co-authored-by: Prasanna Loganathar <[email protected]>
DeFiCh · Jul 4, 2023 · 3075740 · 3075740
1 parent f272e5e
commit 3075740
Show file tree

Hide file tree

Showing 6 changed files with 25 additions and 22 deletions.
diff --git a/.github/workflows/tests-sync.yml b/.github/workflows/tests-sync.yml
@@ -23,8 +23,6 @@ jobs:
     runs-on: [self-hosted, linux, x64, builder]
     steps:
     - uses: actions/checkout@v3
-      with:
-        fetch-depth: 0
 
     - name: Populate environment
       run: ./make.sh ci-export-vars

diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp
@@ -4615,7 +4615,7 @@ Res CPoolSwap::ExecuteSwap(CCustomCSView &view, std::vector<DCT_ID> poolIDs, con
     }
 
     // Single swap if no pool IDs provided
-    auto poolPrice = POOLPRICE_MAX;
+    auto poolPrice = PoolPrice::getMaxValid();
     std::optional<std::pair<DCT_ID, CPoolPair> > poolPair;
     if (poolIDs.empty()) {
         poolPair = view.GetPoolPair(obj.idTokenFrom, obj.idTokenTo);
@@ -4788,7 +4788,7 @@ Res CPoolSwap::ExecuteSwap(CCustomCSView &view, std::vector<DCT_ID> poolIDs, con
     }
 
     // Reject if price paid post-swap above max price provided
-    if (height >= static_cast<uint32_t>(consensus.FortCanningHeight) && obj.maxPrice != POOLPRICE_MAX) {
+    if (height >= static_cast<uint32_t>(consensus.FortCanningHeight) && !obj.maxPrice.isAboveValid()) {
         if (swapAmountResult.nValue != 0) {
             const auto userMaxPrice = arith_uint256(obj.maxPrice.integer) * COIN + obj.maxPrice.fraction;
             if (arith_uint256(obj.amountFrom) * COIN / swapAmountResult.nValue > userMaxPrice) {
@@ -4822,7 +4822,7 @@ Res SwapToDFIorDUSD(CCustomCSView &mnview,
     obj.idTokenFrom = tokenId;
     obj.idTokenTo   = DCT_ID{0};
     obj.amountFrom  = amount;
-    obj.maxPrice    = POOLPRICE_MAX;
+    obj.maxPrice    = PoolPrice::getMaxValid();
 
     auto poolSwap = CPoolSwap(obj, height);
     auto token    = mnview.GetToken(tokenId);

diff --git a/src/masternodes/poolpairs.h b/src/masternodes/poolpairs.h
@@ -43,10 +43,16 @@ struct PoolPrice {
     }
 
     bool operator!=(const PoolPrice &rhs) const { return integer != rhs.integer || fraction != rhs.fraction; }
-};
 
-static constexpr auto POOLPRICE_MAX =
-    PoolPrice{std::numeric_limits<CAmount>::max(), std::numeric_limits<CAmount>::max()};
+    static constexpr PoolPrice getMaxValid() {
+        return { MAX_MONEY / COIN, MAX_MONEY % COIN };
+    }
+
+    bool isAboveValid() const {
+        const auto maxPrice = PoolPrice::getMaxValid();
+        return ((integer > maxPrice.integer) || (integer == maxPrice.integer && fraction >= maxPrice.fraction));
+    }
+};
 
 struct CPoolSwapMessage {
     CScript from, to;

diff --git a/src/masternodes/rpc_customtx.cpp b/src/masternodes/rpc_customtx.cpp
@@ -235,13 +235,14 @@ class CCustomTxRpcVisitor {
         rpcInfo.pushKV("fromAmount", ValueFromAmount(obj.amountFrom));
         rpcInfo.pushKV("toAddress", ScriptToString(obj.to));
         rpcInfo.pushKV("toToken", obj.idTokenTo.ToString());
-
-        CAmount userMaxPrice = std::numeric_limits<CAmount>::max();
-        if ((arith_uint256(obj.maxPrice.integer) * COIN + obj.maxPrice.fraction) < userMaxPrice) {
-            userMaxPrice = (obj.maxPrice.integer * COIN) + obj.maxPrice.fraction;
+
+        if (obj.maxPrice.isAboveValid()) {
+            auto price = PoolPrice::getMaxValid();
+            rpcInfo.pushKV("maxPrice", ValueFromAmount((price.integer * COIN) + price.fraction));
+        }
+        else {
+            rpcInfo.pushKV("maxPrice", ValueFromAmount((obj.maxPrice.integer * COIN) + obj.maxPrice.fraction));
         }
-
-        rpcInfo.pushKV("maxPrice", ValueFromAmount(userMaxPrice));
     }
 
     void operator()(const CPoolSwapMessageV2 &obj) const {

diff --git a/src/masternodes/rpc_poolpair.cpp b/src/masternodes/rpc_poolpair.cpp
@@ -179,15 +179,13 @@ void CheckAndFillPoolSwapMessage(const JSONRPCRequest &request, CPoolSwapMessage
         if (!token2)
             throw JSONRPCError(RPC_INVALID_PARAMETER, "TokenTo was not found");
 
+        CAmount maxPrice = std::numeric_limits<CAmount>::max();
         if (!metadataObj["maxPrice"].isNull()) {
-            CAmount maxPrice              = AmountFromValue(metadataObj["maxPrice"]);
-            poolSwapMsg.maxPrice.integer  = maxPrice / COIN;
-            poolSwapMsg.maxPrice.fraction = maxPrice % COIN;
-        } else {
-            // There is no maxPrice calculation anymore
-            poolSwapMsg.maxPrice.integer  = std::numeric_limits<CAmount>::max();
-            poolSwapMsg.maxPrice.fraction = std::numeric_limits<CAmount>::max();
+            maxPrice = AmountFromValue(metadataObj["maxPrice"]);
         }
+
+        poolSwapMsg.maxPrice.integer  = maxPrice / COIN;
+        poolSwapMsg.maxPrice.fraction = maxPrice % COIN;
     }
 }
 

diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
@@ -98,7 +98,7 @@ BOOST_AUTO_TEST_CASE(rpc_rawparams)
     BOOST_CHECK_EQUAL(find_value(resultsObj, "fromAmount").get_real(), 2.0);
     BOOST_CHECK_EQUAL(find_value(resultsObj, "toAddress").get_str(), "dKTimnyUteWgQCGaFe2m49MzYuU7sSVv2N");
     BOOST_CHECK_EQUAL(find_value(resultsObj, "toToken").get_str(), "1");
-    BOOST_CHECK_EQUAL(find_value(resultsObj, "maxPrice").get_real(), 3973919657.0);
+    BOOST_CHECK_EQUAL(find_value(resultsObj, "maxPrice").get_real(), 92233720368.54776);
 
     BOOST_CHECK_NO_THROW(r = CallRPC(std::string("decodecustomtx ")+txHexString + " true"));
     BOOST_CHECK_THROW(CallRPC(std::string("decodecustomtx ")+txHexString+" extra"), std::runtime_error);