Blockhash revert issue in last 10 blocks

[c4-bot-8]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/instructions/block_information.cairo#L158

Vulnerability details

Impact

The blockhash of the last 10 blocks is supposed to be zero since they are unavailable based on the documentation and protocol rules. However, trying to get the blockhash of these blocks causes an unexpected revert, breaking the protocol rules.

Proof of Concept

According to the documentation, "the last 10 blocks are not available, and 0 is returned instead."
https://docs.kakarot.org/differences/#evm-opcodes
https://github.com/kkrt-labs/kakarot-ssj/blob/935c2238ac0b42c910afd3efeb96f003c1742edf/crates/evm/src/instructions/block_information.cairo#L26-L27

The issue occurs when trying to get the blockhash for block.blocknumber - 1 to block.blocknumber - 9, which causes a revert instead of returning 0 as expected by the protocol.

When the blockhash opcode is executed, the following function is called:

        blockhash:
        let syscall_ptr = cast([fp - 10], felt*);
        let pedersen_ptr = cast([fp - 9], HashBuiltin*);
        let range_check_ptr = [fp - 8];
        let stack = cast([fp - 6], model.Stack*);
        let evm = cast([fp - 3], model.EVM*);
        Internals.blockhash(evm);

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/instructions/block_information.cairo#L55

    func blockhash{
        syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr, stack: model.Stack*
    }(evm: model.EVM*) {
        let (block_number) = Stack.pop();
        if (block_number.high != 0) {
            Stack.push_uint256(Uint256(0, 0));
            return ();
        }

        let lower_bound = Helpers.saturated_sub(evm.message.env.block_number, 256);
        let in_range = is_in_range(block_number.low, lower_bound, evm.message.env.block_number);

        if (in_range == FALSE) {
            Stack.push_uint256(Uint256(0, 0));
            return ();
        }

        let (implementation) = Kakarot_cairo1_helpers_class_hash.read();
        let (blockhash) = ICairo1Helpers.library_call_get_block_hash(
            implementation, block_number.low
        );
        let (blockhash_high, blockhash_low) = split_felt(blockhash);
        Stack.push_uint256(Uint256(low=blockhash_low, high=blockhash_high));
        return ();
    }

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/instructions/block_information.cairo#L158

The is_in_range function checks if the block number is between blocknumber - 256 and blocknumber, but this is incorrect. It should check between blocknumber - 256 and blocknumber - 10.

let in_range = is_in_range(block_number.low, lower_bound, evm.message.env.block_number);

func is_in_range{range_check_ptr}(value, lower, upper) -> felt {
    let res = is_le(lower, value);
    if (res == 0) {
        ap += 26;
        return res;
    }
    return is_nn(upper - 1 - value);
}

https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/common/math_cmp.cairo#L65

PoC

In the following PoC, there are three tests to retrieve the blockhash for block numbers block.blocknumber - 10, block.blocknumber - 9, and block.blocknumber - 1. The first test, test_blockhash_10_block_before, completes successfully, but the second and third tests, test_blockhash_9_block_before and test_blockhash_1_block_before, fail due to a Block number out of range error.

To run the end-to-end tests correctly, the make test-end-to-end12 command should be used, as defined in the Makefile.

test-end-to-end12: deploy
	uv run pytest tests/end_to_end/Simple/test_blockhash.py -m "not slow" -s --seed 42	

import pytest
import pytest_asyncio

from kakarot_scripts.utils.kakarot import deploy


@pytest_asyncio.fixture(scope="package")
async def target():
    return await deploy(
        "Simple",
        "BlockhashContract",
    )



