Skip to content

Fix signing for non-MASP IBC Transfers #2

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix signing for non-MASP IBC Transfers #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 3 additions & 5 deletions app/src/parser_impl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
44 changes: 29 additions & 15 deletions app/src/parser_impl_txn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
12 changes: 6 additions & 6 deletions app/src/parser_print_txn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down
3 changes: 2 additions & 1 deletion app/src/parser_txdef.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@ typedef struct {
header_t header;
sections_t sections;
uint8_t maspTx_idx;
bool isMasp;
bool hasMaspTx;
bool hasMaspBuilder;
} transaction_t;


Expand Down
2 changes: 1 addition & 1 deletion tests/testvectors.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50947,4 +50947,4 @@
],
"valid": true
}
]
]
Loading