web3.js icon indicating copy to clipboard operation
web3.js copied to clipboard

Bad input interpretation for `type` parameter to transaction; also exact type is not always respected

Open haltman-at opened this issue 4 years ago • 11 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Current Behavior

The type parameter to the transaction has some problems with the interpretation of its input. Currently, these problems don't matter since the largest legal transaction type is 2, but they could in the future.

Specifically:

  1. If a decimal string is given, it will be interpreted as hex rather than as decimal.
  2. If a number (as number or BN; I haven't tried BigNumber but I assume it's the same) is given, it will be treated as if it were a decimal string and have its decimal digits treated as hex. I.e., if you enter the number 10, it's treated as 16! Worse yet, if you enter the number 0x10, intending to get 16, you'll instead get 0x16! This one is definitely the worst part, I'd say.
  3. Types greater than or equal to 0xc0 are treated as if they were 0, rather than being rejected as illegal.

Also, the exact type entered is not always respected. For instance if you specify a type of 1 but don't specify an access list, you'll get a transaction of type 0, rather than a transaction of type 1 with an empty access list.

Expected Behavior

  1. Numbers should be treated as the numbers that they are, not have their decimal digits read as hex.
  2. Decimal strings, that do not have a "0x" prefix, should be treated as decimal, not hex.
  3. Types greater than or equal to 0xc0 should be treated as invalid, not 0.
  4. The exact type entered should always be respected; type 1 should not be converted to type 0 when no access list is specified (instead the access list should be treated as empty).

Steps to Reproduce

Examples to try:

web3.eth.sendTransaction({ from: accounts[0], to: accounts[1], type: 10 }) //yields 16 instead of 10
web3.eth.sendTransaction({ from: accounts[0], to: accounts[1], type: "10" }) //yields 16 instead of 10
web3.eth.sendTransaction({ from: accounts[0], to: accounts[1], type: 0x10 }) //yields 0x16 instead of 0x10!
web3.eth.sendTransaction({ from: accounts[0], to: accounts[1], type: "0xc0" }) //yields 0 instead of invalid
web3.eth.sendTransaction({ from: accounts[0], to: accounts[1], type: 1 }) //yields 0 rather than 1 unless accessList is also given

Web3.js Version

1.5.3

Environment

  • Operating System: Release Linux Mint 20.2 Uma 64-bit
  • Node.js Version: 12.21.0
  • NPM Version: 6.14.11

Anything Else?

That last one about type 1 should maybe be a separate issue, let me know if you want me to split this up.

haltman-at avatar Dec 03 '21 21:12 haltman-at

HI @haltman-at Thanks for reaching out to us. We will investigate this issue and get back to you on it.

nazarhussain avatar Dec 09 '21 17:12 nazarhussain

Hey there @haltman-at, just wanted to say thank you for providing such a detailed description of the issue, with it's edge cases! We greatly appreciate you reporting this to us, so that we can do something about it

P.s. I'm currently rewriting the logic behind tx validaiton and population, and I can verify that this won't be an issue in 4.x. 4.x will accept numbers, number strings, hex strings, and bigints as number types and convert them all to hex before doing anything with them. It also won't overwrite user input for tx.type if specified, or try to downgrade tx.type based on undefined tx properties

spacesailor24 avatar Jan 27 '22 12:01 spacesailor24

Tl;dr

  • I don't believe Web3 is responsible for this bug
    • I believe the issue might be the use of parseInt(rpc.type, 16); in ganache
    • Web3 should probably format type to always be a hex string before sending to provider to avoid issues like this
  • When passing type: 1 with no accessList, the transaction sent by Web3 is correct, but both the results from Geth and Ganache have type: '0x0'
    • Web3 currently defaults accessList to [] when using web3-eth-accounts to sign a transaction here, but this is not happening when web3.eth.sendTransaction is called (we should change this)
  • We should consider throwing errors for transaction types unsupported by Web3
    • I think it's probably best to submit whatever the user gives us to the provider in case the provider has access to a network that supports the type. However it could be argued that if Web3 doesn't do expected type related things to the transaction before sending, then Web3 is producing malformed transactions

I was able to reproduce the incorrect type issues in error messages reported when trying to run:

const Web3 = require('web3');

const ENV = require('./env.json');

let web3;

(async () => {
    web3 = new Web3(ENV.ganache.providerUrl);
    const accounts = await web3.eth.getAccounts();
    const tx = await web3.eth.sendTransaction({
        from: accounts[0],
        to: accounts[0],
        type: 10
    });
    console.log('Receipt', await web3.eth.getTransactionReceipt(tx.transactionHash));
})()

