Fixed potential modexp exp len overflow #6686
Conversation
arkpar
added
A0-pleasereview 🤓
arkpar
added
F0-consensus 💣
|
wouldn't the right approach to simply use |
ethcore/src/builtin.rs
Outdated
| let mod_len = mod_len.low_u64(); | ||
| let m = max(mod_len, base_len); | ||
| if m == 0 { | ||
| return U256::zero(); | ||
| } | ||
| if exp_len > max_len { |
There was a problem hiding this comment.
The conditions look a bit messy now. What about this:
// max(mod_len, base_len) == 0
if mod_len.is_zero() && base_len.is_zero() {
return U256::zero()
}
let max_len = U256::from(u32::max_value() / 2);
if base_len > max_len || mod_len > max_len || exp_len > max_len {
return U256::max_value();
}
let (base_len, exp_len, mod_len) = (base_len.low_u64(), exp_len.low_u64(), mod_len.low_u64());
...
| @@ -133,8 +132,7 @@ impl ModexpPricer { | |||
| let bit_index = if exp_low.is_zero() { 0 } else { (255 - exp_low.leading_zeros()) as u64 }; | |||
| if len <= 32 { | |||
| bit_index | |||
| } | |||
| else { | |||
| } else { | |||
There was a problem hiding this comment.
👍 Was hurting my eyes a bit :)
is that a hard bound specified by the EIP? otherwise, why introduce the arbitrary (and technically wrong) restriction, when really gas should be the only limiting factor? Doing computations on numbers a few gigabytes long is within reach of modern computers even if impractical |
|
This PR does not introduce any additional restrictions. Restriction for 32-bit addressable memory is already in the evm implementation. There are quite a few places where we choose more efficient gas calculation in plain integers. This is one of them. Changing that is out of scope of this PR |
| @@ -101,18 +101,17 @@ impl Pricer for ModexpPricer { | |||
| let exp_len = read_len(); | |||
There was a problem hiding this comment.
It won't let me add comments on the actual line, but on line 92 above here: Why do we zero-extend the input and read_exact instead of just using read? The buffer's already filled with zeroes.
| let m = max(mod_len, base_len); | ||
| if m == 0 { | ||
| return U256::zero(); | ||
| } | ||
| // read fist 32-byte word of the exponent. |
rphmeier
added
A8-looksgood 🦄
| let max_len = U256::from(u32::max_value() / 2); | ||
| if base_len > max_len || mod_len > max_len { | ||
| if base_len > max_len || mod_len > max_len || exp_len > max_len { |
There was a problem hiding this comment.
does this work properly with base = 1, mod = 1, exp = 2^256-1?
There was a problem hiding this comment.
Yes, according to the spec it should fail because gas calculation formula would return a huge value.
No description provided.