diff --git a/src/ripple/app/tests/MultiSign.test.cpp b/src/ripple/app/tests/MultiSign.test.cpp index 758239690..993ba1c1b 100644 --- a/src/ripple/app/tests/MultiSign.test.cpp +++ b/src/ripple/app/tests/MultiSign.test.cpp @@ -527,6 +527,81 @@ public: env.require (owners (alice, 0)); } + // We want to always leave an account signable. Make sure the that we + // disallow removing the last way a transaction may be signed. + void test_keyDisable() + { + using namespace jtx; + Env env(*this); + Account const alice {"alice", KeyType::ed25519}; + env.fund(XRP(1000), alice); + + // There are three negative tests we need to make: + // M0. A lone master key cannot be disabled. + // R0. A lone regular key cannot be removed. + // L0. A lone signer list cannot be removed. + // + // Additionally, there are 6 positive tests we need to make: + // M1. The master key can be disabled if there's a regular key. + // M2. The master key can be disabled if there's a signer list. + // + // R1. The regular key can be removed if there's a signer list. + // R2. The regular key can be removed if the master key is enabled. + // + // L1. The signer list can be removed if the master key is enabled. + // L2. The signer list can be removed if there's a regular key. + + // Master key tests. + // M0: A lone master key cannot be disabled. + env(fset (alice, asfDisableMaster), + sig(alice), ter(tecNO_ALTERNATIVE_KEY)); + + // Add a regular key. + Account const alie {"alie", KeyType::ed25519}; + env(regkey (alice, alie)); + + // M1: The master key can be disabled if there's a regular key. + env(fset (alice, asfDisableMaster), sig(alice)); + + // R0: A lone regular key cannot be removed. + env(regkey (alice, disabled), sig(alie), ter(tecNO_ALTERNATIVE_KEY)); + + // Add a signer list. + env(signers(alice, 1, {{bogie, 1}}), sig (alie)); + + // R1: The regular key can be removed if there's a signer list. + env(regkey (alice, disabled), sig(alie)); + + // L0; A lone signer list cannot be removed. + auto const baseFee = env.config.FEE_DEFAULT; + env(signers(alice, jtx::none), msig(bogie), + fee(2 * baseFee), ter(tecNO_ALTERNATIVE_KEY)); + + // Enable the master key. + env(fclear (alice, asfDisableMaster), msig(bogie), fee(2 * baseFee)); + + // L1: The signer list can be removed if the master key is enabled. + env(signers(alice, jtx::none), msig(bogie), fee(2 * baseFee)); + + // Add a signer list. + env(signers(alice, 1, {{bogie, 1}}), sig (alice)); + + // M2: The master key can be disabled if there's a signer list. + env(fset (alice, asfDisableMaster), sig(alice)); + + // Add a regular key. + env(regkey (alice, alie), msig(bogie), fee(2 * baseFee)); + + // L2: The signer list can be removed if there's a regular key. + env(signers(alice, jtx::none), sig(alie)); + + // Enable the master key. + env(fclear (alice, asfDisableMaster), sig(alie)); + + // R2: The regular key can be removed if the master key is enabled. + env(regkey (alice, disabled), sig(alie)); + } + // See if every kind of transaction can be successfully multi-signed. void test_txTypes() { @@ -621,6 +696,7 @@ public: test_masterSigners(); test_regularSigners(); test_heterogeneousSigners(); + test_keyDisable(); test_txTypes(); } }; diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index a998265cb..628bdd98f 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -191,8 +191,18 @@ SetAccount::doApply () return tecNEED_MASTER_KEY; } - if (!sle->isFieldPresent (sfRegularKey)) + if ((!sle->isFieldPresent (sfRegularKey)) && + (!view().peek (keylet::signers (account_)))) + { + // Account has no regular key or multi-signer signer list. + + // Prevent transaction changes until we're ready. + if ((RIPPLE_ENABLE_MULTI_SIGN) || + view().flags() & tapENABLE_TESTING) + return tecNO_ALTERNATIVE_KEY; + return tecNO_REGULAR_KEY; + } j_.trace << "Set lsfDisableMaster."; uFlagsOut |= lsfDisableMaster; diff --git a/src/ripple/app/tx/impl/SetRegularKey.cpp b/src/ripple/app/tx/impl/SetRegularKey.cpp index a9ba21337..8cd67cd00 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.cpp +++ b/src/ripple/app/tx/impl/SetRegularKey.cpp @@ -77,8 +77,11 @@ SetRegularKey::doApply () } else { - if (sle->isFlag (lsfDisableMaster)) - return tecMASTER_DISABLED; + if (sle->isFlag (lsfDisableMaster) && + !view().peek (keylet::signers (account_))) + // Account has disabled master key and no multi-signer signer list. + return tecNO_ALTERNATIVE_KEY; + sle->makeFieldAbsent (sfRegularKey); } diff --git a/src/ripple/app/tx/impl/SetSignerList.cpp b/src/ripple/app/tx/impl/SetSignerList.cpp index 6f838f656..e5257f8d6 100644 --- a/src/ripple/app/tx/impl/SetSignerList.cpp +++ b/src/ripple/app/tx/impl/SetSignerList.cpp @@ -45,7 +45,7 @@ SetSignerList::determineOperation(STTx const& tx, { // Check the quorum. A non-zero quorum means we're creating or replacing // the list. A zero quorum means we're destroying the list. - auto const quorum = tx.getFieldU32(sfSignerQuorum); + auto const quorum = tx[sfSignerQuorum]; std::vector sign; Operation op = unknown; @@ -68,8 +68,7 @@ SetSignerList::determineOperation(STTx const& tx, op = destroy; } - return std::make_tuple(tesSUCCESS, - quorum, sign, op); + return std::make_tuple(tesSUCCESS, quorum, sign, op); } TER @@ -115,21 +114,16 @@ SetSignerList::preflight (PreflightContext const& ctx) TER SetSignerList::doApply () { - // All operations require our ledger index. Compute that once and pass it - // to our handlers. - uint256 const index = getSignerListIndex (account_); - // Perform the operation preCompute() decided on. switch (do_) { case set: - return replaceSignerList (index); + return replaceSignerList (); case destroy: - return destroySignerList (index); + return destroySignerList (); default: - // Fall through intentionally break; } assert (false); // Should not be possible to get here. @@ -210,24 +204,27 @@ SetSignerList::validateQuorumAndSignerEntries ( } TER -SetSignerList::replaceSignerList (uint256 const& index) +SetSignerList::replaceSignerList () { - // This may be either a create or a replace. Preemptively destroy any + 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 = destroySignerList (index)) - return ter; + if (TER const ter = removeSignersFromLedger ( + accountKeylet, ownerDirKeylet, signerListKeylet)) + return ter; - auto const sle = view().peek( - keylet::account(account_)); + auto const sle = view().peek(accountKeylet); // Compute new reserve. Verify the account has funds to meet the reserve. - std::uint32_t const oldOwnerCount = sle->getFieldU32 (sfOwnerCount); + auto const oldOwnerCount = (*sle)[sfOwnerCount]; std::uint32_t const addedOwnerCount = ownerCountDelta (signers_.size ()); auto const newReserve = - view().fees().accountReserve( - oldOwnerCount + addedOwnerCount); + 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 @@ -236,20 +233,14 @@ SetSignerList::replaceSignerList (uint256 const& index) return tecINSUFFICIENT_RESERVE; // Everything's ducky. Add the ltSIGNER_LIST to the ledger. - auto signerList = std::make_shared(ltSIGNER_LIST, index); + auto signerList = std::make_shared(signerListKeylet); view().insert (signerList); - writeSignersToLedger (signerList); - - // Lambda for call to dirAdd. - auto describer = [&] (SLE::ref sle, bool dummy) - { - ownerDirDescriber (sle, dummy, account_); - }; + writeSignersToSLE (signerList); // Add the signer list to the account's directory. std::uint64_t hint; - TER result = dirAdd(ctx_.view (), - hint, getOwnerDirIndex (account_), index, describer); + TER result = dirAdd(ctx_.view (), hint, ownerDirKeylet.key, + signerListKeylet.key, describeOwnerDir (account_)); JLOG(j_.trace) << "Create signer list for account " << toBase58(account_) << ": " << transHuman (result); @@ -260,57 +251,60 @@ SetSignerList::replaceSignerList (uint256 const& index) signerList->setFieldU64 (sfOwnerNode, hint); // If we succeeded, the new entry counts against the creator's reserve. - adjustOwnerCount(view(), - sle, addedOwnerCount); + adjustOwnerCount(view(), sle, addedOwnerCount); return result; } TER -SetSignerList::destroySignerList (uint256 const& index) +SetSignerList::destroySignerList () { - // See if there's an ltSIGNER_LIST for this account. - SLE::pointer signerList = - view().peek (keylet::signers(index)); + auto const accountKeylet = keylet::account (account_); + // Destroying the signer list is only allowed if either the master key + // is enabled or there is a regular key. + SLE::pointer ledgerEntry = view().peek (accountKeylet); + if ((ledgerEntry->isFlag (lsfDisableMaster)) && + (!ledgerEntry->isFieldPresent (sfRegularKey))) + return tecNO_ALTERNATIVE_KEY; - // If the signer list doesn't exist we've already succeeded in deleting it. - if (!signerList) - return tesSUCCESS; + auto const ownerDirKeylet = keylet::ownerDir (account_); + auto const signerListKeylet = keylet::signers (account_); + return removeSignersFromLedger( + accountKeylet, ownerDirKeylet, signerListKeylet); +} +TER +SetSignerList::removeSignersFromLedger (Keylet const& accountKeylet, + Keylet const& ownerDirKeylet, Keylet const& signerListKeylet) +{ // We have to examine the current SignerList so we know how much to // reduce the OwnerCount. - std::int32_t removeFromOwnerCount = 0; - auto const k = keylet::signers(account_); - SLE::pointer accountSignersList = - view().peek (k); - if (accountSignersList) - { - STArray const& actualList = - accountSignersList->getFieldArray (sfSignerEntries); - removeFromOwnerCount = ownerCountDelta (actualList.size ()) * -1; - } + SLE::pointer signers = view().peek (signerListKeylet); + + // If the signer list doesn't exist we've already succeeded in deleting it. + if (!signers) + return tesSUCCESS; + + STArray const& actualList = signers->getFieldArray (sfSignerEntries); + int const removeFromOwnerCount = ownerCountDelta (actualList.size()) * -1; // Remove the node from the account directory. - std::uint64_t const hint (signerList->getFieldU64 (sfOwnerNode)); + auto const hint = (*signers)[sfOwnerNode]; - TER const result = dirDelete(ctx_.view (), false, hint, - getOwnerDirIndex (account_), index, false, (hint == 0)); + TER const result = dirDelete(ctx_.view(), false, hint, + ownerDirKeylet.key, signerListKeylet.key, false, (hint == 0)); if (result == tesSUCCESS) adjustOwnerCount(view(), - view().peek(keylet::account(account_)), - removeFromOwnerCount); + view().peek(accountKeylet), removeFromOwnerCount); - ctx_.view ().erase (signerList); + ctx_.view().erase (signers); return result; } -// VFALCO NOTE This name is misleading, the signers -// are not written to the ledger they are -// added to the SLE. void -SetSignerList::writeSignersToLedger (SLE::pointer ledgerEntry) +SetSignerList::writeSignersToSLE (SLE::pointer const& ledgerEntry) const { // Assign the quorum. ledgerEntry->setFieldU32 (sfSignerQuorum, quorum_); @@ -333,7 +327,9 @@ SetSignerList::writeSignersToLedger (SLE::pointer ledgerEntry) ledgerEntry->setFieldArray (sfSignerEntries, toLedger); } -std::size_t +// The return type is signed so it is compatible with the 3rd argument +// of adjustOwnerCount() (which must be signed). +int SetSignerList::ownerCountDelta (std::size_t entryCount) { // We always compute the full change in OwnerCount, taking into account: @@ -347,7 +343,12 @@ SetSignerList::ownerCountDelta (std::size_t entryCount) // o And each signer in the list costs 1 more OwnerCount unit. // So, at a minimum, adding a SignerList with 1 entry costs 3 OwnerCount // units. A SignerList with 8 entries would cost 10 OwnerCount units. - return 2 + entryCount; + // + // The static_cast should always be safe since entryCount should always + // be in the range from 1 to 8. We've got a lot of room to grow. + assert (entryCount >= STTx::minMultiSigners); + assert (entryCount <= STTx::maxMultiSigners); + return 2 + static_cast(entryCount); } } // namespace ripple diff --git a/src/ripple/app/tx/impl/SetSignerList.h b/src/ripple/app/tx/impl/SetSignerList.h index 7b458a713..e360bd64f 100644 --- a/src/ripple/app/tx/impl/SetSignerList.h +++ b/src/ripple/app/tx/impl/SetSignerList.h @@ -76,12 +76,14 @@ private: AccountID const& account, beast::Journal j); - TER replaceSignerList (uint256 const& index); - TER destroySignerList (uint256 const& index); + TER replaceSignerList (); + TER destroySignerList (); - void writeSignersToLedger (SLE::pointer ledgerEntry); + TER removeSignersFromLedger (Keylet const& accountKeylet, + Keylet const& ownerDirKeylet, Keylet const& signerListKeylet); + void writeSignersToSLE (SLE::pointer const& ledgerEntry) const; - static std::size_t ownerCountDelta (std::size_t entryCount); + static int ownerCountDelta (std::size_t entryCount); }; } // ripple diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index d025a8ae2..3a0da87fa 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -185,7 +185,7 @@ enum TER tecNO_LINE_REDUNDANT = 127, tecPATH_DRY = 128, tecUNFUNDED = 129, // Deprecated, old ambiguous unfunded. - tecMASTER_DISABLED = 130, + tecNO_ALTERNATIVE_KEY = 130, tecNO_REGULAR_KEY = 131, tecOWNERS = 132, tecNO_ISSUER = 133, diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index 661d8164e..54df2f5b5 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -48,7 +48,7 @@ bool transResultInfo (TER code, std::string& token, std::string& text) { tecNO_LINE_REDUNDANT, "tecNO_LINE_REDUNDANT", "Can't set non-existent line to default." }, { tecPATH_DRY, "tecPATH_DRY", "Path could not send partial amount." }, { tecPATH_PARTIAL, "tecPATH_PARTIAL", "Path could not send full amount." }, - { tecMASTER_DISABLED, "tecMASTER_DISABLED", "Master key is disabled." }, + { tecNO_ALTERNATIVE_KEY, "tecNO_ALTERNATIVE_KEY", "The operation would remove the last way to sign a transaction."}, { tecNO_REGULAR_KEY, "tecNO_REGULAR_KEY", "Regular key is not set." }, { tecUNFUNDED, "tecUNFUNDED", "One of _ADD, _OFFER, or _SEND. Deprecated." }, diff --git a/src/ripple/test/jtx/impl/Env_test.cpp b/src/ripple/test/jtx/impl/Env_test.cpp index 10c7a7db5..a40e904b6 100644 --- a/src/ripple/test/jtx/impl/Env_test.cpp +++ b/src/ripple/test/jtx/impl/Env_test.cpp @@ -332,7 +332,7 @@ public: env.require(nflags("alice", asfDisableMaster)); env(fset("alice", asfDisableMaster), sig("alice")); env.require(flags("alice", asfDisableMaster)); - env(regkey("alice", disabled), ter(tecMASTER_DISABLED)); + env(regkey("alice", disabled), ter(tecNO_ALTERNATIVE_KEY)); env(noop("alice")); env(noop("alice"), sig("alice"), ter(tefMASTER_DISABLED)); env(noop("alice"), sig("eric"));