fix(contracts-bedrock): xDomainMsgSender Reset on Nested/Re-entered Calls

[refcell]

Description

This test introduces 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.

The minimal solution here is to cache the xDomainMsgSender. ie

+ address cached = xDomainMsgSender;
xDomainMsgSender = _sender;
bool success = SafeCall.call(_target, gasleft() - RELAY_GAS_BUFFER, _value, _message);
- xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
+ xDomainMsgSender = cached;

@clabby created a neat diagram to demonstrate the constructed stack:

To break this down further, we can demonstrate a scenario where a victim contract may be exploited by using the xDomainMsgSender.

Suppose a relayMessage targets the function victim() on external contract Victim. This looks like:

contract Victim {
  function victim() external {
       exploiter.exploit();
       sendEther(xdm.xDomainMessageSender());
  }
}

The exploiter is another external contract that unfortunately re-enters the XDM relayMessage function, reseting the xDomainMsgSender (returned by xdm.xDomainMessageSender()). This can simply be:

contract Exploiter {
  function exploit() external {
       // Prior to this call, the `xDomainMsgSender` is set to the victim's `_sender`.
       xdm.relayMessage(...);
       // After this call, the xDomainMessageSender() function will now return the reset `Constants.DEFAULT_L2_SENDER`
  }
}

So, when the exploit() function bubble back up in the victim() function, the value returned by xdm.xDomainMessageSender() will not be the expected _sender, but instead reset to the Constants.DEFAULT_L2_SENDER. This will result in the Victim sending ether to the unintended address.

You can now image if any external contract called by the relayMessage calls external logic and relies on the value of the xDomainMsgSender returned by the xDomainMessageSender() function, it would not be the intended address.

In the implemented solution, this issue is fixed since inside the exploit() function, the previous xDomainMsgSender is used as opposed to the hardcoded Constants.DEFAULT_L2_SENDER so the victim() function will get the expected _sender from the xdm.xDomainMessageSender() call.

Metadata

Fixes CLI-3834

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7d1ee35

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]

✅ Deploy Preview for opstack-docs canceled.

Name	Link
🔨 Latest commit	7d1ee35
🔍 Latest deploy log	https://app.netlify.com/sites/opstack-docs/deploys/64382973f2bf010008c70847

[refcell]

Closing since solution implemented in #5444 is preferred.