Offer create exploit fix (#10)

Co-authored-by: tequ <git@tequ.dev>
Co-authored-by: Nicholas Dudfield <ndudfield@gmail.com>

src/test/app/Offer_test.cpp

@@ -5104,6 +5104,36 @@ public:
 
         using namespace jtx;
 
+        // OfferID is only valid when Hooks are enabled.
+        {
+            Env env{*this, features - featureHooks};
+            auto const gw = Account{"gateway"};
+            auto const alice = Account{"alice"};
+            auto const USD = gw["USD"];
+
+            env.fund(XRP(10000), gw, alice);
+            env.close();
+
+            env(trust(alice, USD(1000)));
+            env.close();
+
+            env(pay(gw, alice, USD(200)));
+            env.close();
+
+            uint256 const offerId{getOfferIndex(alice, env.seq(alice))};
+            env(offer(alice, XRP(50), USD(50)));
+            env.close();
+
+            uint256 const newOfferId{getOfferIndex(alice, env.seq(alice))};
+            env(offer(alice, XRP(50), USD(50)),
+                offer_id(offerId),
+                ter(temDISABLED));
+            env(offer_cancel(alice), offer_id(offerId), ter(temDISABLED));
+
+            BEAST_EXPECT(env.le(keylet::unchecked(offerId)));
+            BEAST_EXPECT(!env.le(keylet::unchecked(newOfferId)));
+        }
+
         // OfferCreate
         {
             Env env{*this, features};
@@ -5481,6 +5511,71 @@ public:
             env.balance(taker, XRP) == takerXRPBalance);
     }
 
+    void
+    testFixCancelOfferExploit(FeatureBitset features)
+    {
+        testcase("fixCancelOfferExploit");
+        using namespace jtx;
+        Account const alice("alice");
+        Account const bob("bob");
+        Account const charlie("charlie");
+        auto const USD = alice["USD"];
+        auto const bobUSD = bob["USD"];
+
+        Env env(*this, features);
+
+        env.fund(XRP(1000), alice, bob);
+        env.close();
+
+        // OfferCancel: not ltOffer, exist account(alice)
+        uint256 const nonOfferId{keylet::account(alice.id()).key};
+        env(offer_cancel(bob), offer_id(nonOfferId), ter(tecNO_PERMISSION));
+        BEAST_EXPECT(env.le(keylet::unchecked(nonOfferId)));
+
+        // OfferCreate: not ltOffer
+        env(offer(bob, XRP(100), bobUSD(100)),
+            offer_id(nonOfferId),
+            ter(tecNO_PERMISSION));
+        BEAST_EXPECT(env.le(keylet::unchecked(nonOfferId)));
+
+        // OfferCancel: not ltOffer, not exist account(charlie)
+        uint256 const nonExistId{keylet::account(charlie.id()).key};
+        env(offer_cancel(bob), offer_id(nonExistId), ter(tesSUCCESS));
+        BEAST_EXPECT(!env.le(keylet::unchecked(nonExistId)));
+
+        // OfferCreate: not ltOffer, not exist account(charlie)
+        env(offer(bob, XRP(100), bobUSD(100)),
+            offer_id(nonExistId),
+            ter(tesSUCCESS));
+        BEAST_EXPECT(!env.le(keylet::unchecked(nonExistId)));
+
+        // create alice offer
+        uint256 const aliceOfferId{getOfferIndex(alice, env.seq(alice))};
+        env(offer(alice, XRP(100), USD(100)));
+        env.close();
+
+        // OfferCancel: bob cannot cancel alice's offer
+        env(offer_cancel(bob), offer_id(aliceOfferId), ter(tecNO_PERMISSION));
+        BEAST_EXPECT(env.le(keylet::unchecked(aliceOfferId)));
+
+        // OfferCreates: bob cannot cancel alice's offer
+        env(offer(bob, XRP(100), bobUSD(100)),
+            offer_id(aliceOfferId),
+            ter(tecNO_PERMISSION));
+        BEAST_EXPECT(env.le(keylet::unchecked(aliceOfferId)));
+
+        // An expired OfferCreate still cannot cancel another account's Offer.
+        uint256 const expiredBobOfferId{getOfferIndex(bob, env.seq(bob))};
+        env(offer(bob, XRP(100), bobUSD(100)),
+            offer_id(aliceOfferId),
+            json(sfExpiration.fieldName, lastClose(env)),
+            ter(tecNO_PERMISSION));
+        BEAST_EXPECT(env.le(keylet::unchecked(aliceOfferId)));
+        BEAST_EXPECT(!env.le(keylet::unchecked(expiredBobOfferId)));
+
+        env(offer_cancel(alice), offer_id(aliceOfferId), ter(tesSUCCESS));
+    }
+
     void
     testAll(FeatureBitset features)
     {
@@ -5544,6 +5639,7 @@ public:
         testRmSmallIncreasedQOffersIOU(features);
         testOfferID(features);
         testFillOrKill(features);
+        testFixCancelOfferExploit(features);
     }
 
     void

