Assessment reportsPublic findings
Back to Zellic site
ā†—
Assessment reports>Scroll zkEVM>Informational findings>Improvements possible for testing out-of-gas on ,MCOPY
GeneralOverview
Findings
Critical (4)
Medium (1)
Informational (2)Incorrect gas-cost calculation for MCOPY out-of-gas test casesImprovements possible for testing out-of-gas on MCOPY
DiscussionMore robust constraints ensuring that all rw table entries are legitimateUnused tx_id column in StepStateAsserting assumptions about equal gas costsComment explaining gas-cost calculations for MCOPY is incorrectMistakes in the example table in the EVM circuit documentationIncorrect comments for memory gadgets
Audit ResultsAssessment Results
Category: Code Maturity

Improvements possible for testing out-of-gas on MCOPY

Informational Severity
Informational Impact
N/A Likelihood

Description

The EVM circuit has some tests to test out-of-gas errors being handled by the ErrorOOGMemoryCopyGadget on MCOPY execution, given by the following test function:

#[test]
fn test_oog_memory_copy_for_mcopy() {
    for (src_offset, dest_offset, copy_size) in TESTING_MCOPY_PARIS {
        let testing_data =
            TestingData::new_for_mcopy(*src_offset, *dest_offset, *copy_size, None);

        test_root(&testing_data);
        test_internal(&testing_data);
    }
}

This function first uses TestingData::new_for_mcopy to obtain a TestingData struct, containing bytecode executing an MCOPY instruction as well as the calculated gas cost. Then test_root and test_internal are called, which perform the actual test. In the former case, the bytecode including the MCOPY will be executed directly (at the top level of the transaction), and in the latter case, it will be executed one level lower, with a wrapper contract calling the bytecode to be tested. In both cases, the amount of gas is adjusted so that it should be just one gas too little to execute the MCOPY instruction:

fn test_root(testing_data: &TestingData) {
    let gas_cost = GasCost::TX
        .0
!         // Decrease expected gas cost (by 1) to trigger out of gas error.
!         .checked_add(testing_data.gas_cost - 1)
        .unwrap_or(MOCK_BLOCK_GAS_LIMIT);
    let gas_cost = if gas_cost > MOCK_BLOCK_GAS_LIMIT {
        MOCK_BLOCK_GAS_LIMIT
    } else {
        gas_cost
    };

    let ctx = TestContext::<2, 1>::new(
        None,
        account_0_code_account_1_no_code(testing_data.bytecode.clone()),
        |mut txs, accs| {
            txs[0]
                .from(accs[1].address)
                .to(accs[0].address)
                .gas(gas_cost.into());
        },
        |block, _tx| block.number(0xcafe_u64),
    )
    .unwrap();

    CircuitTestBuilder::new_from_test_ctx(ctx)
        .params(CircuitsParams {
            max_copy_rows: 1750,
            ..Default::default()
        })
        .run();
}

fn test_internal(testing_data: &TestingData) {
    // ...

    // Code A calls code B.
    let code_a = bytecode! {
        // populate memory in A's context.
        PUSH8(U256::from_big_endian(&rand_bytes(8)))
        PUSH1(0x00) // offset
        MSTORE
        // call ADDR_B.
        PUSH1(0x00) // retLength
        PUSH1(0x00) // retOffset
        PUSH32(0x00) // argsLength
        PUSH32(0x20) // argsOffset
        PUSH1(0x00) // value
        PUSH32(addr_b.to_word()) // addr
!         // Decrease expected gas cost (by 1) to trigger out of gas error.
!         PUSH32(gas_cost_b - 1) // gas
        CALL
        STOP
    };

    // ...
}

If we run this test with RUST_BACKTRACE=1 RUST_LOG=trace cargo test test_oog_memory_copy_for_mcopy -- --nocapture, we can also see in the debug output that an out-of-memory error happens, as seen here for the very last test case:

