Missing `state->address_step=3;` in `michelson_read_address`?

[fishilico]

Hello,
While reading the state machine in src/operations.c I am wondering how this code works:

ledger-app-tezos/src/operations.c

Lines 199 to 213 in 58797b2

	switch (state->address_step) {
	case 2:
	// Need 1 byte for signature, plus the rest of the hash.
	if (state->addr_length != HASH_SIZE + 1) {
	PARSE_ERROR();
	}
	CALL_SUBPARSER(parse_next_type, byte, &(state->subsub_state), sizeof(state->signature_type));
	memcpy(&(state->signature_type), &(state->subsub_state.body), sizeof(state->signature_type));
	case 3:
	CALL_SUBPARSER(parse_next_type, byte, &(state->subsub_state), sizeof(state->key_hash));
	memcpy(&(state->key_hash), &(state->subsub_state.body), sizeof(state->key_hash));

More precisely after case 0: ... state->address_step=1; and case 1: ... state->address_step=2;, why is there no state->address_step=3; on line 209, at the end of case 2:? The case block "falls through" into case 3 and then the function returns in CALL_SUBPARSER (line 212). When I add some printf statements in this function to analyze where the flow goes, it seems that the "subparsing" goes back to line 207 instead of line 212 and that the code in lines 213-215 is never executed. Did I understand the code correctly or did I miss something?

[mikereinhart]

Thanks for opening this issue, this is indeed a bug!

Did you encounter this while attempting an operation with a Michelson address in binary format or were you inspecting code? The reason I ask - tezos-client only prepares addresses as ASCII so we aren't aware of a real world instance where this code is used.

As we think about the best solution for this, if the code is dead in practice since only ASCII is used, the best solution could be to remove it and avoid the unnecessary feature (and the testing challenges that come from not being able to create binary formatted addresses from tezos-client).

Additional feedback on how you encountered this will help us make the right decision here. This will most likely be done as part of a batch of changes, whenever that may be. Thanks again!

[fishilico]

Did you encounter this while attempting an operation with a Michelson address in binary format or were you inspecting code?

Thanks for your quick reply! I am working on a fuzzing+code coverage tool and wanted to test it on some Ledger Nano applications. I found this bug while investigating why some lines of code appeared to never be reached according to the code coverage report. So I do not know whether a real Tezos transaction runs the buggy code path.

[nmm5055]

Hello,
While reading the state machine in src/operations.c I am wondering how this code works:

ledger-app-tezos/src/operations.c

Lines 199 to 213 in 58797b2

	switch (state->address_step) {
	case 2:
	// Need 1 byte for signature, plus the rest of the hash.
	if (state->addr_length != HASH_SIZE + 1) {
	PARSE_ERROR();
	}
	CALL_SUBPARSER(parse_next_type, byte, &(state->subsub_state), sizeof(state->signature_type));
	memcpy(&(state->signature_type), &(state->subsub_state.body), sizeof(state->signature_type));
	case 3:
	CALL_SUBPARSER(parse_next_type, byte, &(state->subsub_state), sizeof(state->key_hash));
	memcpy(&(state->key_hash), &(state->subsub_state.body), sizeof(state->key_hash));

More precisely after case 0: ... state->address_step=1; and case 1: ... state->address_step=2;, why is there no state->address_step=3; on line 209, at the end of case 2:? The case block "falls through" into case 3 and then the function returns in CALL_SUBPARSER (line 212). When I add some printf statements in this function to analyze where the flow goes, it seems that the "subparsing" goes back to line 207 instead of line 212 and that the code in lines 213-215 is never executed. Did I understand the code correctly or did I miss something?

tz1dk5tsD3vr1ib7XY2CULdWa81vMBbVbjDj