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

Opcode BLOCKHASH for last 10 block numbers make starkent transaction revert #3

Open
c4-bot-10 opened this issue Oct 12, 2024 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-5 edited-by-warden grade-b Q-12 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_21_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards 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-10
Copy link
Contributor

c4-bot-10 commented Oct 12, 2024

Lines of code

https://github.com/kkrt-labs/kakarot-ssj/blob/935c2238ac0b42c910afd3efeb96f003c1742edf/crates/contracts/src/cairo1_helpers.cairo#L153

Vulnerability details

Impact

When the opcode is BLOCKHASH ( i.e 0x40 ), the cairo1_helpers.cairo::get_block_hash function is called ( interpreter.cairo::exec_opcode -> block_information.cairo::blockhash -> cairo1_helpers.cairo::get_block_hash )

The cairo1_helpers.cairo::get_block_hash function is as follows :

fn get_block_hash(self: @TContractState, block_number: u64) -> felt252 {
    starknet::syscalls::get_block_hash_syscall(block_number).unwrap_syscall()
}

This function uses get_block_hash_syscall to get the block hash of the specified block number but it return error if the block number is greater than current block number - 10 as per the starknet docs. Whereas, as per the kakarot docs, it should return 0 for the blocks not available :

Item Ethereum Kakarot
BLOCKHASH Get the hash of one of the 256 most recent complete blocks The last 10 blocks are not available, and 0 is returned instead

So, rather than returning 0 as per the docs, it's using unwrap_syscall() which will make the transaction revert with Failure reason: 0x426c6f636b206e756d626572206f7574206f662072616e6765 ('Block number out of range').

It also makes sense to return 0 for the blocks not available rather than reverting the starknet transaction which in EVM world, would forcefully revert the transaction. So any contract using BLOCKHASH 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 BLOCKHASH opcode to make the transaction revert forcefully.

Proof of Concept

Add this file named test_kakarot_blockhash_bug.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_blockhash(
        self, evm: Contract, origin
    ):
        # current block_number = 42
        
        params = {
                "value": 0,
                "code": "602840",
                "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_blockhash_bug.py::TestKakarot::test_execute_blockhash"

After running uv run deploy, the block number will be 42 which can be seen in the first terminal where the nodes are running. And the test will execute the BLOCKHASH opcode for the block number 0x28 ( i.e. 40 in decimals ) which is 2 blocks behind the current block number.

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:49:\nGot an exception while executing a hint: Execution failed. Failure reason: 0x426c6f636b206e756d626572206f7574206f662072616e6765 ('Block number out of range').\nCairo traceback (most recent call last):\nUnknown location (pc=0:24011)\nUnknown location (pc=0:23821)\nUnknown location (pc=0:23776)\nUnknown location (pc=0:23234)\nUnknown location (pc=0:22731)\nUnknown location (pc=0:22729)\nUnknown location (pc=0:21839)\nUnknown location (pc=0:9160)\nUnknown location (pc=0:9345)\nUnknown location (pc=0:5379)\n"}

You can see the error Block number out of range which is due to the BLOCKHASH opcode for last 10 block numbers.

Tools Used

Python test suite

Recommended Mitigation Steps

It can be fixed by returning 0 rather than calling unwrap_syscall() in the cairo1_helpers.cairo::get_block_hash:

fn get_block_hash(self: @TContractState, block_number: u64) -> felt252 {
-    starknet::syscalls::get_block_hash_syscall(block_number).unwrap_syscall()
+    starknet::syscalls::get_block_hash_syscall(block_number).unwrap_or(0)
}

NOTE: make sure to run scarb build -p contracts in kakarot-ssj folder and copy the files from kakarot-ssj/target/dev to kakarot/build/ssj before running the test again.

Assessed type

Error

@c4-bot-10 c4-bot-10 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Oct 12, 2024
c4-bot-8 added a commit that referenced this issue Oct 12, 2024
@c4-bot-12 c4-bot-12 added 🤖_21_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Oct 25, 2024
@howlbot-integration howlbot-integration bot added sufficient quality report This report is of sufficient quality duplicate-37 labels Oct 28, 2024
@ClementWalter
Copy link

Severity: Low

Comment: dup 130, 5, 3, 37

Function is not following the specs. We will implement it.
No risk of loss assets due to a reverting transaction.

@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 duplicate-5 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 duplicate-37 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
Copy link
Contributor

c4-judge commented Nov 7, 2024

dmvt marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 7, 2024
@c4-judge
Copy link
Contributor

dmvt changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 16, 2024
@C4-Staff C4-Staff reopened this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-5 edited-by-warden grade-b Q-12 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_21_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards 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