From a3e940844d4eb755872e4b7f403f37b2c3299c88 Mon Sep 17 00:00:00 2001 From: Richard Holland Date: Sun, 9 Jul 2023 08:02:09 +0000 Subject: [PATCH] fix signerlist import crashbug --- src/ripple/app/tx/impl/Import.cpp | 162 +++++++++++++++++------ src/ripple/app/tx/impl/SetSignerList.cpp | 83 +++--------- src/ripple/app/tx/impl/SetSignerList.h | 87 ++++++++++++ src/test/app/Import_test.cpp | 8 +- 4 files changed, 228 insertions(+), 112 deletions(-) diff --git a/src/ripple/app/tx/impl/Import.cpp b/src/ripple/app/tx/impl/Import.cpp index 26e53d434..3588cb951 100644 --- a/src/ripple/app/tx/impl/Import.cpp +++ b/src/ripple/app/tx/impl/Import.cpp @@ -33,6 +33,7 @@ #include #include #include +#include namespace ripple { @@ -191,6 +192,13 @@ Import::preflight(PreflightContext const& ctx) if (!stpTrans) return temMALFORMED; + if (stpTrans->isFieldPresent(sfTicketSequence)) + { + JLOG(ctx.j.warn()) + << "Import: cannot use TicketSequence XPOP."; + return temMALFORMED; + } + // check if txn is emitted or a psuedo if (isPseudoTx(*stpTrans) || stpTrans->isFieldPresent(sfEmitDetails)) { @@ -952,16 +960,24 @@ Import::doApply() // why aren't transactors stateful, why does this need to be recomputed each time... STAmount burn = stpTrans->getFieldAmount(sfFee); + + if (!isXRP(burn) || burn < beast::zero) + { + JLOG(ctx_.journal.warn()) + << "Import: inner fee was not XRP value."; + return tefINTERNAL; + } + uint32_t importSequence = stpTrans->getFieldU32(sfSequence); auto const id = ctx_.tx[sfAccount]; auto const k = keylet::account(id); - auto const ksl = keylet::signers(id); - auto sle = view().peek(k); - std::optional setSignerEntries; + std::optional< + std::vector> setSignerEntries; + uint32_t setSignerQuorum { 0 }; std::optional setRegularKey; auto const signingKey = ctx_.tx.getSigningPubKey(); bool const signedWithMaster = !signingKey.empty() && calcAccountID(PublicKey(makeSlice(signingKey))) == id; @@ -973,9 +989,50 @@ Import::doApply() { if (tt == ttSIGNER_LIST_SET) { - JLOG(ctx_.journal.warn()) << "SingerListSet"; - // key import: signer list - setSignerEntries = stpTrans->getFieldArray(sfSignerEntries); + if (stpTrans->isFieldPresent(sfSignerQuorum) && + stpTrans->isFieldPresent(sfSignerEntries)) + { + auto const entries = stpTrans->getFieldArray(sfSignerEntries); + + if (entries.empty()) + { + JLOG(ctx_.journal.warn()) << "Import: SignerListSet entires empty, skipping."; + } + else + { + JLOG(ctx_.journal.trace()) << "Import: SingerListSet"; + // key import: signer list + setSignerEntries.emplace(); + setSignerEntries->reserve(entries.size()); + for (auto const& e: entries) + { + if (!e.isFieldPresent(sfAccount) || !e.isFieldPresent(sfSignerWeight)) + { + JLOG(ctx_.journal.warn()) + << "Import: SignerListSet entry lacked a required field (Account/SignerWeight). " + << "Skipping SignerListSet."; + setSignerEntries = std::nullopt; + break; + } + + std::optional tag; + if (e.isFieldPresent(sfWalletLocator)) + tag = e.getFieldH256(sfWalletLocator); + + setSignerEntries->emplace_back( + e.getAccountID(sfAccount), + e.getFieldU16(sfSignerWeight), + tag); + } + setSignerQuorum = stpTrans->getFieldU32(sfSignerQuorum); + } + } + else + { + JLOG(ctx_.journal.warn()) + << "Import: SingerListSet lacked either populated SignerEntries or SignerQuorum, ignoring."; + } + } else if (tt == ttREGULAR_KEY_SET) { @@ -987,6 +1044,18 @@ Import::doApply() bool const create = !sle; + // compute the amount they receive first because the amount is maybe needed for computing setsignerlist later + STAmount startBal = create ? STAmount(INITIAL_IMPORT_XRP) : sle->getFieldAmount(sfBalance); + STAmount finalBal = startBal + burn; + + // this should never happen + if (finalBal < startBal) + { + JLOG(ctx_.journal.warn()) + << "Import: logic error finalBal < startBal."; + return tefINTERNAL; + } + if (create) { // Create the account. @@ -999,20 +1068,9 @@ Import::doApply() sle->setFieldU32(sfImportSequence, importSequence); sle->setFieldU32(sfSequence, seqno); + sle->setFieldU32(sfOwnerCount, 0); - STAmount initBal = STAmount(INITIAL_IMPORT_XRP) + burn; - - if (initBal <= beast::zero) - { - JLOG(ctx_.journal.warn()) - << "Import: inital balance <= 0"; - - return tefINTERNAL; - } - - JLOG(ctx_.journal.warn()) << "Import: inital balance" << initBal; - - sle->setFieldAmount(sfBalance, initBal); + sle->setFieldAmount(sfBalance, finalBal); } else if (sle->getFieldU32(sfImportSequence) >= importSequence) @@ -1023,25 +1081,19 @@ Import::doApply() return tefINTERNAL; } - if (setSignerEntries) { - JLOG(ctx_.journal.warn()) << "signer list set"; - auto signerList = std::make_shared(ksl); - signerList->setFieldArray(sfSignerEntries, *setSignerEntries); - view().insert(signerList); - } - else if (setRegularKey) + if (setRegularKey) { - JLOG(ctx_.journal.warn()) << "set regular key"; + JLOG(ctx_.journal.warn()) << "Import: actioning SetRegularKey " << *setRegularKey << " acc: " << id; sle->setAccountID(sfRegularKey, *setRegularKey); } if (create) { - JLOG(ctx_.journal.warn()) << "create"; + JLOG(ctx_.journal.trace()) << "Import: creating account " << id; if (!signedWithMaster) { // disable master if the account is created using non-master key - JLOG(ctx_.journal.warn()) << "create - disable master"; + JLOG(ctx_.journal.warn()) << "Import: keying of " << id << " is unclear - disable master"; sle->setAccountID(sfRegularKey, noAccount()); sle->setFieldU32(sfFlags, lsfDisableMaster); } @@ -1050,23 +1102,13 @@ Import::doApply() else { // account already exists - JLOG(ctx_.journal.warn()) << "update - import sequence"; + JLOG(ctx_.journal.trace()) << "Import: updating existing account " << id; sle->setFieldU32(sfImportSequence, importSequence); - - // credit the PoB - if (burn > beast::zero) - { - JLOG(ctx_.journal.warn()) << "update - credit burn"; - STAmount startBal = sle->getFieldAmount(sfBalance); - STAmount finalBal = startBal + burn; - if (finalBal > startBal) - sle->setFieldAmount(sfBalance, finalBal); - } + sle->setFieldAmount(sfBalance, finalBal); view().update(sle); } - // todo: manifest sequence needs to be recorded to prevent certain types of replay attack auto const infoVL = getVLInfo(*xpop, ctx_.journal); @@ -1079,7 +1121,7 @@ Import::doApply() if (!sleVL) { // create VL import seq counter - JLOG(ctx_.journal.warn()) << "create vl seq - insert import sequence + public key"; + JLOG(ctx_.journal.trace()) << "Import: create vl seq - insert import sequence + public key"; sleVL = std::make_shared(keyletVL); sleVL->setFieldU32(sfImportSequence, infoVL->first); sleVL->setFieldVL(sfPublicKey, infoVL->second.slice()); @@ -1087,7 +1129,7 @@ Import::doApply() } else { - JLOG(ctx_.journal.warn()) << "update vl - insert import sequence"; + JLOG(ctx_.journal.trace()) << "Import: update vl"; uint32_t current = sleVL->getFieldU32(sfImportSequence); if (current > infoVL->first) @@ -1107,6 +1149,40 @@ Import::doApply() } } + /// this logic is executed last after the account might have already been created + if (setSignerEntries) + { + JLOG(ctx_.journal.trace()) + << "Import: actioning SignerListSet " + << "quorum: " << setSignerQuorum << " " + << "size: " << setSignerEntries->size(); + + Sandbox sb(&view()); + TER result = + SetSignerList::replaceSignersFromLedger( + ctx_.app, + sb, + ctx_.journal, + id, + setSignerQuorum, + *setSignerEntries, + finalBal.xrp()); + + if (result == tesSUCCESS) + { + JLOG(ctx_.journal.trace()) + << "Import: successful SignerListSet"; + sb.apply(ctx_.rawView()); + } + else + { + JLOG(ctx_.journal.warn()) + << "Import: SetSignerList failed with code " + << result << " acc: " << id; + } + } + + return tesSUCCESS; } diff --git a/src/ripple/app/tx/impl/SetSignerList.cpp b/src/ripple/app/tx/impl/SetSignerList.cpp index 07cc705ba..c0cab01fd 100644 --- a/src/ripple/app/tx/impl/SetSignerList.cpp +++ b/src/ripple/app/tx/impl/SetSignerList.cpp @@ -312,70 +312,14 @@ SetSignerList::validateQuorumAndSignerEntries( TER SetSignerList::replaceSignerList() { - auto const accountKeylet = keylet::account(account_); - auto const ownerDirKeylet = keylet::ownerDir(account_); - auto const signerListKeylet = keylet::signers(account_); - - // This may be either a create or a replace. Preemptively remove any - // old signer list. May reduce the reserve, so this is done before - // checking the reserve. - if (TER const ter = removeSignersFromLedger( + return replaceSignersFromLedger( ctx_.app, - view(), - accountKeylet, - ownerDirKeylet, - signerListKeylet, - j_)) - return ter; - - auto const sle = view().peek(accountKeylet); - if (!sle) - return tefINTERNAL; - - // Compute new reserve. Verify the account has funds to meet the reserve. - std::uint32_t const oldOwnerCount{(*sle)[sfOwnerCount]}; - - // The required reserve changes based on featureMultiSignReserve... - int addedOwnerCount{1}; - std::uint32_t flags{lsfOneOwnerCount}; - if (!ctx_.view().rules().enabled(featureMultiSignReserve)) - { - addedOwnerCount = signerCountBasedOwnerCountDelta( - signers_.size(), ctx_.view().rules()); - flags = 0; - } - - XRPAmount const newReserve{ - view().fees().accountReserve(oldOwnerCount + addedOwnerCount)}; - - // We check the reserve against the starting balance because we want to - // allow dipping into the reserve to pay fees. This behavior is consistent - // with CreateTicket. - if (mPriorBalance < newReserve) - return tecINSUFFICIENT_RESERVE; - - // Everything's ducky. Add the ltSIGNER_LIST to the ledger. - auto signerList = std::make_shared(signerListKeylet); - view().insert(signerList); - writeSignersToSLE(signerList, flags); - - auto viewJ = ctx_.app.journal("View"); - // Add the signer list to the account's directory. - auto const page = ctx_.view().dirInsert( - ownerDirKeylet, signerListKeylet, describeOwnerDir(account_)); - - JLOG(j_.trace()) << "Create signer list for account " << toBase58(account_) - << ": " << (page ? "success" : "failure"); - - if (!page) - return tecDIR_FULL; - - signerList->setFieldU64(sfOwnerNode, *page); - - // If we succeeded, the new entry counts against the - // creator's reserve. - adjustOwnerCount(view(), sle, addedOwnerCount, viewJ); - return tesSUCCESS; + ctx_.view(), + j_, + account_, + quorum_, + signers_, + mPriorBalance); } TER @@ -400,21 +344,24 @@ SetSignerList::destroySignerList() void SetSignerList::writeSignersToSLE( + ApplyView& view, SLE::pointer const& ledgerEntry, - std::uint32_t flags) const + std::uint32_t flags, + std::uint32_t quorum, + std::vector const& signers) { // Assign the quorum, default SignerListID, and flags. - ledgerEntry->setFieldU32(sfSignerQuorum, quorum_); + ledgerEntry->setFieldU32(sfSignerQuorum, quorum); ledgerEntry->setFieldU32(sfSignerListID, defaultSignerListID_); if (flags) // Only set flags if they are non-default (default is zero). ledgerEntry->setFieldU32(sfFlags, flags); bool const expandedSignerList = - ctx_.view().rules().enabled(featureExpandedSignerList); + view.rules().enabled(featureExpandedSignerList); // Create the SignerListArray one SignerEntry at a time. - STArray toLedger(signers_.size()); - for (auto const& entry : signers_) + STArray toLedger(signers.size()); + for (auto const& entry : signers) { toLedger.emplace_back(sfSignerEntry); STObject& obj = toLedger.back(); diff --git a/src/ripple/app/tx/impl/SetSignerList.h b/src/ripple/app/tx/impl/SetSignerList.h index 2e0b2b041..1b3910b85 100644 --- a/src/ripple/app/tx/impl/SetSignerList.h +++ b/src/ripple/app/tx/impl/SetSignerList.h @@ -73,6 +73,15 @@ public: beast::Journal j); private: + static + void + writeSignersToSLE( + ApplyView& view, + SLE::pointer const& ledgerEntry, + std::uint32_t flags, + std::uint32_t quorum, + std::vector const& signers); + static std::tuple< NotTEC, std::uint32_t, @@ -96,6 +105,84 @@ private: void writeSignersToSLE(SLE::pointer const& ledgerEntry, std::uint32_t flags) const; + +public: + template + static TER + replaceSignersFromLedger( + Application& app, + V& view, + beast::Journal j, + AccountID const& acc, + std::uint32_t quorum, + std::vector const& signers, + XRPAmount const mPriorBalance) + { + auto const accountKeylet = keylet::account(acc); + auto const ownerDirKeylet = keylet::ownerDir(acc); + auto const signerListKeylet = keylet::signers(acc); + + // This may be either a create or a replace. Preemptively remove any + // old signer list. May reduce the reserve, so this is done before + // checking the reserve. + if (TER const ter = removeSignersFromLedger( + app, + view, + accountKeylet, + ownerDirKeylet, + signerListKeylet, + j)) + return ter; + + auto const sle = view.peek(accountKeylet); + if (!sle) + return tefINTERNAL; + + // Compute new reserve. Verify the account has funds to meet the reserve. + std::uint32_t const oldOwnerCount{(*sle)[sfOwnerCount]}; + + // The required reserve changes based on featureMultiSignReserve... + int addedOwnerCount{1}; + std::uint32_t flags{lsfOneOwnerCount}; + if (!view.rules().enabled(featureMultiSignReserve)) + { + addedOwnerCount = signerCountBasedOwnerCountDelta( + signers.size(), view.rules()); + flags = 0; + } + + XRPAmount const newReserve{ + view.fees().accountReserve(oldOwnerCount + addedOwnerCount)}; + + // We check the reserve against the starting balance because we want to + // allow dipping into the reserve to pay fees. This behavior is consistent + // with CreateTicket. + if (mPriorBalance < newReserve) + return tecINSUFFICIENT_RESERVE; + + // Everything's ducky. Add the ltSIGNER_LIST to the ledger. + auto signerList = std::make_shared(signerListKeylet); + view.insert(signerList); + writeSignersToSLE(view, signerList, flags, quorum, signers); + + // Add the signer list to the account's directory. + auto const page = view.dirInsert( + ownerDirKeylet, signerListKeylet, describeOwnerDir(acc)); + + JLOG(j.trace()) << "Create signer list for account " << toBase58(acc) + << ": " << (page ? "success" : "failure"); + + if (!page) + return tecDIR_FULL; + + signerList->setFieldU64(sfOwnerNode, *page); + + // If we succeeded, the new entry counts against the + // creator's reserve. + adjustOwnerCount(view, sle, addedOwnerCount, j); + return tesSUCCESS; + } + }; } // namespace ripple diff --git a/src/test/app/Import_test.cpp b/src/test/app/Import_test.cpp index 49ec5fc7b..a6ca0db7b 100644 --- a/src/test/app/Import_test.cpp +++ b/src/test/app/Import_test.cpp @@ -33,7 +33,12 @@ getRaw) outer txid: #include #include #include - +#define BEAST_REQUIRE(x) \ + { \ + BEAST_EXPECT(!!(x)); \ + if (!(x)) \ + return; \ + } namespace ripple { namespace test { @@ -2541,6 +2546,7 @@ class Import_test : public beast::unit_test::suite postAlice == preAlice + feeDrops + XRP(2) + XRP(0.000002)); auto const [signers, signersSle] = signersKeyAndSle(*env.current(), alice); + BEAST_REQUIRE(!!signersSle); auto const signerEntries = signersSle->getFieldArray(sfSignerEntries); BEAST_EXPECT(signerEntries.size() == 1);