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.

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: stop messaging #7

Merged
merged 10 commits into from
May 8, 2024
36 changes: 36 additions & 0 deletions src/contracts/BaseOpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol';
import {ICrossDomainMessenger} from 'interfaces/external/ICrossDomainMessenger.sol';

abstract contract BaseOpUSDCBridgeAdapter is Ownable, IOpUSDCBridgeAdapter {
using SafeERC20 for IERC20;
Expand All @@ -18,6 +19,9 @@ abstract contract BaseOpUSDCBridgeAdapter is Ownable, IOpUSDCBridgeAdapter {
/// @inheritdoc IOpUSDCBridgeAdapter
address public linkedAdapter;

/// @inheritdoc IOpUSDCBridgeAdapter
bool public isMessagingDisabled;

modifier linkedAdapterMustBeInitialized() {
if (linkedAdapter == address(0)) revert IOpUSDCBridgeAdapter_LinkedAdapterNotSet();
_;
Expand Down Expand Up @@ -57,4 +61,36 @@ abstract contract BaseOpUSDCBridgeAdapter is Ownable, IOpUSDCBridgeAdapter {
* @param _amount The amount of tokens to mint
*/
function receiveMessage(address _user, uint256 _amount) external virtual;

/**
* @notice Send a message to the linked adapter to call receiveStopMessaging() and stop outgoing messages.
* @dev Only callable by the owner of the adapter
excaliborr marked this conversation as resolved.
Show resolved Hide resolved
*/
function stopMessaging(uint32 _minGasLimit) external virtual onlyOwner linkedAdapterMustBeInitialized {
excaliborr marked this conversation as resolved.
Show resolved Hide resolved
isMessagingDisabled = true;
ICrossDomainMessenger(MESSENGER).sendMessage(
linkedAdapter, abi.encodeWithSignature('receiveStopMessaging()'), _minGasLimit
);
emit MessagingStopped();
}

/**
* @notice Receive the stop messaging message from the linked adapter and stop outgoing messages
*/
function receiveStopMessaging() external virtual linkedAdapterMustBeInitialized {
// Ensure the message is coming from the linked adapter
_checkCrossDomainMsgSender(linkedAdapter);
isMessagingDisabled = true;
emit MessagingStopped();
}

/**
* @notice Check if the sender is the cross domain messenger and the expected sender
* @param _expectedSender The expected sender of the message
*/
function _checkCrossDomainMsgSender(address _expectedSender) internal view {
if (msg.sender != MESSENGER || ICrossDomainMessenger(MESSENGER).xDomainMessageSender() != _expectedSender) {
Copy link
Contributor

@excaliborr excaliborr May 8, 2024

Choose a reason for hiding this comment

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

Since its just one line of code I dont think its worth it to make it an internal function as it adds extra JUMP opcodes making it more expensive, lets just put the if statement in the receiveStopMessaging

revert IOpUSDCBridgeAdapter_NotLinkedAdapter();
}
}
}
10 changes: 3 additions & 7 deletions src/contracts/L1OpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ contract L1OpUSDCBridgeAdapter is BaseOpUSDCBridgeAdapter {
* @param _minGasLimit Minimum gas limit that the message can be executed with
*/
function send(uint256 _amount, uint32 _minGasLimit) external override linkedAdapterMustBeInitialized {
// Ensure the linked adapter is set
if (linkedAdapter == address(0)) revert IOpUSDCBridgeAdapter_LinkedAdapterNotSet();
// Ensure messaging is enabled
if (isMessagingDisabled) revert IOpUSDCBridgeAdapter_MessagingDisabled();

// Transfer the tokens to the contract
IERC20(USDC).transferFrom(msg.sender, address(this), _amount);
Expand All @@ -40,12 +40,8 @@ contract L1OpUSDCBridgeAdapter is BaseOpUSDCBridgeAdapter {
* @param _amount The amount of tokens to mint
*/
function receiveMessage(address _user, uint256 _amount) external override linkedAdapterMustBeInitialized {
// TODO: Add logic to check that messaging wasnt stopped

// Ensure the message is coming from the linked adapter
if (msg.sender != MESSENGER || ICrossDomainMessenger(MESSENGER).xDomainMessageSender() != linkedAdapter) {
revert IOpUSDCBridgeAdapter_NotLinkedAdapter();
}
_checkCrossDomainMsgSender(linkedAdapter);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here


// Transfer the tokens to the user
IERC20(USDC).transfer(_user, _amount);
Expand Down
31 changes: 29 additions & 2 deletions src/interfaces/IOpUSDCBridgeAdapter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
*/
event LinkedAdapterSet(address _linkedAdapter);

/**
* @notice Emitted when messaging is stopped
*/
event MessagingStopped();

/**
* @notice Emitted when a message is sent to the linked adapter
* @param _user The user that sent the message
Expand All @@ -31,6 +36,11 @@
ERRORS
//////////////////////////////////////////////////////////////*/

/**
* @notice Error when messaging is disabled
*/
error IOpUSDCBridgeAdapter_MessagingDisabled();

/**
* @notice Error when the caller is not the linked adapter
*/
Expand All @@ -47,15 +57,15 @@

/**
* @notice Fetches address of the USDC token
* @return _USDC Address of the USDC token
* @return _usdc Address of the USDC token
*/
function USDC() external view returns (address _USDC);
function USDC() external view returns (address _usdc);

Check warning on line 62 in src/interfaces/IOpUSDCBridgeAdapter.sol

View workflow job for this annotation

GitHub Actions / Lint Commit Messages

Function name must be in mixedCase

/**
* @notice Fetches address of the CrossDomainMessenger to send messages to L1 <-> L2
* @return _messenger Address of the messenger
*/
function MESSENGER() external view returns (address _messenger);

Check warning on line 68 in src/interfaces/IOpUSDCBridgeAdapter.sol

View workflow job for this annotation

GitHub Actions / Lint Commit Messages

Function name must be in mixedCase

/**
* @notice Fetches address of the linked adapter on L2 to send messages to and receive from
Expand All @@ -63,6 +73,12 @@
*/
function linkedAdapter() external view returns (address _linkedAdapter);

/**
* @notice Fetches whether messaging is disabled
* @return _isMessagingDisabled Whether messaging is disabled
*/
function isMessagingDisabled() external view returns (bool _isMessagingDisabled);

/*///////////////////////////////////////////////////////////////
LOGIC
//////////////////////////////////////////////////////////////*/
Expand All @@ -72,7 +88,7 @@
* @dev Only the owner can call this function
* @param _linkedAdapter The address of the linked adapter
*/
function setLinkedAdapter(address _linkedAdapter) external;

Check warning on line 91 in src/interfaces/IOpUSDCBridgeAdapter.sol

View workflow job for this annotation

GitHub Actions / Lint Commit Messages

Function order is incorrect, external function can not go after external view function (line 80)

/**
* @notice Send the message to the linked adapter to mint the bridged representation on the linked chain
Expand All @@ -88,4 +104,15 @@
* @param _amount The amount of tokens to mint
*/
function receiveMessage(address _user, uint256 _amount) external;

/**
* @notice Send a message to the linked adapter to call receiveStopMessaging() and stop outgoing messages.
* @dev Only callable by the owner of the adapter.
excaliborr marked this conversation as resolved.
Show resolved Hide resolved
*/
function stopMessaging(uint32 _minGasLimit) external;

/**
* @notice Receive the stop messaging message from the linked adapter and stop outgoing messages
*/
function receiveStopMessaging() external;
}
55 changes: 53 additions & 2 deletions test/unit/BaseOpUSDCBridgeAdapter.t.sol
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.25;

import {BaseOpUSDCBridgeAdapter} from 'contracts/BaseOpUSDCBridgeAdapter.sol';
import {Test} from 'forge-std/Test.sol';
import {BaseOpUSDCBridgeAdapter, IOpUSDCBridgeAdapter} from 'contracts/BaseOpUSDCBridgeAdapter.sol';
import {StdStorage, Test, stdStorage} from 'forge-std/Test.sol';

contract TestOpUSDCBridgeAdapter is BaseOpUSDCBridgeAdapter {
constructor(address _USDC, address _messenger) BaseOpUSDCBridgeAdapter(_USDC, _messenger) {}
Expand Down Expand Up @@ -52,3 +52,54 @@ contract UnitInitialization is Base {
adapter.setLinkedAdapter(_linkedAdapter);
}
}

contract UnitStopMessaging is Base {
using stdStorage for StdStorage;

event MessagingStopped();

function testStopMessaging() public {
address _linkedAdapter = makeAddr('linkedAdapter');
Copy link
Contributor

Choose a reason for hiding this comment

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

fuzz _linkedAdapter and _minGasLimit

bytes memory _messageData = abi.encodeWithSignature('receiveStopMessaging()');
uint32 _minGasLimit = 21_000;

stdstore.target(address(adapter)).sig('linkedAdapter()').checked_write(_linkedAdapter);
Copy link
Contributor

Choose a reason for hiding this comment

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

honestly I think its much cleaner and simpler if we just use setLinkedAdapter instead of stdstore, they do the same thing and we already have setLinkedAdapter fully covered in other test cases

vm.mockCall(
_messenger,
abi.encodeWithSignature('sendMessage(address,bytes,uint32)', _linkedAdapter, _messageData, _minGasLimit),
abi.encode('')
);

vm.expectEmit(true, true, true, true);
Copy link
Contributor

@excaliborr excaliborr May 8, 2024

Choose a reason for hiding this comment

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

Lets split the expect emit into a different test case, and also use vm.expectCall for this one to ensure we are calling what we mock

emit MessagingStopped();

vm.prank(_owner);
adapter.stopMessaging(_minGasLimit);
assertEq(adapter.isMessagingDisabled(), true, 'Messaging should be disabled');
}

function testReceiveStopMessaging() public {
address _linkedAdapter = makeAddr('linkedAdapter');

stdstore.target(address(adapter)).sig('linkedAdapter()').checked_write(_linkedAdapter);
vm.mockCall(_messenger, abi.encodeWithSignature('xDomainMessageSender()'), abi.encode(_linkedAdapter));

vm.expectEmit(true, true, true, true);
emit MessagingStopped();

vm.prank(_messenger);
adapter.receiveStopMessaging();
assertEq(adapter.isMessagingDisabled(), true, 'Messaging should be disabled');
}

function testReceiveStopMessagingWrongSender() public {
address _linkedAdapter = makeAddr('linkedAdapter');
address _notLinkedAdapter = makeAddr('notLinkedAdapter');

stdstore.target(address(adapter)).sig('linkedAdapter()').checked_write(_linkedAdapter);
vm.prank(_notLinkedAdapter);
Copy link
Contributor

Choose a reason for hiding this comment

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

msg.sender is supposed to be MESSENGER I would change this to _notMessenger to be clear.

We also need one more test case for the case of when msg.sender is messenger but xDomainMessageSender() is not the linkedAdapter

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

vm.expectRevert(IOpUSDCBridgeAdapter.IOpUSDCBridgeAdapter_NotLinkedAdapter.selector);
adapter.receiveStopMessaging();
assertEq(adapter.isMessagingDisabled(), false, 'Messaging should not be disabled');
}
}
19 changes: 18 additions & 1 deletion test/unit/L1OpUSDCBridgeAdapter.t.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
pragma solidity ^0.8.25;

import {L1OpUSDCBridgeAdapter} from 'contracts/L1OpUSDCBridgeAdapter.sol';
import {Test} from 'forge-std/Test.sol';
import {StdStorage, Test, stdStorage} from 'forge-std/Test.sol';
Copy link
Contributor

Choose a reason for hiding this comment

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

StdStorage and stdStorage are now unused i believe, so we can remove them

import {IOpUSDCBridgeAdapter} from 'interfaces/IOpUSDCBridgeAdapter.sol';

abstract contract Base is Test {
Expand All @@ -22,6 +22,8 @@ abstract contract Base is Test {
}

contract UnitMessaging is Base {
using stdStorage for StdStorage;

function testSendMessage(uint256 _amount, uint32 _minGasLimit, address _linkedAdapter) external {
vm.assume(_linkedAdapter != address(0));

Expand Down Expand Up @@ -66,6 +68,21 @@ contract UnitMessaging is Base {
adapter.send(_amount, _minGasLimit);
}

function testSendMessageRevertsOnMessagingStopped() external {
// Mock storage
// link adapter and isMessagingDisabled are in the same storage slot
// _linkedAdapter = eeb02b2b8d37153666cba8021567b92a6e22de64
// isMessagingDisabled = true = 0x00000001
stdstore.target(address(adapter)).sig('linkedAdapter()').checked_write(
Copy link
Contributor

@excaliborr excaliborr May 8, 2024

Choose a reason for hiding this comment

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

this use of stdstore is creative, but its much simpler and easy to maintain if we just create a test setter function to manipulate storage, see how I have a TestOPUSDCBridgeAdapter that extends the real one, add a setter function to make isMessageDisabled true in the test contract.

This also makes it much easier to maintain if we ever need to change the storage layout

bytes32(0x000000000000000000000001eeb02b2b8d37153666cba8021567b92a6e22de64)
);

// Execute
vm.prank(_user);
vm.expectRevert(IOpUSDCBridgeAdapter.IOpUSDCBridgeAdapter_MessagingDisabled.selector);
adapter.send(0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

fuzz the variables instead of 0,0

}

function testSendMessageEmitsEvent(uint256 _amount, uint32 _minGasLimit, address _linkedAdapter) external {
vm.assume(_linkedAdapter != address(0));

Expand Down
Loading