decode_legacy_tx allows validation of signatures with chain_id that are larger than felt, and overflows

[howlbot-integration]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/utils/eth_transaction.cairo#L72

Vulnerability details

Impact

In decode_legacy_tx, data_len for chain_id is not constrained to be <31, which allows validation of signatures with chain_id that are larger than felt, and overflows

Proof of Concept

chain_id is gotten from items[6].data via the bytes_to_felt helper function.

Looking at bytes_to_felt function, we will observe that the return value will overflow if len>31, because current would have been shifted 2^(8*32) times, which overflows the max felt value of 2^252:

    func bytes_to_felt(len: felt, ptr: felt*) -> felt {
        if (len == 0) {
            return 0;
        }
        tempvar current = 0;

        // len, ptr, ?, ?, current
        // ?, ? are intermediate steps created by the compiler to unfold the
        // complex expression.
        loop:
        let len = [ap - 5];
        let ptr = cast([ap - 4], felt*);
        let current = [ap - 1];

        tempvar len = len - 1;
        tempvar ptr = ptr + 1;
        tempvar current = current * 256 + [ptr - 1];//@audit-info overflow if len>31

        static_assert len == [ap - 5];
        static_assert ptr == [ap - 4];
        static_assert current == [ap - 1];
        jmp loop if len != 0;

        return current;
    }

Since items[6].data_len is not constrained to be <=31, absurd values of chain_id(containing >31 data_len) can be passed, which would overflow, and pass the signature validation

Recommended Mitigation Steps

assert that items[6].data_len<=31:

assertassert_nn(31 - items[6].data_len);

Assessed type

Under/Overflow

[ClementWalter]

Severity: Medium

Comment: Ok

[c4-judge]

dmvt marked the issue as selected for report