Add nonReentrant to relayMessage()

[maurelian]

Description

This is a fix to Sam's bug.

Questions

Additionally, @karlfloersch pointed out that another way to address this would be checking that the msg.sender == address(0), because this condition can only be satisfied at the first contract ovmCALLed by run.

Metadata

Fixes

• Fixes Audit/tob 6 message passing reentrancy risk 201 #105
• Also https://github.com/ethereum-optimism/roadmap/issues/394 (private issue)

Contributing Agreement

• I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

[ben-chain]

Lookin' good! I see the question above -- to clarify, this will only work in the OVM_L2CrossDomainMessenger. The reason is because of account abstraction, OVM state transitions do not include a from, just a to, meaning that the entrypoint contract will get address(0) for ovmCALLER, during the very first ovmCALL only. Since the contract uses the compiler (+build ovm), we can easily access ovmCALLER with a normal msg.sender.

It strikes me that this property could be worth explicitly adding to our Execution Manager tests which use the ExecutionManagerTestRunner. I think we want to merge this ASAP though, so I'm gonna approve this one and create another ticket for that.

[ben-chain]

Requested a couple small changes but lookin' good!

[ben-chain]

FWIW, this code path speaks directly to the rollup contracts (OVM_CanonicalTransactionChain.enqueue specifically) so it should not be re-enterable based on the behavior of that target, so it should not be needed.

I'd recommend removing it, because the L1 SSTORE is gonna be expensive. If you think that it's worth keeping, we can, but we should also add it to sendMessage, since the functions serve similar purposes.

[maurelian]

OVM_BaseCrossDomainMessenger.sendMessage() already has the nonReentrant modifier on it. You can see above that I moved it down a couple lines to improve the style. But looking at both sendMessage and replayMessage more closely, I agree that it looks pretty safe to remove them.

[ben-chain]

Beautiful, LGTM!