src/xrpld/app/tx/detail/CancelOffer.cpp

@@ -39,6 +39,9 @@ CancelOffer::preflight(PreflightContext const& ctx)
         return temINVALID_FLAG;
     }
 
+    if (!ctx.rules.enabled(featureHooks) && ctx.tx.isFieldPresent(sfOfferID))
+        return temDISABLED;
+
     if ((!ctx.tx.isFieldPresent(sfOfferSequence) &&
          !ctx.tx.isFieldPresent(sfOfferID)) ||
         (ctx.tx.isFieldPresent(sfOfferSequence) &&
@@ -64,6 +67,7 @@ CancelOffer::preclaim(PreclaimContext const& ctx)
         return terNO_ACCOUNT;
 
     auto const offerSequence = ctx.tx[~sfOfferSequence];
+    auto const offerID = ctx.tx[~sfOfferID];
 
     if (offerSequence && (*sle)[sfSequence] <= *offerSequence)
     {
@@ -72,6 +76,26 @@ CancelOffer::preclaim(PreclaimContext const& ctx)
         return temBAD_SEQUENCE;
     }
 
+    if (offerID)
+    {
+        auto const offerkeylet = keylet::unchecked(*offerID);
+        if (auto const sleCancel = ctx.view.read(offerkeylet))
+        {
+            if (sleCancel->getFieldU16(sfLedgerEntryType) != ltOFFER)
+            {
+                JLOG(ctx.j.debug())
+                    << "OfferCancel specified non-offer ledger object";
+                return tecNO_PERMISSION;
+            }
+            else if (sleCancel->getAccountID(sfAccount) != id)
+            {
+                JLOG(ctx.j.debug())
+                    << "OfferCancel specified offer not owned by sender";
+                return tecNO_PERMISSION;
+            }
+        }
+    }
+
     return tesSUCCESS;
 }
 
@@ -100,6 +124,7 @@ CancelOffer::doApply()
         else
             JLOG(j_.debug()) << "Trying to cancel offer #" << *offerSequence;
 
+        // checked in preclaim
         if (sleOffer->getFieldU16(sfLedgerEntryType) != ltOFFER)
         {
             JLOG(j_.debug()) << "OfferCancel specified non-offer ledger object";

src/xrpld/app/tx/detail/CreateOffer.cpp

@@ -181,6 +181,25 @@ CreateOffer::preclaim(PreclaimContext const& ctx)
     if (offerID && cancelSequence)
         return temBAD_SEQUENCE;
 
+    if (offerID)
+    {
+        if (auto const sleCancel = ctx.view.read(keylet::unchecked(*offerID)))
+        {
+            if (sleCancel->getFieldU16(sfLedgerEntryType) != ltOFFER)
+            {
+                JLOG(ctx.j.debug())
+                    << "OfferCreate specified non-offer ledger object";
+                return tecNO_PERMISSION;
+            }
+            else if (sleCancel->getAccountID(sfAccount) != id)
+            {
+                JLOG(ctx.j.debug())
+                    << "OfferCreate specified offer not owned by sender";
+                return tecNO_PERMISSION;
+            }
+        }
+    }
+
     // This can probably be simplified to make sure that you cancel sequences
     // before the transaction sequence number.
     if (cancelSequence && (uAccountSequence <= *cancelSequence))