Skip to content

Non-MASP IBC Txs cause device crash #88

@murisi

Description

@murisi

Attempting to sign pure IBC Txs (i.e. those not involving MASP) causes the hardware wallet to crash. The hardware wallet crashes before it has the chance to display the transaction. This issue should be reproducible using any of the pure IBC test vectors - 7_IBC_Transfer_0 for instance.

The cause is probably at

if(ctx->tx_obj->transaction.isMasp || ctx->tx_obj->ibc.is_ibc) {
CHECK_ERROR(verifyShieldedHash(ctx))
}
. Here ctx->tx_obj->ibc.is_ibc being true is sufficient to trigger the execution of verifyShieldedHash. This is problematic because a pure IBC Txs do not contain MASP Transactions nor do they contain MaspBuilders, hence lines inside verifyShieldedHash like
if (tx_hash_txId(ctx->tx_obj, tx_id_hash) != zxerr_ok) {
are senseless and probably cause crashes. This issue was probably not observed in the make cpp_test because these MASP assumptions only kick in when LEDGER_SPECIFIC is defined.

This regression seems to have been introduced in #69 in response to #67 . It seems to me that the root cause of this issue is that we have not specified precisely when to sign/display/reject certain kinds of transactions, apologies. So let me try to elaborate the conditions that the Ledger app must enforce.

  • MASP Transactions must accompany MASP Transfers (regardless of whether they are pure, an IBC FT Transfer, or an IBC NFT Transfer). I.e. if inside a Transfer object, shielded_hash = Some(...), then the Tx must contain a Transaction with that TxId.
  • MASP Builders must accompany MASP Transactions. I.e. if the Tx contains a MASP Transaction, then there must be a builder whose target_hash is the TxId of the Transaction. And the information in the MaspBuilder must be consistent with the Transaction.
  • Only display MASP data/fields when they are explicitly referenced by the Tx's Data section. I.e. for MASP data to be relevant and displayed, it must be the case that the Data section directly/indirectly contains a Transfer whose shielded_hash = Some(...).
  • Only sign MASP sections that have been displayed to the user and confirmed by them. I.e. only include the MASP Transaction hash in the wrapper signature if a (IBC or non-IBC) Transfer satisfying the condition that shielded_hash = Some(...) is embedded in the Tx.
  • Support signing non-MASP IBC Transfers. I.e. it can be the case that namadac sends the hardware wallet an IBC FT or NFT Tx which doe not utilize the MASP. These must be supported (as they were in the past).

Put differently, the following propositions can vary independently: Tx contains a MASP Transaction section, Tx contains a MaspBuilder section, Tx contains a Transfer that contains a shielded_hash, and Tx is IBC. And the Ledger app should be able to handle (whether it sends namadac an APDU error without displaying anything, or it correctly displays the Tx to the user and allows them to sign) all combinations without crashing. Thanks!

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions