From 4c73683569e7fa4740bc0ffcc7e0772b32b96f4d Mon Sep 17 00:00:00 2001 From: Alexandre Esteves Date: Mon, 3 May 2021 16:11:21 +0100 Subject: [PATCH 1/3] Fix parsing of calldata of unrecognized methods --- src/evm_parse.c | 43 ++++++++++++++++++++---------------- src/parser.h | 1 + tests/eth-tests.js | 54 ++++++++++++++++++++++++++++++++++++---------- 3 files changed, 69 insertions(+), 29 deletions(-) diff --git a/src/evm_parse.c b/src/evm_parse.c index 72e10615..4fa1d04a 100644 --- a/src/evm_parse.c +++ b/src/evm_parse.c @@ -549,37 +549,44 @@ enum parse_rv parse_abi_call_data(struct EVM_ABI_state *const state, } } - state->state++; - initFixed(&state->argument_state.fixedState, sizeof(state->argument_state)); - if(meta->known_endpoint) { + state->state = ABISTATE_ARGUMENTS; + initFixed(&state->argument_state.fixedState, sizeof(state->argument_state)); if(hasValue) REJECT("No currently supported methods are marked as 'payable'"); char *method_name = PIC(meta->known_endpoint->method_name); ADD_PROMPT("Contract Call", method_name, strlen(method_name), strcpy_prompt); return PARSE_RV_PROMPT; } else { - state->state = ABISTATE_DONE; - // Probably we have to allow this, as the metamask constraint means _this_ endpoint will be getting stuff it doesn't understand a lot. - static char const isPresentLabel[]="Is Present (unsafe)"; - set_next_batch_size(&meta->prompt, 2); + state->state = ABISTATE_UNRECOGNIZED; ADD_ACCUM_PROMPT("Transfer", output_evm_prompt_to_string); - ADD_PROMPT("Contract Data", isPresentLabel, sizeof(isPresentLabel), strcpy_prompt); return PARSE_RV_PROMPT; } } case ABISTATE_ARGUMENTS: { - if(state->argument_index >= meta->known_endpoint->parameters_count) - return PARSE_RV_DONE; - sub_rv = parseFixed(&state->argument_state.fixedState, input, ETHEREUM_WORD_SIZE); // TODO: non-word size values + if(state->state == ABISTATE_ARGUMENTS) { + if(state->argument_index >= meta->known_endpoint->parameters_count) + return PARSE_RV_DONE; + sub_rv = parseFixed(&state->argument_state.fixedState, input, ETHEREUM_WORD_SIZE); // TODO: non-word size values + if(sub_rv != PARSE_RV_DONE) return sub_rv; + const struct contract_endpoint_param parameter = meta->known_endpoint->parameters[state->argument_index++]; + char *argument_name = PIC(parameter.name); + void (*setup_prompt)(uint8_t *buffer, output_prompt_t const *const prompt) = PIC(parameter.setup_prompt); + SET_PROMPT_VALUE(setup_prompt(((struct FixedState*)(&state->argument_state))->buffer, + &entry->data.output_prompt)); + initFixed(&state->argument_state.fixedState, sizeof(state->argument_state)); + ADD_ACCUM_PROMPT_ABI(argument_name, PIC(parameter.output_prompt)); + return PARSE_RV_PROMPT; + } + } + + // Probably we have to allow this, as the metamask constraint means _this_ endpoint will be getting stuff it doesn't understand a lot. + case ABISTATE_UNRECOGNIZED: { + sub_rv = parseFixed(&state->argument_state.fixedState, input, state->data_length - ETHEREUM_SELECTOR_SIZE); // TODO: non-word size values if(sub_rv != PARSE_RV_DONE) return sub_rv; - const struct contract_endpoint_param parameter = meta->known_endpoint->parameters[state->argument_index++]; - char *argument_name = PIC(parameter.name); - void (*setup_prompt)(uint8_t *buffer, output_prompt_t const *const prompt) = PIC(parameter.setup_prompt); - SET_PROMPT_VALUE(setup_prompt(((struct FixedState*)(&state->argument_state))->buffer, - &entry->data.output_prompt)); - initFixed(&state->argument_state.fixedState, sizeof(state->argument_state)); - ADD_ACCUM_PROMPT_ABI(argument_name, PIC(parameter.output_prompt)); + state->state = ABISTATE_DONE; + static char const isPresentLabel[]="Is Present (unsafe)"; + ADD_PROMPT("Contract Data", isPresentLabel, sizeof(isPresentLabel), strcpy_prompt); return PARSE_RV_PROMPT; } diff --git a/src/parser.h b/src/parser.h index 4a274485..c5806bba 100644 --- a/src/parser.h +++ b/src/parser.h @@ -447,6 +447,7 @@ struct EVM_assetCall_state { enum abi_state_t { ABISTATE_SELECTOR, ABISTATE_ARGUMENTS, + ABISTATE_UNRECOGNIZED, ABISTATE_DONE, }; diff --git a/tests/eth-tests.js b/tests/eth-tests.js index 43743231..25173914 100644 --- a/tests/eth-tests.js +++ b/tests/eth-tests.js @@ -97,6 +97,26 @@ const testDeploy = (chainId, withAmount) => async function () { ); }; +const testUnrecognizedCall = (chainId, gasPrice, gasLimit, amountPrompt, amountHex, address, fee, calldata) => async function () { + const tx = rawUnsignedTransaction(chainId, { + nonce: '0x0a', + gasPrice: '0x' + gasPrice, + gasLimit: '0x' + gasLimit, + to: '0x' + address, + value: '0x' + amountHex, + data: '0x' + calldata, + }); + + const prompts = + [[{header: "Transfer", body: amountPrompt + " to " + '0x' + address}], + [{header: "Contract Data", body: "Is Present (unsafe)"}], + [{header: "Maximum Fee", body: fee}], + [finalizePrompt] + ]; + + await testSigning(this, chainId, prompts, tx); +}; + const testCall = (chainId, data, method, args) => async function () { const address = 'df073477da421520cf03af261b782282c304ad66'; const tx = rawUnsignedTransaction(chainId, { @@ -169,16 +189,29 @@ describe("Eth app compatibility tests", async function () { } }); - it('can sign a transaction with unrecognized calldata via the ethereum ledgerjs module', async function() { - await testSigning(this, 43112, - [[{header:"Transfer", body: "0.000000001 nAVAX to 0x0102030400000000000000000000000000000002"}, - {header:"Contract Data", body: "Is Present (unsafe)"}], - [{header:"Maximum Fee", body: "1410000000 GWEI"}], - [{header:"Finalize", body: "Transaction"}] - ], - 'f83880856d6e2edc00832dc6c0940102030400000000000000000000000000000002019190000102030405060708090a0b0c0d0e0f82a8688080' - ); - }); + it('can sign a transaction with unrecognized calldata via the ethereum ledgerjs module', + testUnrecognizedCall(43112, + '6d6e2edc00', + '2dc6c0', + "0.000000001 nAVAX", '01', + "0102030400000000000000000000000000000002", + '1410000000 GWEI', + '90000102030405060708090a0b0c0d0e0f' + ) + ); + + it('can sign a multiple-apdu transaction with unrecognized calldata via the ethereum ledgerjs module', + testUnrecognizedCall(43112, + '6d6e2edc00', + '2dc6c0', + "0.000000001 nAVAX", '01', + "0102030400000000000000000000000000000002", + '1410000000 GWEI', + 'a415bcad000000000000000000000000d3896bdd73e61a4275e27f660ddf095522f0a1d30000000000000000000000000000000000000000000000000de0b6b3a7640000000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000000000000000000000000006f0f6da1852857d7789f68a28bba866671f3880d' + ) + ); + + it('can sign a ERC20PresetMinterPauser pause contract call', testCall(43113, '8456cb59', 'pause', [])); it('can sign a ERC20PresetMinterPauser unpause contract call', testCall(43113, '3f4ba83a', 'unpause', [])); @@ -263,7 +296,6 @@ describe("Eth app compatibility tests", async function () { expect(rv).to.not.equalBytes("9000"); }); - // TODO: something about these should-fail tests having no prompts causes the ones ran after it to fail it.skip('won\'t sign a transaction via a truely gargantuan number', async function() { From 4c2ccf2261191cf5d5080a8d6686abd510159fc3 Mon Sep 17 00:00:00 2001 From: Alexandre Esteves Date: Mon, 3 May 2021 16:23:31 +0100 Subject: [PATCH 2/3] Version 0.5.2 --- Makefile | 2 +- tests/basic-tests.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 7464fc1b..f539f2f4 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ GIT_DESCRIBE ?= $(shell git describe --tags --abbrev=8 --always --long --dirty 2 VERSION_TAG ?= $(shell echo "$(GIT_DESCRIBE)" | cut -f1 -d-) APPVERSION_M=0 APPVERSION_N=5 -APPVERSION_P=1 +APPVERSION_P=2 APPVERSION=$(APPVERSION_M).$(APPVERSION_N).$(APPVERSION_P) # Only warn about version tags if specified/inferred diff --git a/tests/basic-tests.js b/tests/basic-tests.js index 485f235e..82fa84ab 100644 --- a/tests/basic-tests.js +++ b/tests/basic-tests.js @@ -3,7 +3,7 @@ describe("Basic Tests", () => { it('can fetch the version of the app', async function () { const cfg = await this.ava.getAppConfiguration(); expect(cfg).to.be.a('object'); - expect(cfg).to.have.property("version", "0.5.1"); + expect(cfg).to.have.property("version", "0.5.2"); expect(cfg).to.have.property("name", "Avalanche"); }); it('returns the expected wallet ID', async function () { From af940ba4e1ce892fd3f8dbd0f14edf0eb9ef4c5b Mon Sep 17 00:00:00 2001 From: Alexandre Esteves Date: Mon, 3 May 2021 22:23:04 +0100 Subject: [PATCH 3/3] Cleanup --- src/evm_parse.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/evm_parse.c b/src/evm_parse.c index 4fa1d04a..e6859441 100644 --- a/src/evm_parse.c +++ b/src/evm_parse.c @@ -555,29 +555,27 @@ enum parse_rv parse_abi_call_data(struct EVM_ABI_state *const state, if(hasValue) REJECT("No currently supported methods are marked as 'payable'"); char *method_name = PIC(meta->known_endpoint->method_name); ADD_PROMPT("Contract Call", method_name, strlen(method_name), strcpy_prompt); - return PARSE_RV_PROMPT; } else { state->state = ABISTATE_UNRECOGNIZED; ADD_ACCUM_PROMPT("Transfer", output_evm_prompt_to_string); - return PARSE_RV_PROMPT; } + + return PARSE_RV_PROMPT; } case ABISTATE_ARGUMENTS: { - if(state->state == ABISTATE_ARGUMENTS) { - if(state->argument_index >= meta->known_endpoint->parameters_count) - return PARSE_RV_DONE; - sub_rv = parseFixed(&state->argument_state.fixedState, input, ETHEREUM_WORD_SIZE); // TODO: non-word size values - if(sub_rv != PARSE_RV_DONE) return sub_rv; - const struct contract_endpoint_param parameter = meta->known_endpoint->parameters[state->argument_index++]; - char *argument_name = PIC(parameter.name); - void (*setup_prompt)(uint8_t *buffer, output_prompt_t const *const prompt) = PIC(parameter.setup_prompt); - SET_PROMPT_VALUE(setup_prompt(((struct FixedState*)(&state->argument_state))->buffer, - &entry->data.output_prompt)); - initFixed(&state->argument_state.fixedState, sizeof(state->argument_state)); - ADD_ACCUM_PROMPT_ABI(argument_name, PIC(parameter.output_prompt)); - return PARSE_RV_PROMPT; - } + if(state->argument_index >= meta->known_endpoint->parameters_count) + return PARSE_RV_DONE; + sub_rv = parseFixed(&state->argument_state.fixedState, input, ETHEREUM_WORD_SIZE); // TODO: non-word size values + if(sub_rv != PARSE_RV_DONE) return sub_rv; + const struct contract_endpoint_param parameter = meta->known_endpoint->parameters[state->argument_index++]; + char *argument_name = PIC(parameter.name); + void (*setup_prompt)(uint8_t *buffer, output_prompt_t const *const prompt) = PIC(parameter.setup_prompt); + SET_PROMPT_VALUE(setup_prompt(((struct FixedState*)(&state->argument_state))->buffer, + &entry->data.output_prompt)); + initFixed(&state->argument_state.fixedState, sizeof(state->argument_state)); + ADD_ACCUM_PROMPT_ABI(argument_name, PIC(parameter.output_prompt)); + return PARSE_RV_PROMPT; } // Probably we have to allow this, as the metamask constraint means _this_ endpoint will be getting stuff it doesn't understand a lot.