receive: improve wallet file corruption detection

src/MainWindow.cpp

@@ -1591,7 +1591,12 @@ void MainWindow::onInitiateTransaction() {
 void MainWindow::onKeysCorrupted() {
     if (!m_criticalWarningShown) {
         m_criticalWarningShown = true;
-        Utils::showError(this, "Wallet keys are corrupted", "WARNING!\n\nTo prevent LOSS OF FUNDS do NOT continue to use this wallet file.\n\nRestore your wallet from seed.\n\nPlease report this incident to the Feather developers.\n\nWARNING!");
+        Utils::showError(this, "Potential wallet file corruption detected",
+                         "WARNING!\n\n"
+                         "To prevent LOSS OF FUNDS do NOT continue to use this wallet file.\n\n"
+                         "Restore your wallet from seed, keys, or device.\n\n"
+                         "Please report this incident to the Feather developers.\n\n"
+                         "WARNING!", {}, "report_an_issue");
         m_sendWidget->disallowSending();
     }
 }

src/ReceiveWidget.cpp

@@ -30,7 +30,7 @@ ReceiveWidget::ReceiveWidget(Wallet *wallet, QWidget *parent)
     ui->addresses->header()->setSectionResizeMode(SubaddressModel::Address, QHeaderView::ResizeToContents);
     ui->addresses->header()->setSectionResizeMode(SubaddressModel::Label, QHeaderView::Stretch);
 
-    connect(ui->addresses->selectionModel(), &QItemSelectionModel::currentChanged, [=](QModelIndex current, QModelIndex prev){
+    connect(ui->addresses->selectionModel(), &QItemSelectionModel::selectionChanged, [=](const QItemSelection &selected, const QItemSelection &deselected){
         this->updateQrCode();
     });
     connect(m_model, &SubaddressModel::modelReset, [this](){
@@ -74,12 +74,12 @@ ReceiveWidget::ReceiveWidget(Wallet *wallet, QWidget *parent)
     m_showTransactionsAction = new QAction("Show transactions");
     connect(m_showTransactionsAction, &QAction::triggered, this, &ReceiveWidget::onShowTransactions);
     connect(ui->addresses, &QTreeView::customContextMenuRequested, this, &ReceiveWidget::showContextMenu);
-
-    connect(ui->btn_generateSubaddress, &QPushButton::clicked, this, &ReceiveWidget::generateSubaddress);
+    connect(ui->addresses, &SubaddressView::copyAddress, this, &ReceiveWidget::copyAddress);
 
     connect(ui->qrCode, &ClickableLabel::clicked, this, &ReceiveWidget::showQrCodeDialog);
     connect(ui->search, &QLineEdit::textChanged, this, &ReceiveWidget::setSearchFilter);
 
+    connect(ui->btn_generateSubaddress, &QPushButton::clicked, this, &ReceiveWidget::generateSubaddress);
     connect(ui->btn_createPaymentRequest, &QPushButton::clicked, this, &ReceiveWidget::createPaymentRequest);
 }
 
@@ -104,9 +104,32 @@ void ReceiveWidget::focusSearchbar() {
     ui->search->setFocus();
 }
 
+QString ReceiveWidget::getAddress(quint32 minorIndex) {
+    bool ok;
+    QString reason;
+    QString address = m_wallet->getAddressSafe(m_wallet->currentSubaddressAccount(), minorIndex, ok, reason);
+
+    if (!ok) {
+        Utils::showError(this, "Unable to get address",
+                         QString("Reason: %1\n\n"
+                                 "WARNING!\n\n"
+                                 "Potential wallet file corruption detected.\n\n"
+                                 "To prevent LOSS OF FUNDS do NOT continue to use this wallet file.\n\n"
+                                 "Restore your wallet from seed, keys, or device.\n\n"
+                                 "Please report this incident to the Feather developers.\n\n"
+                                 "WARNING!").arg(reason), {}, "report_an_issue");
+        return {};
+    }
+
+    return address;
+}
+
 void ReceiveWidget::copyAddress() {
-    QModelIndex index = ui->addresses->currentIndex();
-    Utils::copyColumn(&index, SubaddressModel::Address);
+    SubaddressRow* row = this->currentEntry();
+    if (!row) return;
+
+    QString address = this->getAddress(row->getRow());
+    Utils::copyToClipboard(address);
 }
 
 void ReceiveWidget::copyLabel() {
@@ -115,7 +138,7 @@ void ReceiveWidget::copyLabel() {
 }
 
 void ReceiveWidget::editLabel() {
-    QModelIndex index = ui->addresses->currentIndex().siblingAtColumn(m_model->ModelColumn::Label);
+    QModelIndex index = ui->addresses->currentIndex().siblingAtColumn(SubaddressModel::ModelColumn::Label);
     ui->addresses->setCurrentIndex(index);
     ui->addresses->edit(index);
 }
@@ -164,24 +187,20 @@ void ReceiveWidget::showContextMenu(const QPoint &point) {
 }
 
 void ReceiveWidget::createPaymentRequest() {
-    QModelIndex index = ui->addresses->currentIndex();
-    if (!index.isValid()) {
-        return;
-    }
+    SubaddressRow* row = this->currentEntry();
+    if (!row) return;
 
-    QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString();
+    QString address = this->getAddress(row->getRow());
 
     PaymentRequestDialog dialog{this, m_wallet, address};
     dialog.exec();
 }
 
 void ReceiveWidget::onShowTransactions() {
-    QModelIndex index = ui->addresses->currentIndex();
-    if (!index.isValid()) {
-        return;
-    }
+    SubaddressRow* row = this->currentEntry();
+    if (!row) return;
 
-    QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString();
+    QString address = this->getAddress(row->getRow());
     emit showTransactions(address);
 }
 
@@ -209,14 +228,14 @@ void ReceiveWidget::generateSubaddress() {
 }
 
 void ReceiveWidget::updateQrCode(){
-    QModelIndex index = ui->addresses->currentIndex();
-    if (!index.isValid()) {
+    SubaddressRow* row = this->currentEntry();
+    if (!row) {
         ui->qrCode->clear();
         ui->btn_createPaymentRequest->hide();
         return;
     }
 
-    QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString();
+    QString address = this->getAddress(row->getRow());
     const QrCode qrc(address, QrCode::Version::AUTO, QrCode::ErrorCorrectionLevel::MEDIUM);
 
     int width = ui->qrCode->width() - 4;
@@ -227,11 +246,10 @@ void ReceiveWidget::updateQrCode(){
 }
 
 void ReceiveWidget::showQrCodeDialog() {
-    QModelIndex index = ui->addresses->currentIndex();
-    if (!index.isValid()) {
-        return;
-    }
-    QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString();
+    SubaddressRow* row = this->currentEntry();
+    if (!row) return;
+
+    QString address = this->getAddress(row->getRow());
     QrCode qr(address, QrCode::Version::AUTO, QrCode::ErrorCorrectionLevel::HIGH);
     QrCodeDialog dialog{this, &qr, "Address"};
     dialog.exec();

src/ReceiveWidget.h

@@ -31,6 +31,7 @@ public:
     void focusSearchbar();
 
 public slots:
+    QString getAddress(quint32 minorIndex);
     void copyAddress();
     void copyLabel();
     void editLabel();

src/libwalletqt/Subaddress.cpp

@@ -120,33 +120,53 @@ bool Subaddress::refresh(quint32 accountIndex)
     emit refreshStarted();
     
     this->clearRows();
-    for (qsizetype i = 0; i < m_wallet2->get_num_subaddresses(accountIndex); ++i)
+
+    bool potentialWalletFileCorruption = false;
+
+    for (quint32 i = 0; i < m_wallet2->get_num_subaddresses(accountIndex); ++i)
     {
-        QString address = QString::fromStdString(m_wallet2->get_subaddress_as_str({accountIndex, (uint32_t)i}));
+        cryptonote::subaddress_index index = {accountIndex, i};
+        cryptonote::account_public_address address = m_wallet2->get_subaddress(index);
+
+        // Make sure we have previously generated Di
+        auto idx =  m_wallet2->get_subaddress_index(address);
+        if (!idx) {
+            potentialWalletFileCorruption = true;
+            break;
+        }
+
+        // Verify mapping
+        if (idx != index) {
+            potentialWalletFileCorruption = true;
+            break;
+        }
+
+        QString addressStr = QString::fromStdString(cryptonote::get_account_address_as_str(m_wallet2->nettype(), !index.is_zero(), address));
 
         auto* row = new SubaddressRow{this,
                                       i,
-                                      address,
-                                      QString::fromStdString(m_wallet2->get_subaddress_label({accountIndex, (uint32_t)i})),
+                                      addressStr,
+                                      QString::fromStdString(m_wallet2->get_subaddress_label(index)),
                                       m_wallet2->get_subaddress_used({accountIndex, (uint32_t)i}),
-                                      this->isHidden(address),
-                                      this->isPinned(address)
+                                      this->isHidden(addressStr),
+                                      this->isPinned(addressStr)
         };
         
         m_rows.append(row);
     }
 
     // Make sure keys are intact. We NEVER want to display incorrect addresses in case of memory corruption.
-    bool keysCorrupt = m_wallet2->get_device_type() == hw::device::SOFTWARE && !m_wallet2->verify_keys();
+    potentialWalletFileCorruption = potentialWalletFileCorruption || (m_wallet2->get_device_type() == hw::device::SOFTWARE && !m_wallet2->verify_keys());
 
-    if (keysCorrupt) {
-        clearRows();
+    if (potentialWalletFileCorruption) {
         LOG_ERROR("KEY INCONSISTENCY DETECTED, WALLET IS IN CORRUPT STATE.");
+        clearRows();
+        emit corrupted();
     }
 
     emit refreshFinished();
 
-    return !keysCorrupt;
+    return !potentialWalletFileCorruption;
 }
 
 qsizetype Subaddress::count() const

src/libwalletqt/Subaddress.h

@@ -46,6 +46,7 @@ public:
 signals:
     void refreshStarted() const;
     void refreshFinished() const;
+    void corrupted() const;
 
 private:
     explicit Subaddress(Wallet *wallet, tools::wallet2 *wallet2, QObject *parent);

src/libwalletqt/Wallet.cpp

@@ -82,6 +82,10 @@ Wallet::Wallet(Monero::Wallet *wallet, QObject *parent)
     connect(this, &Wallet::updated, this, &Wallet::onUpdated);
     connect(this, &Wallet::heightsRefreshed, this, &Wallet::onHeightsRefreshed);
     connect(this, &Wallet::transactionCommitted, this, &Wallet::onTransactionCommitted);
+
+    connect(m_subaddress, &Subaddress::corrupted, [this]{
+       emit keysCorrupted();
+    });
 }
 
 // #################### Status ####################
@@ -185,6 +189,85 @@ QString Wallet::address(quint32 accountIndex, quint32 addressIndex) const {
     return QString::fromStdString(m_wallet2->get_subaddress_as_str({accountIndex, addressIndex}));
 }
 
+QString Wallet::getAddressSafe(quint32 accountIndex, quint32 addressIndex, bool &ok, QString &reason) const {
+    ok = false;
+
+    // If we copy an address to clipboard or create a QR code, there must not be a spark of doubt that
+    // the address belongs to our wallet.
+
+    // This function does a number of sanity checks, some seemingly unnecessary or redundant to ensure
+    // that, yes, we own this address. It provides basic protection against memory corruption errors.
+
+    if (accountIndex >= this->numSubaddressAccounts()) {
+        reason = "Account index exceeds number of pre-computed subaddress accounts";
+        return {};
+    }
+
+    if (addressIndex >= this->numSubaddresses(accountIndex)) {
+        reason = "Address index exceeds number of pre-computed subaddresses";
+        return {};
+    }
+
+    // Realistically, nobody will have more than 1M subaddresses or accounts in their wallet.
+    if (accountIndex >= 1000000) {
+        reason = "Account index exceeds safety limit";
+        return {};
+    }
+
+    if (addressIndex >= 1000000) {
+        reason = "Address index exceeds safety limit";
+        return {};
+    }
+
+    // subaddress public spendkey (Di) = Hs(secret viewkey || subaddress index)G + primary address public spendkey (B)
+    // subaddress public viewkey  (Ci) = D * secret viewkey (a)
+
+    if (!m_wallet2->verify_keys()) {
+        reason = "Unable to verify viewkey";
+        return {};
+    }
+
+    cryptonote::subaddress_index index = {accountIndex, addressIndex};
+    cryptonote::account_public_address address = m_wallet2->get_subaddress(index);
+
+    // Make sure we have previously generated Di
+    auto idx =  m_wallet2->get_subaddress_index(address);
+    if (!idx) {
+        reason = "No mapping found for this subaddress public spendkey";
+        return {};
+    }
+
+    // Verify mapping
+    if (idx != index) {
+        reason = "Invalid subaddress public spendkey mapping";
+        return {};
+    }
+
+    // Recompute address
+    cryptonote::account_public_address address2 = m_wallet2->get_subaddress(idx.value());
+    if (address != address2) {
+        reason = "Recomputed address does not match original address";
+        return {};
+    }
+
+    std::string address_str = m_wallet2->get_subaddress_as_str(index);
+
+    // Make sure address is parseable
+    cryptonote::address_parse_info info;
+    if (!cryptonote::get_account_address_from_str(info, m_wallet2->nettype(), address_str)) {
+        reason = "Unable to parse address";
+        return {};
+    }
+
+    if (info.address != address) {
+        reason = "Parsed address does not match original address";
+        return {};
+    }
+
+    ok = true;
+    return QString::fromStdString(address_str);
+}
+
 SubaddressIndex Wallet::subaddressIndex(const QString &address) const {
     std::pair<uint32_t, uint32_t> i;
     if (!m_walletImpl->subaddressIndex(address.toStdString(), i)) {
@@ -479,14 +562,7 @@ void Wallet::onRefreshed(bool success, const QString &message) {
 void Wallet::refreshModels() {
     m_history->refresh();
     m_coins->refresh();
-    bool r = this->subaddress()->refresh(this->currentSubaddressAccount());
-
-    if (!r) {
-        // This should only happen if wallet keys got corrupted or were tampered with
-        // The list of subaddresses is wiped to prevent loss of funds
-        // Notify MainWindow to display an error message
-        emit keysCorrupted();
-    }
+    this->subaddress()->refresh(this->currentSubaddressAccount());
 }
 
 // #################### Hardware wallet ####################

src/libwalletqt/Wallet.h

@@ -154,8 +154,8 @@ public:
     void updateBalance();
 
     // ##### Subaddresses and Accounts #####
-    //! returns wallet's public address
     QString address(quint32 accountIndex, quint32 addressIndex) const;
+    QString getAddressSafe(quint32 accountIndex, quint32 addressIndex, bool &ok, QString &reason) const;
 
     //! returns the subaddress index of the address
     SubaddressIndex subaddressIndex(const QString &address) const;

src/libwalletqt/rows/SubaddressRow.cpp

@@ -3,7 +3,7 @@
 
 #include "SubaddressRow.h"
 
-qsizetype SubaddressRow::getRow() const {
+quint32 SubaddressRow::getRow() const {
     return m_row;
 }
 

src/libwalletqt/rows/SubaddressRow.h

@@ -11,7 +11,7 @@ class SubaddressRow : public QObject
     Q_OBJECT
     
 public:
-    SubaddressRow(QObject *parent, qsizetype row, const QString& address, const QString &label, bool used, bool hidden, bool pinned)
+    SubaddressRow(QObject *parent, quint32 row, const QString& address, const QString &label, bool used, bool hidden, bool pinned)
         : QObject(parent)
         , m_row(row)
         , m_address(address)
@@ -20,7 +20,7 @@ public:
         , m_hidden(hidden)
         , m_pinned(pinned) {}
     
-    qsizetype getRow() const;
+    [[nodiscard]] quint32 getRow() const;
     const QString& getAddress() const;
     const QString& getLabel() const;
     bool isUsed() const;
@@ -28,7 +28,7 @@ public:
     bool isPinned() const;
     
 private:
-    qsizetype m_row;
+    quint32 m_row;
     QString m_address;
     QString m_label;
     bool m_used = false;

src/model/SubaddressView.cpp

@@ -4,6 +4,7 @@
 #include "SubaddressView.h"
 
 #include "utils/Utils.h"
+#include "SubaddressModel.h"
 
 SubaddressView::SubaddressView(QWidget *parent) : QTreeView(parent) {
 
@@ -15,8 +16,12 @@ void SubaddressView::keyPressEvent(QKeyEvent *event){
     if(!selectedIndexes().isEmpty()){
         if(event->matches(QKeySequence::Copy)){
             QModelIndex index = this->currentIndex();
+            if (index.column() == SubaddressModel::ModelColumn::Address) {
+                emit copyAddress();
+            } else {
                 Utils::copyColumn(&index, index.column());
             }
+        }
         else
             QTreeView::keyPressEvent(event);
     }

src/model/SubaddressView.h

@@ -10,10 +10,14 @@
 
 class SubaddressView : public QTreeView
 {
+Q_OBJECT
 
 public:
     SubaddressView(QWidget* parent = nullptr);
 
+signals:
+    void copyAddress();
+
 protected:
     void keyPressEvent(QKeyEvent *event);
 };