Merge pull request #63 from obsidiansystems/fix-unrecognized

Fix parsing of calldata of unrecognized methods
obsidiansystems · May 3, 2021 · 91cc06a · 91cc06a
2 parents 3837765 + af940ba
commit 91cc06a
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 23 deletions.
diff --git 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/src/evm_parse.c b/src/evm_parse.c
@@ -549,23 +549,18 @@ 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;
     }
+
+    return PARSE_RV_PROMPT;
   }
 
   case ABISTATE_ARGUMENTS: {
@@ -583,6 +578,16 @@ enum parse_rv parse_abi_call_data(struct EVM_ABI_state *const state,
     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;
+    state->state = ABISTATE_DONE;
+    static char const isPresentLabel[]="Is Present (unsafe)";
+    ADD_PROMPT("Contract Data", isPresentLabel, sizeof(isPresentLabel), strcpy_prompt);
+    return PARSE_RV_PROMPT;
+  }
+
   case ABISTATE_DONE:
     return PARSE_RV_DONE;
   }

diff --git 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/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 () {

diff --git 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() {