Transaction Hashes are Computed Incorrectly

[mgraczyk]

ganache-core uses EIP-155 hashes to identify transactions. This causes JSON-RPC calls like eth_sendRawTransaction to return unexpected values. The EIP-155 hash should be used to generate a signature. It should not be used to identify the transaction in the RPC mechanism.

Expected Behavior

$ curl -X POST --data '{"jsonrpc":"2.0","method":"eth_sendRawTransaction","params":["0xf86b808504e3b2920082520894a3bd45f7b000a6d3d2359b0577ed34adbdfc25eb87038d7ea4c68000802aa0dec7e8ff13c93cdc9a75432f01730bdc3a20fc0eaf820a7234c1aabdbf6ccb64a0601845af5cbd02e9c443f870bdbee61c93a5533c4c3f84931a0e1dfab30d39ab"],"id":1}' https://ropsten.infura.io
{"jsonrpc":"2.0","id":1,"result":"0xe1ba8a320f9fc6846127b244faf9825028d7298c4d34c4aa0fc57d48b85a7ab3"}

Current Behavior

$ curl -X POST --data '{"jsonrpc":"2.0","method":"eth_sendRawTransaction","params":["0xf86b808504e3b2920082520894a3bd45f7b000a6d3d2359b0577ed34adbdfc25eb87038d7ea4c68000802aa0dec7e8ff13c93cdc9a75432f01730bdc3a20fc0eaf820a7234c1aabdbf6ccb64a0601845af5cbd02e9c443f870bdbee61c93a5533c4c3f84931a0e1dfab30d39ab"],"id":1}' localhost:8545
{"id":1,"jsonrpc":"2.0","result":"0x56860ff2721668edd33f5f86abdbd44fe1a8e8f3efba1a12dc76f2c9da53b836"}

Notice that the hash is 0x56860ff2721668edd33f5f86abdbd44fe1a8e8f3efba1a12dc76f2c9da53b836 instead of sha3(data) = 0xe1ba8a320f9fc6846127b244faf9825028d7298c4d34c4aa0fc57d48b85a7ab3

Possible Solution

Always use sha3(rawData) to identify signed transactions, including over json rpc.

Steps to Reproduce (for bugs)

Run gananche-cli in the background using this:

const secretKey = Buffer.from('8874F01D72E7BE5D5417B55E0F21698692704F408C1AFF382641DB0423F1B2E1', "hex");
const config = {
  "accounts": [
    { "balance": 1000000 * 1000000000000000000,
      "secretKey": secretKey
    }, ], };

require("ganache-cli").server(config).listen(8545);

Then run curl as above.

Context

Transaction hashes computed outside of ganache-core/ganache-cli do not match those expected or returned by ganache-X. This makes it difficult to use ganache-X for testing applications that interact with other Ethereum nodes.

Your Environment

• Version used: Ganache CLI v7.0.0-beta.0 (ganache-core: 3.0.0-beta.0)
• Operating System and version: macOS 10.13.2

[mgraczyk]

@benjamincburns Any thoughts? This makes it difficult to use ganache-cli for testing.

[benjamincburns]

This makes it difficult to use ganache-cli for testing.

Can you please qualify this a bit? What kind of testing are we talking about here? What, specifically, is/are your test/tests validating which cannot be validated in ganache-cli today?

Bear in mind that ganache is really designed for testing Dapp interaction with the network via the RPC interface. As it is today, Ganache isn't designed to be 100% compliant with all of the various cryptographic requirements which true nodes must follow. This is because we offer certain testing features which would be precluded by strict adherence to the cryptographic rules of the protocol. For example, with Ganache you have the ability to pose as any account without knowing its private key.

That said, I can imagine in the future we might be able to selectively drop some of these features when running in a "secure" mode, and provide stricter adherence to cryptographic rules/consensus mechanisms.

[mgraczyk]

Thanks for the response. I think this issue is important for any Dapp that sends signed transactions.

I can go into more detail if you'd like, but for brevity I'll say that there is no way to robustly send transactions unless the hash can be computed before sending. This is because the eth_sendRawTransaction RPC call can time out, in which case the transaction hash will never be returned. In order to query about the status of the transaction using eth_getTransactionByHash, the hash must be known.

