Merge pull request #4079 from dmkozh/overflow_fix

Limit the maximum Soroban resource fee. Reviewed-by: graydon
stellar · Dec 4, 2023 · 55c65bf · 55c65bf
2 parents 518a2ea + 90487ea
commit 55c65bf
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 7 deletions.
diff --git a/src/transactions/FeeBumpTransactionFrame.cpp b/src/transactions/FeeBumpTransactionFrame.cpp
@@ -183,6 +183,11 @@ FeeBumpTransactionFrame::checkValid(Application& app,
                                     uint64_t lowerBoundCloseTimeOffset,
                                     uint64_t upperBoundCloseTimeOffset)
 {
+    if (!XDRProvidesValidFee())
+    {
+        getResult().result.code(txMALFORMED);
+        return false;
+    }
     LedgerTxn ltx(ltxOuter);
     int64_t minBaseFee = ltx.loadHeader().current().baseFee;
     resetResults(ltx.loadHeader().current(), minBaseFee, false);

diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp
@@ -47,6 +47,12 @@
 
 namespace stellar
 {
+namespace
+{
+// Limit to the maximum resource fee allowed for transaction,
+// roughly 112 million lumens.
+int64_t const MAX_RESOURCE_FEE = 1LL << 50;
+} // namespace
 
 using namespace std;
 using namespace stellar::txbridge;
@@ -1389,7 +1395,8 @@ TransactionFrame::XDRProvidesValidFee() const
         {
             return false;
         }
-        if (declaredSorobanResourceFee() < 0)
+        int64_t resourceFee = declaredSorobanResourceFee();
+        if (resourceFee < 0 || resourceFee > MAX_RESOURCE_FEE)
         {
             return false;
         }
@@ -1875,4 +1882,4 @@ TransactionFrame::getDiagnosticEvents() const
 {
     return mSorobanExtension->mDiagnosticEvents;
 }
-}
+} // namespace stellar
diff --git a/src/transactions/test/TxEnvelopeTests.cpp b/src/transactions/test/TxEnvelopeTests.cpp
@@ -2643,15 +2643,21 @@ TEST_CASE("soroban transaction validation", "[tx][envelope][soroban]")
             auto tx = sorobanTransactionFrameFromOpsWithTotalFee(
                 app->getNetworkID(), root, {op0}, {}, resources,
                 std::numeric_limits<uint32_t>::max(),
-                // An absurdly large resource fee, way beyond anything that
-                // could occur in practice and way beyond uint32_t::max, but not
-                // so bit that subsequent arithmetic overflows int64_t later on
-                // (and triggers UB).
-                std::numeric_limits<int64_t>::max() >> 1);
+                static_cast<int64_t>(std::numeric_limits<uint32_t>::max()) + 1);
             LedgerTxn ltx(app->getLedgerTxnRoot());
             REQUIRE(!tx->checkValid(*app, ltx, 0, 0, 0));
             REQUIRE(tx->getResult().result.code() == txSOROBAN_INVALID);
         }
+        SECTION("resource fee is max int64")
+        {
+            auto tx = sorobanTransactionFrameFromOpsWithTotalFee(
+                app->getNetworkID(), root, {op0}, {}, resources,
+                std::numeric_limits<uint32_t>::max(),
+                std::numeric_limits<int64_t>::max());
+            LedgerTxn ltx(app->getLedgerTxnRoot());
+            REQUIRE(!tx->checkValid(*app, ltx, 0, 0, 0));
+            REQUIRE(tx->getResult().result.code() == txMALFORMED);
+        }
         SECTION("total fee is exactly uint32 max")
         {
             auto tx = sorobanTransactionFrameFromOpsWithTotalFee(
@@ -2691,6 +2697,31 @@ TEST_CASE("soroban transaction validation", "[tx][envelope][soroban]")
             REQUIRE(!tx->checkValid(*app, ltx, 0, 0, 0));
             REQUIRE(tx->getResult().result.code() == txFEE_BUMP_INNER_FAILED);
         }
+        SECTION("resource fee is negative with fee bump")
+        {
+            auto innerTx = sorobanTransactionFrameFromOpsWithTotalFee(
+                app->getNetworkID(), root, {op0}, {}, resources,
+                std::numeric_limits<uint32_t>::max(), -1);
+            auto tx = feeBump(*app, root, innerTx,
+                              std::numeric_limits<int64_t>::max(),
+                              /* useInclusionAsFullFee */ true);
+            LedgerTxn ltx(app->getLedgerTxnRoot());
+            REQUIRE(!tx->checkValid(*app, ltx, 0, 0, 0));
+            REQUIRE(tx->getResult().result.code() == txMALFORMED);
+        }
+        SECTION("resource fee is max int64 with fee bump")
+        {
+            auto innerTx = sorobanTransactionFrameFromOpsWithTotalFee(
+                app->getNetworkID(), root, {op0}, {}, resources,
+                std::numeric_limits<uint32_t>::max(),
+                std::numeric_limits<int64_t>::max());
+            auto tx = feeBump(*app, root, innerTx,
+                              std::numeric_limits<int64_t>::max(),
+                              /* useInclusionAsFullFee */ true);
+            LedgerTxn ltx(app->getLedgerTxnRoot());
+            REQUIRE(!tx->checkValid(*app, ltx, 0, 0, 0));
+            REQUIRE(tx->getResult().result.code() == txMALFORMED);
+        }
     }
 
     SECTION("multiple ops are not allowed")