[2024-06-04T18:40:55Z TRACE zkevm_circuits::evm_circuit::execution] assign_exec_step offset: 48 state ErrorOutOfGasMemoryCopy step: ExecStep { call_index: 1, rw_indices: [(Stack, 22), (Stack, 23), (Stack, 24), (CallContext, 49), (CallContext, 50), (CallContext, 51), (CallContext, 52), (CallContext, 53), (CallContext, 54), (CallContext, 55), (CallContext, 56), (CallContext, 57), (CallContext, 58), (CallContext, 59), (CallContext, 60), (CallContext, 61), (CallContext, 62)], copy_rw_counter_delta: 0, execution_state: ErrorOutOfGasMemoryCopy, rw_counter: 95, program_counter: 99, stack_pointer: 1021, gas_left: 14, gas_cost: 42, memory_size: 0, reversible_write_counter: 0, reversible_write_counter_delta: 0, log_id: 0, opcode: Some(MCOPY), block_num: 0, aux_data: None } call: Call { id: 50, is_root: false, is_create: false, code_hash: 69088479414407603174779605424463024523545942567839557216382057906686480814179, rw_counter_end_of_reversion: 111, caller_id: 1, depth: 2, caller_address: 0x000000000000000000000000000000000cafe111, callee_address: 0x000000000000000000000000000000000cafe222, code_address: Some(0x000000000000000000000000000000000cafe222), call_data_offset: 0, call_data_length: 0, return_data_offset: 0, return_data_length: 0, last_callee_id: 0, last_callee_return_data_offset: 0, last_callee_return_data_length: 0, value: 0, is_success: false, is_persistent: false, is_static: false }
[2024-06-04T18:40:55Z DEBUG zkevm_circuits::evm_circuit::execution::error_oog_memory_copy] ErrorOutOfGasMemoryCopy: opcode = MCOPY, gas_left = 14, gas_cost = 42

However, the way these tests are run does not check that an out-of-gas error actually happens on the MCOPY.

We verified this by changing the amount of gas available to be sufficient:

fn test_root(testing_data: &TestingData) {
    let gas_cost = GasCost::TX
        .0
        // Decrease expected gas cost (by 1) to trigger out of gas error.
-         .checked_add(testing_data.gas_cost - 1)
+         .checked_add(testing_data.gas_cost + 100000)
        .unwrap_or(MOCK_BLOCK_GAS_LIMIT);
    let gas_cost = if gas_cost > MOCK_BLOCK_GAS_LIMIT {
        MOCK_BLOCK_GAS_LIMIT
    } else {
        gas_cost
    };

    // ...
}

fn test_internal(testing_data: &TestingData) {
    // ...

    // Code A calls code B.
    let code_a = bytecode! {
        // populate memory in A's context.
        PUSH8(U256::from_big_endian(&rand_bytes(8)))
        PUSH1(0x00) // offset
        MSTORE
        // call ADDR_B.
        PUSH1(0x00) // retLength
        PUSH1(0x00) // retOffset
        PUSH32(0x00) // argsLength
        PUSH32(0x20) // argsOffset
        PUSH1(0x00) // value
        PUSH32(addr_b.to_word()) // addr
        // Decrease expected gas cost (by 1) to trigger out of gas error.
-        PUSH32(gas_cost_b - 1) // gas
+        PUSH32(gas_cost_b + 100000) // gas
        CALL
        STOP
    };

    // ...
}

The tests still pass. However, no out-of-gas error occurs:

