Incorrect ByteArray can be deserialized

[howlbot-integration]

Lines of code

https://github.com/kkrt-labs/kakarot-lib/blob/c2c7cb400f85c3699a6902946bcf4428d5b4fc61/src/CairoLib.sol#L220
https://github.com/kkrt-labs/kakarot-lib/blob/c2c7cb400f85c3699a6902946bcf4428d5b4fc61/src/CairoLib.sol#L230-L236

Vulnerability details

Proof of Concept

Validation of pendingWordLen

The assertion pendingWordLen <= 31 is not strict enough. Pending word must not be of length 31, because in this case it must be a full word.

This is also mentioned in the documentation: https://docs.starknet.io/architecture-and-concepts/smart-contracts/serialization-of-cairo-types/#serialization_of_byte_arrays

pending_word: felt252
The bytes that remain after filling the data array with full 31-byte chunks. The pending word consists of at most 30 bytes.

Validation of fullWordsLength

The number of full words is not validated, but used as a limit for iteration. If the number is higher then the actual number of full words, the full words are repeated and the output is corrupted. If the number is lower, execution reverts but with wrong error.

Test cases

Add to kakarot-lib/test/CairoLib.t.sol:

    function testInvalidPendingWordLength2() public {
        bytes memory input = abi.encodePacked(
            uint256(0),
            uint256(0x4c6f72656d20697073756d20646f6c6f722073697420616d65742c20636f6e), // 31 byte
            uint256(31) // Invalid pendingWordLen, because maximum 30 is allowed
        );

        vm.expectRevert("Invalid pending word length");
        CairoLib.byteArrayToString(input);

        // Successfully deserialized:
        // string memory result = CairoLib.byteArrayToString(input);
        // assertEq(result, "Lorem ipsum dolor sit amet, con", "Conversion failed");
    }

    function testInvalidPendingWordLength3() public {
        bytes memory input = abi.encodePacked(
            uint256(0),
            uint256(0),
            uint256(31) // Invalid pendingWordLen, because maximum 30 is allowed
        );

        vm.expectRevert("Invalid pending word length");
        CairoLib.byteArrayToString(input);
    }

    function testFullWord100() public pure {
        bytes memory data = abi.encodePacked(
            uint256(20), // Incorrect fullWordsLength
            uint256(0x48656c6c6f20576f726c642c20746869732069732061206c6f6e6765722073), // "Hello World, this is a longer s"
            uint256(0x7472696e672e), // "tring."
            uint256(6) // pendingWordLen
        );

        // Deserialization succeeds, but the result is corrupted:
        string memory result = CairoLib.byteArrayToString(data);
        assertEq(result, "Hello World, this is a longer string.");
    }

    function testMultipleFullWords1() public {
        bytes memory data = abi.encodePacked(
            uint256(1),
            uint256(0x4c6f72656d20697073756d20646f6c6f722073697420616d65742c20636f6e),
            uint256(0x73656374657475722061646970697363696e6720656c69742c207365642064),
            uint256(0x6f20656975736d6f642074656d706f7220696e6369646964756e7420757420),
            uint256(0x6c61626f726520657420646f6c6f7265206d61676e6120616c697175612e),
            uint256(30)
        );

        vm.expectRevert("Invalid byte array length");
        CairoLib.byteArrayToString(data);
    }

Recommended Mitigation Steps

Validation of pendingWordLen

Replace the assertion on line 220:

require(pendingWordLen <= 31, "Invalid pending word length");

with:

require(pendingWordLen <= 30, "Invalid pending word length");

Validation of fullWordsLength

Remove this assertion on the line 204:

require(data.length >= 96, "Invalid byte array length");

Instead, add this snippet after the fullWordsLength is known, e.g. on line 221:

    // Calculate the expected minimum length of data
    uint256 expectedMinLength = 96 + (fullWordsLength * 32);
    require(data.length >= expectedMinLength, "Data length does not match fullWordsLength");

Assessed type

en/de-code

[ClementWalter]

Severity: Invalid

Comment: This doesn’t change the final output, ie the overall behavior of the function.

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b