Should verifySignature really verify signature?

[fanatid]

Right now method verifySignature just restore public key from signature, but does not really verify that restored public key corresponds to private key which was used for signing transaction. Should we restore public key and then verify signature with it right after?

[fanatid]

ping @holgerd77

[holgerd77]

Hi Kirill, thanks for your PR on this, just for some context: I am not so super-deep into this library so it always takes me some time to dig into stuff like this and understand the implications.

If someone else also want to comment on this or give an assessment, that would also help.

[danjm]

@fanatid Can you clarify: do you mean doing verification beyond what ethUtil.ecrecover does? See: https://github.com/ethereumjs/ethereumjs-util/blob/master/index.js#L383-L391

[fanatid]

ethUtil.ecrecover does not do any verification, it's only trying recover public key.
What I thought, should we check recovered public key against signature in transaction and msg hash? Because this will reflect method name verifySignature.

[danjm]

Sorry for the slow delay. I've just finished making some improvements to the code, and should be able to resolve this quickly now.

What I thought, should we check recovered public key against signature in transaction and msg hash?

Okay, I think I see what you are saying, I need a little more clarification.

Right now, verifySignature passes the signature values (v, r, and s) and the msg hash to ecrecover to retrieve the public key. Are you proposing that we should then use the newly retrived public key with the signature to generate a new hash, and then check to make sure that hash is the same as the one used to to recover the public key?

And is your open PR on this issue meant to be a failing test that another change will fix?

[fanatid]

Yes, I propose that after public key recovering library should verify signature against this key.

[ryanio]

I have been looking into this.

From my understanding when constructing a Transaction object this lib does not accept a from to store the sender address but instead computes it from the signature in verifySignature() and assigns it to this._senderPubKey.

I understand why OP is confused because he is in fact right that the method does not verify the signature against a separately passed from address, but just recovers it. It does verify against a completely invalid signature though such as:

https://github.com/ethereumjs/ethereumjs-tx/blob/5c81b38b366c8548f9601d8c60a29f0af394db63/test/api.ts#L81-L87

The test case proposed in ethereumjs/ethereumjs-tx#105 actually works as intended (other than the name being confusing) as modifying the r just just changes the signature to derive from a different sender address.

A better case to test, that passes, is that modifying the signature changes the intended sender address:

  t.test(
    "should return a different sender address of a modified signature",
    function(st) {
      var tx = new Transaction(transactions[0].serialize());
      tx.r[0] += 1;
      tx.verifySignature();
      st.notEqual(transactions[0].sendersAddress, tx.getSenderPublicKey());
      st.end();
    }
  );

At this point it looks this like this issue and ethereumjs/ethereumjs-tx#105 can be closed.

[holgerd77]

@ryanio eventually you can update the description of the test mentioned above with something more fitting along your work on #812, will otherwise follow the recommendation and close here.