From 7772481d7baa1c5cdd022a4509856987eeb3ae98 Mon Sep 17 00:00:00 2001 From: gzeon Date: Thu, 2 Nov 2023 19:06:31 +0800 Subject: [PATCH 1/3] feat: add AccessControlUpgradeable to Bridge --- src/bridge/AbsBridge.sol | 41 +++++++++++++++++-------------- src/bridge/ERC20Bridge.sol | 3 --- src/bridge/IBridge.sol | 11 +++++++++ src/bridge/IERC20Bridge.sol | 10 -------- src/mocks/BridgeStub.sol | 2 ++ src/test-helpers/BridgeTester.sol | 2 ++ test/storage/Bridge | 35 +++++++++++++++----------- test/storage/ERC20Bridge | 36 +++++++++++++++------------ 8 files changed, 78 insertions(+), 62 deletions(-) diff --git a/src/bridge/AbsBridge.sol b/src/bridge/AbsBridge.sol index bafd56f95..1fceca473 100644 --- a/src/bridge/AbsBridge.sol +++ b/src/bridge/AbsBridge.sol @@ -6,6 +6,7 @@ pragma solidity ^0.8.4; import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import { NotContract, @@ -22,22 +23,14 @@ import "../libraries/DelegateCallAware.sol"; import {L1MessageType_batchPostingReport} from "../libraries/MessageTypes.sol"; -/** - * @title Staging ground for incoming and outgoing messages - * @notice Holds the inbox accumulator for sequenced and delayed messages. - * Since the escrow is held here, this contract also contains a list of allowed - * outboxes that can make calls from here and withdraw this escrow. - */ -abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge { - using AddressUpgradeable for address; - +abstract contract AbsBridgeStorage is IBridge { struct InOutInfo { uint256 index; bool allowed; } - mapping(address => InOutInfo) private allowedDelayedInboxesMap; - mapping(address => InOutInfo) private allowedOutboxesMap; + mapping(address => InOutInfo) internal allowedDelayedInboxesMap; + mapping(address => InOutInfo) internal allowedOutboxesMap; address[] public allowedDelayedInboxList; address[] public allowedOutboxList; @@ -55,6 +48,25 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge { uint256 public override sequencerReportedSubMessageCount; + /** + * @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; + + address public nativeToken; +} + +/** + * @title Staging ground for incoming and outgoing messages + * @notice Holds the inbox accumulator for sequenced and delayed messages. + * Since the escrow is held here, this contract also contains a list of allowed + * outboxes that can make calls from here and withdraw this escrow. + */ +abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge, AbsBridgeStorage, AccessControlUpgradeable { + using AddressUpgradeable for address; + address internal constant EMPTY_ACTIVEOUTBOX = address(type(uint160).max); modifier onlyRollupOrOwner() { @@ -298,11 +310,4 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge { /// @dev get base fee which is emitted in `MessageDelivered` event and then picked up and /// used in ArbOs to calculate the submission fee for retryable ticket function _baseFeeToReport() internal view virtual returns (uint256); - - /** - * @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; } diff --git a/src/bridge/ERC20Bridge.sol b/src/bridge/ERC20Bridge.sol index c67e1ba06..cb62d5bb5 100644 --- a/src/bridge/ERC20Bridge.sol +++ b/src/bridge/ERC20Bridge.sol @@ -24,9 +24,6 @@ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; contract ERC20Bridge is AbsBridge, IERC20Bridge { using SafeERC20 for IERC20; - /// @inheritdoc IERC20Bridge - address public nativeToken; - /// @inheritdoc IERC20Bridge function initialize(IOwnable rollup_, address nativeToken_) external initializer onlyDelegated { if (nativeToken_ == address(0)) revert InvalidTokenSet(nativeToken_); diff --git a/src/bridge/IBridge.sol b/src/bridge/IBridge.sol index 22388b4bd..0c1796013 100644 --- a/src/bridge/IBridge.sol +++ b/src/bridge/IBridge.sol @@ -34,6 +34,17 @@ interface IBridge { event RollupUpdated(address rollup); + /** + * @dev Token that is escrowed in bridge on L1 side and minted on L2 as native currency. + * When set to address(0), the bridge use the L1 native token (e.g. ETH) + * Fees are paid in this token. There are certain restrictions on the native token: + * - The token can't be rebasing or have a transfer fee + * - The token must only be transferrable via a call to the token address itself + * - The token must only be able to set allowance via a call to the token address itself + * - The token must not have a callback on transfer, and more generally a user must not be able to make a transfer to themselves revert + */ + function nativeToken() external view returns (address); + function allowedDelayedInboxList(uint256) external returns (address); function allowedOutboxList(uint256) external returns (address); diff --git a/src/bridge/IERC20Bridge.sol b/src/bridge/IERC20Bridge.sol index b2b4551db..878598b7c 100644 --- a/src/bridge/IERC20Bridge.sol +++ b/src/bridge/IERC20Bridge.sol @@ -9,16 +9,6 @@ import "./IOwnable.sol"; import "./IBridge.sol"; interface IERC20Bridge is IBridge { - /** - * @dev token that is escrowed in bridge on L1 side and minted on L2 as native currency. - * Fees are paid in this token. There are certain restrictions on the native token: - * - The token can't be rebasing or have a transfer fee - * - The token must only be transferrable via a call to the token address itself - * - The token must only be able to set allowance via a call to the token address itself - * - The token must not have a callback on transfer, and more generally a user must not be able to make a transfer to themselves revert - */ - function nativeToken() external view returns (address); - /** * @dev Enqueue a message in the delayed inbox accumulator. * These messages are later sequenced in the SequencerInbox, either diff --git a/src/mocks/BridgeStub.sol b/src/mocks/BridgeStub.sol index 2e2dee05e..b153ac5d2 100644 --- a/src/mocks/BridgeStub.sol +++ b/src/mocks/BridgeStub.sol @@ -32,6 +32,8 @@ contract BridgeStub is IBridge, IEthBridge { address public sequencerInbox; uint256 public override sequencerReportedSubMessageCount; + address public nativeToken; + function setSequencerInbox(address _sequencerInbox) external override { sequencerInbox = _sequencerInbox; emit SequencerInboxUpdated(_sequencerInbox); diff --git a/src/test-helpers/BridgeTester.sol b/src/test-helpers/BridgeTester.sol index 0bcbe00fc..70525c388 100644 --- a/src/test-helpers/BridgeTester.sol +++ b/src/test-helpers/BridgeTester.sol @@ -46,6 +46,8 @@ contract BridgeTester is Initializable, DelegateCallAware, IBridge, IEthBridge { IOwnable public rollup; address public sequencerInbox; + address public nativeToken; + modifier onlyRollupOrOwner() { if (msg.sender != address(rollup)) { address rollupOwner = rollup.owner(); diff --git a/test/storage/Bridge b/test/storage/Bridge index 9ed7f48a2..9fe406cea 100644 --- a/test/storage/Bridge +++ b/test/storage/Bridge @@ -1,15 +1,20 @@ -| Name | Type | Slot | Offset | Bytes | Contract | -|----------------------------------|------------------------------------------------|------|--------|-------|------------------------------| -| _initialized | bool | 0 | 0 | 1 | src/bridge/Bridge.sol:Bridge | -| _initializing | bool | 0 | 1 | 1 | src/bridge/Bridge.sol:Bridge | -| allowedDelayedInboxesMap | mapping(address => struct AbsBridge.InOutInfo) | 1 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| allowedOutboxesMap | mapping(address => struct AbsBridge.InOutInfo) | 2 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| allowedDelayedInboxList | address[] | 3 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| allowedOutboxList | address[] | 4 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| _activeOutbox | address | 5 | 0 | 20 | src/bridge/Bridge.sol:Bridge | -| delayedInboxAccs | bytes32[] | 6 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| sequencerInboxAccs | bytes32[] | 7 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| rollup | contract IOwnable | 8 | 0 | 20 | src/bridge/Bridge.sol:Bridge | -| sequencerInbox | address | 9 | 0 | 20 | src/bridge/Bridge.sol:Bridge | -| sequencerReportedSubMessageCount | uint256 | 10 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| __gap | uint256[40] | 11 | 0 | 1280 | src/bridge/Bridge.sol:Bridge | +| Name | Type | Slot | Offset | Bytes | Contract | +|----------------------------------|--------------------------------------------------------------|------|--------|-------|------------------------------| +| _initialized | bool | 0 | 0 | 1 | src/bridge/Bridge.sol:Bridge | +| _initializing | bool | 0 | 1 | 1 | src/bridge/Bridge.sol:Bridge | +| allowedDelayedInboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 1 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| allowedOutboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 2 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| allowedDelayedInboxList | address[] | 3 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| allowedOutboxList | address[] | 4 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| _activeOutbox | address | 5 | 0 | 20 | src/bridge/Bridge.sol:Bridge | +| delayedInboxAccs | bytes32[] | 6 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| sequencerInboxAccs | bytes32[] | 7 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| rollup | contract IOwnable | 8 | 0 | 20 | src/bridge/Bridge.sol:Bridge | +| sequencerInbox | address | 9 | 0 | 20 | src/bridge/Bridge.sol:Bridge | +| sequencerReportedSubMessageCount | uint256 | 10 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| __gap | uint256[40] | 11 | 0 | 1280 | src/bridge/Bridge.sol:Bridge | +| nativeToken | address | 51 | 0 | 20 | src/bridge/Bridge.sol:Bridge | +| __gap | uint256[50] | 52 | 0 | 1600 | src/bridge/Bridge.sol:Bridge | +| __gap | uint256[50] | 102 | 0 | 1600 | src/bridge/Bridge.sol:Bridge | +| _roles | mapping(bytes32 => struct AccessControlUpgradeable.RoleData) | 152 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| __gap | uint256[49] | 153 | 0 | 1568 | src/bridge/Bridge.sol:Bridge | diff --git a/test/storage/ERC20Bridge b/test/storage/ERC20Bridge index 1b0d838b4..5363da368 100644 --- a/test/storage/ERC20Bridge +++ b/test/storage/ERC20Bridge @@ -1,16 +1,20 @@ -| Name | Type | Slot | Offset | Bytes | Contract | -|----------------------------------|------------------------------------------------|------|--------|-------|----------------------------------------| -| _initialized | bool | 0 | 0 | 1 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| _initializing | bool | 0 | 1 | 1 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| allowedDelayedInboxesMap | mapping(address => struct AbsBridge.InOutInfo) | 1 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| allowedOutboxesMap | mapping(address => struct AbsBridge.InOutInfo) | 2 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| allowedDelayedInboxList | address[] | 3 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| allowedOutboxList | address[] | 4 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| _activeOutbox | address | 5 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| delayedInboxAccs | bytes32[] | 6 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| sequencerInboxAccs | bytes32[] | 7 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| rollup | contract IOwnable | 8 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| sequencerInbox | address | 9 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| sequencerReportedSubMessageCount | uint256 | 10 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| __gap | uint256[40] | 11 | 0 | 1280 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| nativeToken | address | 51 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| Name | Type | Slot | Offset | Bytes | Contract | +|----------------------------------|--------------------------------------------------------------|------|--------|-------|----------------------------------------| +| _initialized | bool | 0 | 0 | 1 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| _initializing | bool | 0 | 1 | 1 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| allowedDelayedInboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 1 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| allowedOutboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 2 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| allowedDelayedInboxList | address[] | 3 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| allowedOutboxList | address[] | 4 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| _activeOutbox | address | 5 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| delayedInboxAccs | bytes32[] | 6 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| sequencerInboxAccs | bytes32[] | 7 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| rollup | contract IOwnable | 8 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| sequencerInbox | address | 9 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| sequencerReportedSubMessageCount | uint256 | 10 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| __gap | uint256[40] | 11 | 0 | 1280 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| nativeToken | address | 51 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| __gap | uint256[50] | 52 | 0 | 1600 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| __gap | uint256[50] | 102 | 0 | 1600 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| _roles | mapping(bytes32 => struct AccessControlUpgradeable.RoleData) | 152 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| __gap | uint256[49] | 153 | 0 | 1568 | src/bridge/ERC20Bridge.sol:ERC20Bridge | From b8cd977696c186a444fb3d6c4d2db5775612e2f2 Mon Sep 17 00:00:00 2001 From: gzeon Date: Thu, 2 Nov 2023 19:13:56 +0800 Subject: [PATCH 2/3] format: fix and move struct to ibridge --- src/bridge/AbsBridge.sol | 13 +++++++------ src/bridge/IBridge.sol | 5 +++++ src/mocks/BridgeStub.sol | 5 ----- src/test-helpers/BridgeTester.sol | 5 ----- test/storage/Bridge | 4 ++-- test/storage/ERC20Bridge | 4 ++-- 6 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/bridge/AbsBridge.sol b/src/bridge/AbsBridge.sol index 1fceca473..1d8ce9e61 100644 --- a/src/bridge/AbsBridge.sol +++ b/src/bridge/AbsBridge.sol @@ -24,11 +24,6 @@ import "../libraries/DelegateCallAware.sol"; import {L1MessageType_batchPostingReport} from "../libraries/MessageTypes.sol"; abstract contract AbsBridgeStorage is IBridge { - struct InOutInfo { - uint256 index; - bool allowed; - } - mapping(address => InOutInfo) internal allowedDelayedInboxesMap; mapping(address => InOutInfo) internal allowedOutboxesMap; @@ -64,7 +59,13 @@ abstract contract AbsBridgeStorage is IBridge { * Since the escrow is held here, this contract also contains a list of allowed * outboxes that can make calls from here and withdraw this escrow. */ -abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge, AbsBridgeStorage, AccessControlUpgradeable { +abstract contract AbsBridge is + Initializable, + DelegateCallAware, + IBridge, + AbsBridgeStorage, + AccessControlUpgradeable +{ using AddressUpgradeable for address; address internal constant EMPTY_ACTIVEOUTBOX = address(type(uint160).max); diff --git a/src/bridge/IBridge.sol b/src/bridge/IBridge.sol index 0c1796013..0c91d6c5b 100644 --- a/src/bridge/IBridge.sol +++ b/src/bridge/IBridge.sol @@ -8,6 +8,11 @@ pragma solidity >=0.6.9 <0.9.0; import "./IOwnable.sol"; interface IBridge { + struct InOutInfo { + uint256 index; + bool allowed; + } + event MessageDelivered( uint256 indexed messageIndex, bytes32 indexed beforeInboxAcc, diff --git a/src/mocks/BridgeStub.sol b/src/mocks/BridgeStub.sol index b153ac5d2..05361b449 100644 --- a/src/mocks/BridgeStub.sol +++ b/src/mocks/BridgeStub.sol @@ -11,11 +11,6 @@ import "../bridge/IBridge.sol"; import "../bridge/IEthBridge.sol"; contract BridgeStub is IBridge, IEthBridge { - struct InOutInfo { - uint256 index; - bool allowed; - } - mapping(address => InOutInfo) private allowedDelayedInboxesMap; //mapping(address => InOutInfo) private allowedOutboxesMap; diff --git a/src/test-helpers/BridgeTester.sol b/src/test-helpers/BridgeTester.sol index 70525c388..2de8f0afa 100644 --- a/src/test-helpers/BridgeTester.sol +++ b/src/test-helpers/BridgeTester.sol @@ -30,11 +30,6 @@ import "../libraries/DelegateCallAware.sol"; contract BridgeTester is Initializable, DelegateCallAware, IBridge, IEthBridge { using AddressUpgradeable for address; - struct InOutInfo { - uint256 index; - bool allowed; - } - mapping(address => InOutInfo) private allowedInboxesMap; mapping(address => InOutInfo) private allowedOutboxesMap; diff --git a/test/storage/Bridge b/test/storage/Bridge index 9fe406cea..37dc4bf9a 100644 --- a/test/storage/Bridge +++ b/test/storage/Bridge @@ -2,8 +2,8 @@ |----------------------------------|--------------------------------------------------------------|------|--------|-------|------------------------------| | _initialized | bool | 0 | 0 | 1 | src/bridge/Bridge.sol:Bridge | | _initializing | bool | 0 | 1 | 1 | src/bridge/Bridge.sol:Bridge | -| allowedDelayedInboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 1 | 0 | 32 | src/bridge/Bridge.sol:Bridge | -| allowedOutboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 2 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| allowedDelayedInboxesMap | mapping(address => struct IBridge.InOutInfo) | 1 | 0 | 32 | src/bridge/Bridge.sol:Bridge | +| allowedOutboxesMap | mapping(address => struct IBridge.InOutInfo) | 2 | 0 | 32 | src/bridge/Bridge.sol:Bridge | | allowedDelayedInboxList | address[] | 3 | 0 | 32 | src/bridge/Bridge.sol:Bridge | | allowedOutboxList | address[] | 4 | 0 | 32 | src/bridge/Bridge.sol:Bridge | | _activeOutbox | address | 5 | 0 | 20 | src/bridge/Bridge.sol:Bridge | diff --git a/test/storage/ERC20Bridge b/test/storage/ERC20Bridge index 5363da368..1effcfbbf 100644 --- a/test/storage/ERC20Bridge +++ b/test/storage/ERC20Bridge @@ -2,8 +2,8 @@ |----------------------------------|--------------------------------------------------------------|------|--------|-------|----------------------------------------| | _initialized | bool | 0 | 0 | 1 | src/bridge/ERC20Bridge.sol:ERC20Bridge | | _initializing | bool | 0 | 1 | 1 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| allowedDelayedInboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 1 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | -| allowedOutboxesMap | mapping(address => struct AbsBridgeStorage.InOutInfo) | 2 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| allowedDelayedInboxesMap | mapping(address => struct IBridge.InOutInfo) | 1 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | +| allowedOutboxesMap | mapping(address => struct IBridge.InOutInfo) | 2 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | | allowedDelayedInboxList | address[] | 3 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | | allowedOutboxList | address[] | 4 | 0 | 32 | src/bridge/ERC20Bridge.sol:ERC20Bridge | | _activeOutbox | address | 5 | 0 | 20 | src/bridge/ERC20Bridge.sol:ERC20Bridge | From 8103c3a0d66b893461ea2a659dfac6957bfa4dab Mon Sep 17 00:00:00 2001 From: gzeon Date: Thu, 2 Nov 2023 19:16:21 +0800 Subject: [PATCH 3/3] fix: natspec --- src/bridge/AbsBridge.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bridge/AbsBridge.sol b/src/bridge/AbsBridge.sol index 1d8ce9e61..40f2b45cc 100644 --- a/src/bridge/AbsBridge.sol +++ b/src/bridge/AbsBridge.sol @@ -50,6 +50,7 @@ abstract contract AbsBridgeStorage is IBridge { */ uint256[40] private __gap; + /// @inheritdoc IBridge address public nativeToken; }