During the staticcall to the precompile the revert() couldn't be catched, which could lead to unintended behaviour.

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L223-L233 https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L286-L299 https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L354-L362 https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L284-L294

Vulnerability details

Impact

The preimageOracle calls precompile addresses with the purpose to:

1. Verify the KZG proof by calling the point evaluation precompile
2. compute SHA2-256 hash

3. Or just call arbitrary precompile to get the result.

   However, the problem is that if the staticcall to the precompiled contract fails(proof is invalid / sha256 calculation failed) the function will not be able to revert, since the call would always succeed, and it would simply move forward, which could lead to unintended behaviour.

   • Why the function wouldn't be able to catch the revert? Because When calling precompiles, be aware that on error/”failure”, the call is still successful. A failed precompile call simply has a returndatasize of 0. It means that, when calling precompiles, do the check on the returndatasize not the "success" of the call to determine if it failed

If we dive deeper into the problem, we could find that it doesn't return 0, it returns nothing! So, instead of checking for 0, we would need to check the return data size because such behaviour could lead to the unintended behaviour. the staticcall(s) will fail, thus returning no result and the execution will continue with the incorrect data.

Additionaly, it worth to mention that during the loadPrecompilePreimagePart function, the precompile address isn't checked. It means, if the address will be passed without code deployed, the static call will always return true! -> https://dacian.me/solidity-inline-assembly-vulnerabilities#heading-external-call-to-non-existent-contract It could lead to the unintended behaviour and potential manipulations of the preimages

Proof of Concept

Traces: [3350501] PreimageOracle_Test::setUp() ├─ [3305933] → new PreimageOracle@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f │ └─ ← [Return] 14793 bytes of code ├─ [0] VM::label(PreimageOracle: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], "PreimageOracle") │ └─ ← [Return] └─ ← [Stop]

[1040497855] PreimageOracle_Test::test_loadPrecompilePreimagePart_succeeds() ├─ [0] VM::expectRevert(InvalidProof()) │ └─ ← [Return] ├─ [1040488837] PreimageOracle::loadPrecompilePreimagePart(0, 0x000000000000000000000000000000000000000A, 0xdeadbeef) │ ├─ [0] console::log("Gas Left before starting the assembly block", 1056934336 [1.056e9]) [staticcall] │ │ └─ ← [Stop] │ ├─ [0] 0x000000000000000000000000000000000000000A::deadbeef() [staticcall] │ │ └─ ← [PrecompileError] │ ├─ [0] console::log("Gas Left to finish the storing the data", 16514354 [1.651e7]) [staticcall] │ │ └─ ← [Stop] │ ├─ [0] console::log("Eventually, final gasleft is", 16446890 [1.644e7]) [staticcall] │ │ └─ ← [Stop] │ └─ ← [Stop] └─ ← [Revert] call did not revert as expected

Tools Used

Manual Review / Foundry

Recommended Mitigation Steps

Use returndatacopy and returndatasize to handle the return data dynamically, making sure you capture all the return data regardless of its size.

Assessed type

Other

[Inphi]

This is not a valid report. staticall writes the precompile call status to the top of the stack. In solidity/yul, this is the return value of the staticall. The PreimageOracle reads this value to determine whether or not to finish loading the precompile result. This is done for all staticall sites including load256PreimagePart, loadBlobPreimagePart, and loadPrecompilePreimagePart (which is a special case where we explicitly want the success/revert result of the staticcall).

We do not check the precompile address during loadPrecompilePreimagePart to allow the op-program to use request newly added precompiles on L1 without upgrading the PreimageOracle. We permit users to load preimages via loadPrecompilePreimagePart for arbitrary, non-precompille, addresses because the op-program will never query them.

[c4-judge]

zobront marked the issue as unsatisfactory: Invalid