Gas charged after computation for precompiles may lead to running out of steps

[c4-bot-8]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/interpreter.cairo#L96

Vulnerability details

Impact

The interpreter.cairo::exec_opcode function is called whenever any opcode is executed. This function is as follows:

    // @notice Decode the current opcode and execute associated function.
    // @dev The function uses an internal jump table to execute the corresponding opcode
    // @param evm The pointer to the execution context.
    // @return EVM The pointer to the updated execution context.
    func exec_opcode{
        syscall_ptr: felt*,
        pedersen_ptr: HashBuiltin*,
        range_check_ptr,
        bitwise_ptr: BitwiseBuiltin*,
        stack: model.Stack*,
        memory: model.Memory*,
        state: model.State*,
    }(evm: model.EVM*) -> model.EVM* {

      ...

->    let (output_len, output, gas_used, revert_code) = Precompiles.exec_precompile(
          evm.message.code_address.evm,
          evm.message.calldata_len,
          evm.message.calldata,
          caller_code_address,
          caller_address,
      );

      let precompile_reverted = is_not_zero(revert_code);
      if (precompile_reverted != FALSE) {
          // No need to charge gas as precompiles can only trigger EXCEPTIONAL_REVERT
          // which will consume the entire gas of the context.
          let evm = EVM.stop(evm, output_len, output, revert_code);
          tempvar range_check_ptr = range_check_ptr;
          tempvar evm = evm;
      } else {
          // Charge gas before stopping
->        let evm = EVM.charge_gas(evm, gas_used);
          let evm = EVM.stop(evm, output_len, output, evm.reverted);
          tempvar range_check_ptr = range_check_ptr;
          tempvar evm = evm;
      }

      ...

    }

If the precompile is going to take a lot of gas ( which can be precalculated ), then rather than charging high gas, the transaction would revert due to running out of steps. As it can consume more than 10,000,000 whereas the max steps allowed is 10,000,000 as per starknet docs ( refer: Max transaction size (Cairo steps) ). So in mainnet, this transaction will revert due to out of steps error.

So any contract using precompiles will be vulnerable to this issue or if a contract makes a callback to another contract by making sure to only send limited gas and handle the revert case properly, the other contract can call the precompile with such a data that will consume a lot of gas which will make the transaction revert forcefully. Plus, the contract doesn't even need to send a lot of gas ( or may only send 0 gas ) while making STATICCALL to precompile.

Proof of Concept

Consider blake2f precompile which charges gas as follows:

static_gas = 0
dynamic_gas = rounds * 1

So if the number of rounds is very large, then the gas charged will also be very high but the transaction will revert due to running out of steps thus it won't charge any gas even if zero gas was send while calling the precompile using STATICCALL opcode.

NOTE: First 4 bytes ( out of 213 bytes ) in the calldata is the number of rounds.

So consider the following code:

#define macro MAIN() = takes(0) returns(0) {

    // First 4 bytes in memory = 0xffffffff = 4294967295
    0xff 0x00 mstore8
    0xff 0x01 mstore8
    0xff 0x02 mstore8
    0xff 0x03 mstore8
    
    // 0xff 0x00 mstore8
    0x40 // retSize = 64 bytes
    0x00 // retOffset
    0xd5 // argSize = 213 bytes
    0x00 // argOffset
    0x09 // address = blake2f
    0x00 // gas = 0
    staticcall
}

