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

recoverTransaction fails if r or s value start with zero byte omitted in serialized tx #2902

Closed
d10r opened this issue Jun 18, 2019 · 2 comments

Comments

@d10r
Copy link

d10r commented Jun 18, 2019

Starting with web3 1.0.0-beta.38, web3.eth.accounts.recoverTransaction() fails if the r or s component of the signature start with a leading zero which is not included in the RLP encoded value.
This can be demonstrated with this example snippet from the web3 documentation: https://web3js.readthedocs.io/en/1.0/web3-eth.html#id73
Just put a nonce of 0x2c (instead of 0x00). This will lead to an s component with leading zero.

Tx = require('ethereumjs-tx');
privateKey = new Buffer('e331b6d69882b4cb4ea581d88e0b604039a3de5967688d3dcffdd2270c0fd109', 'hex');
rawTx = { nonce: '0x2c', gasPrice: '0x09184e72a000', gasLimit: '0x2710', to: '0x0000000000000000000000000000000000000000', value: '0x00', data: '0x7f7465737432000000000000000000000000000000000000000000000000000000600057' }
tx = new Tx(rawTx);
tx.sign(privateKey);
serializedTx = tx.serialize();

then this:

> web3.eth.accounts.recoverTransaction("0x" + serializedTx.toString("hex"));

Error: The recovery param is more than two bits
    at assert (/home/didi/src/artis/artis-misc/fancy-cashier/node_modules/minimalistic-assert/index.js:5:11)
    at EC.recoverPubKey (/home/didi/src/artis/artis-misc/fancy-cashier/node_modules/elliptic/lib/elliptic/ec/index.js:194:3)
    at Object.recover (/home/didi/src/artis/artis-misc/fancy-cashier/node_modules/eth-lib/lib/account.js:48:33)
    at Proxy.recoverTransaction (/home/didi/src/artis/artis-misc/fancy-cashier/node_modules/web3-eth-accounts/dist/web3-eth-accounts.cjs.js:440:28)

With web3 1.0.0-37, this doesn't happen.
Also, sendSignedTransaction() accepts the transaction. (However it will fail anyway - even with the sender account funded - because the gas limit is set to 10k. That should probably be changed in the documentation.)

I'm not sure if the spec allows RLP encoded transactions to omit such leading zeroes or not.
#1289 suggests that may be the case.
Others seem to have stumbled over it too, see https://github.com/ethereumjs/ethereumjs-tx/issues/140, but they seem to have concluded that stripping leading zeroes is allowed, thus I'm reporting here.

@nivida
Copy link
Contributor

nivida commented Jun 20, 2019

Thanks for opening this issue! I will test and fix the recovery method of the accounts module during the fix of #2777. Because this is a duplication of the issue I've referenced do I close it for now.

@nivida nivida closed this as completed Jun 20, 2019
@d10r
Copy link
Author

d10r commented Jul 16, 2019

@nvidi any chance that the fix would then be backported to the stable 1.0 release?
#2777 describes an issue which is quite a corner case (chainId > 0x3fffffff) while the issue described here happens on all chains and without doing anything non-standard, as can be seen in the example snippets. It would thus be desirable to have it fixed in a stable release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants