diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index aa53c6173..dfdd139df 100644 --- a/src/test/app/Offer_test.cpp +++ b/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 diff --git a/src/xrpld/app/tx/detail/CancelOffer.cpp b/src/xrpld/app/tx/detail/CancelOffer.cpp index 1b7e7fb6e..712451652 100644 --- a/src/xrpld/app/tx/detail/CancelOffer.cpp +++ b/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"; diff --git a/src/xrpld/app/tx/detail/CreateOffer.cpp b/src/xrpld/app/tx/detail/CreateOffer.cpp index 76f356a76..26483121d 100644 --- a/src/xrpld/app/tx/detail/CreateOffer.cpp +++ b/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))