MASP Transactions must accompany MASP Transfers. MASP Builders must a…

…ccompany MASP Transactions. Only sign displayed sections. Support signing non-MASP IBC Transfers.
heliaxdev · Oct 30, 2024 · 13caab9 · 13caab9
1 parent ef0450c
commit 13caab9
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 29 deletions.
diff --git a/app/src/crypto.c b/app/src/crypto.c
@@ -411,7 +411,7 @@ zxerr_t crypto_sign(const parser_tx_t *txObj, uint8_t *output, uint16_t outputLe
     signature_section.hashes.hashesLen += 2;
 
     // Include Masp hash in the signature if it's there
-    if (txObj->transaction.isMasp) {
+    if ((txObj->typeTx == Transfer && txObj->transfer.has_shielded_hash) || (txObj->typeTx == IBC && txObj->ibc.transfer.has_shielded_hash)) {
 #if !defined(APP_TESTING)
         if (get_state() != STATE_EXTRACT_SPENDS) {
             return zxerr_unknown;

diff --git a/app/src/parser_impl.c b/app/src/parser_impl.c
@@ -27,9 +27,7 @@ parser_error_t _read(parser_context_t *ctx, parser_tx_t *v) {
 
     CHECK_ERROR(validateTransactionParams(v))
 
-    if(ctx->tx_obj->transaction.isMasp || ctx->tx_obj->ibc.is_ibc) {
-        CHECK_ERROR(verifyShieldedHash(ctx))
-    }
+    CHECK_ERROR(verifyShieldedHash(ctx))
 
     if (ctx->offset != ctx->bufferLen) {
         return parser_unexpected_unparsed_bytes;
@@ -51,7 +49,7 @@ parser_error_t getNumItems(const parser_context_t *ctx, uint8_t *numItems) {
             break;
 
         case Transfer:
-            if(ctx->tx_obj->transaction.isMasp) {
+            if(ctx->tx_obj->transfer.has_shielded_hash) {
                 uint8_t items = 1;
                 items += 3 * ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs; // print from outputs
                 items += 3 * ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends; // print from spends
@@ -130,7 +128,7 @@ parser_error_t getNumItems(const parser_context_t *ctx, uint8_t *numItems) {
 
         case IBC:
             *numItems = (app_mode_expert() ?  IBC_EXPERT_PARAMS : IBC_NORMAL_PARAMS);
-            if(ctx->tx_obj->transaction.isMasp) {
+            if(ctx->tx_obj->ibc.transfer.has_shielded_hash) {
                 *numItems += 3 * ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs; // print from outputs
                 *numItems += 3 * ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends; // print from spends
             }

diff --git a/app/src/parser_impl_txn.c b/app/src/parser_impl_txn.c
@@ -1255,7 +1255,8 @@ parser_error_t readSections(parser_context_t *ctx, parser_tx_t *v) {
     if (v->transaction.sections.sectionLen > 7) {
         return parser_invalid_output_buffer;
     }
-    v->transaction.isMasp = false;
+    v->transaction.hasMaspTx = false;
+    v->transaction.hasMaspBuilder = false;
     v->transaction.sections.extraDataLen = 0;
     v->transaction.sections.signaturesLen = 0;
 
@@ -1296,11 +1297,12 @@ parser_error_t readSections(parser_context_t *ctx, parser_tx_t *v) {
 #if defined(COMPILE_MASP)
             case DISCRIMINANT_MASP_TX:
                 // Identify tx has masp tx
-                v->transaction.isMasp = true;
+                v->transaction.hasMaspTx = true;
                 CHECK_ERROR(readMaspTx(ctx, &v->transaction.sections.maspTx))
                 v->transaction.maspTx_idx = i+1;
                 break;
             case DISCRIMINANT_MASP_BUILDER:
+                v->transaction.hasMaspBuilder = true;
                 CHECK_ERROR(readMaspBuilder(ctx, &v->transaction.sections.maspBuilder))
                 break;
 #endif
@@ -1421,24 +1423,36 @@ parser_error_t verifyShieldedHash(parser_context_t *ctx) {
     }
 
 #if defined(LEDGER_SPECIFIC)
-    // compute tx_id hash
     uint8_t tx_id_hash[HASH_LEN] = {0};
-    if (tx_hash_txId(ctx->tx_obj, tx_id_hash) != zxerr_ok) {
-        return parser_unexpected_error;
-    }
-
-    if (ctx->tx_obj->transaction.sections.maspBuilder.target_hash.len == HASH_LEN) {
-        if (memcmp(tx_id_hash, ctx->tx_obj->transaction.sections.maspBuilder.target_hash.ptr, HASH_LEN) != 0) {
+    if (ctx->tx_obj->transaction.hasMaspTx) {
+        if (!ctx->tx_obj->transaction.hasMaspBuilder) {
+            // MASP Trabsactions must be accompanied by MASP Builders
+            return parser_unexpected_error;
+        } else if (tx_hash_txId(ctx->tx_obj, tx_id_hash) != zxerr_ok) {
+            // compute tx_id hash
+            return parser_unexpected_error;
+        } else if (memcmp(tx_id_hash, ctx->tx_obj->transaction.sections.maspBuilder.target_hash.ptr, HASH_LEN) != 0) {
+            // MASP Builder must have the correct TxId
             return parser_invalid_target_hash;
         }
     }
 
-    if (ctx->tx_obj->transfer.has_shielded_hash && memcmp(ctx->tx_obj->transfer.shielded_hash.ptr, tx_id_hash, HASH_LEN) != 0) {
-        return parser_invalid_target_hash;
-    }
-
-    if(ctx->tx_obj->ibc.transfer.has_shielded_hash && memcmp(ctx->tx_obj->ibc.transfer.shielded_hash.ptr, tx_id_hash, HASH_LEN) != 0) {
-        return parser_invalid_target_hash;
+    if (ctx->tx_obj->typeTx == Transfer && ctx->tx_obj->transfer.has_shielded_hash) {
+        if (!ctx->tx_obj->transaction.hasMaspTx) {
+            // MASP transfers must be accompanied by MASP Transactions
+            return parser_unexpected_error;
+        } else if (memcmp(ctx->tx_obj->transfer.shielded_hash.ptr, tx_id_hash, HASH_LEN) != 0) {
+            // MASP Transactions must have the correct TxId
+            return parser_invalid_target_hash;
+        }
+    } else if (ctx->tx_obj->typeTx == IBC && ctx->tx_obj->ibc.transfer.has_shielded_hash) {
+        if (!ctx->tx_obj->transaction.hasMaspTx) {
+            // IBC MASP transfers must be accompanied by MASP Transactions
+            return parser_unexpected_error;
+        } else if (memcmp(ctx->tx_obj->ibc.transfer.shielded_hash.ptr, tx_id_hash, HASH_LEN) != 0) {
+            // MASP Transactions must have the correct TxId
+            return parser_invalid_target_hash;
+        }
     }
 #endif
 

diff --git a/app/src/parser_print_txn.c b/app/src/parser_print_txn.c
@@ -174,8 +174,8 @@ static parser_error_t printTransferTxn( const parser_context_t *ctx,
     // Get pointer to the outputs
     bytes_t out = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.outputs;
     // Compute number of spends/outs in the builder tx , and number of itemns to be printer for each
-    uint32_t n_spends = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends * (uint32_t) ctx->tx_obj->transaction.isMasp;
-    uint32_t n_outs = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs * (uint32_t) ctx->tx_obj->transaction.isMasp;
+    uint32_t n_spends = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends * (uint32_t) ctx->tx_obj->transfer.has_shielded_hash;
+    uint32_t n_outs = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs * (uint32_t) ctx->tx_obj->transfer.has_shielded_hash;
     uint16_t spend_index = 0;
     uint16_t out_index = 0;
 
@@ -1132,8 +1132,8 @@ static parser_error_t printIBCTxn( const parser_context_t *ctx,
     // Get pointer to the outputs
     bytes_t out = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.outputs;
     // Compute number of spends/outs in the builder tx , and number of itemns to be printer for each
-    uint32_t n_spends = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends * (uint32_t) ctx->tx_obj->transaction.isMasp;
-    uint32_t n_outs = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs * (uint32_t) ctx->tx_obj->transaction.isMasp;
+    uint32_t n_spends = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends * (uint32_t) ctx->tx_obj->ibc.transfer.has_shielded_hash;
+    uint32_t n_outs = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs * (uint32_t) ctx->tx_obj->ibc.transfer.has_shielded_hash;
     uint16_t spend_index = 0;
     uint16_t out_index = 0;
 
@@ -1445,8 +1445,8 @@ static parser_error_t printNFTIBCTxn( const parser_context_t *ctx,
     // Get pointer to the outputs
     bytes_t out = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.outputs;
     // Compute number of spends/outs in the builder tx , and number of itemns to be printer for each
-    uint32_t n_spends = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends * (uint32_t) ctx->tx_obj->transaction.isMasp;
-    uint32_t n_outs = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs * (uint32_t) ctx->tx_obj->transaction.isMasp;
+    uint32_t n_spends = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_spends * (uint32_t) ctx->tx_obj->ibc.transfer.has_shielded_hash;
+    uint32_t n_outs = ctx->tx_obj->transaction.sections.maspBuilder.builder.sapling_builder.n_outputs * (uint32_t) ctx->tx_obj->ibc.transfer.has_shielded_hash;
     uint16_t spend_index = 0;
     uint16_t out_index = 0;
 

diff --git a/app/src/parser_txdef.h b/app/src/parser_txdef.h
@@ -280,7 +280,8 @@ typedef struct {
     header_t header;
     sections_t sections;
     uint8_t maspTx_idx;
-    bool isMasp;
+    bool hasMaspTx;
+    bool hasMaspBuilder;
 } transaction_t;
 
 

diff --git a/tests/testvectors.json b/tests/testvectors.json
@@ -50947,4 +50947,4 @@
         ],
         "valid": true
     }
-]
+]