[2024-06-04T18:46:28Z TRACE zkevm_circuits::evm_circuit::execution] assign_exec_step offset: 48 state MCOPY step: ExecStep { call_index: 1, rw_indices: [(Stack, 22), (Stack, 23), (Stack, 24), (Memory, 2), (Memory, 3), (Memory, 4), (Memory, 5), (Memory, 6), (Memory, 7), (Memory, 8), (Memory, 9)], copy_rw_counter_delta: 8, execution_state: MCOPY, rw_counter: 95, program_counter: 99, stack_pointer: 1021, gas_left: 100015, gas_cost: 42, memory_size: 0, reversible_write_counter: 0, reversible_write_counter_delta: 0, log_id: 0, opcode: Some(MCOPY), block_num: 0, aux_data: None } call: Call { id: 50, is_root: false, is_create: false, code_hash: 69088479414407603174779605424463024523545942567839557216382057906686480814179, rw_counter_end_of_reversion: 0, caller_id: 1, depth: 2, caller_address: 0x000000000000000000000000000000000cafe111, callee_address: 0x000000000000000000000000000000000cafe222, code_address: Some(0x000000000000000000000000000000000cafe222), call_data_offset: 0, call_data_length: 0, return_data_offset: 0, return_data_length: 0, last_callee_id: 0, last_callee_return_data_offset: 0, last_callee_return_data_length: 0, value: 0, is_success: true, is_persistent: true, is_static: false }
[2024-06-04T18:46:28Z TRACE zkevm_circuits::evm_circuit::execution] assign_exec_step offset: 50 state STOP step: ExecStep { call_index: 1, rw_indices: [(CallContext, 49), (CallContext, 50), (CallContext, 51), (CallContext, 52), (CallContext, 53), (CallContext, 54), (CallContext, 55), (CallContext, 56), (CallContext, 57), (CallContext, 58), (CallContext, 59), (CallContext, 60), (CallContext, 61)], copy_rw_counter_delta: 0, execution_state: STOP, rw_counter: 106, program_counter: 100, stack_pointer: 1024, gas_left: 99973, gas_cost: 0, memory_size: 288, reversible_write_counter: 0, reversible_write_counter_delta: 0, log_id: 0, opcode: Some(STOP), block_num: 0, aux_data: None } call: Call { id: 50, is_root: false, is_create: false, code_hash: 69088479414407603174779605424463024523545942567839557216382057906686480814179, rw_counter_end_of_reversion: 0, caller_id: 1, depth: 2, caller_address: 0x000000000000000000000000000000000cafe111, callee_address: 0x000000000000000000000000000000000cafe222, code_address: Some(0x000000000000000000000000000000000cafe222), call_data_offset: 0, call_data_length: 0, return_data_offset: 0, return_data_length: 0, last_callee_id: 0, last_callee_return_data_offset: 0, last_callee_return_data_length: 0, value: 0, is_success: true, is_persistent: true, is_static: false }

Impact

Should the gas-cost calculation done by TestingData::new_for_mcopy be correct, these tests will test that the assignments to the ErrorOOGMemoryCopyGadget satisfy the constraints. However, should TestingData::new_for_mcopy return an incorrect gas cost that is too high, then these tests will not actually use the ErrorOOGMemoryCopyGadget. Those constraints will thus not be tested. Furthermore, the current tests do not ensure that the gas costs calculated inside the circuit match those calculated by TestingData::new_for_mcopy.

Note that Finding refā†— precisely discusses a bug in the gas-cost calculation by TestingData::new_for_mcopy.

Recommendations

We recommend to enhance the testing code in the following way. Firstly, the bytecode is run twice. Once with available gas set to testing_data.gas_cost - 1 and once with testing_data.gas_cost. Secondly, the testing code checks whether the test actually did reach an out-of-gas error on the MCOPY instruction. The test should then only succeed if all of the following are true:

  1. Both runs succeeded, as in the constraints were satisfied.

  2. The first run reached an out-of-gas error on the MCOPY instruction.

  3. The second run did not reach an out-of-gas error on the MCOPY instruction.

This will ensure that the gas-cost calculation done by TestingData::new_for_mcopy matches the calculation done in circuit in the cases tested and that the ErrorOOGMemoryCopyGadget is indeed tested.

Remediation

Scroll informed us that they are considering to add the discussed tests in the future.

Zellic Ā© 2024Back to top ā†‘