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

Bridge: add pausable outbox execution, inbox updating, and delayed msg enqueueing #107

Draft
wants to merge 2 commits into
base: bridge-access-control
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 14 additions & 13 deletions src/bridge/AbsBridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
import "./IBridge.sol";
import "./Messages.sol";
import "../libraries/DelegateCallAware.sol";
import "./BridgePausable.sol";

import {L1MessageType_batchPostingReport} from "../libraries/MessageTypes.sol";

Expand Down Expand Up @@ -65,7 +66,7 @@ abstract contract AbsBridge is
DelegateCallAware,
IBridge,
AbsBridgeStorage,
AccessControlUpgradeable
BridgePausable
{
using AddressUpgradeable for address;

Expand All @@ -81,6 +82,10 @@ abstract contract AbsBridge is
_;
}

function postUpgradeInit() public onlyDelegated onlyProxyOwner {
_grantAllPauseRolesTo(rollup.owner());
}

/// @notice Allows the rollup owner to set another rollup address
function updateRollupAddress(IOwnable _rollup) external onlyRollupOrOwner {
rollup = _rollup;
Expand Down Expand Up @@ -119,12 +124,8 @@ abstract contract AbsBridge is
)
external
onlySequencerInbox
returns (
uint256 seqMessageIndex,
bytes32 beforeAcc,
bytes32 delayedAcc,
bytes32 acc
)
whenSequencerInboxMsgsNotPaused
returns (uint256 seqMessageIndex, bytes32 beforeAcc, bytes32 delayedAcc, bytes32 acc)
{
if (
sequencerReportedSubMessageCount != prevMessageCount &&
Expand All @@ -146,11 +147,11 @@ abstract contract AbsBridge is
}

/// @inheritdoc IBridge
function submitBatchSpendingReport(address sender, bytes32 messageDataHash)
external
onlySequencerInbox
returns (uint256)
{

function submitBatchSpendingReport(
address sender,
bytes32 messageDataHash
) external onlySequencerInbox whenDelayedMessageEnqueueNotPaused returns (uint256) {
return
addMessageToDelayedAccumulator(
L1MessageType_batchPostingReport,
Expand Down Expand Up @@ -225,7 +226,7 @@ abstract contract AbsBridge is
address to,
uint256 value,
bytes calldata data
) external returns (bool success, bytes memory returnData) {
) external whenOutboxExecutionNotPaused returns (bool success, bytes memory returnData) {
if (!allowedOutboxes(msg.sender)) revert NotOutbox(msg.sender);
if (data.length > 0 && !to.isContract()) revert NotContract(to);
address prevOutbox = _activeOutbox;
Expand Down
17 changes: 8 additions & 9 deletions src/bridge/AbsInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
NotAllowedOrigin,
NotOrigin,
NotRollupOrOwner,
RetryableData
RetryableData,
Deprecated
} from "../libraries/Error.sol";
import "./IInboxBase.sol";
import "./ISequencerInbox.sol";
Expand Down Expand Up @@ -110,13 +111,13 @@ abstract contract AbsInbox is DelegateCallAware, PausableUpgradeable, IInboxBase
}

/// @inheritdoc IInboxBase
function pause() external onlyRollupOrOwner {
_pause();
function pause() external {
revert Deprecated();
}

/// @inheritdoc IInboxBase
function unpause() external onlyRollupOrOwner {
_unpause();
function unpause() external {
revert Deprecated();
}

/* solhint-disable func-name-mixedcase */
Expand All @@ -133,7 +134,6 @@ abstract contract AbsInbox is DelegateCallAware, PausableUpgradeable, IInboxBase
/// @inheritdoc IInboxBase
function sendL2MessageFromOrigin(bytes calldata messageData)
external
whenNotPaused
onlyAllowed
returns (uint256)
{
Expand All @@ -149,7 +149,6 @@ abstract contract AbsInbox is DelegateCallAware, PausableUpgradeable, IInboxBase
/// @inheritdoc IInboxBase
function sendL2Message(bytes calldata messageData)
external
whenNotPaused
onlyAllowed
returns (uint256)
{
Expand All @@ -165,7 +164,7 @@ abstract contract AbsInbox is DelegateCallAware, PausableUpgradeable, IInboxBase
address to,
uint256 value,
bytes calldata data
) external whenNotPaused onlyAllowed returns (uint256) {
) external onlyAllowed returns (uint256) {
// arbos will discard unsigned tx with gas limit too large
if (gasLimit > type(uint64).max) {
revert GasLimitTooLarge();
Expand Down Expand Up @@ -194,7 +193,7 @@ abstract contract AbsInbox is DelegateCallAware, PausableUpgradeable, IInboxBase
address to,
uint256 value,
bytes calldata data
) external whenNotPaused onlyAllowed returns (uint256) {
) external onlyAllowed returns (uint256) {
// arbos will discard unsigned tx with gas limit too large
if (gasLimit > type(uint64).max) {
revert GasLimitTooLarge();
Expand Down
3 changes: 2 additions & 1 deletion src/bridge/Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ contract Bridge is AbsBridge, IEthBridge {
function initialize(IOwnable rollup_) external initializer onlyDelegated {
_activeOutbox = EMPTY_ACTIVEOUTBOX;
rollup = rollup_;
_grantAllPauseRolesTo(rollup_.owner());
}

/// @inheritdoc IEthBridge
function enqueueDelayedMessage(
uint8 kind,
address sender,
bytes32 messageDataHash
) external payable returns (uint256) {
) external payable whenDelayedMessageEnqueueNotPaused returns (uint256) {
return _enqueueDelayedMessage(kind, sender, messageDataHash, msg.value);
}

Expand Down
133 changes: 133 additions & 0 deletions src/bridge/BridgePausable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// Copyright 2021-2022, Offchain Labs, Inc.
// For license information, see https://github.com/nitro/blob/master/LICENSE
// SPDX-License-Identifier: BUSL-1.1
import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
import {
OutboxExecutionPaused,
OutboxExecutionNotPaused,
SequencerInboxMsgsPaused,
SequencerInboxMsgsNotPaused,
DelayedMessagesEnqueuePaused,
DelayedMessagesEnqueueNotPaused
} from "../libraries/Error.sol";
pragma solidity ^0.8.4;

contract BridgePausable is AccessControlUpgradeable {
bool internal _outboxExecutionPaused;
bool internal _sequencerInboxMsgsPaused;
bool internal _delayedMessageEnqueuePaused;

bytes32 public constant PAUSE_OUTBOX_EXECUTION_ROLE = keccak256("PAUSE_OUTBOX_EXECUTION_ROLE");
bytes32 public constant UNPAUSE_OUTBOX_EXECUTION_ROLE =
keccak256("UNPAUSE_OUTBOX_EXECUTION_ROLE");

bytes32 public constant PAUSE_SEQUENCER_INBOX_MSGS_ROLE =
keccak256("PAUSE_SEQUENCER_INBOX_MSGS_ROLE");
bytes32 public constant UNPAUSE_SEQUENCER_INBOX_MSGS_ROLE =
keccak256("UNPAUSE_SEQUENCER_INBOX_MSGS_ROLE");

bytes32 public constant PAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE =
keccak256("PAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE");
bytes32 public constant UNPAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE =
keccak256("UNPAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE");

modifier whenOutboxExecutionNotPaused() {
if (_outboxExecutionPaused) {
revert OutboxExecutionPaused();
}
_;
}

modifier whenOutboxExecutionPaused() {
if (!_outboxExecutionPaused) {
revert OutboxExecutionNotPaused();
}
_;
}

modifier whenSequencerInboxMsgsNotPaused() {
if (_sequencerInboxMsgsPaused) {
revert SequencerInboxMsgsPaused();
}
_;
}

modifier whenSequencerInboxMsgsPaused() {
if (!_sequencerInboxMsgsPaused) {
revert SequencerInboxMsgsNotPaused();
}
_;
}

modifier whenDelayedMessageEnqueueNotPaused() {
if (_delayedMessageEnqueuePaused) {
revert DelayedMessagesEnqueuePaused();
}
_;
}

modifier whenDelayedMessageEnqueuePaused() {
if (!_delayedMessageEnqueuePaused) {
revert DelayedMessagesEnqueueNotPaused();
}
_;
}

function _grantAllPauseRolesTo(address owner) internal {
_grantRole(DEFAULT_ADMIN_ROLE, owner);
_grantRole(PAUSE_OUTBOX_EXECUTION_ROLE, owner);
_grantRole(UNPAUSE_OUTBOX_EXECUTION_ROLE, owner);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinking maybe the separate pause/unpause roles aren't necessary (and if we wanted to split up pause/unpause affordances we could do so via smart contract wallet)

_grantRole(PAUSE_SEQUENCER_INBOX_MSGS_ROLE, owner);
_grantRole(UNPAUSE_SEQUENCER_INBOX_MSGS_ROLE, owner);
_grantRole(PAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE, owner);
_grantRole(UNPAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE, owner);
}

function pauseOutboxExecution()
external
onlyRole(PAUSE_OUTBOX_EXECUTION_ROLE)
whenOutboxExecutionNotPaused
{
_outboxExecutionPaused = true;
}

function unpauseOutboxExecution()
external
onlyRole(UNPAUSE_OUTBOX_EXECUTION_ROLE)
whenOutboxExecutionPaused
{
_outboxExecutionPaused = false;
}

function pauseSequencerInboxMsgs()
external
onlyRole(PAUSE_SEQUENCER_INBOX_MSGS_ROLE)
whenSequencerInboxMsgsNotPaused
{
_sequencerInboxMsgsPaused = true;
}

function unpauseSequencerInboxMsgs()
external
onlyRole(UNPAUSE_SEQUENCER_INBOX_MSGS_ROLE)
whenSequencerInboxMsgsPaused
{
_sequencerInboxMsgsPaused = false;
}

function pauseDelayedMsgsEnque()
external
onlyRole(PAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE)
whenDelayedMessageEnqueueNotPaused
{
_delayedMessageEnqueuePaused = true;
}

function unpauseDelayedMsgsEnque()
external
onlyRole(UNPAUSE_DELAYED_MESSAGE_ENQUEUE_ROLE)
whenDelayedMessageEnqueuePaused
{
_delayedMessageEnqueuePaused = false;
}
}
3 changes: 2 additions & 1 deletion src/bridge/ERC20Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ contract ERC20Bridge is AbsBridge, IERC20Bridge {
nativeToken = nativeToken_;
_activeOutbox = EMPTY_ACTIVEOUTBOX;
rollup = rollup_;
_grantAllPauseRolesTo(rollup_.owner());
}

/// @inheritdoc IERC20Bridge
Expand All @@ -38,7 +39,7 @@ contract ERC20Bridge is AbsBridge, IERC20Bridge {
address sender,
bytes32 messageDataHash,
uint256 tokenFeeAmount
) external returns (uint256) {
) external whenDelayedMessageEnqueueNotPaused returns (uint256) {
return _enqueueDelayedMessage(kind, sender, messageDataHash, tokenFeeAmount);
}

Expand Down
6 changes: 3 additions & 3 deletions src/bridge/ERC20Inbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract ERC20Inbox is AbsInbox, IERC20Inbox {
}

/// @inheritdoc IERC20Inbox
function depositERC20(uint256 amount) public whenNotPaused onlyAllowed returns (uint256) {
function depositERC20(uint256 amount) public onlyAllowed returns (uint256) {
address dest = msg.sender;

// solhint-disable-next-line avoid-tx-origin
Expand Down Expand Up @@ -66,7 +66,7 @@ contract ERC20Inbox is AbsInbox, IERC20Inbox {
uint256 maxFeePerGas,
uint256 tokenTotalFeeAmount,
bytes calldata data
) external whenNotPaused onlyAllowed returns (uint256) {
) external onlyAllowed returns (uint256) {
return
_createRetryableTicket(
to,
Expand All @@ -92,7 +92,7 @@ contract ERC20Inbox is AbsInbox, IERC20Inbox {
uint256 maxFeePerGas,
uint256 tokenTotalFeeAmount,
bytes calldata data
) public whenNotPaused onlyAllowed returns (uint256) {
) public onlyAllowed returns (uint256) {
return
_unsafeCreateRetryableTicket(
to,
Expand Down
4 changes: 2 additions & 2 deletions src/bridge/IInboxBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ interface IInboxBase is IDelayedMessageProvider {

// ---------- onlyRollupOrOwner functions ----------

/// @notice pauses all inbox functionality
/// @notice deprecated in favor of pausing on Bridge
function pause() external;

/// @notice unpauses all inbox functionality
/// @notice deprecated in favor of pausing on Bridge
function unpause() external;

/// @notice add or remove users from allowList
Expand Down
Loading
Loading