[mgraczyk]

My tests check that valid transactions are eventually committed even when eth_sendRawTransaction times out.

[benjamincburns]

If you need the transaction receipt to determine when to update your application state, I'd think that you're better off relying on logs subscriptions/filters for that.

Ultimately if transaction submissions timeout, there's really very little your application can do. Assuming this is an http or socket timeout, you should set this high enough to make it an unlikely occurrence, and notify the user when it happens, perhaps with a link to EtherScan which they can use to check the state of the transaction and decide what to do from there.

Trying to make the transport layer have perfect availability is always going to be a losing battle. For example, what happens if after that timeout occurs your app can't connect to any node for a substantial period of time? Or, assuming this interaction is client side, what happens if the user refreshes the page after the transaction is sent, but before the hash comes back?

If these situations are capable of breaking your Dapp for other reasons (e.g. because you need to check the results/logs from one transaction in order to fire off the next, but failing to fire off the next leaves you in some bad/irreversible intermediate state), then I'd suggest that you have an architectural problem in your contracts, and you should focus on eliminating that problem from the structure of your application rather than trying to make a failure-prone transport layer have perfect availability.

[mgraczyk]

Thanks for the response. I think we agree that there is no way to make the transport layer have perfect availability. Unfortunately the fix is actually opposite what you are saying. Because the transport layer is not perfect, it is 100% required for security that transactions are logged locally before sending. Once a signed transaction is externalized, it must be presumed that it will eventually commit until it can be proved otherwise. Client applications that don't do this will cause their users to have their funds stolen.

relying on logs subscriptions/filters for that.

There are no logs when the transaction times out, and these are much less efficient for the real node than eth_getTransactionByHash.

perhaps with a link to EtherScan which they can use to check the state of the transaction and decide what to do from there.

Creating such a link requires knowledge of the transaction hash before the timeout.

what happens if the user refreshes the page after the transaction is sent, but before the hash comes back?

Robust, secure applications will log the transaction locally and durably before attempting to send. When the user refreshes, the transaction will still be known.

then I'd suggest that you have an architectural problem in your contracts

This issue exists without any smart contracts. Imagine a wallet that provides fault tolerant nonce management to its users. It can't be implemented efficiently unless it is possible to compute the transaction hash before sending the transaction.

[benjamincburns]

Client applications that don't do this will cause their users to have their funds stolen.

I don't really understand this claim, but I'm not sure that I need to right now. Honestly, I'm really not the best person to evaluate the merits of one approach vs the other, as I haven't studied contract/dapp security very intensively.

I'd strongly encourage you to contribute your thoughts on this subject to ConsenSys Diligence's guide on ethereum smart contract best practices. The GH repo behind this is here. If they know of better approaches to mitigate the problems you're describing, they'll suggest them. If not, they'll accept your contribution, and the Dapp development world will be a better place for it.

[mgraczyk]

Thanks for the link.

I'm not sure what this has to do with smart contracts though. I'm not talking about smart contracts. I'm talking about a bug in ganache-core in which transaction hashes are computed incorrectly.

I understand if this bug isn't high priority for you. Would you accept a pull request that fixes it?

[mgraczyk]

I don't really understand this claim, but I'm not sure that I need to right now.

Just to be concrete, bugs like the one I'm describing are common in the cryptocurrency world. I was able to find an example by looking at the most recent 10 transactions sent to CryptoKittiesCore.

Compare T1 and T2.

It is clear that the user only meant to send T1. T2 was sent because the cryptokitties app did not carefully manage outgoing transactions. In this case, the user only lost $0.05, but I've seen cases with far worse consequences.

My application makes it much harder for users to make this mistake, but it is difficult to test with ganache-cli.

[mgraczyk]

Another curious example: It appears a similar bug has been causing this account to lose its owner ~$10 an hour for the last 48 days, for a total loss of around $11,500.

[benjamincburns]

Would you accept a pull request that fixes it?

Definitely, so long as it doesn't break the features I mentioned earlier.

