Skip to content
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.

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

Revert with proper panic if array index out of bounds #606

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions crates/test-files/fixtures/features/arrays.fe
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
contract Foo:
my_array: Array<u256, 10>

pub fn get_from_storage(self, index: u256) -> u256:
return self.my_array[index]

pub fn get_from_memory(index: u256) -> u256:
let my_array: Array<u256, 10> = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
return my_array[index]
4 changes: 4 additions & 0 deletions crates/test-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ pub fn encoded_over_or_underflow() -> Vec<u8> {
encode_revert("Panic(uint256)", &[uint_token(0x11)])
}

pub fn encoded_panic_out_of_bounds() -> Vec<u8> {
encode_revert("Panic(uint256)", &[uint_token(0x32)])
}

pub fn encoded_div_or_mod_by_zero() -> Vec<u8> {
encode_revert("Panic(uint256)", &[uint_token(0x12)])
}
Expand Down
31 changes: 31 additions & 0 deletions crates/tests/src/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,37 @@ fn test_assert() {
})
}

#[test]
fn test_arrays() {
with_executor(&|mut executor| {
let harness = deploy_contract(&mut executor, "arrays.fe", "Foo", &[]);

harness.test_function(
&mut executor,
"get_from_memory",
&[uint_token(9)],
Some(&uint_token(10)),
);

validate_revert(
harness.capture_call(&mut executor, "get_from_memory", &[uint_token(10)]),
&encoded_panic_out_of_bounds(),
);

harness.test_function(
&mut executor,
"get_from_storage",
&[uint_token(9)],
Some(&uint_token(0)),
);

validate_revert(
harness.capture_call(&mut executor, "get_from_storage", &[uint_token(10)]),
&encoded_panic_out_of_bounds(),
);
})
}

#[rstest(fixture_file, input, expected,
case("for_loop_with_static_array.fe", &[], uint_token(30)),
case("for_loop_with_static_array_from_sto.fe", &[], uint_token(6)),
Expand Down
1 change: 1 addition & 0 deletions crates/yulgen/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ pub fn numeric_min_max() -> HashMap<Integer, (yul::Expression, yul::Expression)>
pub const PANIC_FAILED_ASSERTION: usize = 0x01;
pub const PANIC_OVER_OR_UNDERFLOW: usize = 0x11;
pub const PANIC_DIV_OR_MOD_BY_ZERO: usize = 0x12;
pub const PANIC_OUT_OF_BOUNDS: usize = 0x32;

pub const ERROR_INSUFFICIENT_FUNDS_TO_SEND_VALUE: usize = 0x100;
pub const ERROR_FAILED_SEND_VALUE: usize = 0x101;
Expand Down
3 changes: 2 additions & 1 deletion crates/yulgen/src/operations/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,5 +139,6 @@ pub fn indexed_array(
index: yul::Expression,
) -> yul::Expression {
let inner_size = literal_expression! { (typ.inner.size()) };
expression! { add([array], (mul([index], [inner_size]))) }
let array_length = literal_expression! { (typ.size) };
expression! { get_array_item([array], [array_length], [index], [inner_size] ) }
}
17 changes: 17 additions & 0 deletions crates/yulgen/src/runtime/functions/data.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
use crate::constants::PANIC_OUT_OF_BOUNDS;
use crate::operations::revert as revert_operations;

use yultsur::*;

/// Return all data runtime functions
Expand All @@ -15,6 +18,7 @@ pub fn all() -> Vec<yul::Statement> {
ceil32(),
cloadn(),
free(),
get_array_item(),
load_data_string(),
map_value_ptr(),
mcopym(),
Expand Down Expand Up @@ -380,3 +384,16 @@ pub fn load_data_string() -> yul::Statement {
}
}
}

/// Returns a pointer to the array item at the requested index.
/// Reverts with a panic if the index is out of bounds.
pub fn get_array_item() -> yul::Statement {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize that this might eventually be handled at Fe level (std lib) but it is small enough to add now and is preventing me from adding further differential contract fuzzing tests as this comes up quite frequently when fuzzing with arrays involved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't hesitate to add yul utility functions; it'll be a while before everything can be fe-ified.

function_definition! {
function get_array_item(array_ptr, array_length, index, inner_size) -> ptr {
(if (iszero((lt(index, array_length)))) {
[revert_operations::panic_revert(PANIC_OUT_OF_BOUNDS)]
} )
(ptr := add(array_ptr, (mul(index, inner_size))))
}
}
}
3 changes: 3 additions & 0 deletions newsfragments/606.feature.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Added an out of bounds check for accessing array items.
If an array index is retrieved at an index that is not within
the bounds of the array it now reverts with `Panic(0x32)`.