Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(L2toL2CDM): revert on target call failure in relayMessage #12526

Merged
merged 1 commit into from
Oct 21, 2024
Merged

feat(L2toL2CDM): revert on target call failure in relayMessage #12526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/semver-lock.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@
"sourceCodeHash": "0xd08a2e6514dbd44e16aa312a1b27b2841a9eab5622cbd05a39c30f543fad673c"
},
"src/L2/L2ToL2CrossDomainMessenger.sol": {
"initCodeHash": "0xf760d814018281b36d9a6a0ab16a23348effb33cf0ab299e3022b59283e46160",
"sourceCodeHash": "0xe8d99e4702d90814089c4a80e259c891a95f6d4750c7220fc6b2672c26ef2700"
"initCodeHash": "0x6eb3718548d97b69c201a75c27a3a5a77400ebd5e0a9bdcc06e2cc28f9b4a689",
"sourceCodeHash": "0x4bb08a8a9d5de37d1fb2dd8a9bcc483305510c32508f0be2d54757e6e257aedb"
},
"src/L2/OptimismSuperchainERC20.sol": {
"initCodeHash": "0xd5c84e45746fd741d541a917ddc1cc0c7043c6b21d5c18040d4bc999d6a7b2db",
Expand Down
30 changes: 5 additions & 25 deletions packages/contracts-bedrock/snapshots/abi/L2ToL2CrossDomainMessenger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,31 +175,6 @@
"stateMutability": "view",
"type": "function"
},
{
"anonymous": false,
"inputs": [
{
"indexed": true,
"internalType": "uint256",
"name": "source",
"type": "uint256"
},
{
"indexed": true,
"internalType": "uint256",
"name": "messageNonce",
"type": "uint256"
},
{
"indexed": true,
"internalType": "bytes32",
"name": "messageHash",
"type": "bytes32"
}
],
"name": "FailedRelayedMessage",
"type": "event"
},
{
"anonymous": false,
"inputs": [
Expand Down Expand Up @@ -306,5 +281,10 @@
"inputs": [],
"name": "ReentrantCall",
"type": "error"
},
{
"inputs": [],
"name": "TargetCallFailed",
"type": "error"
}
]
23 changes: 10 additions & 13 deletions packages/contracts-bedrock/src/L2/L2ToL2CrossDomainMessenger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ error MessageAlreadyRelayed();
/// @notice Thrown when a reentrant call is detected.
error ReentrantCall();

/// @notice Thrown when a call to the target contract during message relay fails.
error TargetCallFailed();

/// @custom:proxied true
/// @custom:predeploy 0x4200000000000000000000000000000000000023
/// @title L2ToL2CrossDomainMessenger
Expand All @@ -64,8 +67,8 @@ contract L2ToL2CrossDomainMessenger is IL2ToL2CrossDomainMessenger, ISemver, Tra
uint16 public constant messageVersion = uint16(0);

/// @notice Semantic version.
/// @custom:semver 1.0.0-beta.8
string public constant version = "1.0.0-beta.8";
/// @custom:semver 1.0.0-beta.9
string public constant version = "1.0.0-beta.9";

/// @notice Mapping of message hashes to boolean receipt values. Note that a message will only be present in this
/// mapping if it has successfully been relayed on this chain, and can therefore not be relayed again.
Expand All @@ -92,12 +95,6 @@ contract L2ToL2CrossDomainMessenger is IL2ToL2CrossDomainMessenger, ISemver, Tra
/// @param messageHash Hash of the message that was relayed.
event RelayedMessage(uint256 indexed source, uint256 indexed messageNonce, bytes32 indexed messageHash);

/// @notice Emitted whenever a message fails to be relayed on this chain.
/// @param source Chain ID of the source chain.
/// @param messageNonce Nonce associated with the messsage sent
/// @param messageHash Hash of the message that failed to be relayed.
event FailedRelayedMessage(uint256 indexed source, uint256 indexed messageNonce, bytes32 indexed messageHash);

/// @notice Retrieves the sender of the current cross domain message. If not entered, reverts.
/// @return sender_ Address of the sender of the current cross domain message.
function crossDomainMessageSender() external view onlyEntered returns (address sender_) {
Expand Down Expand Up @@ -200,13 +197,13 @@ contract L2ToL2CrossDomainMessenger is IL2ToL2CrossDomainMessenger, ISemver, Tra

bool success = SafeCall.call(target, msg.value, message);

if (success) {
successfulMessages[messageHash] = true;
emit RelayedMessage(source, nonce, messageHash);
} else {
emit FailedRelayedMessage(source, nonce, messageHash);
if (!success) {
revert TargetCallFailed();
}

tremarkley marked this conversation as resolved.
Show resolved Hide resolved
successfulMessages[messageHash] = true;
emit RelayedMessage(source, nonce, messageHash);

_storeMessageMetadata(0, address(0));
}

Expand Down
19 changes: 6 additions & 13 deletions packages/contracts-bedrock/test/L2/L2ToL2CrossDomainMessenger.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import {
MessageTargetCrossL2Inbox,
MessageTargetL2ToL2CrossDomainMessenger,
MessageAlreadyRelayed,
ReentrantCall
ReentrantCall,
TargetCallFailed
} from "src/L2/L2ToL2CrossDomainMessenger.sol";

/// @title L2ToL2CrossDomainMessengerWithModifiableTransientStorage
Expand Down Expand Up @@ -402,12 +403,6 @@ contract L2ToL2CrossDomainMessengerTest is Test {
address target = address(this);
bytes memory message = abi.encodeWithSelector(this.mockTargetReentrant.selector, _source2, _nonce, _sender2);

// Look for correct emitted event
vm.expectEmit(Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER);
emit L2ToL2CrossDomainMessenger.FailedRelayedMessage(
_source1, _nonce, keccak256(abi.encode(block.chainid, _source1, _nonce, _sender1, target, message))
);

// Ensure the target contract is called with the correct parameters
vm.expectCall({ callee: target, msgValue: _value, data: message });

Expand All @@ -426,6 +421,8 @@ contract L2ToL2CrossDomainMessengerTest is Test {
returnData: ""
});

// Expect a revert with the TargetCallFailed selector
vm.expectRevert(TargetCallFailed.selector);
hoax(Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER, _value);
l2ToL2CrossDomainMessenger.relayMessage{ value: _value }(id, sentMessage);

Expand Down Expand Up @@ -673,12 +670,6 @@ contract L2ToL2CrossDomainMessengerTest is Test {
// Ensure that the target contract reverts
vm.mockCallRevert({ callee: _target, msgValue: _value, data: _message, revertData: abi.encode(false) });

// Look for correct emitted event
vm.expectEmit(Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER);
emit L2ToL2CrossDomainMessenger.FailedRelayedMessage(
_source, _nonce, keccak256(abi.encode(block.chainid, _source, _nonce, _sender, _target, _message))
);

ICrossL2Inbox.Identifier memory id =
ICrossL2Inbox.Identifier(Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER, _blockNum, _logIndex, _time, _source);
bytes memory sentMessage = abi.encodePacked(
Expand All @@ -693,6 +684,8 @@ contract L2ToL2CrossDomainMessengerTest is Test {
returnData: ""
});

// Expect a revert with the TargetCallFailed selector
vm.expectRevert(TargetCallFailed.selector);
hoax(Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER, _value);
l2ToL2CrossDomainMessenger.relayMessage{ value: _value }(id, sentMessage);
}
Expand Down