Update ethereumjs-util to 5.1.3 #241
Conversation
lib/precompiled/01-ecrecover.js
Outdated
| @@ -27,7 +27,7 @@ module.exports = function (opts) { | |||
|
|
|||
| var publicKey | |||
| try { | |||
| publicKey = utils.ecrecover(msgHash, v, r, s) | |||
| publicKey = utils.ecrecover(msgHash, utils.bufferToInt(v), r, s) | |||
There was a problem hiding this comment.
This is silently truncating the input to 53 bits. Since v must be either 0 or 1, it should be validated prior (without truncation).
I suggest something along the lines of:
v = new BN(v)
if (!v.eqn(0) && !v.eqn(1))
// fail
and then within the ecrecover call it can use v.toNumber()
There was a problem hiding this comment.
Thanks @axic.
This is silently truncating the input to 53 bits.
Afaik the validation happens in utils.bufferToInt. It throws for numbers larger than 53 bits, as it calls BN().toNumber and this call throws if that's the case.
Since v must be either 0 or 1, it should be validated prior
Should some extra validation be added to make sure it's 0 or 1? In case this validation fails, should the function just throw?
There was a problem hiding this comment.
Should some extra validation be added to make sure it's 0 or 1? In case this validation fails, should the function just throw?
There's no other way to fail there unfortunately.
There was a problem hiding this comment.
Since v must be either 0 or 1
Sorry, this actually wants 27 or 28 and ecrecover() itself will verify the range. I guess new BN(v).toNumber() could be a good compromise if we rely on ethereumjs-util to do the validation.
There was a problem hiding this comment.
cool, I hadn't seen ecrecover validated the input too.
I understand then that no further validation is needed here, as BN checks for the bit length and ethereumjs-util for the actual two-value range?
There was a problem hiding this comment.
Please use BN(v).toNumber() here and not utils.bufferToInt(v)
lib/opFns.js
Outdated
| @@ -914,7 +914,7 @@ function memLoad (runState, offset, length) { | |||
|
|
|||
| // shortcut | |||
| if (length.isZero()) { | |||
| return new Buffer('') | |||
| return Buffer.from('') | |||
There was a problem hiding this comment.
This could be replaced with Buffer.alloc(0).
lib/opFns.js
Outdated
| @@ -1023,7 +1023,7 @@ function makeCall (runState, callOptions, localOpts, cb) { | |||
| callOptions.depth = runState.depth + 1 | |||
|
|
|||
| // empty the return data buffer | |||
| runState.lastReturned = new Buffer([]) | |||
| runState.lastReturned = Buffer.from([]) | |||
There was a problem hiding this comment.
This could be replaced with Buffer.alloc(0).
|
Also this line could be replaced with |
|
@axic I applied the requested changes, let me know if there's anything else that needs to be updated, thanks! |
|
@jchavarri thanks! Can you please apply #241 (comment), update to the latest ethereumjs-util release (there was one meanwhile) and use rebase instead of merging? |
jchavarri
force-pushed
the
update-ethereumjs-util-v5
branch
from
5d6e21d
to
033d05f
Compare
|
@axic done! One question, just curious: why using |
jchavarri
force-pushed
the
update-ethereumjs-util-v5
branch
from
033d05f
to
4becb15
Compare
|
|
axic
force-pushed
the
update-ethereumjs-util-v5
branch
from
4becb15
to
f1a8db6
Compare
Enforce hex prefixing for address strings
Fixes #156.
Still wrapping my head around the code, so any suggestions are welcomed. I just made sure the state tests were passing.