Signing data returns wrong signature

[sc0Vu]

Hi,

Thanks for this great library. Recently I try to sign transaction, it returned signature with wrong length (r: 31 bytes, s: 32 bytes). After debug deeply, found out that if the first bytes is 0, the library will remove.

Data:

// signature from ecsign, length of r is 32
{ r: <Buffer 00 18 16 2b ...... 45 a8 e8>,
  s: <Buffer 0d 25 7b ...... 89 f8 a0>,
  v: 27 }

// signature from ethereumjs-tx, length of r is 31
{ r: <Buffer 18 16 2b ......  45 a8 e8>,
  s: <Buffer 0d 25 7b ...... 89 f8 a0>,
  v: 27 }

Do you have any idea about this?
Thanks!

UPDATE by @alcuadrado: These two signatures are valid. They are equivalent when RLP-encoded, and that's how the Ethereum network sees them. This is currently (2019-05-10) a known limitation of this library, it trims all leading zeros of r and s.

WORKAROUND: You can transform r and s into big ints before comparing them.

[sc0Vu]

Found out this: https://github.com/ethereumjs/ethereumjs-util/blob/master/src/object.ts#L50

[shahafn]

Why is this issue closed? we still get this bug on version 1.3.7
tx hash: 8f610cb46763e02cab5e1f7d0ac5c85a6f3c320f9b50ac66cb2e8effd096092c
private key: cf5de3123d7ee4e0c66761793f1cc258324ecdf677fe3422e4cd0d87b9132322
wrong sig:
1c
29c71cb3d3c9c85e15c03f396e8a9da5fb521f6e1d738cb2c321456f86314576
45a32cfcfe114025720ad8d9c20b6d89d37ac51b9220148930fadd2e5393c9
correct sig:
1c
29c71cb3d3c9c85e15c03f396e8a9da5fb521f6e1d738cb2c321456f86314576
0045a32cfcfe114025720ad8d9c20b6d89d37ac51b9220148930fadd2e5393c9

[alcuadrado]

@shahafn can you provide all of the transaction's values?

[shahafn]

I don't have this particular transaction, but it is still the case that the above tx hash & private key produce this signature. Isn't it possible to inject this tx hash to your signature algorithm to reproduce it?

[alcuadrado]

Do you know how you got that hash at least? Was it a tx id?

[forshtat]

Do you know how you got that hash at least? Was it a tx id?

That is actually pretty easy to get a transaction whose signature will have 'r' or 's' shorter than 32 bytes. I ran this code in a truffle console and it took seconds:

var t = require("ethereumjs-tx")
var f = function(nonce) { 
  var b = Buffer.from("4f3edf983ac636a65a842ce7c78d9aa706d3b113bce9c46f30d7d21715b23b1d", "hex");
  var tx = new t({nonce: nonce});
  tx.sign(b);
  return tx;
}
var res; var i =1; 
do {
  res = f(i++); 
} while (res.r.length === 32)

This meant to me that I would have to manually pad the values with zero bytes as all other tools require 32-byte size for a signature field, so I switched to a different library.

So the question is whether there is a rationale behind this or is it an actual bug? There is even a test for it, 'should accept lesser r values', which seems completely ill-advised to me, as I would expect the result of tx.r = toBuffer('0x0005') tx.r.toString('hex')
to be equal "0000000000000000000000000000000000000000000000000000000000000005"

[alcuadrado]

Thanks @alex-forshtat-tbk! I wasn't aware that this was so common and easy to reproduce.

I'll reopen this issue and investigate it later.

[drewstone]

Can confirm that I have Invalid Signature errors though they're not as informative as telling me what exactly is the issue. With certain functions in my contract, signing works, and with others it does not work.

[sc0Vu]

@alcuadrado I close it because there is an option called allowLess. If allowLess is true, it will remove all the lead zero bytes.

For example:

<Buffer 00 00 00 18 16 2b ...... 45 a8 e8>, // allowLess false
<Buffer 18 16 2b ...... 45 a8 e8>, // allowLess true

If you check the fields options, only data and to is false.

const fields = [
      {
        name: 'nonce',
        length: 32,
        allowLess: true,
        default: new Buffer([]),
      },
      {
        name: 'gasPrice',
        length: 32,
        allowLess: true,
        default: new Buffer([]),
      },
      {
        name: 'gasLimit',
        alias: 'gas',
        length: 32,
        allowLess: true,
        default: new Buffer([]),
      },
      {
        name: 'to',
        allowZero: true,
        length: 20,
        default: new Buffer([]),
      },
      {
        name: 'value',
        length: 32,
        allowLess: true,
        default: new Buffer([]),
      },
      {
        name: 'data',
        alias: 'input',
        allowZero: true,
        default: new Buffer([]),
      },
      {
        name: 'v',
        allowZero: true,
        default: new Buffer([opts.chain || opts.common ? this._common.chainId() : 0x1c]),
      },
      {
        name: 'r',
        length: 32,
        allowZero: true,
        allowLess: true,
        default: new Buffer([]),
      },
      {
        name: 's',
        length: 32,
        allowZero: true,
        allowLess: true,
        default: new Buffer([]),
      },
    ]

Seems like we have to check the length by ourselves.

I'd made some change in cloned repo: sc0Vu/ethereumjs-tx@9dce2ab

[alcuadrado]

@sc0Vu this is related with allowLess, removing it will break this library. Many signatures will be invalid.

RLP expects numbers to be big-endian without leading zeros, that's why they are removed. This is intended behavior, at least for now.

IMO the internal representation is too coupled with the RLP serialization and leads to confusing situations like this. But changing this would be a major task.

@s1na have you encountered something similar in other libraries?

[s1na]

@alcuadrado Hm, not sure I had faced this exact issue before.

IMO the internal representation is too coupled with the RLP serialization and leads to confusing situations like this. But changing this would be a major task.

Fully agree with this. And yeah I can imagine de-coupling them would be a bigger task than this issue, but I think we should pursue it nevertheless.

Currently the sign method modifies the internal state. What if additionally we also return a signature object (which doesn't strip preceding zeros)?

[alcuadrado]

@alcuadrado Hm, not sure I had faced this exact issue before.

IMO the internal representation is too coupled with the RLP serialization and leads to confusing situations like this. But changing this would be a major task.

Fully agree with this. And yeah I can imagine de-coupling them would be a bigger task than this issue, but I think we should pursue it nevertheless.

Currently the sign method modifies the internal state. What if additionally we also return a signature object (which doesn't strip preceding zeros)?

I saw that you opened #709, let's move this discussion there.

@sc0Vu I updated your first message with an explanation about this. I'm closing this again.

If someone encounters the same problem, please see my note in the first message of this issue.

[SurfingNerd]

Unfortunately there is still a issue with R & S values with leading zeros. i have created a detailed example that makes it easy to understand: https://gist.github.com/SurfingNerd/2be70efd789912b5a9a51d662c38ed40

[forshtat]

I couldn't disagree more with closing this issue and opening another, with a way larger scope and vaguely referencing this issue in it's description. This is a bug in a library, and if it is still there => this issue should remain open.

[holgerd77]

Will reopen.

[alcuadrado]

I created https://github.com/ethereumjs/ethereumjs-tx/issues/164 that documents this behavior.

[ryanio]

This should now be resolved in #812 by storing R & S as BN.