fix(ctb): Per-message reentrancy guard for relayMessage

[clabby]

Overview

Changes the CrossDomainMessenger's relayMessage reentrancy guard to be per-message rather than disallowing reentrancy entirely.

Dislikes about this soln
- The CrossDomainMessenger no longer needs to inherit ReentrancyGuardUpgradeable, but we would need to alter the storage layouts in order to remove this. Furthers the case for EIP-1967 esq storage slots. EDIT: Added a gap array instead.

Metadata
Closes CLI-3388

[changeset-bot]

🦋 Changeset detected

Latest commit: 6b9373f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@eth-optimism/contracts-bedrock	Minor
@eth-optimism/actor-tests	Patch
@eth-optimism/sdk	Patch
@eth-optimism/chain-mon	Patch
@eth-optimism/message-relayer	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[clabby]

Current dependencies on/for this PR:

• develop
  • PR fix(ctb): Per-message reentrancy guard for relayMessage #4919 👈

This comment was auto-generated by Graphite.

[mergify]

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

[codecov]

Codecov Report

Merging #4919 (6b9373f) into develop (04cd7b2) will decrease coverage by 0.61%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4919      +/-   ##
===========================================
- Coverage    40.57%   39.97%   -0.61%     
===========================================
  Files          324      303      -21     
  Lines        19537    18909     -628     
  Branches       767      653     -114     
===========================================
- Hits          7928     7558     -370     
+ Misses       11009    10755     -254     
+ Partials       600      596       -4     

Flag	Coverage Δ
bedrock-go-tests	35.87% <100.00%> (+0.04%)	⬆️
contracts-bedrock-tests	49.23% <100.00%> (+0.32%)	⬆️
contracts-tests	98.86% <ø> (ø)
core-utils-tests	?
dtl-tests	47.15% <ø> (ø)
fault-detector-tests	33.88% <ø> (ø)
sdk-tests	38.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ts-bedrock/contracts/L1/L1CrossDomainMessenger.sol	60.00% <ø> (ø)
...ts-bedrock/contracts/L2/L2CrossDomainMessenger.sol	60.00% <ø> (ø)
op-chain-ops/genesis/config.go	29.71% <100.00%> (-0.45%)	⬇️
...drock/contracts/universal/CrossDomainMessenger.sol	73.80% <100.00%> (+4.57%)	⬆️
packages/core-utils/src/common/hex-strings.ts
packages/core-utils/src/optimism/encoding.ts
packages/core-utils/src/common/bn.ts
packages/core-utils/src/common/misc.ts
packages/core-utils/src/external/ethers/index.ts
packages/core-utils/src/optimism/alias.ts
... and 17 more

[maurelian]

It's not entirely clear to me that this is the right solution to the problem. Are we sure that we even still need a reentrancy guard here?

What invariants are we trying to preserve?

[mergify]

Hey @clabby! This PR has merge conflicts. Please fix them before continuing review.

[clabby]

It's not entirely clear to me that this is the right solution to the problem. Are we sure that we even still need a reentrancy guard here?

What invariants are we trying to preserve?

The big invariant that we are trying to preserve here is that, if given gas >= the minimum gas limit, a cross domain message will always successfully be relayed if the transaction itself does not revert. Shooting over a DM for more.

[smartcontracts]

I would strongly prefer not using assembly here and just stick with a simple mapping. This is the sort of stuff that IMO should be saved for the optimization release. Any changes to the Bedrock code at this point should be dead simple.

[smartcontracts]

Feel strongly enough about not having any assembly here that I'm going to block this for now. Much rather have a simple mapping, especially because we want to reduce the mental overhead down to an absolute minimum for any auditor that's going to be looking at this code before the Bedrock vote.

Dismissing earlier review given the updates.

[maurelian]

The big invariant that we are trying to preserve here is that, if given gas >= the minimum gas limit, a cross domain message will always successfully be relayed if the transaction itself does not revert. Shooting over a DM for more.

We can also satisfy that invariant by removing the reentrancy guard entirely. If we can't do that, then there must be another invariant we need to maintain? Even without assembly code, this adds complexity so we need to ensure that it's justified.

[clabby]

The big invariant that we are trying to preserve here is that, if given gas >= the minimum gas limit, a cross domain message will always successfully be relayed if the transaction itself does not revert. Shooting over a DM for more.

We can also satisfy that invariant by removing the reentrancy guard entirely. If we can't do that, then there must be another invariant we need to maintain? Even without assembly code, this adds complexity so we need to ensure that it's justified.

The other very important invariant we need to maintain here is that "cross domain messages should only be able to be successfully relayed once." With the current design, removing the reentrancy guard entirely would allow for relaying a message multiple times before the message hash was assigned to the successfulMessages or failedMessages mapping. See: here

[tynes]

Ideally we can dedupe the tests in the future but fine for now

[tynes]

This looks good to me, includes test coverage and is a simple implementation

[mergify]

This PR has been added to the merge queue, and will be merged soon.

[mergify]

This PR is next in line to be merged, and will be merged as soon as checks pass.