diff --git a/include/xrpl/tx/transactors/proposal/ProposalHelpers.h b/include/xrpl/ledger/helpers/ProposalHelpers.h similarity index 79% rename from include/xrpl/tx/transactors/proposal/ProposalHelpers.h rename to include/xrpl/ledger/helpers/ProposalHelpers.h index 8ca1f88fdd..0a09dfd887 100644 --- a/include/xrpl/tx/transactors/proposal/ProposalHelpers.h +++ b/include/xrpl/ledger/helpers/ProposalHelpers.h @@ -8,6 +8,28 @@ namespace xrpl::proposal { +/** + * Owner-reserve increments held by a proposal of an ordinary transaction. + */ +constexpr std::uint32_t kProposalOwnerCount = 5; + +/** + * Owner-reserve increments held by a proposal of a Batch transaction. A + * proposed Batch stores up to eight inner transactions plus multi-account + * signatures, so it reserves more than an ordinary proposed transaction. + */ +constexpr std::uint32_t kBatchProposalOwnerCount = 10; + +/** + * Owner-reserve increments held by a proposal of the given transaction. + */ +inline std::uint32_t +proposalOwnerCount(STObject const& proposedTx) +{ + return proposedTx.getFieldU16(sfTransactionType) == ttBATCH ? kBatchProposalOwnerCount + : kProposalOwnerCount; +} + /** * Whether the proposed transaction is itself a proposal transaction, which * would nest one proposal inside another. @@ -21,6 +43,19 @@ isProposalTx(STObject const& proposedTx) return proposedTx.getFieldU16(sfTransactionType) == ttTRANSACTION_PROPOSAL_CREATE; } +/** + * Whether the proposed transaction is independently submittable through the + * ordinary multi-sign path: not a nested proposal, not a pseudo-transaction, + * not itself flagged as someone else's inner batch transaction, and — if it + * is a Batch — none of its own inner transactions is a nested proposal or a + * pseudo-transaction either. A Batch inner transaction cannot itself be + * pseudo (preflight0 rejects the pseudo/tfInnerBatchTxn combination + * generically), but that guard lives outside this feature, so it is checked + * again here rather than relied upon. + */ +bool +isValidProposal(STObject const& proposedTx); + /** * Whether the proposed transaction carries any signature field. * @@ -49,26 +84,4 @@ hasEmptySigningPubKey(STObject const& proposedTx) proposedTx.getFieldVL(sfSigningPubKey).empty(); } -/** - * Owner-reserve increments held by a proposal of an ordinary transaction. - */ -constexpr std::uint32_t kProposalOwnerCount = 5; - -/** - * Owner-reserve increments held by a proposal of a Batch transaction. A - * proposed Batch stores up to eight inner transactions plus multi-account - * signatures, so it reserves more than an ordinary proposed transaction. - */ -constexpr std::uint32_t kBatchProposalOwnerCount = 10; - -/** - * Owner-reserve increments held by a proposal of the given transaction. - */ -inline std::uint32_t -proposalOwnerCount(STObject const& proposedTx) -{ - return proposedTx.getFieldU16(sfTransactionType) == ttBATCH ? kBatchProposalOwnerCount - : kProposalOwnerCount; -} - } // namespace xrpl::proposal diff --git a/src/libxrpl/ledger/helpers/ProposalHelpers.cpp b/src/libxrpl/ledger/helpers/ProposalHelpers.cpp new file mode 100644 index 0000000000..4adf57ab3a --- /dev/null +++ b/src/libxrpl/ledger/helpers/ProposalHelpers.cpp @@ -0,0 +1,39 @@ +#include + +#include +#include +#include +#include +#include +#include + +namespace xrpl::proposal { + +bool +isValidProposal(STObject const& proposedTx) +{ + if (isProposalTx(proposedTx)) + return false; + + if (isPseudoTx(proposedTx)) + return false; + + if (proposedTx.isFieldPresent(sfFlags) && + (proposedTx.getFieldU32(sfFlags) & tfInnerBatchTxn) != 0u) + return false; + + if (proposedTx.getFieldU16(sfTransactionType) == ttBATCH && + proposedTx.isFieldPresent(sfRawTransactions)) + { + STArray const& innerTxns = proposedTx.getFieldArray(sfRawTransactions); + for (STObject const& inner : innerTxns) + { + if (isProposalTx(inner) || isPseudoTx(inner)) + return false; + } + } + + return true; +} + +} // namespace xrpl::proposal diff --git a/src/libxrpl/ledger/helpers/SponsorHelpers.cpp b/src/libxrpl/ledger/helpers/SponsorHelpers.cpp index 7e0c041854..433a5f74b5 100644 --- a/src/libxrpl/ledger/helpers/SponsorHelpers.cpp +++ b/src/libxrpl/ledger/helpers/SponsorHelpers.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -56,6 +57,7 @@ isReserveSponsorAllowed(TxType txType) ttACCOUNT_SET, ttREGULAR_KEY_SET, ttSPONSORSHIP_TRANSFER, + ttTRANSACTION_PROPOSAL_CREATE, }; return kReserveSponsorAllowed.contains(txType); } @@ -255,6 +257,8 @@ isLedgerEntryOwner(ReadView const& view, SLE const& sle, AccountID const& accoun // to tecNO_PERMISSION. return false; } + case ltTRANSACTION_PROPOSAL: + return sle.getAccountID(sfOwner) == account; default: // LCOV_EXCL_START UNREACHABLE("xrpl::isLedgerEntryOwner : object is not supported by sponsorship."); @@ -278,6 +282,7 @@ isLedgerEntrySupportedBySponsorship(SLE const& sle) case ltSIGNER_LIST: case ltCREDENTIAL: case ltRIPPLE_STATE: + case ltTRANSACTION_PROPOSAL: return true; default: return false; @@ -304,6 +309,11 @@ getLedgerEntryOwnerCount(SLE const& sle) return 1; return 2 + static_cast(sle.getFieldArray(sfSignerEntries).size()); } + case ltTRANSACTION_PROPOSAL: + // Mirror TransactionProposalCreate's own reserve sizing so that + // creation and sponsorship accounting agree: a proposed Batch + // reserves more than an ordinary proposal. + return proposal::proposalOwnerCount(sle.getFieldObject(sfProposedTransaction)); case ltACCOUNT_ROOT: // LCOV_EXCL_START UNREACHABLE("AccountRoots are not supported by object sponsorship."); diff --git a/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp b/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp index cc404d154f..1b6c5b3986 100644 --- a/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp +++ b/src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp @@ -5,7 +5,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -15,14 +17,15 @@ #include #include #include -#include #include +#include #include #include -#include +#include #include #include +#include #include namespace xrpl { @@ -47,25 +50,13 @@ TransactionProposalCreate::preflight(PreflightContext const& ctx) // The proposed transaction must be independently submittable through the // ordinary multi-sign path: no nested proposals, no pseudo-transactions, - // no batch inner transactions. - if (proposal::isProposalTx(proposedTx)) + // no batch inner transactions — and, if it is a Batch, none of its own + // inner transactions may be a nested proposal or a pseudo-transaction + // either. + if (!proposal::isValidProposal(proposedTx)) { - JLOG(ctx.j.debug()) << "TransactionProposalCreate: nested proposal."; - return temINVALID; - } - - if (isPseudoTx(proposedTx)) - { - JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn is a " - "pseudo-transaction."; - return temINVALID; - } - - if (proposedTx.isFieldPresent(sfFlags) && - ((proposedTx.getFieldU32(sfFlags) & tfInnerBatchTxn) != 0u)) - { - JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn " - "carries tfInnerBatchTxn."; + JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn is not " + "independently submittable."; return temINVALID; } @@ -174,8 +165,76 @@ TransactionProposalCreate::preclaim(PreclaimContext const& ctx) if (isPseudoAccount(sleTarget)) return tecNO_PERMISSION; + // Only the target account itself, an account on its SignerList, or (if + // the proposed transaction's own type has been delegated by the target, + // Permission Delegation / XLS-75) that delegate or an account on the + // delegate's own SignerList, may create a proposal against it. Otherwise + // any account could spam or squat the target's Tickets with unwanted + // proposals (On-Chain Cosigner V1 scope). + if (AccountID const proposer = ctx.tx.getAccountID(sfAccount); proposer != target) + { + // Whether `proposer` is `account` itself or an entry on `account`'s + // applicable SignerList. + auto isAuthorizedFor = [&](AccountID const& account) -> std::expected { + if (proposer == account) + return true; + + auto const sleSigners = ctx.view.read(keylet::signerList(account)); + if (!sleSigners) + return false; + + auto const accountSigners = SignerEntries::deserialize(*sleSigners, ctx.j, "ledger"); + if (!accountSigners) + return std::unexpected(TER{accountSigners.error()}); + + return std::ranges::any_of( + *accountSigners, [&](auto const& entry) { return entry.account == proposer; }); + }; + + auto isSigner = isAuthorizedFor(target); + if (!isSigner) + return isSigner.error(); + + // A delegate that the target has granted permission over the + // proposed transaction's own type — or one of that delegate's own + // signers — is equally authorized: it will need to help complete + // the proposed transaction's own authorization anyway once the + // proposal is submitted. + if (!*isSigner && proposedTx.isFieldPresent(sfDelegate)) + { + AccountID const delegateAccount = proposedTx.getAccountID(sfDelegate); + // NOLINTNEXTLINE(readability-suspicious-call-argument) + auto const sleDelegate = ctx.view.read(keylet::delegate(target, delegateAccount)); + if (sleDelegate && + isTesSuccess(checkTxPermission(sleDelegate, STTx{STObject{proposedTx}}))) + { + isSigner = isAuthorizedFor(delegateAccount); + if (!isSigner) + return isSigner.error(); + } + } + + if (!*isSigner) + { + JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposer is " + "not the target account, one of its " + "signers, or an authorized delegate."; + return tecNO_PERMISSION; + } + } + std::uint32_t const ticketSequence = proposedTx.getFieldU32(sfTicketSequence); + // The proposal reserves the ticket for as long as it exists (On-Chain + // Cosigner spec §4.2.1, §5.3.2): a ticket that doesn't exist yet can't be + // reserved. + if (!ctx.view.exists(keylet::ticket(target, ticketSequence))) + { + JLOG(ctx.j.debug()) << "TransactionProposalCreate: target ticket " + "does not exist."; + return tefNO_TICKET; + } + if (ctx.view.exists(keylet::txProposal(target, ticketSequence))) { JLOG(ctx.j.debug()) << "TransactionProposalCreate: duplicate proposal."; diff --git a/src/test/app/TransactionProposalCreate_test.cpp b/src/test/app/TransactionProposalCreate_test.cpp index f6909e2e77..0dc2d04ec8 100644 --- a/src/test/app/TransactionProposalCreate_test.cpp +++ b/src/test/app/TransactionProposalCreate_test.cpp @@ -3,12 +3,15 @@ #include #include #include +#include #include +#include #include #include #include #include #include +#include #include #include #include @@ -19,7 +22,10 @@ #include #include #include +#include +#include #include +#include #include #include #include @@ -59,23 +65,24 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features - featureCosign}; - Account const alice{"alice"}; Account const target{"target"}; Account const bob{"bob"}; - env.fund(XRP(10000), alice, target, bob); + env.fund(XRP(10000), target, bob); env.close(); std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); env(proposal::create( - alice, + target, proposal::unsignedPayload(env, pay(target, bob, XRP(1)), targetTicketSeq), proposal::expiration(env, 100s)), Ter(temDISABLED)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, targetTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == 0); + // Its own Ticket is the only thing target owns; the rejected + // proposal adds nothing on top of it. + BEAST_EXPECT(ownerCount(env, target) == 1); } // The proposed transaction must be a transaction that could be submitted on @@ -92,10 +99,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; Account const target{"target"}; Account const bob{"bob"}; - env.fund(XRP(10000), alice, target, bob); + env.fund(XRP(10000), target, bob); env.close(); std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); @@ -107,11 +113,13 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite return proposal::unsignedPayload(env, pay(target, bob, XRP(1)), targetTicketSeq); }; + // target's own Ticket is the only thing it owns throughout; a + // rejected proposal never adds anything on top of it. auto reject = [&](json::Value const& proposedTx, TER expected) { - env(proposal::create(alice, proposedTx, expiration), Ter(expected)); + env(proposal::create(target, proposedTx, expiration), Ter(expected)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, targetTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, target) == 1); }; // A pseudo-transaction is never submittable by an account. @@ -135,6 +143,20 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite reject(tx, temINVALID); } + // Nor may a proposed Batch smuggle a nested proposal in as one of its + // own inner transactions. + { + json::Value const nestedProposal = + proposal::create(target, payload(), proposal::expiration(env, 100s)); + json::Value const tx = proposal::unsignedBatch( + env, + target, + targetTicketSeq, + tfAllOrNothing, + {proposal::innerTx(nestedProposal, env.seq(target))}); + reject(tx, temINVALID); + } + // The proposed transaction must be ticket-based: a missing // TicketSequence, or a live Sequence alongside it, is rejected. { @@ -158,10 +180,10 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // Expiration must be present and non-zero. { - env(proposal::create(alice, payload(), 0), Ter(temBAD_EXPIRATION)); + env(proposal::create(target, payload(), 0), Ter(temBAD_EXPIRATION)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, targetTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, target) == 1); } } @@ -183,10 +205,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; Account const target{"target"}; Account const bob{"bob"}; - env.fund(XRP(10000), alice, target, bob); + env.fund(XRP(10000), target, bob); env.close(); std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); @@ -206,8 +227,11 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite return proposal::unsignedPayload(env, pay(target, bob, drops(++paid)), targetTicketSeq); }; auto sponsoredPayment = [&]() { + // bob is just standing in for an arbitrary sponsor here; every case + // is rejected for carrying a signature field before the sponsor + // itself is ever examined. json::Value tx = pay(target, bob, drops(++paid)); - tx[sfSponsor.getJsonName()] = alice.human(); + tx[sfSponsor.getJsonName()] = bob.human(); tx[sfSponsorFlags.getJsonName()] = spfSponsorFee; return proposal::unsignedPayload(env, tx, targetTicketSeq); }; @@ -227,10 +251,10 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite }; auto reject = [&](json::Value const& proposedTx) { - env(proposal::create(alice, proposedTx, expiration), Ter(temBAD_SIGNER)); + env(proposal::create(target, proposedTx, expiration), Ter(temBAD_SIGNER)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, targetTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, target) == 1); }; // Every way of filling in a signature. The payload's own signature @@ -282,15 +306,15 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite }; std::vector const places{ - {payment, [](json::Value& tx) -> json::Value& { return tx; }}, - {loanSet, - [](json::Value& tx) -> json::Value& { + {.payload = payment, .at = [](json::Value& tx) -> json::Value& { return tx; }}, + {.payload = loanSet, + .at = [](json::Value& tx) -> json::Value& { auto& o = tx[sfCounterpartySignature.getJsonName()]; o = json::Value{json::ValueType::Object}; return o; }}, - {sponsoredPayment, - [](json::Value& tx) -> json::Value& { + {.payload = sponsoredPayment, + .at = [](json::Value& tx) -> json::Value& { auto& o = tx[sfSponsorSignature.getJsonName()]; o = json::Value{json::ValueType::Object}; return o; @@ -298,8 +322,8 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // A BatchSigners entry names the account it speaks for; the other // two co-signatures are fixed by the transaction they belong to and // do not. - {batchTx, - [&](json::Value& tx) -> json::Value& { + {.payload = batchTx, + .at = [&](json::Value& tx) -> json::Value& { auto& o = tx[sfBatchSigners.getJsonName()][0u][sfBatchSigner.getJsonName()]; o[jss::Account] = bob.human(); return o; @@ -356,11 +380,10 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; Account const target{"target"}; Account const bob{"bob"}; Account const carol{"carol"}; // never funded - env.fund(XRP(10000), alice, target, bob); + env.fund(XRP(10000), target, bob); env.close(); std::uint32_t const firstTicketSeq = proposal::createTicket(env, target, 3); @@ -371,13 +394,17 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite return proposal::unsignedPayload(env, pay(target, bob, XRP(1)), ticketSeq); }; + // target's three Tickets are owned throughout, so its OwnerCount + // never drops below 3; each successful proposal adds kProposalOwnerCount + // on top of that baseline. + // The proposal's own expiration has already passed. { - env(proposal::create(alice, payload(firstTicketSeq), proposal::expiration(env, 0s)), + env(proposal::create(target, payload(firstTicketSeq), proposal::expiration(env, 0s)), Ter(tecEXPIRED)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, firstTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, target) == 3); } // The proposed transaction's own ledger bound has passed: the ordinary @@ -385,10 +412,10 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite { json::Value tx = payload(firstTicketSeq); tx[sfLastLedgerSequence.getJsonName()] = env.current()->seq() - 1; - env(proposal::create(alice, tx, expiration), Ter(tecEXPIRED)); + env(proposal::create(target, tx, expiration), Ter(tecEXPIRED)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, firstTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, target) == 3); } // A LastLedgerSequence equal to the current ledger leaves no window to @@ -398,44 +425,250 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite { json::Value tx = payload(firstTicketSeq); tx[sfLastLedgerSequence.getJsonName()] = env.current()->seq(); - env(proposal::create(alice, tx, expiration), Ter(tecEXPIRED)); + env(proposal::create(target, tx, expiration), Ter(tecEXPIRED)); env.close(); BEAST_EXPECT(!proposal::entry(env, target, firstTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, target) == 3); } // With no ledger bound on the proposed transaction, the proposal is // created normally. { - env(proposal::create(alice, payload(firstTicketSeq), expiration)); + env(proposal::create(target, payload(firstTicketSeq), expiration)); env.close(); BEAST_EXPECT(proposal::entry(env, target, firstTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == proposal::kProposalOwnerCount); + BEAST_EXPECT(ownerCount(env, target) == 3 + proposal::kProposalOwnerCount); } // The target and ticket already carry a proposal. { - env(proposal::create(alice, payload(firstTicketSeq), expiration), Ter(tecDUPLICATE)); + env(proposal::create(target, payload(firstTicketSeq), expiration), Ter(tecDUPLICATE)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == proposal::kProposalOwnerCount); + BEAST_EXPECT(ownerCount(env, target) == 3 + proposal::kProposalOwnerCount); } // A different ticket of the same target is a different proposal. { - env(proposal::create(alice, payload(firstTicketSeq + 1), expiration)); + env(proposal::create(target, payload(firstTicketSeq + 1), expiration)); env.close(); BEAST_EXPECT(proposal::entry(env, target, firstTicketSeq + 1)); - BEAST_EXPECT(ownerCount(env, alice) == 2 * proposal::kProposalOwnerCount); + BEAST_EXPECT(ownerCount(env, target) == 3 + (2 * proposal::kProposalOwnerCount)); } - // The target account does not exist, so it can never sign. + // The target account does not exist, so it can never sign. target + // itself is just standing in here as an arbitrary funded submitter — + // the account under test is carol, the (nonexistent) target. { env(proposal::create( - alice, proposal::unsignedPayload(env, pay(carol, bob, XRP(1)), 1), expiration), + target, proposal::unsignedPayload(env, pay(carol, bob, XRP(1)), 1), expiration), Ter(tecNO_TARGET)); env.close(); BEAST_EXPECT(!proposal::entry(env, carol, 1)); - BEAST_EXPECT(ownerCount(env, alice) == 2 * proposal::kProposalOwnerCount); + BEAST_EXPECT(ownerCount(env, target) == 3 + (2 * proposal::kProposalOwnerCount)); + } + + // The referenced ticket does not exist: the proposal would reserve a + // ticket that was never created (On-Chain Cosigner spec §5.3.2). + { + std::uint32_t const noSuchTicketSeq = firstTicketSeq + 100; + env(proposal::create(target, payload(noSuchTicketSeq), expiration), Ter(tefNO_TICKET)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, noSuchTicketSeq)); + BEAST_EXPECT(ownerCount(env, target) == 3 + (2 * proposal::kProposalOwnerCount)); + } + } + + // Only the target account itself, or an account on its SignerList, may + // create a proposal against it. Otherwise any unrelated account could + // spam or squat the target's Tickets with unwanted proposals (On-Chain + // Cosigner V1 authorization scope). + void + testProposerAuthorization(FeatureBitset features) + { + testcase("reject proposal from an unauthorized proposer"); + + using namespace jtx; + using namespace std::chrono_literals; + + Env env{*this, features}; + + Account const target{"target"}; + Account const signer{"signer"}; + Account const stranger{"stranger"}; + Account const bob{"bob"}; + env.fund(XRP(10000), target, signer, stranger, bob); + env.close(); + + env(signers(target, 1, {{signer, 1}})); + env.close(); + + auto payload = [&](std::uint32_t ticketSeq) { + return proposal::unsignedPayload(env, pay(target, bob, XRP(1)), ticketSeq); + }; + + // The target account itself needs no SignerList entry. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create(target, payload(ticketSeq), proposal::expiration(env, 100s))); + env.close(); + BEAST_EXPECT(proposal::entry(env, target, ticketSeq)); + } + + // An account on the target's SignerList may propose for it. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create(signer, payload(ticketSeq), proposal::expiration(env, 100s))); + env.close(); + BEAST_EXPECT(proposal::entry(env, target, ticketSeq)); + } + + // An account that is neither the target nor on its SignerList may not. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create(stranger, payload(ticketSeq), proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, ticketSeq)); + BEAST_EXPECT(ownerCount(env, stranger) == 0); + } + + // A target with no SignerList at all may only be proposed for by + // itself. + { + Account const bare{"bare"}; + env.fund(XRP(10000), bare); + env.close(); + + std::uint32_t const ticketSeq = proposal::createTicket(env, bare); + env(proposal::create( + stranger, + proposal::unsignedPayload(env, pay(bare, bob, XRP(1)), ticketSeq), + proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, bare, ticketSeq)); + } + + // A SignerList with several entries authorizes every one of them, not + // just the first, matching a real-world multi-signer setup rather + // than only ever exercising a single-signer list. + { + Account const s1{"s1"}; + Account const s2{"s2"}; + Account const s3{"s3"}; + Account const s4{"s4"}; + Account const s5{"s5"}; + env.fund(XRP(10000), s1, s2, s3, s4, s5); + env.close(); + + env(signers(target, 3, {{s1, 1}, {s2, 1}, {s3, 1}, {s4, 1}, {s5, 1}})); + env.close(); + + for (Account const& s : {s1, s2, s3, s4, s5}) + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create(s, payload(ticketSeq), proposal::expiration(env, 100s))); + env.close(); + BEAST_EXPECT(proposal::entry(env, target, ticketSeq)); + } + + // The old SignerList's sole signer is no longer on the new one. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create(signer, payload(ticketSeq), proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, ticketSeq)); + } + + // An unrelated account still may not. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create(stranger, payload(ticketSeq), proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, ticketSeq)); + } + } + } + + // The target account may delegate authority over the proposed + // transaction's own type to another account (Permission Delegation, + // XLS-75); if it does, that delegate — or an account on the delegate's + // own SignerList — may also create the proposal, since it will need to + // help complete the proposed transaction's own authorization anyway. + // Naming an account as Delegate in the proposed transaction is not + // itself trusted: a real DelegateSet grant is required. + void + testDelegatedProposedTx(FeatureBitset features) + { + testcase("proposer authorized through a delegated proposed txn"); + + using namespace jtx; + using namespace std::chrono_literals; + + Env env{*this, features}; + + Account const target{"target"}; + Account const delegateAcct{"delegateAcct"}; + Account const ds1{"ds1"}; // on delegateAcct's own SignerList + Account const ds2{"ds2"}; // on delegateAcct's own SignerList + Account const stranger{"stranger"}; + Account const bob{"bob"}; + env.fund(XRP(10000), target, delegateAcct, ds1, ds2, stranger, bob); + env.close(); + + auto delegatedPayload = [&](std::uint32_t ticketSeq) { + json::Value tx = pay(target, bob, XRP(1)); + tx[sfDelegate.jsonName] = delegateAcct.human(); + return proposal::unsignedPayload(env, tx, ticketSeq); + }; + + // Without a real DelegateSet grant, naming an account as Delegate in + // the proposed transaction does not authorize it. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create( + delegateAcct, delegatedPayload(ticketSeq), proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, ticketSeq)); + } + + // The target grants delegateAcct permission over Payment transactions. + env(delegate::set(target, delegateAcct, {"Payment"})); + env.close(); + + // The delegate itself may now create the proposal. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create( + delegateAcct, delegatedPayload(ticketSeq), proposal::expiration(env, 100s))); + env.close(); + BEAST_EXPECT(proposal::entry(env, target, ticketSeq)); + } + + // An account on the delegate's own SignerList may likewise create it. + { + env(signers(delegateAcct, 1, {{ds1, 1}, {ds2, 1}})); + env.close(); + + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create( + ds1, delegatedPayload(ticketSeq), proposal::expiration(env, 100s))); + env.close(); + BEAST_EXPECT(proposal::entry(env, target, ticketSeq)); + } + + // An account with no relationship to the target or the delegate is + // still rejected. + { + std::uint32_t const ticketSeq = proposal::createTicket(env, target); + env(proposal::create( + stranger, delegatedPayload(ticketSeq), proposal::expiration(env, 100s)), + Ter(tecNO_PERMISSION)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, ticketSeq)); } } @@ -490,10 +723,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; // the proposer - Account const target{"target"}; // the account the proposal is for + Account const target{"target"}; Account const bob{"bob"}; - env.fund(XRP(10000), alice, target, bob); + env.fund(XRP(10000), target, bob); env.close(); std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); @@ -506,14 +738,14 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite std::uint32_t const expiration = proposal::expiration(env, 100s); - env(proposal::create(alice, proposedTx, expiration)); + env(proposal::create(target, proposedTx, expiration)); env.close(); auto const sle = proposal::entry(env, target, targetTicketSeq); if (!BEAST_EXPECT(sle)) return; - BEAST_EXPECT(sle->getAccountID(sfOwner) == alice.id()); + BEAST_EXPECT(sle->getAccountID(sfOwner) == target.id()); BEAST_EXPECT(sle->getFieldU32(sfExpiration) == expiration); auto const stored = sle->getFieldObject(sfProposedTransaction); @@ -523,9 +755,10 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite BEAST_EXPECT(stored.getFieldVL(sfSigningPubKey).empty()); // The proposal reserves several owner increments against the proposer. - // The target only owns the Ticket used by the proposed transaction. - BEAST_EXPECT(ownerCount(env, alice) == proposal::kProposalOwnerCount); - BEAST_EXPECT(ownerCount(env, target) == 1); + // Here target is both: it owns the Ticket used by the proposed + // transaction, and it owns the proposal itself since it is proposing + // for its own account. + BEAST_EXPECT(ownerCount(env, target) == 1 + proposal::kProposalOwnerCount); } // A proposal carries a transaction of any type: what the proposal requires @@ -542,13 +775,12 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; // the proposer - Account const target{"target"}; // the account the proposals are for + Account const target{"target"}; Account const bob{"bob"}; Account const gw{"gw"}; // NOLINTNEXTLINE(readability-identifier-naming) auto const USD = gw["USD"]; - env.fund(XRP(10000), alice, target, bob, gw); + env.fund(XRP(10000), target, bob, gw); env.close(); // One payload per transaction type, each straight from the generator @@ -572,12 +804,14 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite { std::uint32_t const ticketSeq = firstTicketSeq + static_cast(i); env(proposal::create( - alice, proposal::unsignedPayload(env, payloads[i], ticketSeq), expiration)); + target, proposal::unsignedPayload(env, payloads[i], ticketSeq), expiration)); env.close(); BEAST_EXPECT(proposal::entry(env, target, ticketSeq)); } - BEAST_EXPECT(ownerCount(env, alice) == payloads.size() * proposal::kProposalOwnerCount); + // target owns one Ticket per payload plus one proposal per payload. + BEAST_EXPECT( + ownerCount(env, target) == payloads.size() * (1 + proposal::kProposalOwnerCount)); } // A proposed transaction may itself require an auxiliary co-signer beyond @@ -596,10 +830,10 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; // the proposer - Account const borrower{"borrower"}; // the target account + Account const borrower{"borrower"}; // the target account, proposing for itself + Account const bob{"bob"}; // an arbitrary sponsor placeholder - env.fund(XRP(10000), alice, borrower); + env.fund(XRP(10000), borrower, bob); env.close(); std::uint32_t const expiration = proposal::expiration(env, 100s); @@ -612,7 +846,7 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite json::Value const tx = proposal::unsignedPayload(env, loan::set(borrower, uint256{1}, 1'000), ticketSeq); - env(proposal::create(alice, tx, expiration)); + env(proposal::create(borrower, tx, expiration)); env.close(); BEAST_EXPECT(proposal::entry(env, borrower, ticketSeq)); } @@ -623,10 +857,11 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite std::uint32_t const ticketSeq = proposal::createTicket(env, borrower); json::Value tx = sponsor::transfer(borrower, tfSponsorshipCreate); - tx[sfSponsor.getJsonName()] = alice.human(); + tx[sfSponsor.getJsonName()] = bob.human(); tx[sfSponsorFlags.getJsonName()] = spfSponsorReserve; - env(proposal::create(alice, proposal::unsignedPayload(env, tx, ticketSeq), expiration)); + env(proposal::create( + borrower, proposal::unsignedPayload(env, tx, ticketSeq), expiration)); env.close(); BEAST_EXPECT(proposal::entry(env, borrower, ticketSeq)); } @@ -648,6 +883,7 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Account const bob{"bob"}; env.fund(XRP(10000), target, bob); env.close(); + proposal::authorizeProposer(env, target, alice); std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); @@ -675,6 +911,183 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, alice) == proposal::kProposalOwnerCount); } + // The proposal's reserve can instead be sponsored: the reserve is charged + // to the sponsor's account, and the ledger object records the sponsor, the + // same as any other reserve-sponsorable object (TransactionProposalCreate + // is on the reserve-sponsorship allow-list). + void + testSponsoredReserve(FeatureBitset features) + { + testcase("proposer reserve sponsored"); + + using namespace jtx; + using namespace std::chrono_literals; + + // Reserve sponsorship requires the Sponsor amendment, independent of + // Cosign: with Cosign enabled but Sponsor disabled, a proposal that + // tries to attach a sponsor is rejected before it ever reaches the + // reserve-sponsorship allow-list. + { + Env env{*this, features - featureSponsor}; + + Account const alice{"alice"}; + Account const target{"target"}; + Account const bob{"bob"}; + Account const backer{"backer"}; + env.fund(XRP(10000), alice, target, bob, backer); + env.close(); + proposal::authorizeProposer(env, target, alice); + + std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); + json::Value const proposedTx = + proposal::unsignedPayload(env, pay(target, bob, XRP(1)), targetTicketSeq); + + env(proposal::create(alice, proposedTx, proposal::expiration(env, 100s)), + sponsor::As(backer, spfSponsorReserve), + Sig(sfSponsorSignature, backer), + Ter(temDISABLED)); + env.close(); + BEAST_EXPECT(!proposal::entry(env, target, targetTicketSeq)); + } + + Env env{*this, features}; + + Account const alice{"alice"}; // the proposer + Account const target{"target"}; // the account the proposal is for + Account const bob{"bob"}; + Account const backer{"backer"}; // sponsors alice's proposal reserve + + env.fund(XRP(10000), alice, target, bob, backer); + env.close(); + proposal::authorizeProposer(env, target, alice); + + std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); + json::Value const proposedTx = + proposal::unsignedPayload(env, pay(target, bob, XRP(1)), targetTicketSeq); + + env(proposal::create(alice, proposedTx, proposal::expiration(env, 100s)), + sponsor::As(backer, spfSponsorReserve), + Sig(sfSponsorSignature, backer)); + env.close(); + + auto const sle = proposal::entry(env, target, targetTicketSeq); + if (!BEAST_EXPECT(sle)) + return; + + BEAST_EXPECT(sle->isFieldPresent(sfSponsor)); + BEAST_EXPECT(sle->getAccountID(sfSponsor) == backer.id()); + + // alice still owns the proposal — her OwnerCount reflects that, same + // as an unsponsored proposal. What moves to the sponsor is the + // reserve requirement itself, tracked separately: alice's owner count + // is covered by backer's sponsorship rather than her own balance. + BEAST_EXPECT(ownerCount(env, alice) == proposal::kProposalOwnerCount); + BEAST_EXPECT(ownerCount(env, backer) == 0); + BEAST_EXPECT(sponsoredOwnerCount(env, alice) == proposal::kProposalOwnerCount); + BEAST_EXPECT(sponsoringOwnerCount(env, backer) == proposal::kProposalOwnerCount); + } + + // A proposal's sponsored reserve can be reassigned to a new sponsor + // through SponsorshipTransfer, the same as any other reserve-sponsored + // ledger entry. + void + testSponsorshipTransfer(FeatureBitset features) + { + testcase("proposer reserve sponsorship transferred"); + + using namespace jtx; + using namespace std::chrono_literals; + + Env env{*this, features}; + + Account const alice{"alice"}; // the proposer + Account const target{"target"}; // the account the proposal is for + Account const bob{"bob"}; + Account const backer1{"backer1"}; // the original sponsor + Account const backer2{"backer2"}; // the new sponsor + + env.fund(XRP(10000), alice, target, bob, backer1, backer2); + env.close(); + proposal::authorizeProposer(env, target, alice); + + std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); + json::Value const proposedTx = + proposal::unsignedPayload(env, pay(target, bob, XRP(1)), targetTicketSeq); + + env(proposal::create(alice, proposedTx, proposal::expiration(env, 100s)), + sponsor::As(backer1, spfSponsorReserve), + Sig(sfSponsorSignature, backer1)); + env.close(); + + BEAST_EXPECT(sponsoringOwnerCount(env, backer1) == proposal::kProposalOwnerCount); + BEAST_EXPECT(sponsoringOwnerCount(env, backer2) == 0); + + Keylet const proposalKeylet = keylet::txProposal(target.id(), targetTicketSeq); + + env(sponsor::transfer(alice, tfSponsorshipReassign, proposalKeylet.key), + sponsor::As(backer2, spfSponsorReserve), + Sig(sfSponsorSignature, backer2)); + env.close(); + + auto const sle = proposal::entry(env, target, targetTicketSeq); + if (!BEAST_EXPECT(sle)) + return; + + BEAST_EXPECT(sle->isFieldPresent(sfSponsor)); + BEAST_EXPECT(sle->getAccountID(sfSponsor) == backer2.id()); + + // alice's own OwnerCount is unaffected by the reassignment: only the + // sponsor of the redirected reserve changes. + BEAST_EXPECT(ownerCount(env, alice) == proposal::kProposalOwnerCount); + BEAST_EXPECT(sponsoredOwnerCount(env, alice) == proposal::kProposalOwnerCount); + BEAST_EXPECT(sponsoringOwnerCount(env, backer1) == 0); + BEAST_EXPECT(sponsoringOwnerCount(env, backer2) == proposal::kProposalOwnerCount); + } + + // TransactionProposalCreate's own transaction fee can be sponsored like + // any other transaction's, independent of whether its reserve is + // sponsored (On-Chain Cosigner spec sponsorship is orthogonal to fee + // sponsorship). + void + testFeeSponsored(FeatureBitset features) + { + testcase("proposal creation fee sponsored"); + + using namespace jtx; + using namespace std::chrono_literals; + + Env env{*this, features}; + + Account const target{"target"}; // the proposer, proposing for itself + Account const bob{"bob"}; + Account const backer{"backer"}; // sponsors target's transaction fee + + env.fund(XRP(10000), target, bob, backer); + env.close(); + + std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); + json::Value const proposedTx = + proposal::unsignedPayload(env, pay(target, bob, XRP(1)), targetTicketSeq); + + auto const targetBalance = env.balance(target); + auto const backerBalance = env.balance(backer); + // A generous fixed fee: the exact amount isn't the point of this + // test, only that the sponsor pays it instead of the proposer, so it + // should comfortably clear the minimum even under local fee escalation + // rather than assume the reference fee is some specific small value. + STAmount const feeAmt = XRP(1); + + env(proposal::create(target, proposedTx, proposal::expiration(env, 100s)), + Fee(feeAmt), + sponsor::As(backer, spfSponsorFee), + Sig(sfSponsorSignature, backer)); + env.close(); + + BEAST_EXPECT(proposal::entry(env, target, targetTicketSeq)); + BEAST_EXPECT(env.balance(target) == targetBalance); + BEAST_EXPECT(env.balance(backer) == backerBalance - feeAmt); + } + // A proposed Batch holds several inner transactions and the signatures of // every account they touch, so it reserves more than an ordinary proposal. void @@ -687,10 +1100,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; Account const target{"target"}; Account const bob{"bob"}; - env.fund(XRP(10000), alice, target, bob); + env.fund(XRP(10000), target, bob); env.close(); std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); @@ -705,11 +1117,12 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite {proposal::innerTx(pay(target, bob, XRP(1)), env.seq(target)), proposal::innerTx(pay(target, bob, XRP(1)), env.seq(target) + 1)}); - env(proposal::create(alice, proposedTx, proposal::expiration(env, 100s))); + env(proposal::create(target, proposedTx, proposal::expiration(env, 100s))); env.close(); BEAST_EXPECT(proposal::entry(env, target, targetTicketSeq)); - BEAST_EXPECT(ownerCount(env, alice) == proposal::kBatchProposalOwnerCount); + // target owns its own Ticket plus the batch proposal. + BEAST_EXPECT(ownerCount(env, target) == 1 + proposal::kBatchProposalOwnerCount); } // A multi-account Batch is the primary motivating case (On-Chain Cosigner spec §10): its inner @@ -727,10 +1140,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite Env env{*this, features}; - Account const alice{"alice"}; - Account const target{"target"}; // outer account of the batch + Account const target{"target"}; // outer account of the batch, proposing for itself Account const bob{"bob"}; // a distinct inner participant - env.fund(XRP(10000), alice, target, bob); + env.fund(XRP(10000), target, bob); env.close(); std::uint32_t const targetTicketSeq = proposal::createTicket(env, target); @@ -745,7 +1157,7 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite {proposal::innerTx(pay(target, bob, XRP(1)), env.seq(target)), proposal::innerTx(pay(bob, target, XRP(1)), env.seq(bob))}); - env(proposal::create(alice, proposedTx, proposal::expiration(env, 100s))); + env(proposal::create(target, proposedTx, proposal::expiration(env, 100s))); env.close(); auto const sle = proposal::entry(env, target, targetTicketSeq); @@ -756,7 +1168,8 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // signatures are collected later through TransactionProposalSign. auto const stored = sle->getFieldObject(sfProposedTransaction); BEAST_EXPECT(!stored.isFieldPresent(sfBatchSigners)); - BEAST_EXPECT(ownerCount(env, alice) == proposal::kBatchProposalOwnerCount); + // target owns its own Ticket plus the batch proposal. + BEAST_EXPECT(ownerCount(env, target) == 1 + proposal::kBatchProposalOwnerCount); } void @@ -775,6 +1188,8 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite // Preclaim testPreclaim(all); + testProposerAuthorization(all); + testDelegatedProposedTx(all); testPseudoTarget(all); // Apply @@ -782,6 +1197,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite testOtherTransactionTypes(all); testAuxiliaryCoSignatureTypes(all); testReserve(all); + testSponsoredReserve(all); + testSponsorshipTransfer(all); + testFeeSponsored(all); testBatchReserve(all); testMultiAccountBatch(all); } diff --git a/src/test/jtx/impl/proposal.cpp b/src/test/jtx/impl/proposal.cpp index b9d7c69a27..c820ef6392 100644 --- a/src/test/jtx/impl/proposal.cpp +++ b/src/test/jtx/impl/proposal.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -95,6 +96,13 @@ unsignedBatch( return unsignedPayload(env, std::move(jv), ticketSeq); } +void +authorizeProposer(Env& env, Account const& target, Account const& proposer) +{ + env(signers(target, 1, {{proposer, 1}})); + env.close(); +} + std::uint32_t createTicket(Env& env, Account const& account, std::uint32_t count) { diff --git a/src/test/jtx/proposal.h b/src/test/jtx/proposal.h index 7482912feb..ddb99665bb 100644 --- a/src/test/jtx/proposal.h +++ b/src/test/jtx/proposal.h @@ -5,9 +5,9 @@ #include #include +#include #include #include -#include #include #include @@ -100,6 +100,23 @@ unsignedBatch( std::vector const& inners, std::optional numSigners = std::nullopt); +/** + * @brief Give @p proposer a place on @p target's SignerList, so it may + * create proposals against @p target. + * + * Only the target account itself, or an account on its SignerList, may + * create a TransactionProposalCreate against it (On-Chain Cosigner V1 + * authorization). Sets a minimal one-signer, quorum-1 list; the quorum + * itself is irrelevant here since it only governs the proposed + * transaction's own completion, not who may propose it. + * + * @param env The test environment. + * @param target The account whose SignerList is set. + * @param proposer The account to add to it. + */ +void +authorizeProposer(Env& env, Account const& target, Account const& proposer); + /** * @brief Create tickets for a proposal to be built against, and close the * ledger.