From 7618b726b3c8149adcf51d5cb369a973130239e3 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Tue, 21 Apr 2026 07:06:17 +0200 Subject: [PATCH] fix: bind batch signer signatures to outer account and sequence --- include/xrpl/protocol/Batch.h | 10 ++- src/libxrpl/protocol/STTx.cpp | 18 +++- src/test/app/Batch_test.cpp | 156 +++++++++++++++++++++++++++++++++- src/test/jtx/impl/batch.cpp | 15 +++- 4 files changed, 192 insertions(+), 7 deletions(-) diff --git a/include/xrpl/protocol/Batch.h b/include/xrpl/protocol/Batch.h index 00cbf641da..7b6a159e64 100644 --- a/include/xrpl/protocol/Batch.h +++ b/include/xrpl/protocol/Batch.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -7,9 +8,16 @@ namespace xrpl { inline void -serializeBatch(Serializer& msg, std::uint32_t const& flags, std::vector const& txids) +serializeBatch( + Serializer& msg, + AccountID const& outerAccount, + std::uint32_t outerSeqValue, + std::uint32_t const& flags, + std::vector const& txids) { msg.add32(HashPrefix::batch); + msg.addBitString(outerAccount); + msg.add32(outerSeqValue); msg.add32(flags); msg.add32(std::uint32_t(txids.size())); for (auto const& txid : txids) diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index dd150aaca6..0fa9992d2c 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -435,7 +435,12 @@ Expected STTx::checkBatchSingleSign(STObject const& batchSigner) const { Serializer msg; - serializeBatch(msg, getFlags(), getBatchTransactionIDs()); + serializeBatch( + msg, + getAccountID(sfAccount), + getSeqValue(), + getFlags(), + getBatchTransactionIDs()); finishMultiSigningData(batchSigner.getAccountID(sfAccount), msg); return singleSignHelper(batchSigner, msg.slice()); } @@ -523,11 +528,18 @@ STTx::checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const // We can ease the computational load inside the loop a bit by // pre-constructing part of the data that we hash. Fill a Serializer // with the stuff that stays constant from signature to signature. + auto const batchSignerAccount = batchSigner.getAccountID(sfAccount); Serializer dataStart; - serializeBatch(dataStart, getFlags(), getBatchTransactionIDs()); + serializeBatch( + dataStart, + getAccountID(sfAccount), + getSeqValue(), + getFlags(), + getBatchTransactionIDs()); + dataStart.addBitString(batchSignerAccount); return multiSignHelper( batchSigner, - std::nullopt, + std::optional(batchSignerAccount), [&dataStart](AccountID const& accountID) -> Serializer { Serializer s = dataStart; finishMultiSigningData(accountID, s); diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 9d72bc84e2..25f3a399f0 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -570,7 +570,12 @@ class Batch_test : public beast::unit_test::suite batch::inner(pay(bob, alice, XRP(5)), bobSeq)); Serializer msg; - serializeBatch(msg, tfAllOrNothing, jt.stx->getBatchTransactionIDs()); + serializeBatch( + msg, + jt.stx->getAccountID(sfAccount), + jt.stx->getSeqValue(), + tfAllOrNothing, + jt.stx->getBatchTransactionIDs()); finishMultiSigningData(bob.id(), msg); auto const sig = xrpl::sign(bob.pk(), bob.sk(), msg.slice()); jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfAccount.jsonName] = @@ -4459,6 +4464,154 @@ class Batch_test : public beast::unit_test::suite } } + void + testOuterBinding(FeatureBitset features) + { + testcase("outer binding"); + + using namespace test::jtx; + + // Signatures captured from one outer account cannot be replayed + // under a different outer account. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + auto const eve = Account("eve"); + env.fund(XRP(10000), alice, bob, carol, eve); + env.close(); + + auto const preCarol = env.balance(carol); + auto const bobSeq = env.seq(bob); + auto const carolSeq = env.seq(carol); + + auto const aliceSeq = env.seq(alice); + auto const batchFee1 = batch::calcBatchFee(env, 2, 2); + auto jt1 = env.jt( + batch::outer(alice, aliceSeq, batchFee1, tfOnlyOne), + batch::inner(pay(bob, alice, XRP(100)), bobSeq), + batch::inner(pay(carol, alice, XRP(50)), carolSeq), + batch::sig(bob, carol)); + + auto const capturedSigners = jt1.jv[sfBatchSigners.jsonName]; + + env(jt1, ter(tesSUCCESS)); + env.close(); + BEAST_EXPECT(env.seq(bob) == bobSeq + 1); + BEAST_EXPECT(env.seq(carol) == carolSeq); + + auto const batchFee2 = batch::calcBatchFee(env, 2, 2); + auto jt2 = env.jtnofill( + batch::outer(eve, env.seq(eve), batchFee2, tfOnlyOne), + batch::inner(pay(bob, alice, XRP(100)), bobSeq), + batch::inner(pay(carol, alice, XRP(50)), carolSeq)); + + jt2.jv[sfBatchSigners.jsonName] = capturedSigners; + env(jt2.jv, ter(temBAD_SIGNATURE)); + env.close(); + + BEAST_EXPECT(env.seq(carol) == carolSeq); + BEAST_EXPECT(env.balance(carol) == preCarol); + } + + // Signatures are bound to the outer sequence; replaying them + // at a higher sequence must fail. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + env.fund(XRP(10000), alice, bob, carol); + env.close(); + + auto const preCarol = env.balance(carol); + auto const bobSeq = env.seq(bob); + auto const carolSeq = env.seq(carol); + + auto const aliceSeq1 = env.seq(alice); + auto const batchFee1 = batch::calcBatchFee(env, 2, 2); + auto jt1 = env.jt( + batch::outer(alice, aliceSeq1, batchFee1, tfOnlyOne), + batch::inner(pay(bob, alice, XRP(500)), bobSeq), + batch::inner(pay(carol, alice, XRP(500)), carolSeq), + batch::sig(bob, carol)); + + auto const capturedSigners = jt1.jv[sfBatchSigners.jsonName]; + + env(jt1, ter(tesSUCCESS)); + env.close(); + BEAST_EXPECT(env.seq(bob) == bobSeq + 1); + BEAST_EXPECT(env.seq(carol) == carolSeq); + + auto const batchFee2 = batch::calcBatchFee(env, 2, 2); + auto jt2 = env.jtnofill( + batch::outer(alice, env.seq(alice), batchFee2, tfOnlyOne), + batch::inner(pay(bob, alice, XRP(500)), bobSeq), + batch::inner(pay(carol, alice, XRP(500)), carolSeq)); + + jt2.jv[sfBatchSigners.jsonName] = capturedSigners; + env(jt2.jv, ter(temBAD_SIGNATURE)); + env.close(); + + BEAST_EXPECT(env.balance(carol) == preCarol); + BEAST_EXPECT(env.seq(carol) == carolSeq); + } + + // Multi-signed batch signer entries are bound to their account; + // reusing inner signatures under a different batch signer must fail. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + auto const dave = Account("dave"); + auto const elsa = Account("elsa"); + env.fund(XRP(10000), alice, bob, carol, dave, elsa); + env.close(); + + env(signers(bob, 2, {{dave, 1}, {elsa, 1}})); + env.close(); + env(signers(carol, 2, {{dave, 1}, {elsa, 1}})); + env.close(); + + auto const seq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, 3, 2); + auto jt1 = env.jt( + batch::outer(alice, seq, batchFee, tfAllOrNothing), + batch::inner(pay(alice, bob, XRP(10)), seq + 1), + batch::inner(pay(bob, alice, XRP(5)), env.seq(bob)), + batch::msig(bob, {dave, elsa}), + ter(tesSUCCESS)); + + auto const bobSignerEntry = + jt1.jv[sfBatchSigners.jsonName][0u]; + + env(jt1, ter(tesSUCCESS)); + env.close(); + + auto const seq2 = env.seq(alice); + auto const batchFee2 = batch::calcBatchFee(env, 3, 2); + auto jt2 = env.jtnofill( + batch::outer(alice, seq2, batchFee2, tfAllOrNothing), + batch::inner(pay(alice, carol, XRP(10)), seq2 + 1), + batch::inner(pay(carol, alice, XRP(5)), env.seq(carol))); + + Json::Value carolSigner; + carolSigner[sfBatchSigner.jsonName][jss::Account] = carol.human(); + carolSigner[sfBatchSigner.jsonName][jss::SigningPubKey] = ""; + carolSigner[sfBatchSigner.jsonName][sfSigners.jsonName] = + bobSignerEntry[sfBatchSigner.jsonName][sfSigners.jsonName]; + + jt2.jv[sfBatchSigners.jsonName][0u] = carolSigner; + env(jt2.jv, ter(temBAD_SIGNATURE)); + env.close(); + } + } + void testUnsortedBatchSigners(FeatureBitset features) { @@ -4529,6 +4682,7 @@ class Batch_test : public beast::unit_test::suite testValidateRPCResponse(features); testBatchCalculateBaseFee(features); testStandaloneInnerBatchFlag(features); + testOuterBinding(features); testUnsortedBatchSigners(features); } diff --git a/src/test/jtx/impl/batch.cpp b/src/test/jtx/impl/batch.cpp index c21711ab17..d0a4412173 100644 --- a/src/test/jtx/impl/batch.cpp +++ b/src/test/jtx/impl/batch.cpp @@ -73,7 +73,12 @@ sig::operator()(Env& env, JTx& jt) const jo[jss::SigningPubKey] = strHex(e.sig.pk().slice()); Serializer msg; - serializeBatch(msg, stx.getFlags(), stx.getBatchTransactionIDs()); + serializeBatch( + msg, + stx.getAccountID(sfAccount), + stx.getSeqValue(), + stx.getFlags(), + stx.getBatchTransactionIDs()); finishMultiSigningData(e.acct.id(), msg); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) auto const sig = xrpl::sign(*publicKeyType(e.sig.pk().slice()), e.sig.sk(), msg.slice()); @@ -112,7 +117,13 @@ msig::operator()(Env& env, JTx& jt) const iso[jss::SigningPubKey] = strHex(e.sig.pk().slice()); Serializer msg; - serializeBatch(msg, stx.getFlags(), stx.getBatchTransactionIDs()); + serializeBatch( + msg, + stx.getAccountID(sfAccount), + stx.getSeqValue(), + stx.getFlags(), + stx.getBatchTransactionIDs()); + msg.addBitString(master.id()); finishMultiSigningData(e.acct.id(), msg); // NOLINTNEXTLINE(bugprone-unchecked-optional-access) auto const sig = xrpl::sign(*publicKeyType(e.sig.pk().slice()), e.sig.sk(), msg.slice());