Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task done
haltman-at opened this issue Dec 3, 2021 · 18 comments · Fixed by #5979
Closed
1 task done
Assignees
Labels
1.x 1.0 related issues Bug Addressing a bug work-in-progress Prevents stalebot from closing a pr

Comments

@haltman-at
Copy link
Contributor

haltman-at commented Dec 3, 2021

Is there an existing issue for this?

  • 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.

@nazarhussain
Copy link
Contributor

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

@spacesailor24
Copy link
Contributor

spacesailor24 commented Jan 27, 2022

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
Copy link
Contributor

spacesailor24 commented Jan 28, 2022

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
}

@davidmurdoch
Copy link

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 trufflesuite/ganache#2230 to track this in Ganache.

@haltman-at
Copy link
Contributor Author

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.

@spacesailor24
Copy link
Contributor

@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 spacesailor24 removed their assignment Feb 10, 2022
@github-actions
Copy link

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 github-actions bot added the Stale Has not received enough activity label Apr 12, 2022
@davidmurdoch
Copy link

Bad bot 😁

@github-actions github-actions bot removed the Stale Has not received enough activity label Apr 13, 2022
@github-actions
Copy link

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 github-actions bot added the Stale Has not received enough activity label Jun 12, 2022
@haltman-at
Copy link
Contributor Author

Not stale!

@github-actions github-actions bot removed the Stale Has not received enough activity label Jun 13, 2022
@davidmurdoch
Copy link

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).

@github-actions
Copy link

github-actions bot commented Oct 3, 2022

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 github-actions bot added the Stale Has not received enough activity label Oct 3, 2022
@haltman-at
Copy link
Contributor Author

Not stale.

@github-actions github-actions bot removed the Stale Has not received enough activity label Oct 4, 2022
@github-actions
Copy link

github-actions bot commented Dec 4, 2022

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 github-actions bot added the Stale Has not received enough activity label Dec 4, 2022
@haltman-at
Copy link
Contributor Author

Not stale.

@github-actions github-actions bot removed the Stale Has not received enough activity label Dec 5, 2022
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

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 github-actions bot added the Stale Has not received enough activity label Feb 3, 2023
@haltman-at
Copy link
Contributor Author

Not stale.

@github-actions github-actions bot removed the Stale Has not received enough activity label Feb 4, 2023
@spacesailor24 spacesailor24 added work-in-progress Prevents stalebot from closing a pr and removed Investigate labels Mar 7, 2023
@spacesailor24 spacesailor24 self-assigned this Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Bug Addressing a bug work-in-progress Prevents stalebot from closing a pr
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@nazarhussain @davidmurdoch @spacesailor24 @haltman-at and others