Unchecked Memory Access and Integer Overflow Risks Lead to Arbitrary Memory Corruption

[howlbot-integration]

Lines of code

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

Vulnerability details

Description

There are several critical memory safety vulnerabilities in the ByteArray conversion functionality. The issues primarily revolve around unsafe memory access patterns, potential integer overflows, and insufficient bounds checking that could lead to memory corruption or unauthorized access.

Code Snippet

function byteArrayToString(bytes memory data) internal pure returns (string memory) {
    require(data.length >= 96, "Invalid byte array length");

    uint256 fullWordsLength;
    uint256 fullWordsPtr;
    uint256 pendingWord;
    uint256 pendingWordLen;

    assembly {
        fullWordsLength := mload(add(data, 32))
        let fullWordsByteLength := mul(fullWordsLength, 32)
        fullWordsPtr := add(data, 64)
        let pendingWordPtr := add(fullWordsPtr, fullWordsByteLength)
        pendingWord := mload(pendingWordPtr)
        pendingWordLen := mload(add(pendingWordPtr, 32))
    }

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

Vulnerability Details

The implementation contains several interrelated security issues that compound to create significant vulnerabilities. At the heart of the problem is the handling of fullWordsLength, which is loaded directly from memory without validation. This value becomes the basis for several critical calculations and memory operations. When the code loads this value using mload(add(data, 32)), it implicitly trusts that this value is reasonable and safe to use in subsequent operations. An attacker could craft input data containing a maliciously large fullWordsLength value that would compromise the entire conversion process.

The situation becomes more dangerous in the multiplication operation mul(fullWordsLength, 32). This calculation determines the byte length for memory access, but without any overflow protection. Consider a scenario where an attacker sets fullWordsLength to a value close to 2^256 / 32. The multiplication would wrap around to a small number, but the pointer arithmetic would be severely compromised. Here's how an attacker might exploit this:

// Attacker's payload construction
function createMaliciousInput() public pure returns (bytes memory) {
    bytes memory data = new bytes(96); // Minimum required length
    
    assembly {
        // Store massive fullWordsLength value that will cause overflow
        mstore(add(data, 32), 0xffffffffffffffffffffffffffffffff)
    }
    
    return data;
}

The memory safety issue extends further when the code calculates pendingWordPtr. This pointer is derived by adding fullWordsByteLength to the base pointer, but there's no verification that this calculation results in a valid memory location. In the EVM, this could lead to accessing memory well beyond the allocated region. The danger is compounded because the code then reads from this location using mload and even attempts to read 32 bytes beyond it for the pendingWordLen.

Here's a concrete example of how pointer arithmetic can be exploited:

function demonstratePointerOverflow() public pure returns (bytes memory) {
    bytes memory data = new bytes(96);
    
    assembly {
        // Calculate a length that when used will cause pointer arithmetic to wrap
        let maxPointer := 0xffffffffffffffffffffffffffffffffffffffff
        let basePtr := add(data, 64)
        let maliciousLength := div(sub(maxPointer, basePtr), 32)
        mstore(add(data, 32), maliciousLength)
    }
    
    return data;
}

The combination of these issues means that an attacker could potentially read from or write to arbitrary memory locations within the EVM's memory space. This could lead to corruption of other variables, exposure of sensitive data, or even complete contract compromise depending on the context in which this function is used.

Recommended Solution

A secure implementation should include comprehensive bounds checking and safe arithmetic operations:

function byteArrayToString(bytes memory data) internal pure returns (string memory) {
    require(data.length >= 96, "Invalid byte array length");

    uint256 fullWordsLength;
    uint256 fullWordsPtr;
    uint256 pendingWord;
    uint256 pendingWordLen;

    assembly {
        // Load and validate fullWordsLength
        fullWordsLength := mload(add(data, 32))
        
        // Prevent overflow in multiplication
        if gt(fullWordsLength, div(sub(0xffffffffffffffffffffffffffffffffffffffff, 64), 32)) {
            revert(0, 0) // "fullWordsLength too large"
        }
        
        let fullWordsByteLength := mul(fullWordsLength, 32)
        
        // Ensure total size is valid
        let requiredSize := add(96, fullWordsByteLength)
        if gt(requiredSize, mload(data)) {
            revert(0, 0) // "Input data too short"
        }
        
        fullWordsPtr := add(data, 64)
        let pendingWordPtr := add(fullWordsPtr, fullWordsByteLength)
        
        // Validate memory bounds
        if gt(add(pendingWordPtr, 32), add(data, mload(data))) {
            revert(0, 0) // "Invalid pending word pointer"
        }
        
        pendingWord := mload(pendingWordPtr)
        pendingWordLen := mload(add(pendingWordPtr, 32))
    }

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

The fix introduces crucial safety checks that validate memory bounds and prevent arithmetic overflow. Most importantly, it ensures that all memory accesses stay within the bounds of the allocated memory region, preventing potential exploits.

Test

contract CairoLibTest {
    function testByteArraySafety() public pure {
        // Test with malicious overflow value
        bytes memory overflowData = new bytes(96);
        assembly {
            mstore(add(overflowData, 32), 0xffffffffffffffffffffffffffffffff)
        }
        // Should revert safely
        
        // Test with boundary pointer calculation
        bytes memory boundaryData = new bytes(96);
        assembly {
            let maxSize := sub(0xffffffffffffffffffffffffffffffffffffffff, 96)
            mstore(add(boundaryData, 32), div(maxSize, 32))
        }
        // Should revert safely
    }
}

Assessed type

Under/Overflow

[ClementWalter]

Severity: Informative

Comment: This doesn’t change the final output, ie the overall behavior of the function.
Also this only to be used on starknet output. The documentation will be updated.

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)