Skip to content

make Bridge pausable #86

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

Closed
wants to merge 4 commits into from
Closed

make Bridge pausable #86

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37af906
make Bridge pausable
DZGoldman Oct 25, 2023
02b38ef
update bridge storage file
DZGoldman Oct 25, 2023
b258b7f
Merge branch 'develop' into make-bridge-pausable
gzeoneth Oct 27, 2023
abc41de
Merge remote-tracking branch 'origin/develop' into make-bridge-pausable
gzeoneth Oct 30, 2023
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
41 changes: 35 additions & 6 deletions src/bridge/AbsBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import {
NotSequencerInbox,
NotOutbox,
InvalidOutboxSet,
BadSequencerMessageNumber
BadSequencerMessageNumber,
Paused,
Unpaused
} from "../libraries/Error.sol";
import "./IBridge.sol";
import "./Messages.sol";
Expand Down Expand Up @@ -57,6 +59,8 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {

address internal constant EMPTY_ACTIVEOUTBOX = address(type(uint160).max);

bool internal _paused;

modifier onlyRollupOrOwner() {
if (msg.sender != address(rollup)) {
address rollupOwner = rollup.owner();
Expand All @@ -66,6 +70,18 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {
}
_;
}
modifier whenNotPaused() {
Copy link
Collaborator

@yahgwai yahgwai Oct 26, 2023

Choose a reason for hiding this comment

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

I think it would be nice to have more granular pauses available in the bridge. Eg:

  • pause withdrawal / outbox
  • pause inbox / delayed messages (we already have this in the inbox contract so maybe we could remove it there and put it here instead)
  • pause seq inbox messages

Copy link
Member

@gzeoneth gzeoneth Nov 2, 2023

Choose a reason for hiding this comment

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

As also mentioned here #74 (review)
we can use OZ AccessControl by having a BridgeStorage base contract that inherit the current storage layout so it doesn't get shifted downward.

if (paused()) {
revert Paused();
}
_;
}
modifier whenPaused() {
if (!paused()) {
revert Unpaused();
}
_;
}

/// @notice Allows the rollup owner to set another rollup address
function updateRollupAddress(IOwnable _rollup) external onlyRollupOrOwner {
Expand Down Expand Up @@ -105,6 +121,7 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {
)
external
onlySequencerInbox
whenNotPaused
returns (
uint256 seqMessageIndex,
bytes32 beforeAcc,
Expand Down Expand Up @@ -135,6 +152,7 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {
function submitBatchSpendingReport(address sender, bytes32 messageDataHash)
external
onlySequencerInbox
whenNotPaused
returns (uint256)
{
return
Expand Down Expand Up @@ -211,7 +229,7 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {
address to,
uint256 value,
bytes calldata data
) external returns (bool success, bytes memory returnData) {
) external whenNotPaused 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 Expand Up @@ -283,9 +301,6 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {
return sequencerInboxAccs.length;
}

/// @dev For the classic -> nitro migration. TODO: remove post-migration.
function acceptFundsFromOldBridge() external payable {}

/// @dev transfer funds provided to pay for crosschain msg
function _transferFunds(uint256 amount) internal virtual;

Expand All @@ -299,10 +314,24 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge {
/// used in ArbOs to calculate the submission fee for retryable ticket
function _baseFeeToReport() internal view virtual returns (uint256);

function pause() external onlyRollupOrOwner whenNotPaused {
_paused = true;
emit BridgePaused(msg.sender);
}

function unpause() external onlyRollupOrOwner whenPaused {
_paused = false;
emit BridgeUnpaused(msg.sender);
}

function paused() public view returns (bool) {
return _paused;
}

/**
* @dev This empty reserved space is put in place to allow future versions to add new
* variables without shifting down storage in the inheritance chain.
* See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
*/
uint256[40] private __gap;
uint256[39] private __gap;
}
8 changes: 8 additions & 0 deletions src/bridge/IBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ interface IBridge {

event RollupUpdated(address rollup);

event BridgePaused(address account);

event BridgeUnpaused(address account);

function allowedDelayedInboxList(uint256) external returns (address);

function allowedOutboxList(uint256) external returns (address);
Expand Down Expand Up @@ -101,4 +105,8 @@ interface IBridge {
function setOutbox(address inbox, bool enabled) external;

function updateRollupAddress(IOwnable _rollup) external;

function pause() external;

function unpause() external;
}
3 changes: 3 additions & 0 deletions src/libraries/Error.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ error AlreadyUnpaused();
/// @dev The contract is paused
error Paused();

/// @dev The contract is unpaused
error Unpaused();

/// @dev msg.value sent to the inbox isn't high enough
error InsufficientValue(uint256 expected, uint256 actual);

Expand Down
6 changes: 4 additions & 2 deletions src/mocks/BridgeStub.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,11 @@ contract BridgeStub is IBridge, IEthBridge {
revert("NOT_IMPLEMENTED");
}

function acceptFundsFromOldBridge() external payable {}

function initialize(IOwnable) external pure {
revert("NOT_IMPLEMENTED");
}

function pause() external {}

function unpause() external {}
}
4 changes: 3 additions & 1 deletion src/test-helpers/BridgeTester.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,5 +240,7 @@ contract BridgeTester is Initializable, DelegateCallAware, IBridge, IEthBridge {

receive() external payable {}

function acceptFundsFromOldBridge() external payable {}
function pause() external {}

function unpause() external {}
}
4 changes: 2 additions & 2 deletions test/storage/Bridge.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ rankdir=LR
color=black
arrowhead=open
node [shape=record, style=filled, fillcolor=gray95 fontname="Courier New"]
8 [label="Bridge \<\<Contract\>\>\n | {{ slot| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11-50 } | { type: \<inherited contract\>.variable (bytes) | { unallocated (30) | bool: Initializable._initializing (1) | bool: Initializable._initialized (1) } | { <5> mapping\(address=\>InOutInfo\): AbsBridge.allowedDelayedInboxesMap (32) } | { <8> mapping\(address=\>InOutInfo\): AbsBridge.allowedOutboxesMap (32) } | { <10> address[]: AbsBridge.allowedDelayedInboxList (32) } | { <12> address[]: AbsBridge.allowedOutboxList (32) } | { unallocated (12) | address: AbsBridge._activeOutbox (20) } | { <15> bytes32[]: AbsBridge.delayedInboxAccs (32) } | { <17> bytes32[]: AbsBridge.sequencerInboxAccs (32) } | { unallocated (12) | IOwnable: AbsBridge.rollup (20) } | { unallocated (12) | address: AbsBridge.sequencerInbox (20) } | { uint256: AbsBridge.sequencerReportedSubMessageCount (32) } | { <61> uint256[40]: AbsBridge.__gap (1280) }}}"]
8 [label="Bridge \<\<Contract\>\>\n | {{ slot| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12-50 } | { type: \<inherited contract\>.variable (bytes) | { unallocated (30) | bool: Initializable._initializing (1) | bool: Initializable._initialized (1) } | { <5> mapping\(address=\>InOutInfo\): AbsBridge.allowedDelayedInboxesMap (32) } | { <8> mapping\(address=\>InOutInfo\): AbsBridge.allowedOutboxesMap (32) } | { <10> address[]: AbsBridge.allowedDelayedInboxList (32) } | { <12> address[]: AbsBridge.allowedOutboxList (32) } | { unallocated (12) | address: AbsBridge._activeOutbox (20) } | { <15> bytes32[]: AbsBridge.delayedInboxAccs (32) } | { <17> bytes32[]: AbsBridge.sequencerInboxAccs (32) } | { unallocated (12) | IOwnable: AbsBridge.rollup (20) } | { unallocated (12) | address: AbsBridge.sequencerInbox (20) } | { uint256: AbsBridge.sequencerReportedSubMessageCount (32) } | { unallocated (31) | bool: AbsBridge._paused (1) } | { <61> uint256[39]: AbsBridge.__gap (1248) }}}"]

1 [label="InOutInfo \<\<Struct\>\>\n | {{ slot| 0 | 1 } | { type: variable (bytes) | { uint256: index (32) } | { unallocated (31) | bool: allowed (1) }}}"]

Expand All @@ -18,7 +18,7 @@ node [shape=record, style=filled, fillcolor=gray95 fontname="Courier New"]

6 [label="bytes32[]: sequencerInboxAccs \<\<Array\>\>\n0x995663702627f3d8fc4237c51a46b303536bb17f3f65e07c08ad05fecbf4d88e | {{ slot| 0 } | { type: variable (bytes) | { bytes32 (32) }}}"]

7 [label="uint256[40]: __gap \<\<Array\>\>\n | {{ slot| 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24 | 25 | 26 | 27 | 28 | 29 | 30 | 31 | 32 | 33 | 34 | 35 | 36 | 37 | 38 | 39 | 40 | 41 | 42 | 43 | 44 | 45 | 46 | 47 | 48 | 49 | 50 } | { type: variable (bytes) | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) }}}"]
7 [label="uint256[39]: __gap \<\<Array\>\>\n | {{ slot| 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24 | 25 | 26 | 27 | 28 | 29 | 30 | 31 | 32 | 33 | 34 | 35 | 36 | 37 | 38 | 39 | 40 | 41 | 42 | 43 | 44 | 45 | 46 | 47 | 48 | 49 | 50 } | { type: variable (bytes) | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) } | { uint256 (32) }}}"]

8:5 -> 1
8:8 -> 2
Expand Down