bug: codecopy fails on high offset value

[greged93]

Bug Report

Kakarot version: 32b516e

Current behavior:
The CODECOPY opcode will fail if provided with a high value for the offset argument. However, specs state that if offset is > bytecode length, 0s should simply be copied in the memory.

Expected behavior:
Testing with a high value for the bytecode offset should pass with memory being filled with 0s.

Steps to reproduce:

Add the below test to the suite of tests and run scarb test:

#[test]
#[available_gas(20000000)]
fn test_codecopy_error() {
    test_codecopy(0, 0xFFFFFFFD, 2)
}

Test should fail with

evm::tests::test_instructions::test_environment_information::test_codecopy_error - panicked with [155785504329508738615720351733824384887 ('u32_sub Overflow'), ].

[lambda-0x]

@greged93 can you link which part of the spec says that?

EDIT: spec is kinda hard to read, but evm.codes confirms that it should copy 0's

[lambda-0x]

panic is happening inside this branch, will open a PR with fix

kakarot-ssj/crates/evm/src/instructions/environmental_information.cairo

Lines 91 to 93 in 32b516e

	let slice_size = if (offset + size > bytecode.len()) {
	bytecode.len() - offset
	} else {

[greged93]

I think you need to be careful about offset + size as well. This is the way geth and reth handle this.

[lambda-0x]

good point, also i am not sure why we are converting all offsets, and size variable inside implementation to u32 does spec says anything about what can be the maximum value of those variables?

geth and reth also are converting them to u64 and usize (which most probably is u64)

CC: @khaeljy @enitrat

[khaeljy]

Nice one!
I did test if offset + size > bytecode.len(), but I forgot to check if offset > bytecode.len()!

I use a u32 because bytecode is a Span, and Span is indexed in u32 in cairo (unless I'm mistaken).
This raises a broader question: wouldn't it be necessary to review the management of memory, call_data and bytecode to allow u64 indexing?

[lambda-0x]

right, i came to same conclusion (#234 (comment)) when trying to test cases where offset + size > u32::MAX but due to indexing issue wasn't able to do it.

Is it possible to implement IndexView trait on Array and Span for u64 ourself?

[enitrat]

Is it possible to implement IndexView trait on Array and Span for u64 outself?

I don't believe it is, at least I've never seen it

This raises a broader question: wouldn't it be necessary to review the management of memory, call_data and bytecode to allow u64 indexing?

A:

The MSTORE opcode stores 256 bits to memory and costs 3 units of gas and the gas limit of a transaction is 30 million, so if you put a maximum-sized transaction through you could store ~2.5 gigabits to memory. That would be pointless, though, since memory is volatile and that would all just poof away when the transaction was done and there's no room in that transaction for other operations to actually do anything with that data.

2.5 gigabits is 3.125e+8 bytes, which is way less than whatever a usize allows us to store, so I'm going to keep the memory usize-indexed

offset + size > u32::MAX

in practice that would never happen because the gas costs for this would be insanely high and beyond tx gas limits

[enitrat]

I think you need to be careful about offset + size as well. This is the way geth and reth handle this.

I raised this in #203 (comment) aswell

[greged93]

Offset is offset in bytecode no? So you could have high offset in bytecode (just because, doesn't cost anything) and low length. Then offset + size overflows

Concerning your comment 203, it concerns returndatacopy, but imo codecopy should not be able to fail in the same manner

[enitrat]

imo codecopy should not be able to fail in the same manner

whh not? In both cases values from the stack can eventually be too high 🤔

[greged93]

Well specs and evm codes dont mention any reverts on high values, too high of a code offset should be handled gracefully as here