yields (when running against ganache@^7.0.1):

/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/web3-core-helpers/lib/errors.js:28
        var err = new Error('Returned error: ' + message);
                  ^

Error: Returned error: Invalid transaction type: 16
    at Object.ErrorResponse (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/web3-core-helpers/lib/errors.js:28:19)
    at /home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/web3-core-requestmanager/lib/index.js:305:36
    at XMLHttpRequest.request.onreadystatechange (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/web3-providers-http/lib/index.js:98:13)
    at XMLHttpRequestEventTarget.dispatchEvent (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/xhr2-cookies/dist/xml-http-request-event-target.js:34:22)
    at XMLHttpRequest._setReadyState (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/xhr2-cookies/dist/xml-http-request.js:208:14)
    at XMLHttpRequest._onHttpResponseEnd (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/xhr2-cookies/dist/xml-http-request.js:318:14)
    at IncomingMessage.<anonymous> (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/xhr2-cookies/dist/xml-http-request.js:289:61)
    at IncomingMessage.emit (node:events:402:35)
    at endReadableNT (node:internal/streams/readable:1343:12)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)

Notice the

Error: Returned error: Invalid transaction type: 16

that shows the incorrect type conversion from type: 10 to type: 16.

However, when using @chainsafe/geth-dev-assistant to run an instamine node with unlocked accounts, I don't get an error reported. It seems geth defaults to 0x0 for out of bounds tx types

I console.loged data here and err and result here. This resulted in the console output (when running @chainsafe/geth-dev-assistant and running the above JS code snippet):

This is what Web3 is sending to the provider:

data {
  method: 'eth_sendTransaction',
  params: [
    {
      from: '0x49540f25e04b99980b6861e1ae4e3fc9e62e0caf',
      to: '0x49540f25e04b99980b6861e1ae4e3fc9e62e0caf',
      type: 10, // <- Notice this is still 10 (as a number)
      gasPrice: '0xc2ab958f'
    }
  ],
  callback: undefined
}

And this is the response from Geth:

err, result null {
  jsonrpc: '2.0',
  id: 5,
  result: {
    blockHash: '0x55a30177ae127e08677f7256e3c2d6b5ed4a3cd730035803193ee752c14767a5',
    blockNumber: '0x3',
    contractAddress: null,
    cumulativeGasUsed: '0x5208',
    effectiveGasPrice: '0xc2ab958f',
    from: '0x49540f25e04b99980b6861e1ae4e3fc9e62e0caf',
    gasUsed: '0x5208',
    logs: [],
    logsBloom: '0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000',
    status: '0x1',
    to: '0x49540f25e04b99980b6861e1ae4e3fc9e62e0caf',
    transactionHash: '0xe472c2a5d456bbdd54be1588fc8144004b967b6013e227881ea2a69efe339a2c',
    transactionIndex: '0x0',
    type: '0x0' // <- Notice how this is now '0x0'
  }
}

Now when executing the same JS code snippet against a Ganache node:

Data sent by Web3 to Ganache node:

data {
  method: 'eth_sendTransaction',
  params: [
    {
      from: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      to: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      type: 10, // <- Notice this is still 10 (as a number)
      gasPrice: '0x77359400'
    }
  ],
  callback: undefined
}

Our response from Ganache:

err, result null {
  id: 4,
  jsonrpc: '2.0',
  error: {
    message: 'Invalid transaction type: 16', // <- Notice incorrect type conversion, 10 (as a number) was treated as an unprefixed hexadecimal 
    stack: 'Error: Invalid transaction type: 16\n' +
      '    at Function.typeOf (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:181313)\n' +
      '    at Function.typeOfRPC (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:181539)\n' +
      '    at Function.fromRpc (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:180311)\n' +
      '    at EthereumApi.eth_sendTransaction (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:15938)\n' +
      '    at EthereumApi.n.value (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:106491)\n' +
      '    at /home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:238400\n' +
      '    at RequestCoordinator.<anonymous> (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:238228)\n' +
      '    at /home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:238471\n' +
      '    at new Promise (<anonymous>)\n' +
      '    at RequestCoordinator.queue (/home/anon/tmp/web3-4597-incorrect-tx-type/node_modules/ganache/dist/node/1.js:2:238330)',
    code: -32700
  }
}

As you can see in the above output, the error is coming from Function.typeOf in the ganache package here