@pytest.mark.asyncio(scope="package")
@pytest.mark.Simple
class Test:

    class TestBlockhash1:
        async def test_blockhash_10_block_before(self, target):
            result1, result2, result3 = await target.getData(10)
            print("current block number is: " + str(result1))
            print("target block number is: " + str(result2))
            print("blockhash of the target block number is: " + str(result3.hex()))

    class TestBlockhash2:
        async def test_blockhash_9_block_before(self, target):
            result1, result2, result3 = await target.getData(9)
            print("current block number is: " + str(result1))
            print("target block number is: " + str(result2))
            print("blockhash of the target block number is: " + str(result3.hex()))

    class TestBlockhash3:
        async def test_blockhash_1_block_before(self, target):
            result1, result2, result3 = await target.getData(1)
            print("current block number is: " + str(result1))
            print("target block number is: " + str(result2))
            print("blockhash of the target block number is: " + str(result3.hex()))
                     

pragma solidity ^0.8.0;

contract BlockhashContract {
    function getData(
        uint256 _diff
    )
        public
        view
        returns (
            uint256 blockNumber,
            uint256 targetBlockNumber,
            bytes32 blockHashOfTargetBlockNumber
        )
    {
        return (
            block.number,
            block.number - _diff,
            blockhash(block.number - _diff)
        );
    }
}

The output shows that the first test, "test_blockhash_10_block_before", passed, but the second and third tests failed.

current block number is: 43
target block number is: 33
blockhash of the target block number is: 0000000000000000000000000000000000000000000000000000000000000000
.FF

FAILED tests/end_to_end/Simple/test_blockhash.py::Test::TestBlockhash2::test_blockhash_9_block_before - 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:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23352)\nUnknown location (pc=0:22540)\nUnknown location (pc=0:9951)\nUnknown location (pc=0:10136)\nUnknown location (pc=0:5935)\n"}


FAILED tests/end_to_end/Simple/test_blockhash.py::Test::TestBlockhash3::test_blockhash_1_block_before - 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:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23354)\nUnknown location (pc=0:23352)\nUnknown location (pc=0:22540)\nUnknown location (pc=0:9951)\nUnknown location (pc=0:10136)\nUnknown location (pc=0:5935)\n"}

Tools Used

Recommended Mitigation Steps

The code below should be modified:

    func blockhash{
        syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr, stack: model.Stack*
    }(evm: model.EVM*) {
        let (block_number) = Stack.pop();
        if (block_number.high != 0) {
            Stack.push_uint256(Uint256(0, 0));
            return ();
        }

        let lower_bound = Helpers.saturated_sub(evm.message.env.block_number, 256);
-        let in_range = is_in_range(block_number.low, lower_bound, evm.message.env.block_number);
+        let in_range = is_in_range(block_number.low, lower_bound, evm.message.env.block_number - 10);

        if (in_range == FALSE) {
            Stack.push_uint256(Uint256(0, 0));
            return ();
        }

        let (implementation) = Kakarot_cairo1_helpers_class_hash.read();
        let (blockhash) = ICairo1Helpers.library_call_get_block_hash(
            implementation, block_number.low
        );
        let (blockhash_high, blockhash_low) = split_felt(blockhash);
        Stack.push_uint256(Uint256(low=blockhash_low, high=blockhash_high));
        return ();
    }

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/instructions/block_information.cairo#L168

Assessed type

Context

[ClementWalter]

Severity: Low

Comment: See issue #3 for comment; dup #130, #5, #3, #37

[c4-judge]

dmvt marked the issue as selected for report

[dmvt]

This fits as a medium issue in my opinion. Though, as mentioned in the comment on #3, there are no loss of funds, there is an impact to the availability of the protocol given that transactions revert unexpectedly.

[ClementWalter]

To summarize

• block hash is not available in starknet for the last 10 blocks
• we can either panic, or push 0, none of this being compliant with the Eth spec.
• this is however already part of our documentation
• the valid finding here is that there was a discrepency between the doc (push 0) and the implemented behavior (panic)
• we could either fix the doc, or push 0 as written instead of panicking

Because there is no way to implement the spec, we think that is not a vulnerabiliy but a documented limitation. We agree on the low as we need to fix the doc (or update the code to push 0).

[dmvt]

Agree with the sponsor. This is low and a documentation issue.

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as not selected for report