Skip to content

Fix padding and gas cost computation issues in precompiles #961

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.

Sign up for GitHub

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

Merged
sorpaas merged 9 commits into from
Jan 16, 2023
Merged

Fix padding and gas cost computation issues in precompiles #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
57974fb
pad input if too short
nanocryk Jan 4, 2023
b9ca007
pad input in modexp
nanocryk Jan 6, 2023
4d2ac73
pad all input fields + fix wrong test
nanocryk Jan 6, 2023
fb5d3d9
fix modexp gas cost
nanocryk Jan 6, 2023
d57800b
remove dbg!
nanocryk Jan 6, 2023
922fda0
remove redundant arg + import vec!
nanocryk Jan 6, 2023
1ef1d6d
use copy_from_slice
nanocryk Jan 9, 2023
634812a
clippy
nanocryk Jan 9, 2023
b9703c3
clippy
nanocryk Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions frame/evm/precompile/bn128/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,37 +26,43 @@ use fp_evm::{
};
use sp_core::U256;

fn read_fr(input: &[u8], start_inx: usize) -> Result<bn::Fr, PrecompileFailure> {
if input.len() < start_inx + 32 {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("Input not long enough".into()),
});
/// Copy bytes from input to target.
fn read_input(source: &[u8], target: &mut [u8], offset: usize) {
// Out of bounds, nothing to copy.
if source.len() <= offset {
return;
}

bn::Fr::from_slice(&input[start_inx..(start_inx + 32)]).map_err(|_| PrecompileFailure::Error {
// Find len to copy up to target len, but not out of bounds.
let len = core::cmp::min(target.len(), source.len() - offset);
target[..len].copy_from_slice(&source[offset..][..len]);
}

fn read_fr(input: &[u8], start_inx: usize) -> Result<bn::Fr, PrecompileFailure> {
let mut buf = [0u8; 32];
read_input(input, &mut buf, start_inx);

bn::Fr::from_slice(&buf).map_err(|_| PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid field element".into()),
})
}

fn read_point(input: &[u8], start_inx: usize) -> Result<bn::G1, PrecompileFailure> {
use bn::{AffineG1, Fq, Group, G1};

if input.len() < start_inx + 64 {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("Input not long enough".into()),
});
}
let mut px_buf = [0u8; 32];
let mut py_buf = [0u8; 32];
read_input(input, &mut px_buf, start_inx);
read_input(input, &mut py_buf, start_inx + 32);

let px = Fq::from_slice(&input[start_inx..(start_inx + 32)]).map_err(|_| {
PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point x coordinate".into()),
}
let px = Fq::from_slice(&px_buf).map_err(|_| PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point x coordinate".into()),
})?;
let py = Fq::from_slice(&input[(start_inx + 32)..(start_inx + 64)]).map_err(|_| {
PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point y coordinate".into()),
}

let py = Fq::from_slice(&py_buf).map_err(|_| PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point y coordinate".into()),
})?;

