The function `loadLocalData` does not properly check the `_partOffset`.

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/main/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L134-L150

Vulnerability details

Impact

The function loadLocalData will access memory out of bounds, causing the variable part to have unexpected content.

Details

Function loadLocalData is used to load local data from Preimage and store the corresponding part at _partOffset. The important check here is that _partOffset can't be out of the bound.

// Revert if the given part offset is not within bounds.
if (_partOffset > _size + 8 || _size > 32) { 
    revert PartOffsetOOB();
}

However, this check is incorrect. When the _size = 32 and _partOffset = 40. The content of part will be the memory from the 40 to 72 (40+32), which exceeds the limit of 64.

Tools Used

Vscode

Recommended Mitigation Steps

// Revert if the given part offset is not within bounds.
if (_partOffset > _size + 8 || _partOffset > 32 || _size > 32) { 
    revert PartOffsetOOB();
}

Assessed type

Invalid Validation

[zobront]

I don't know where the idea that there's a limit of 64 comes from. I think this behavior is as it should be, but will turn over to the sponsors to confirm (especially that there is no way to have dirty memory after cause a problem).

[refcell]

It seems the 64 limit comes from the fact that we clean memory 0x20-0x40 and we're only using space 0x00 - 0x40 (0-64). Since the partOffset is used in the mload, it can read 32 bytes at 40 and will overflow the 64. This is allowed because in readPreimage, the dataLen that is used limits the amount of data the VM reads to the valid bytes of the byte32. Anyways, the free memory pointer is at 0x40 and can assumed to be clean, so this doesn't affect the mloaded part content.

[c4-judge]

zobront marked the issue as unsatisfactory: Invalid