Save the above code in a file name blake2f.huff ( You can refer Installing Huff, if you don't have huff installed ).

The above code will call the precompile blake2f where number of rounds will 0xffffffff ( 4294967295 ) and we're sending 0 gas and in reality it should revert at evm level due to out of gas error.

The above code can be converted to bytecode using the following command:

huffc blake2f.huff --bytecode

The output will be:

601d8060093d393df360ff5f5360ff60015360ff60025360ff60035360405f60d55f60095ffa

The runtime code is ( after removing the first few bytes which are responsible for deployment ):

60ff5f5360ff60015360ff60025360ff60035360405f60d55f60095ffa

Add the following cairo file named test_kakarot_blake2f.cairo in kakarot/tests/src/kakarot folder:

%lang starknet

from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin
from starkware.cairo.common.alloc import alloc
from starkware.cairo.common.uint256 import Uint256

from kakarot.library import Kakarot
from kakarot.model import model

func eth_call{
    syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr, bitwise_ptr: BitwiseBuiltin*
}() -> (model.EVM*, model.State*, felt, felt) {
    tempvar origin;
    tempvar to: model.Option;
    tempvar gas_limit;
    tempvar gas_price;
    tempvar nonce;
    let (value_ptr) = alloc();
    tempvar data_len: felt;
    let (data) = alloc();
    tempvar access_list_len: felt;
    let (access_list) = alloc();

    %{
        from kakarot_scripts.utils.uint256 import int_to_uint256

        ids.origin = program_input.get("origin", 0)
        ids.to.is_some = int(bool(program_input.get("to") is not None))
        ids.to.value = program_input.get("to") or 0
        ids.gas_limit = program_input.get("gas_limit", int(2**63 - 1))
        ids.gas_price = program_input.get("gas_price", 0)
        ids.nonce = program_input.get("nonce", 0)
        segments.write_arg(ids.value_ptr, int_to_uint256(program_input.get("value", 0)))
        data = bytes.fromhex(program_input.get("data", "").replace("0x", ""))
        ids.data_len = len(data)
        segments.write_arg(ids.data, list(data))
        ids.access_list_len = 0
    %}

    let (evm, state, gas_used, required_gas) = Kakarot.eth_call(
        nonce=nonce,
        origin=origin,
        to=to,
        gas_limit=gas_limit,
        gas_price=gas_price,
        value=cast(value_ptr, Uint256*),
        data_len=data_len,
        data=data,
        access_list_len=access_list_len,
        access_list=access_list,
    );

    return (evm, state, gas_used, required_gas);
}

Also add the following python test file named test_kakarot_blake2f.py in the same folder i.e kakarot/tests/src/kakarot folder:

from tests.utils.syscall_handler import SyscallHandler, parse_state

class TestKakarotBlake2f:           
        @SyscallHandler.patch("IAccount.is_valid_jumpdest", lambda addr, data: [1])
        @SyscallHandler.patch("IAccount.get_code_hash", lambda addr, data: [0x1, 0x1])
        def test_blake2f(self, cairo_run):
            evm_address = "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b"
            origin = int(evm_address, 16)
            contract_address = "0x000000000000000000000000000000ca1100f022"
            initial_state = {
                contract_address: {
                    "code": "60ff5f5360ff60015360ff60025360ff60035360405f60d55f60095ffa",
                    "storage": {},
                    "balance": 0,
                    "nonce": 0,
                }
            }
            with SyscallHandler.patch_state(parse_state(initial_state)):
                res = cairo_run("eth_call",
                    origin=origin,
                    to=int(contract_address, 16),
                    gas_limit=int("0xfffffff", 16),
                    gas_price=int("0x80", 16),
                    value=0,
                    nonce=0,
                    )
                is_reverted = res[0]["reverted"] != 0
                assert is_reverted == 0

Now run the test using the following command:

cd kakarot/
uv run pytest -s "./tests/src/kakarot/test_kakarot_blake2f.py::TestKakarotBlake2f::test_blake2f"

The test will take very long time ( due to speed of python ) before throwing error that the transaction ran out of steps ( n_steps=10_000_000 ) which will be something like:

...
Error: End of program was not reached
...

NOTE: You can print number of steps by adding the print statement in kakarot/tests/fixtures/starknet.py:

            if len(function_output) > 0:
                final_output = (final_output, *function_output)
        else:
            final_output = function_output
->      print(f"runner.original_steps: {runner.original_steps}")
->      print(f"runner.vm.current_step: {runner.vm.current_step}")
        return final_output

    return _factory

Although it won't print the steps as the transaction will revert before that.

Tools Used

Python test suite

Recommended Mitigation Steps

As the total cost for the precompile can be precalculated, it should be charged before executing the precompile. For some precompile, if they're reverted then all the gas is charged thus this case should also be considered while executing the revert code. So if enough gas isn't sent, the transaction would revert at evm level rather than running out of steps.

Assessed type

Other

[ClementWalter]

Severity: Low

Comment: Gas should be charged before running the computation. No clear attack path.

[c4-judge]

dmvt marked the issue as unsatisfactory:
Invalid

[dmvt]

Also overinflated severity

[Ahmed-Aghadi]

Isn't this an issue:

if a contract makes a callback to another contract by making sure to only send limited gas and handle the revert case properly, the other contract can call the precompile with such a data that will consume a lot of gas which will make the transaction revert forcefully. Plus, the contract doesn't even need to send a lot of gas ( or may only send 0 gas ) while making STATICCALL to precompile.

Here the try/catch block in solidity or checking the return value of call in low level won't matter because the contract will revert on starknet level. So any contract can revert the whole transaction forcefully when it is called by other contracts. I want to know, what make this invalid or overinflated severity?

[dmvt]

Specifically that you marked this high risk. See here: https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#estimating-risk

Ruling stands.

[Ahmed-Aghadi]

Specifically that you marked this high risk. See here: https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#estimating-risk

Ruling stands.

"the function of the protocol or its availability could be impacted" right? So shouldn't it be at least medium? If you think it's low ( if yes, plz let me know why ), then I understand why you mark this as overinflated severity.

[dmvt]

Severity is low. I agree with the sponsor. I will not comment further on this one. Ruling stands.

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)