Insufficient Input Validation in 'PreimageKeyLib' Functions

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageKeyLib.sol#L12-L20

Vulnerability details

Impact

this may cause unexpected behavior due to insufficient input validation in the 'localizeIdent', 'localize', and 'keccak256PreimageKey' functions. This can result in the creation of invalid keys, which can cause failures in key localization and potentially compromise the integrity of systems that rely on those keys.

Proof of Concept

• Invalid Local Context: Pass an empty local context to the 'localize' function.

  
  bytes32 invalidContext = bytes32(0);
  bytes32 key = someKey; // Assume someKey is a valid key

// This should fail because the local context is invalid (empty). bytes32 localizedKey = PreimageKeyLib.localize(key, invalidContext);

- Invalid Identifier: Pass an identifier that is out of the expected range to the *'localizeIdent'* function.

uint256 invalidIdent = 2**248; // Out of the valid range bytes32 localContext = someLocalContext; // Assume someLocalContext is a valid local context

// This should fail because the identifier is out of the expected range. bytes32 localizedKey = PreimageKeyLib.localizeIdent(invalidIdent, localContext);

- Empty Preimage: Pass an empty preimage to the *'keccak256PreimageKey'* function.

bytes memory emptyPreimage = "";

bytes32 preimageKey = PreimageKeyLib.keccak256PreimageKey(emptyPreimage); // This should fail because the preimage is empty.

## Tools Used
Manual review

## Recommended Mitigation Steps
- Add Input Validation in *'localizeIdent'*:

function localizeIdent(uint256 _ident, bytes32 localContext) internal view returns (bytes32 key) { require(_ident < 2**248, "Identifier out of range"); require(_localContext != bytes32(0), "Invalid local context");

assembly {
    key_ := or(shl(248, 1), and(_ident, not(shl(248, 0xFF))))
}
key_ = localize(key_, _localContext);

}

- Add Input Validation in *'localize'*:

function localize(bytes32 _key, bytes32 localContext) internal view returns (bytes32 localizedKey) { require(_localContext != bytes32(0), "Invalid local context");

assembly {
    let ptr := mload(0x40)
    mstore(0, _key)
    mstore(0x20, caller())
    mstore(0x40, _localContext)
    localizedKey_ := or(and(keccak256(0, 0x60), not(shl(248, 0xFF))), shl(248, 1))
    mstore(0x40, ptr)
}

}

- Add Input Validation in *'keccak256PreimageKey'*:

function keccak256PreimageKey(bytes memory preimage) internal pure returns (bytes32 key) { require(_preimage.length > 0, "Preimage cannot be empty");

assembly {
    let size := mload(_preimage)
    let h := keccak256(add(_preimage, 0x20), size)
    key_ := or(and(h, not(shl(248, 0xFF))), shl(248, 2))
}

}

## Assessed type

Library

[clabby]

This report is invalid, as it describes intended behavior.

1. localizeIdent - You are correct that the ident may have its high-order byte chopped off by localizeIdent. This is known, and callers of this function should expect a preimage key type byte to be placed in the high-order byte.
2. localize - This is intended behavior. A local context of 0 is fine - it gets hashed in with the initial key and caller address.
3. keccak256PreimageKey - This is intended behavior. An empty input is a valid input to the keccak256 hash function, and so the key must be able to represent it.

[c4-judge]

zobront marked the issue as unsatisfactory: Invalid