When passing type: 1 with no accessList, the transaction sent by Web3 is correct, but the resulting transaction are not:

data {
  method: 'eth_sendTransaction',
  params: [
    {
      from: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      to: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      type: 1, // <- Correct type (is a number)
      gasPrice: '0x77359400'
    }
  ],
  callback: undefined
}

result from Ganache (and Geth):

err, result null {
  id: 6,
  jsonrpc: '2.0',
  result: {
    transactionHash: '0x4647ca4359ce5f7fabb5a02726e4125b3190e0bfb8cf9f3d1b0f864ec93bd214',
    transactionIndex: '0x0',
    blockNumber: '0x1',
    blockHash: '0x2fd1db192c49e20597b2faeb436f40c1a09a680f6f57552d15c6f52111119a34',
    from: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
    to: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
    cumulativeGasUsed: '0x5208',
    gasUsed: '0x5208',
    contractAddress: null,
    logs: [],
    logsBloom: '0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000',
    status: '0x1',
    effectiveGasPrice: '0x77359400',
    type: '0x0' // <- Type incorrectly switched
  }
}

If I add accessList: [] to the JS code snippet transaction (along with type: 1), we get the expected transaction sent and returned:

data {
  method: 'eth_sendTransaction',
  params: [
    {
      from: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      to: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      type: 1, // <- Correct type
      accessList: [],
      gasPrice: '0x77359400'
    }
  ],
  callback: undefined
}


data {
  method: 'eth_sendTransaction',
  params: [
    {
      from: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      to: '0x05c6ca529328008ceaef38f68b284f9ee56be6cd',
      type: 1, // <- Correct type
      accessList: [],
      gasPrice: '0x77359400'
    }
  ],
  callback: undefined
}

spacesailor24 avatar Jan 28 '22 08:01 spacesailor24

Just commenting to confirm that the bug here is in web3 and Ganache.

Web3 should convert numbers into QUANTITY encoded hex ("0x0", "0x1", "0x2", "0x10", etc). And Ganache should ignore invalid and unsupported types./

I'm unsure if Ganache should reject transactions with a type > 192 though, as harry suggests, since I believe we determined it should permit other arbitrary data in this field, like 10, 16, text strings, null, booleans, etc, as previously these were permitted in a Legacy Transaction (retroactively referred to as type "0x0"). Maybe geth should change to strict parsing here? More investigation is needed. I've opened https://github.com/trufflesuite/ganache/issues/2230 to track this in Ganache.

davidmurdoch avatar Jan 28 '22 15:01 davidmurdoch

OK, so much of this may actually be a Ganache problem. But it seems to me that web3 should still be handling the input formatting so that the 10 -> 16 conversion doesn't arise.

haltman-at avatar Jan 28 '22 17:01 haltman-at

@haltman-at @davidmurdoch

After confirming with the team, we're going to:

  1. Update web3.eth.sendTransaction to format type to a hex string before sending to provider
  2. Add accessList: [] to type: '0x1' transactions if it's undefined
  3. Throw an error if type is out of the bounds: 0x0 to 0x7f

spacesailor24 avatar Feb 01 '22 08:02 spacesailor24

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

github-actions[bot] avatar Apr 12 '22 00:04 github-actions[bot]

Bad bot 😁

davidmurdoch avatar Apr 12 '22 14:04 davidmurdoch

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

github-actions[bot] avatar Jun 12 '22 00:06 github-actions[bot]

Not stale!

haltman-at avatar Jun 12 '22 00:06 haltman-at

It might be worth noting that Arbitrum, or ArbOS (whatever that is), has an "Internal Transaction 106" transaction type that is in use. I couldn't find any documentation or source code about it, but didn't manage to find this: https://nitro-devnet-explorer.arbitrum.io/tx/0x583096902e1b7ca17fc6cfe649488fc6c09b8ce2791e2420090c09f517fc2b6c/internal-transactions

I'm not sure if this should change web3's behavior (I suspect that it should not).

davidmurdoch avatar Aug 03 '22 21:08 davidmurdoch

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

github-actions[bot] avatar Oct 03 '22 00:10 github-actions[bot]

Not stale.

haltman-at avatar Oct 03 '22 00:10 haltman-at

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

github-actions[bot] avatar Dec 04 '22 00:12 github-actions[bot]

Not stale.

haltman-at avatar Dec 04 '22 00:12 haltman-at

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

github-actions[bot] avatar Feb 03 '23 00:02 github-actions[bot]

Not stale.

haltman-at avatar Feb 03 '23 00:02 haltman-at