new_callstack_entry in main_vm/opcodes/call_ret_impl/ret.rs does not set heap_upper_bound and aux_heap_upper_bound in the callstack

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/main_vm/opcodes/call_ret_impl/far_call.rs#L719 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/main_vm/opcodes/call_ret_impl/far_call.rs#L680 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/main_vm/opcodes/call_ret_impl/ret.rs#L296 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/era-zkevm_circuits/src/main_vm/opcodes/call_ret_impl/ret.rs#L253

Vulnerability details

Impact

new_callstack_entry in main_vm/opcodes/ret.rs does not set heap_upper_bound and aux_heap_upper_bound

Proof of Concept

new_callstack_entry in main_vm/opcodes/ret.rs does not set heap_upper_bound and aux_heap_upper_bound

if we take a look at the far_call.rs called, the code properly manage the heap_upper_bound and aux_heap_upper_bound

// we need a completely fresh one
let mut new_callstack_entry = ExecutionContextRecord::uninitialized(cs);
// apply memory stipends right away
new_callstack_entry.heap_upper_bound = UInt32::allocated_constant(
    cs,
    zkevm_opcode_defs::system_params::NEW_FRAME_MEMORY_STIPEND,
);
new_callstack_entry.aux_heap_upper_bound = UInt32::allocated_constant(
    cs,
    zkevm_opcode_defs::system_params::NEW_FRAME_MEMORY_STIPEND,
);

the relevant code is here

such heap growth memory is carefully managed

    let heap_max_accessed = upper_bound.mask(cs, forwarding_data.use_heap);
    let heap_bound = current_callstack_entry.heap_upper_bound;
    let (mut heap_growth, uf) = heap_max_accessed.overflowing_sub(cs, heap_bound);
    heap_growth = heap_growth.mask_negated(cs, uf); // if we access in bounds then it's 0
    let new_heap_upper_bound =
        UInt32::conditionally_select(cs, uf, &heap_bound, &heap_max_accessed);
    let grow_heap = Boolean::multi_and(cs, &[forwarding_data.use_heap, execute]);

    let aux_heap_max_accessed = upper_bound.mask(cs, forwarding_data.use_aux_heap);
    let aux_heap_bound = current_callstack_entry.aux_heap_upper_bound;
    let (mut aux_heap_growth, uf) = aux_heap_max_accessed.overflowing_sub(cs, aux_heap_bound);
    aux_heap_growth = aux_heap_growth.mask_negated(cs, uf); // if we access in bounds then it's 0
    let new_aux_heap_upper_bound =
        UInt32::conditionally_select(cs, uf, &aux_heap_bound, &aux_heap_max_accessed);
    let grow_aux_heap = Boolean::multi_and(cs, &[forwarding_data.use_aux_heap, execute]);

However, in the implementation of ret.rs

such heap upper bound memory boundry validation is missing

In callstack_candidate_for_ret implementation,

in this line of code

  new_callstack_entry.ergs_remaining = new_ergs_left;

note, even the boundry heap_upper_bound and aux_heap_upper_bound is initliazed here

these two heap boundary is not applied in new_callstack_entry, which cause memory management problems and data corruption in the heap if the heap growth is out of the boundary

Tools Used

Manual Review

Recommended Mitigation Steps

applying the heap_upper_bound and aux_heap_upper_bound in the callstack entry

new_callstack_entry.heap_upper_bound = Selectable::conditionally_select(
        cs,
        is_local_frame,
        &heap_bound,
        &new_callstack_entry.heap_upper_bound,
    );

 new_callstack_entry.aux_heap_upper_bound = Selectable::conditionally_select(
        cs,
        is_local_frame,
        &aux_heap_bound,
        &new_callstack_entry.aux_heap_upper_bound,
    );

Assessed type

Invalid Validation

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #683

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)