Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-MASP IBC Txs cause device crash #88

Closed
murisi opened this issue Oct 31, 2024 · 0 comments · May be fixed by heliaxdev/ledger-namada#2
Closed

Non-MASP IBC Txs cause device crash #88

murisi opened this issue Oct 31, 2024 · 0 comments · May be fixed by heliaxdev/ledger-namada#2

Comments

@murisi
Copy link
Contributor

murisi commented Oct 31, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants