From a92a4dd861bf8aca1238f7955925a36cddf364f3 Mon Sep 17 00:00:00 2001 From: fredlacs <32464905+fredlacs@users.noreply.github.com> Date: Fri, 10 Jun 2022 17:23:38 +0100 Subject: [PATCH 1/4] feat: optional allowlist for addresses that can interact with the inbox --- contracts/src/bridge/Inbox.sol | 60 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 9 deletions(-) diff --git a/contracts/src/bridge/Inbox.sol b/contracts/src/bridge/Inbox.sol index acb66c9ac7..d9f4b075b0 100644 --- a/contracts/src/bridge/Inbox.sol +++ b/contracts/src/bridge/Inbox.sol @@ -32,6 +32,36 @@ import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol"; contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { IBridge public override bridge; + /// ------------------------------------ allow list start ------------------------------------ /// + + bool public allowListEnabled; + mapping(address => bool) public isAllowed; + + event AllowListAddressSet(address indexed user, bool val); + event AllowListEnabledUpdated(bool isEnabled); + + function setAllowList(address[] memory user, bool[] memory val) external onlyOwner { + require(user.length == val.length, "INVALID_INPUT"); + + for (uint256 i = 0; i < user.length; i++) { + isAllowed[user[i]] = val[i]; + emit AllowListAddressSet(user[i], val[i]); + } + } + + function setAllowListEnabled(bool _allowListEnabled) external onlyOwner { + require(_allowListEnabled != allowListEnabled, "ALREADY_SET"); + allowListEnabled = _allowListEnabled; + emit AllowListEnabledUpdated(_allowListEnabled); + } + + modifier onlyAllowed() { + require(isAllowed[msg.sender] || !allowListEnabled, "NOT_ALLOWED"); + _; + } + + /// ------------------------------------ allow list end ------------------------------------ /// + modifier onlyOwner() { // whoevever owns the Bridge, also owns the Inbox. this is usually the rollup contract address bridgeOwner = OwnableUpgradeable(address(bridge)).owner(); @@ -52,6 +82,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { function initialize(IBridge _bridge) external initializer onlyDelegated { if (address(bridge) != address(0)) revert AlreadyInit(); bridge = _bridge; + allowListEnabled = false; __Pausable_init(); } @@ -64,6 +95,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { sstore(i, 0) } } + allowListEnabled = false; bridge = _bridge; } @@ -75,6 +107,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { function sendL2MessageFromOrigin(bytes calldata messageData) external whenNotPaused + onlyAllowed returns (uint256) { // solhint-disable-next-line avoid-tx-origin @@ -95,6 +128,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { external override whenNotPaused + onlyAllowed returns (uint256) { return _deliverMessage(L2_MSG, msg.sender, messageData); @@ -106,7 +140,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { uint256 nonce, address to, bytes calldata data - ) external payable virtual override whenNotPaused returns (uint256) { + ) external payable virtual override whenNotPaused onlyAllowed returns (uint256) { return _deliverMessage( L1MessageType_L2FundedByL1, @@ -128,7 +162,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { uint256 maxFeePerGas, address to, bytes calldata data - ) external payable virtual override whenNotPaused returns (uint256) { + ) external payable virtual override whenNotPaused onlyAllowed returns (uint256) { return _deliverMessage( L1MessageType_L2FundedByL1, @@ -151,7 +185,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { address to, uint256 value, bytes calldata data - ) external virtual override whenNotPaused returns (uint256) { + ) external virtual override whenNotPaused onlyAllowed returns (uint256) { return _deliverMessage( L2_MSG, @@ -174,7 +208,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { address to, uint256 value, bytes calldata data - ) external virtual override whenNotPaused returns (uint256) { + ) external virtual override whenNotPaused onlyAllowed returns (uint256) { return _deliverMessage( L2_MSG, @@ -209,7 +243,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { /// @dev this does not trigger the fallback function when receiving in the L2 side. /// Look into retryable tickets if you are interested in this functionality. /// @dev this function should not be called inside contract constructors - function depositEth() public payable override whenNotPaused returns (uint256) { + function depositEth() public payable override whenNotPaused onlyAllowed returns (uint256) { address sender = msg.sender; // solhint-disable-next-line avoid-tx-origin @@ -232,7 +266,15 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { } /// @notice deprecated in favour of depositEth with no parameters - function depositEth(uint256) external payable virtual override whenNotPaused returns (uint256) { + function depositEth(uint256) + external + payable + virtual + override + whenNotPaused + onlyAllowed + returns (uint256) + { return depositEth(); } @@ -259,7 +301,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { uint256 gasLimit, uint256 maxFeePerGas, bytes calldata data - ) external payable virtual whenNotPaused returns (uint256) { + ) external payable virtual whenNotPaused onlyAllowed returns (uint256) { return unsafeCreateRetryableTicket( to, @@ -296,7 +338,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { uint256 gasLimit, uint256 maxFeePerGas, bytes calldata data - ) external payable virtual override whenNotPaused returns (uint256) { + ) external payable virtual override whenNotPaused onlyAllowed returns (uint256) { // ensure the user's deposit alone will make submission succeed if (msg.value < maxSubmissionCost + l2CallValue) revert InsufficientValue(maxSubmissionCost + l2CallValue, msg.value); @@ -351,7 +393,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { uint256 gasLimit, uint256 maxFeePerGas, bytes calldata data - ) public payable virtual override whenNotPaused returns (uint256) { + ) public payable virtual override whenNotPaused onlyAllowed returns (uint256) { // gas price and limit of 1 should never be a valid input, so instead they are used as // magic values to trigger a revert in eth calls that surface data without requiring a tx trace if (gasLimit == 1 || maxFeePerGas == 1) From a51027c15e43055e8d7a1add86cc8c1cba7c2c5b Mon Sep 17 00:00:00 2001 From: fredlacs <32464905+fredlacs@users.noreply.github.com> Date: Fri, 10 Jun 2022 18:04:59 +0100 Subject: [PATCH 2/4] fix: change to tx origin check --- contracts/src/bridge/Inbox.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/src/bridge/Inbox.sol b/contracts/src/bridge/Inbox.sol index d9f4b075b0..91490175fa 100644 --- a/contracts/src/bridge/Inbox.sol +++ b/contracts/src/bridge/Inbox.sol @@ -56,7 +56,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { } modifier onlyAllowed() { - require(isAllowed[msg.sender] || !allowListEnabled, "NOT_ALLOWED"); + require(isAllowed[tx.origin] || !allowListEnabled, "NOT_ALLOWED"); _; } From b5d91c3330480f455b7c830775f6d940f6a2721e Mon Sep 17 00:00:00 2001 From: fredlacs <32464905+fredlacs@users.noreply.github.com> Date: Mon, 13 Jun 2022 11:09:11 +0100 Subject: [PATCH 3/4] docs: add disclaimer about tx origin usage --- contracts/src/bridge/Inbox.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/src/bridge/Inbox.sol b/contracts/src/bridge/Inbox.sol index 91490175fa..3aea1166fe 100644 --- a/contracts/src/bridge/Inbox.sol +++ b/contracts/src/bridge/Inbox.sol @@ -55,6 +55,10 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { emit AllowListEnabledUpdated(_allowListEnabled); } + /// @dev this modifier checks the tx.origin instead of msg.sender for convenience (ie it allows + /// allowed users to interact with the token bridge without needing the token bridge to be allowList aware). + /// this modifier is not intended to use to be used for security (since this opens the allowList to + /// a smart contract phishing risk). modifier onlyAllowed() { require(isAllowed[tx.origin] || !allowListEnabled, "NOT_ALLOWED"); _; From d96467f5cf08234c9819d74333ce783aed773711 Mon Sep 17 00:00:00 2001 From: fredlacs <32464905+fredlacs@users.noreply.github.com> Date: Tue, 14 Jun 2022 09:39:51 +0100 Subject: [PATCH 4/4] refactor: optimise allowlist validation order and use solidity custom error --- contracts/src/bridge/IInbox.sol | 3 +++ contracts/src/bridge/Inbox.sol | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/src/bridge/IInbox.sol b/contracts/src/bridge/IInbox.sol index eef60dc3dd..3ecf47dd7d 100644 --- a/contracts/src/bridge/IInbox.sol +++ b/contracts/src/bridge/IInbox.sol @@ -23,6 +23,9 @@ error InsufficientValue(uint256 expected, uint256 actual); /// @dev submission cost provided isn't enough to create retryable ticket error InsufficientSubmissionCost(uint256 expected, uint256 actual); +/// @dev address not allowed to interact with the given contract +error NotAllowedOrigin(address origin); + /// @dev used to convey retryable tx data in eth calls without requiring a tx trace /// this follows a pattern similar to EIP-3668 where reverts surface call information error RetryableData( diff --git a/contracts/src/bridge/Inbox.sol b/contracts/src/bridge/Inbox.sol index 3aea1166fe..f216cc6181 100644 --- a/contracts/src/bridge/Inbox.sol +++ b/contracts/src/bridge/Inbox.sol @@ -60,7 +60,7 @@ contract Inbox is DelegateCallAware, PausableUpgradeable, IInbox { /// this modifier is not intended to use to be used for security (since this opens the allowList to /// a smart contract phishing risk). modifier onlyAllowed() { - require(isAllowed[tx.origin] || !allowListEnabled, "NOT_ALLOWED"); + if (allowListEnabled && !isAllowed[tx.origin]) revert NotAllowedOrigin(tx.origin); _; }