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: bridge access control #94

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

feat: bridge access control #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7772481
feat: add AccessControlUpgradeable to Bridge
gzeoneth Nov 2, 2023
b8cd977
format: fix and move struct to ibridge
gzeoneth Nov 2, 2023
8103c3a
fix: natspec
gzeoneth Nov 2, 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
53 changes: 30 additions & 23 deletions src/bridge/AbsBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -22,22 +23,9 @@ 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;

struct InOutInfo {
uint256 index;
bool allowed;
}

mapping(address => InOutInfo) private allowedDelayedInboxesMap;
mapping(address => InOutInfo) private allowedOutboxesMap;
abstract contract AbsBridgeStorage is IBridge {
mapping(address => InOutInfo) internal allowedDelayedInboxesMap;
mapping(address => InOutInfo) internal allowedOutboxesMap;

address[] public allowedDelayedInboxList;
address[] public allowedOutboxList;
Expand All @@ -55,6 +43,32 @@ 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;

/// @inheritdoc IBridge
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() {
Expand Down Expand Up @@ -298,11 +312,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;
}
3 changes: 0 additions & 3 deletions src/bridge/ERC20Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_);
Expand Down
16 changes: 16 additions & 0 deletions src/bridge/IBridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -34,6 +39,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);
Expand Down
10 changes: 0 additions & 10 deletions src/bridge/IERC20Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 2 additions & 5 deletions src/mocks/BridgeStub.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -32,6 +27,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);
Expand Down
7 changes: 2 additions & 5 deletions src/test-helpers/BridgeTester.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -46,6 +41,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();
Expand Down
35 changes: 20 additions & 15 deletions test/storage/Bridge
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 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 |
| 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 |
36 changes: 20 additions & 16 deletions test/storage/ERC20Bridge
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 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 |
| 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 |