Ok(if px == Fq::zero() && py == Fq::zero() {
G1::zero()
} else {
Expand Down
162 changes: 100 additions & 62 deletions frame/evm/precompile/modexp/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@

extern crate alloc;

use alloc::vec::Vec;
use core::{cmp::max, ops::BitAnd};
use alloc::{vec, vec::Vec};
use core::cmp::max;

use num::{BigUint, FromPrimitive, One, ToPrimitive, Zero};

Expand All @@ -38,9 +38,9 @@ const MIN_GAS_COST: u64 = 200;
// https://eips.ethereum.org/EIPS/eip-2565
fn calculate_gas_cost(
base_length: u64,
exp_length: u64,
mod_length: u64,
exponent: &BigUint,
exponent_bytes: &[u8],
) -> u64 {
fn calculate_multiplication_complexity(base_length: u64, mod_length: u64) -> u64 {
let max_length = max(base_length, mod_length);
Expand All @@ -56,18 +56,15 @@ fn calculate_gas_cost(
words * words
}

fn calculate_iteration_count(exp_length: u64, exponent: &BigUint) -> u64 {
fn calculate_iteration_count(exponent: &BigUint, exponent_bytes: &[u8]) -> u64 {
let mut iteration_count: u64 = 0;
let exp_length = exponent_bytes.len() as u64;

if exp_length <= 32 && exponent.is_zero() {
iteration_count = 0;
} else if exp_length <= 32 {
iteration_count = exponent.bits() - 1;
} else if exp_length > 32 {
// construct BigUint to represent (2^256) - 1
let bytes: [u8; 32] = [0xFF; 32];
let max_256_bit_uint = BigUint::from_bytes_be(&bytes);

// from the EIP spec:
// (8 * (exp_length - 32)) + ((exponent & (2**256 - 1)).bit_length() - 1)
//
Expand All @@ -77,21 +74,42 @@ fn calculate_gas_cost(
// must be > 0)
// * the addition can't overflow because the terms are both capped at roughly
// 8 * max size of exp_length (1024)
iteration_count =
(8 * (exp_length - 32)) + exponent.bitand(max_256_bit_uint).bits() - 1;
// * the EIP spec is written in python, in which (exponent & (2**256 - 1)) takes the
// FIRST 32 bytes. However this `BigUint` `&` operator takes the LAST 32 bytes.
// We thus instead take the bytes manually.
Comment thread
nanocryk marked this conversation as resolved.
let exponent_head = BigUint::from_bytes_be(&exponent_bytes[..32]);

iteration_count = (8 * (exp_length - 32)) + exponent_head.bits() - 1;
}

max(iteration_count, 1)
}

let multiplication_complexity = calculate_multiplication_complexity(base_length, mod_length);
let iteration_count = calculate_iteration_count(exp_length, exponent);
let iteration_count = calculate_iteration_count(exponent, exponent_bytes);
max(
MIN_GAS_COST,
multiplication_complexity * iteration_count / 3,
)
}

/// Copy bytes from input to target.
fn read_input(source: &[u8], target: &mut [u8], source_offset: &mut usize) {
// We move the offset by the len of the target, regardless of what we
// actually copy.
let offset = *source_offset;
*source_offset += target.len();

// Out of bounds, nothing to copy.
if source.len() <= offset {
return;
}

// Find len to copy up to target len, but not out of bounds.
let len = core::cmp::min(target.len(), source.len() - offset);
target[..len].copy_from_slice(&source[offset..][..len]);
}

// ModExp expects the following as inputs:
// 1) 32 bytes expressing the length of base
// 2) 32 bytes expressing the length of exponent
Expand All @@ -111,35 +129,35 @@ fn calculate_gas_cost(
impl Precompile for Modexp {
fn execute(handle: &mut impl PrecompileHandle) -> PrecompileResult {
let input = handle.input();
let mut input_offset = 0;

if input.len() < 96 {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("input must contain at least 96 bytes".into()),
});
};
// Yellowpaper: whenever the input is too short, the missing bytes are
// considered to be zero.
let mut base_len_buf = [0u8; 32];
read_input(input, &mut base_len_buf, &mut input_offset);
let mut exp_len_buf = [0u8; 32];
read_input(input, &mut exp_len_buf, &mut input_offset);
let mut mod_len_buf = [0u8; 32];
read_input(input, &mut mod_len_buf, &mut input_offset);

// reasonable assumption: this must fit within the Ethereum EVM's max stack size
let max_size_big = BigUint::from_u32(1024).expect("can't create BigUint");

let mut buf = [0; 32];
buf.copy_from_slice(&input[0..32]);
let base_len_big = BigUint::from_bytes_be(&buf);
let base_len_big = BigUint::from_bytes_be(&base_len_buf);
if base_len_big > max_size_big {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("unreasonably large base length".into()),
});
}

buf.copy_from_slice(&input[32..64]);
let exp_len_big = BigUint::from_bytes_be(&buf);
let exp_len_big = BigUint::from_bytes_be(&exp_len_buf);
if exp_len_big > max_size_big {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("unreasonably large exponent length".into()),
});
}

buf.copy_from_slice(&input[64..96]);
let mod_len_big = BigUint::from_bytes_be(&buf);
let mod_len_big = BigUint::from_bytes_be(&mod_len_buf);
if mod_len_big > max_size_big {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("unreasonably large modulus length".into()),
Expand All @@ -151,11 +169,11 @@ impl Precompile for Modexp {
let exp_len = exp_len_big.to_usize().expect("exp_len out of bounds");
let mod_len = mod_len_big.to_usize().expect("mod_len out of bounds");

// input length should be at least 96 + user-specified length of base + exp + mod
let total_len = base_len + exp_len + mod_len + 96;
if input.len() < total_len {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("insufficient input size".into()),
// if mod_len is 0 output must be empty
if mod_len == 0 {
return Ok(PrecompileOutput {
exit_status: ExitSucceed::Returned,
output: vec![],
Copy link
Copy Markdown
Member

@sorpaas sorpaas Jan 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually right? EIP-198 states:

returns an output (BASE**EXPONENT) % MODULUS as a byte array with the same length as the modulus.

So if I'm to understand the spec, this should be a number of zeros, instead of an empty array.

Also is this special case actually needed? L166 already has:

if base_len == 0 && mod_len == 0 {

Copy link
Copy Markdown
Contributor Author

@nanocryk nanocryk Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base len is encoded before mod len, so a truncated input could have non-zero base len but zero mod len.

As the EIP states that it returns a byte array with the same length as the modulus, then if mod len is 0 the output should be zero len too.

I'm not too familiar with Go but this seems to return zero len array:
https://github.com/ethereum/go-ethereum/blob/v1.10.26/core/vm/contracts.go#L386

});
}

Expand All @@ -165,21 +183,22 @@ impl Precompile for Modexp {
BigUint::zero()
} else {
// read the numbers themselves.
let base_start = 96; // previous 3 32-byte fields
let base = BigUint::from_bytes_be(&input[base_start..base_start + base_len]);
let mut base_buf = vec![0u8; base_len];
read_input(input, &mut base_buf, &mut input_offset);
let base = BigUint::from_bytes_be(&base_buf);

let exp_start = base_start + base_len;
let exponent = BigUint::from_bytes_be(&input[exp_start..exp_start + exp_len]);
let mut exp_buf = vec![0u8; exp_len];
read_input(input, &mut exp_buf, &mut input_offset);
let exponent = BigUint::from_bytes_be(&exp_buf);

let mut mod_buf = vec![0u8; mod_len];
read_input(input, &mut mod_buf, &mut input_offset);
let modulus = BigUint::from_bytes_be(&mod_buf);

// do our gas accounting
let gas_cost =
calculate_gas_cost(base_len as u64, exp_len as u64, mod_len as u64, &exponent);
let gas_cost = calculate_gas_cost(base_len as u64, mod_len as u64, &exponent, &exp_buf);

handle.record_cost(gas_cost)?;
let input = handle.input();

let mod_start = exp_start + exp_len;
let modulus = BigUint::from_bytes_be(&input[mod_start..mod_start + mod_len]);

if modulus.is_zero() || modulus.is_one() {
BigUint::zero()
Expand Down Expand Up @@ -228,7 +247,7 @@ mod tests {
}

#[test]
fn test_empty_input() -> Result<(), PrecompileFailure> {
fn test_empty_input() {
let input = Vec::new();

let cost: u64 = 1;
Expand All @@ -242,25 +261,17 @@ mod tests {
let mut handle = MockHandle::new(input, Some(cost), context);

match Modexp::execute(&mut handle) {
Ok(_) => {
panic!("Test not expected to pass");
Ok(precompile_result) => {
assert_eq!(precompile_result.output.len(), 0);
}
Err(e) => {
assert_eq!(
e,
PrecompileFailure::Error {
exit_status: ExitError::Other(
"input must contain at least 96 bytes".into()
)
}
);
Ok(())
Err(_) => {
panic!("Modexp::execute() returned error"); // TODO: how to pass error on?
}
}
}

#[test]
fn test_insufficient_input() -> Result<(), PrecompileFailure> {
fn test_insufficient_input() {
let input = hex::decode(
"0000000000000000000000000000000000000000000000000000000000000001\
0000000000000000000000000000000000000000000000000000000000000001\
Expand All @@ -279,17 +290,12 @@ mod tests {
let mut handle = MockHandle::new(input, Some(cost), context);

match Modexp::execute(&mut handle) {
Ok(_) => {
panic!("Test not expected to pass");
Ok(precompile_result) => {
assert_eq!(precompile_result.output.len(), 1);
assert_eq!(precompile_result.output, vec![0x00]);
}
Err(e) => {
assert_eq!(
e,
PrecompileFailure::Error {
exit_status: ExitError::Other("insufficient input size".into())
}
);
Ok(())
Err(_) => {
panic!("Modexp::execute() returned error"); // TODO: how to pass error on?
}
}
}
Expand Down Expand Up @@ -474,4 +480,36 @@ mod tests {
let expected = BigUint::parse_bytes(b"0", 10).unwrap();
assert_eq!(result, expected);
}

#[test]
fn test_long_exp_gas_cost_matches_specs() {
let input = vec![
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 38, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 96, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
16, 0, 0, 0, 255, 255, 255, 2, 0, 0, 179, 0, 0, 2, 0, 0, 122, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 255, 251, 0, 0, 0, 0, 4, 38, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 96, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 16, 0, 0, 0, 255, 255, 255, 2, 0, 0, 179, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255,
255, 255, 255, 249,
];

let context: Context = Context {
address: Default::default(),
caller: Default::default(),
apparent_value: From::from(0),
};

let mut handle = MockHandle::new(input, Some(100_000), context);

let _ = Modexp::execute(&mut handle).expect("Modexp::execute() returned error");

assert_eq!(handle.gas_used, 7104); // gas used when ran in geth
}
}