wallet2: keep multisig import state consistent across failed refreshes
What the developer wrote
Authored by woodser
wallet2: keep multisig import state consistent across failed refreshes
What changed, and why it matters
This change fixes a bug in Monero's multisig wallet import process. Previously, if importing multisig data failed partway through, the wallet could be left with a mix of old and new internal state, potentially causing confusion or incorrect behavior on the next import attempt. The patch now validates all incoming data in a temporary staging area and only replaces the wallet's live state after everything checks out, keeping things consistent even when something goes wrong.
Treat as a bug-fix commit with possible security implications for multisig wallet reliability. Review whether the inconsistent state could lead to spendable outputs being overlooked or incorrect transaction signing. No immediate emergency action is indicated by the diff alone, but users running multisig wallets should update to include this fix.
Security signals we found
State consistency bug in multisig import failure path
Partial update of m_multisig_rescan_info / m_multisig_rescan_k before validation completes
Potential use of stale or mixed rescan state on subsequent import/refresh
Memory wiping added for sensitive key material in staging container
Evidence from the diff
In wallet2::import_multisig, the code previously appended parsed multisig_info directly into m_multisig_rescan_info and copied transfer keys into m_multisig_rescan_k before all validation completed. A later failure (e.g., wrong number of sources, unknown signer, inconsistent signers) would leave those member vectors partially updated. The patch introduces local staging containers (info and k), performs parsing and validation against them, and only moves them into the member variables after all checks pass. It also adds a memwipe scope guard for the staging keys and wipes the old m_multisig_rescan_k before replacement. Additionally, the automatic refresh of pending rescan state now only happens when refresh_after_import is true, preventing an unconditional refresh that could consume pending state unexpectedly.
Changed components
src/wallet/wallet2.cppwallet2::import_multisigmultisig wallet rescan stateInspect captured patch +21 / −10
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 4464489..9e4a6ac 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -14626,9 +14626,12 @@ size_t wallet2::import_multisig(std::vector<cryptonote::blobdata> blobs, bool re
{
CHECK_AND_ASSERT_THROW_MES(m_multisig, "Wallet is not multisig");
- if (!m_multisig_rescan_k.empty() && !m_multisig_rescan_info.empty())
+ // consume pending rescan state from a prior import if refreshing
+ if (refresh_after_import && !m_multisig_rescan_k.empty() && !m_multisig_rescan_info.empty())
refresh(false);
+ // parse and validate locally so failures preserve any pending rescan state
+ std::vector<std::vector<tools::wallet2::multisig_info>> info;
std::unordered_set<crypto::public_key> seen;
for (cryptonote::blobdata &data: blobs)
{
@@ -14688,18 +14691,20 @@ size_t wallet2::import_multisig(std::vector<cryptonote::blobdata> blobs, bool re
}
MINFO(boost::format("%u outputs found") % boost::lexical_cast<std::string>(i.size()));
- m_multisig_rescan_info.push_back(std::move(i));
+ info.push_back(std::move(i));
}
- CHECK_AND_ASSERT_THROW_MES(m_multisig_rescan_info.size() + 1 <= m_multisig_signers.size() && m_multisig_rescan_info.size() + 1 >= m_multisig_threshold, "Wrong number of multisig sources");
+ CHECK_AND_ASSERT_THROW_MES(info.size() + 1 <= m_multisig_signers.size() && info.size() + 1 >= m_multisig_threshold, "Wrong number of multisig sources");
- m_multisig_rescan_k.reserve(m_transfers.size());
+ std::vector<std::vector<rct::key>> k;
+ const epee::scope_guard wiper([&]() { for (auto &v: k) memwipe(v.data(), v.size() * sizeof(v[0])); });
+ k.reserve(m_transfers.size());
for (const auto &td: m_transfers)
- m_multisig_rescan_k.push_back(td.m_multisig_k);
+ k.push_back(td.m_multisig_k);
// how many outputs we're going to update
size_t n_outputs = m_transfers.size();
- for (const auto &pi: m_multisig_rescan_info)
+ for (const auto &pi: info)
if (pi.size() < n_outputs)
n_outputs = pi.size();
@@ -14707,7 +14712,7 @@ size_t wallet2::import_multisig(std::vector<cryptonote::blobdata> blobs, bool re
return 0;
// check signers are consistent
- for (const auto &pi: m_multisig_rescan_info)
+ for (const auto &pi: info)
{
CHECK_AND_ASSERT_THROW_MES(std::find(m_multisig_signers.begin(), m_multisig_signers.end(), pi[0].m_signer) != m_multisig_signers.end(),
"Signer is not a member of this multisig wallet");
@@ -14716,15 +14721,21 @@ size_t wallet2::import_multisig(std::vector<cryptonote::blobdata> blobs, bool re
}
// trim data we don't have info for from all participants
- for (auto &pi: m_multisig_rescan_info)
+ for (auto &pi: info)
pi.resize(n_outputs);
// sort by signer
- if (!m_multisig_rescan_info.empty() && !m_multisig_rescan_info.front().empty())
+ if (!info.empty() && !info.front().empty())
{
- std::sort(m_multisig_rescan_info.begin(), m_multisig_rescan_info.end(), [](const std::vector<tools::wallet2::multisig_info> &i0, const std::vector<tools::wallet2::multisig_info> &i1){ return memcmp(&i0[0].m_signer, &i1[0].m_signer, sizeof(i0[0].m_signer)) < 0; });
+ std::sort(info.begin(), info.end(), [](const std::vector<tools::wallet2::multisig_info> &i0, const std::vector<tools::wallet2::multisig_info> &i1){ return memcmp(&i0[0].m_signer, &i1[0].m_signer, sizeof(i0[0].m_signer)) < 0; });
}
+ // wipe prior pending rescan state and install its replacement only after full validation
+ for (auto &v: m_multisig_rescan_k)
+ memwipe(v.data(), v.size() * sizeof(v[0]));
+ m_multisig_rescan_info = std::move(info);
+ m_multisig_rescan_k = std::move(k);
+
// first pass to determine where to detach the blockchain
for (size_t n = 0; n < n_outputs; ++n)
{
Why this scored 33/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.