Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Compilation of Ledger's app
src/glyphs.c
src/glyphs.h
build/
bin/
debug/
dep/
Expand Down
3 changes: 2 additions & 1 deletion doc/TRANSACTION.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ The hathor transaction serialized
| --- | :---: | --- |
| `version_byte` | 1 | Version byte for the transaction apdu protocol |
| `change_info` | var | Bytes representing the existence of a change output and the bip32 path of the change address |
| `tx_version` | 2 | TX Version, indicates this is a transaction, block, etc. |
| `signal_bits` | 1 | Signal bits |
| `tx_version` | 1 | TX Version, indicates this is a transaction, block, etc. |
| `num_tokens` | 1 | Number of token UIDs present on the data below |
| `num_inputs` | 1 | Number of inputs present on the data below |
| `num_outputs` | 1 | Number of outputs present on the data below |
Expand Down
10 changes: 10 additions & 0 deletions src/constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,13 @@
* Any other means that index-1 is the index of token on the token array
*/
#define TOKEN_DATA_INDEX_MASK 0x7Fu

/**
* Mint authority mask
*/
#define MINT_AUTHORITY_MASK 0x01u

/**
* Melt authority mask
*/
#define MELT_AUTHORITY_MASK 0x02u
25 changes: 16 additions & 9 deletions src/handler/sign_tx.c
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,8 @@ uint16_t read_change_info(buffer_t *cdata) {
* returns a 16 bit error code if it fails and 0 on success
**/
uint16_t read_tx_data(buffer_t *cdata) {
if (!(buffer_read_u16(cdata,
&G_context.tx_info.tx_version,
BE) && // read version bytes (Big Endian)
if (!(buffer_read_u8(cdata, &G_context.tx_info.signal_bits) && // read signal bits
buffer_read_u8(cdata, &G_context.tx_info.tx_version) && // read version bytes
buffer_read_u8(cdata,
&G_context.tx_info.remaining_tokens) && // read number of tokens, inputs
// and outputs, respectively
Expand Down Expand Up @@ -378,7 +377,11 @@ bool _decode_elements() {

// check uid is on registry
int8_t registry_index = find_token_registry_index(uid);
if (registry_index == -1) THROW(TX_STATE_ERR); // not on registry, token MUST be verified
if (registry_index == -1) {
// not on registry, token MUST be verified
PRINTF("[-] sign-tx: Token UID not on registry\n");
THROW(TX_STATE_ERR);
}
G_context.tx_info.tokens[G_context.tx_info.tokens_len++] =
&G_token_symbols.tokens[registry_index];

Expand All @@ -398,6 +401,7 @@ bool _decode_elements() {
// Check data_len
uint16_t input_data_len = read_u16_be(G_context.tx_info.buffer, 33);
if (input_data_len > 0) {
PRINTF("[-] sign-tx: Received a non-zero input data length (%d)\n", input_data_len);
THROW(TX_STATE_ERR);
}

Expand All @@ -418,14 +422,12 @@ bool _decode_elements() {
size_t output_len =
parse_output(G_context.tx_info.buffer, G_context.tx_info.buffer_len, &output);

// check the output token_data is correct
if (output.token_data & TOKEN_DATA_AUTHORITY_MASK) {
// authority outputs are not allowed!
THROW(TX_STATE_ERR);
}
// We exclude the equal case since index == 0 means HTR
if ((output.token_data & TOKEN_DATA_INDEX_MASK) > G_context.tx_info.tokens_len) {
// index out of bounds
PRINTF("[-] sign-tx: Invalid token index (%d), only have %d on registry\n",
output.token_data & TOKEN_DATA_INDEX_MASK,
G_context.tx_info.tokens_len);
THROW(TX_STATE_ERR);
}

Expand All @@ -443,6 +445,9 @@ bool _decode_elements() {
if (output.index == info->index) {
// verify change output is going to user's wallet
if (!verify_change(info, output)) {
PRINTF("[-] sign-tx: Invalid change output (index=%d, token_data=%d)\n",
output.index,
output.token_data);
THROW(TX_STATE_ERR);
}
}
Expand All @@ -462,6 +467,7 @@ bool _decode_elements() {
G_context.tx_info.outputs[G_context.tx_info.buffer_output_len++] = output;
} else {
// We've reached the end of what we should read but the buffer isn't empty
PRINTF("[-] sign-tx: Received more data than expected\n");
THROW(TX_STATE_ERR);
}

Expand Down Expand Up @@ -561,6 +567,7 @@ bool receive_data(buffer_t *cdata, uint8_t chunk) {

switch (decode_elements()) {
case TX_STATE_ERR:
PRINTF("[-] sign-tx: Invalid TX received\n");
explicit_bzero(&G_context, sizeof(G_context));
io_send_sw(SW_INVALID_TX);
ui_menu_main();
Expand Down
12 changes: 12 additions & 0 deletions src/transaction/deserialize.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@
#include "../common/buffer.h"
#include "types.h"

bool is_authority_output(uint8_t token_data) {
return (token_data & TOKEN_DATA_AUTHORITY_MASK) > 0;
}

bool is_mint_authority(uint8_t token_data, uint64_t value) {
return is_authority_output(token_data) && value == MINT_AUTHORITY_MASK;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be && (value & MINT_AUTHORITY_MASK) > 0, shouldn't it?

The same for melt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is the Mint+Melt case, where an output is authority for both mint and melt.
This edge case may be complicated for the user so I decided not to allow this case in the Ledger app.

Do you think we should allow this case as a special Mint+Melt output on the UI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to limit this but I don't see as this complicated, we could show exactly as you wrote: Mint + Melt or Mint and Melt.

If we decide to leave this out, we should just make clear in the design

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve reopened this thread to address the issue. I believe we need to correctly implement the logic in these methods, while managing the mint and melt permissions elsewhere. The current approach might be misleading and prone to errors during future updates.

Regarding the minting and melting capabilities, I found the previous discussion a bit confusing. It doesn’t seem possible to mint and melt the same token simultaneously. So, the real question is whether a token authority should have both capabilities. I believe they should.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to put the reason the discussion was closed, this was meant to be discussed and implemented on another PR.
We can block this PR to solve this issue but this can easily be implemented later depending on the decision.

}

bool is_melt_authority(uint8_t token_data, uint64_t value) {
return is_authority_output(token_data) && value == MELT_AUTHORITY_MASK;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here!

}

/**
* XXX: considering only P2PKH, without timelock
* Validates that a script has the format of P2PKH. Throws an exception if doesn't.
Expand Down
32 changes: 32 additions & 0 deletions src/transaction/deserialize.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,36 @@
*/
size_t parse_output(uint8_t *in, size_t inlen, tx_output_t *output);

/**
* Check if output is an authority output
*
* @param[in] token_data
* Token data
*
* @return true if output is an authority output
*/
bool is_authority_output(uint8_t token_data);

/**
* Check if output is a mint authority
*
* @param[in] token_data
* Token data
* @param[in] value
* Value
* @return true if output is a mint authority
*/
bool is_mint_authority(uint8_t token_data, uint64_t value);

/**
* Check if output is a melt authority
*
* @param[in] token_data
* Token data
* @param[in] value
* Value
* @return true if output is a melt authority
*/
bool is_melt_authority(uint8_t token_data, uint64_t value);

#endif
3 changes: 2 additions & 1 deletion src/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ typedef struct {
uint8_t change_output_index;
bip32_path_t change_bip32_path;
// tx
uint16_t tx_version;
uint8_t signal_bits;
uint8_t tx_version;
uint8_t remaining_tokens;
uint8_t remaining_inputs;
uint8_t outputs_len;
Expand Down
63 changes: 58 additions & 5 deletions src/ui/display.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@
#include "../hathor.h"
#include "../storage.h"
#include "../sw.h"
#include "../transaction/deserialize.h"
#include "action/validate.h"
#include "display.h"
#include "menu.h"

static action_validate_cb g_validate_callback;
static char g_amount[30];
static char g_authority[30];
static bool g_is_authority;
static char g_output_index[10];
static char g_address[B58_ADDRESS_LEN];
static char g_token_symbol[MAX_TOKEN_SYMBOL_LEN + 1];
Expand Down Expand Up @@ -60,6 +63,13 @@ UX_STEP_NOCB(ux_display_address_step,
.title = "Address",
.text = g_address,
});
// Step with title/text for authority
UX_STEP_NOCB(ux_display_authority_step,
bnnn_paging,
{
.title = "Authority",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have room for "Token Authority"? Or is it too big?

.text = g_authority,
});
// Step with title/text for amount
UX_STEP_NOCB(ux_display_amount_step,
bnnn_paging,
Expand Down Expand Up @@ -200,6 +210,15 @@ int ui_display_tx_confirm() {
return 0;
}

// SIGN_TX: confirm authority output
UX_FLOW(ux_display_tx_authority_output_flow,
&ux_display_review_output_step, // Output <curr>/<total>
&ux_display_address_step, // address
&ux_display_authority_step, // Mint authority or Melt authority
&ux_display_approve_step, // accept => decode next component and redisplay if needed
&ux_display_reject_step, // reject => return error
FLOW_LOOP);

// SIGN_TX: confirm output
UX_FLOW(ux_display_tx_output_flow,
&ux_display_review_output_step, // Output <curr>/<total>
Expand Down Expand Up @@ -276,6 +295,8 @@ bool skip_change_outputs() {

/**
* Prepare the UX screen values of the current output to confirm
* Returns true if we have no more outputs on buffer to show.
* Returns false if the next output is ready to be shown to the user for confirmation.
*/
bool prepare_display_output() {
// Check we have confirmed all outputs before attempting to display
Expand Down Expand Up @@ -314,8 +335,11 @@ bool prepare_display_output() {
base58_encode(address, ADDRESS_LEN, b58address, B58_ADDRESS_LEN);
memmove(g_address, b58address, B58_ADDRESS_LEN);

// set g_ammount (HTR value)
// Clean amount and authority
memset(g_amount, 0, sizeof(g_amount));
memset(g_authority, 0, sizeof(g_authority));

// Get token symbol
int8_t token_index = output.token_data & TOKEN_DATA_INDEX_MASK;
char symbol[MAX_TOKEN_SYMBOL_LEN + 1];
uint8_t symbol_len;
Expand All @@ -330,9 +354,34 @@ bool prepare_display_output() {
strlcpy(symbol, token->symbol, MAX_TOKEN_SYMBOL_LEN + 1);
symbol_len = strlen(token->symbol);
}
strlcpy(g_amount, symbol, MAX_TOKEN_SYMBOL_LEN + 1);
g_amount[symbol_len] = ' ';
format_value(output.value, g_amount + symbol_len + 1);

if (is_authority_output(output.token_data)) {
g_is_authority = true;
// set g_authority
strlcpy(g_authority, symbol, MAX_TOKEN_SYMBOL_LEN + 1);
g_authority[symbol_len] = ' ';
if (is_mint_authority(output.token_data, output.value)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you showing what action is being performed? I mean, you can have a mint authority with the following actions: (i) minting, (ii) destroying, and (iii) delegating.

strlcpy(g_authority + symbol_len + 1, "Mint", 30);
} else {
if (is_melt_authority(output.token_data, output.value)) {
strlcpy(g_authority + symbol_len + 1, "Melt", 30);
} else {
// This authority is unknown, so we treat it as invalid
PRINTF("[-] Unknown authority received in value %d\n", output.value);
explicit_bzero(&G_context, sizeof(G_context));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we clear globals here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clearing the global context.
The G_token_symbols can be cleared but it is not a security issue to leave them initialized since only user confirmed tokens enter the G_token_symbols context.

io_send_sw(SW_INVALID_TX);
ui_menu_main();
return true;
}
}
} else {
g_is_authority = false;
// set g_ammount (HTR value)
strlcpy(g_amount, symbol, MAX_TOKEN_SYMBOL_LEN + 1);
g_amount[symbol_len] = ' ';
format_value(output.value, g_amount + symbol_len + 1);
}

return false;
}

Expand All @@ -356,7 +405,11 @@ int ui_display_tx_outputs() {
// skip changes, return ok if there is no more on buffer
if (prepare_display_output()) return 0;
g_validate_callback = &ui_confirm_output; // show next until need more
ux_flow_init(0, ux_display_tx_output_flow, NULL);
if (g_is_authority) {
ux_flow_init(0, ux_display_tx_authority_output_flow, NULL);
} else {
ux_flow_init(0, ux_display_tx_output_flow, NULL);
}

return 0;
}
Expand Down
8 changes: 8 additions & 0 deletions tests/app_client/token.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,11 @@ def serialize(self, signature: Optional[bytes] = None) -> bytes:
cdata = b"".join([cdata, signature])

return cdata

def __str__(self):
return (
"Token("
f"symbol={self.symbol}, "
f"name={self.name}, "
f"uid={self.uid_hex})"
)
21 changes: 16 additions & 5 deletions tests/app_client/transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,13 @@ def __str__(self):
class Transaction:
def __init__(
self,
tx_version,
signal_bits: int,
tx_version: int,
tokens: List[bytes],
inputs: List[TxInput],
outputs: List[TxOutput],
) -> None:
self.signal_bits = signal_bits
self.tx_version = tx_version
self.tokens = tokens
self.inputs = inputs
Expand All @@ -123,7 +125,8 @@ def serialize(self) -> bytes:

cdata = b"".join(
[
self.tx_version.to_bytes(2, byteorder="big"),
self.signal_bits.to_bytes(1, byteorder="big"),
self.tx_version.to_bytes(1, byteorder="big"),
len(self.tokens).to_bytes(1, byteorder="big"),
len(self.inputs).to_bytes(1, byteorder="big"),
len(self.outputs).to_bytes(1, byteorder="big"),
Expand All @@ -148,7 +151,8 @@ def serialize(self) -> bytes:
def from_bytes(cls, hexa: Union[bytes, BytesIO]):
buf: BytesIO = BytesIO(hex) if isinstance(hexa, bytes) else hexa

tx_version: int = read_uint(buf, 16, byteorder="big")
signal_bits: int = read_uint(buf, 8, byteorder="big")
tx_version: int = read_uint(buf, 8, byteorder="big")
num_tokens: int = read_uint(buf, 8, byteorder="big")
num_inputs: int = read_uint(buf, 8, byteorder="big")
num_outputs: int = read_uint(buf, 8, byteorder="big")
Expand All @@ -169,13 +173,20 @@ def from_bytes(cls, hexa: Union[bytes, BytesIO]):
tx_output = TxOutput.from_bytes(output_bytes)
outputs.append(tx_output)

return cls(tx_version, tokens, inputs, outputs)
return cls(signal_bits, tx_version, tokens, inputs, outputs)

def __str__(self):
stokens = [token.hex() for token in self.tokens]
sinputs = [str(inp) for inp in self.inputs]
soutputs = [str(outp) for outp in self.outputs]
return f"Transaction(tokens={stokens}, inputs={sinputs}, outputs={soutputs})"
return (
"Transaction("
f"tx_version={self.tx_version}, "
f"tokens={stokens}, "
f"inputs={sinputs}, "
f"outputs={soutputs}, "
f"signal_bits={self.signal_bits})"
)

def verify_signature(self, signature: bytes, public_key_bytes: bytes):
"""Verify signature from `self.serialize` that returns the sighash_all bytes
Expand Down
Loading