I agree that your solution of persisting the tx hash prior to submission is a strong one. My failure to understand really lies in whether or not this is a security flaw. I don't have enough info to evaluate the second example (account losing $10/hr), so discounting that one, I don't see how anybody's ETH is being stolen, or how a malicious third party would exploit this to their gain.

I'm not sure what this has to do with smart contracts though.

Smart contracts aren't very useful unless you can safely execute transactions against them. I think the subject is likely in scope.

[Ykid]

I met with hash inconsistency issues too. May I know if it is relevant ? I am didn't use EIP155 signature and my test is as follows. Thanks for your help !

From Ganache Cli

• version: Ganache CLI v6.2.3 (ganache-core: 2.3.1)

eth_getTransactionByHash
   > {
   >   "jsonrpc": "2.0",
   >   "method": "eth_getTransactionByHash",
   >   "params": [
   >     "0xbed7a8d01e51b7bd38d5850c889c3ff7c77187096d65759563a3af6948aa14dd"
   >   ],
   >   "id": 0
   > }
 <   {
 <     "id": 0,
 <     "jsonrpc": "2.0",
 <     "result": {
 <       "hash": "0xbed7a8d01e51b7bd38d5850c889c3ff7c77187096d65759563a3af6948aa14dd",
 <       "nonce": "0x0",
 <       "blockHash": "0xa6fae628979e440a3aec283b59bd700903adc4ddc46d5d4bb316d7d688b2cd14",
 <       "blockNumber": "0x2",
 <       "transactionIndex": "0x0",
 <       "from": "0x9e0b01422022d36c49e9e7367bb913a21971f31d",
 <       "to": "0xb5e64298c11ae039a26a9bd337a08e0a16ff8f7c",
 <       "value": "0xde0b6b3a7640000",
 <       "gas": "0x5208",
 <       "gasPrice": "0x10642ac00",
 <       "input": "0x",
 <       "v": "0x1c",
 <       "r": "0x4d27429c02ae442b1a7fed7e97b02d9ac6ac943f9a2aead49cc4d4bc8ef49a86",
 <       "s": "0x33e11db734724dcdb1f5988ea4fcbf9dd992ba74e14288f3801f9cc954c82e7b"
 <     }
 <   }

From Geth

• the transaction is signed without chainId

curl \
  -H "Content-Type: application/json" \
  -X POST \
  --data '{"jsonrpc":"2.0","method":"eth_getTransactionByHash","params":["0x201c35a2b4e65289100b7c06e4ea3f00037f4f8ad57ee6a627611993becebabd"],"id":67}' \
http://localhost:8545 | jq

• geth version

Geth
Version: 1.8.19-stable
Architecture: amd64
Protocol Versions: [63 62]
Network Id: 1
Go Version: go1.11.2
Operating System: darwin
GOPATH=
GOROOT=/usr/local/Cellar/go/1.11.2/libexec

• result

{
  "jsonrpc": "2.0",
  "id": 67,
  "result": {
    "blockHash": "0xcfdb376a115ac00c171c1b0adcbe76d59e2d2476988236b356b770c21ed07344",
    "blockNumber": "0x1",
    "from": "0x9e0b01422022d36c49e9e7367bb913a21971f31d",
    "gas": "0x5208",
    "gasPrice": "0x10642ac00",
    "hash": "0x201c35a2b4e65289100b7c06e4ea3f00037f4f8ad57ee6a627611993becebabd",
    "input": "0x",
    "nonce": "0x0",
    "to": "0xb5e64298c11ae039a26a9bd337a08e0a16ff8f7c",
    "transactionIndex": "0x0",
    "value": "0xde0b6b3a7640000",
    "v": "0x1c",
    "r": "0x4d27429c02ae442b1a7fed7e97b02d9ac6ac943f9a2aead49cc4d4bc8ef49a86",
    "s": "0x33e11db734724dcdb1f5988ea4fcbf9dd992ba74e14288f3801f9cc954c82e7b"
  }
}

[davidmurdoch]

@Ykid We just released new ganache-core and ganache-cli betas with a fix for transaction hash issues.

You can install the beta via npm install ganache-core@beta or npm install ganache-cli@beta -g for the CLI.

Please let me know if these hash miss matches are still an issue!