Skip to content

feat(ctb): Fail relayed XDM messages on reentry#5444

Merged
OptimismBot merged 2 commits intodevelopfrom
jm/nonreentrant-with-replays
Apr 19, 2023
Merged

feat(ctb): Fail relayed XDM messages on reentry#5444
OptimismBot merged 2 commits intodevelopfrom
jm/nonreentrant-with-replays

Conversation

@maurelian
Copy link
Contributor

@maurelian maurelian commented Apr 13, 2023

Description

This PR is an alternative to #5440.

The solution implementation as well as fuzz tests are included in this PR. Invariant testing will be split out to properly iterate and introduce invariant testing.

The sherlock-identified issue in the CrossDomainMessager whereby relayMessage naively reset the xDomainMsgSender after a call. The issue here was that a nested call would reset this value to the Constants.DEFAULT_L2_SENDER which deviates from the expected behavior that the xDomainMsgSender is set to the _sender for the entirety of the subcall.

As opposed to the minimal solution outlined in #5440, this change sets a message as failed upon re-entry to the relayMessage function by checking the value of xDomainMsgSender.

This check is a simple addition of the following

+ // Another message is being relayed, so we just store this for future replay.
+ if (xDomainMsgSender != Constants.DEFAULT_L2_SENDER) {
+     failedMessages[versionedHash] = true;
+     return;
+ }

We then also remove the reentrancyLocks from storage and don't have to worry about re-entrency here since any subcall through the SafeCall lib to the XDM relayMessage will be caught and recorded as failed by the aforementioned xDomainMsgSender check.

Benefits of this solution

  • Removes the reentrancyLocks saving SSTOREs and SLOADs as well as removing the storage mapping altogether.
  • Removes more complexity and proverbially "bites the bullet" now to deal with re-entry.

Potential Solution Cons

  • Deviates from the suggested minimal-solution from multiple sherlock finding reports.
  • Changes state and the codepaths from what has been audited.

Metadata

Fixes CLI-3834

@changeset-bot
Copy link

changeset-bot bot commented Apr 13, 2023

⚠️ No Changeset found

Latest commit: aae9e54

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@netlify
Copy link

netlify bot commented Apr 13, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit aae9e54
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/643f3276c478b6000884469a

@refcell refcell self-assigned this Apr 14, 2023
@tynes
Copy link
Contributor

tynes commented Apr 17, 2023

This definitely seems quite sane, just be sure to squash commits when ready for review

@tynes
Copy link
Contributor

tynes commented Apr 17, 2023

I'm trying to build the bindings locally to see if i get a diff

@refcell refcell marked this pull request as ready for review April 17, 2023 23:02
@refcell refcell requested a review from a team as a code owner April 17, 2023 23:02
@refcell refcell requested a review from tynes April 17, 2023 23:02
@refcell refcell changed the title feat(ctb): Immediately fail any message upon reentry feat(ctb): Fail relayed XDM messages on reentry Apr 17, 2023
@refcell refcell force-pushed the jm/nonreentrant-with-replays branch from e963bfa to b9ac9ef Compare April 18, 2023 17:06
@mergify
Copy link
Contributor

mergify bot commented Apr 18, 2023

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

@mergify mergify bot added conflict and removed conflict labels Apr 18, 2023
@refcell refcell requested a review from tynes April 18, 2023 18:13
@refcell refcell force-pushed the jm/nonreentrant-with-replays branch from a4440a2 to 7ff3c96 Compare April 18, 2023 18:36
Copy link
Contributor Author

@maurelian maurelian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unable to approve since I opened the PR, but LGTM other than one nit in a test file.

@refcell refcell force-pushed the jm/nonreentrant-with-replays branch from 77bbc37 to 74586d6 Compare April 18, 2023 20:53
@mergify
Copy link
Contributor

mergify bot commented Apr 19, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Apr 19, 2023

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

@OptimismBot OptimismBot merged commit a272058 into develop Apr 19, 2023
@OptimismBot OptimismBot deleted the jm/nonreentrant-with-replays branch April 19, 2023 00:30
@mergify mergify bot removed the on-merge-train label Apr 19, 2023
@github-actions github-actions bot mentioned this pull request Jun 5, 2023
seolaoh added a commit to kroma-network/kroma that referenced this pull request Aug 9, 2023
`CrossDomainMessager` whereby `relayMessage` naively reset the
`xDomainMsgSender` after a call. The issue here was that a nested call would
reset this value to the `Constants.DEFAULT_L2_SENDER` which deviates from the
expected behavior that the `xDomainMsgSender` is set to the `_sender` for the
entirety of the subcall.
This change sets a message as failed upon re-entry to the `relayMessage`
function by checking the value of `xDomainMsgSender`. We then also remove the
`reentrancyLocks` from storage and don't have to worry about re-entrency here
since any subcall through the `SafeCall` lib to the XDM `relayMessage` will be
caught and recorded as failed by the aforementioned `xDomainMsgSender` check.
Also removes the revert in `relayMessage` when the minimum gas threshold is not
met.

See:
  - ethereum-optimism/optimism#5475
  - ethereum-optimism/optimism#5444
  - ethereum-optimism/optimism#5493
  - ethereum-optimism/optimism#5508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants