From 94b788addfda42ae2118972e954af6f26a197b15 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 07:33:38 +1000 Subject: [PATCH 1/9] add BridgeRoles contract --- src/lib/BridgeRoles.sol | 101 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 src/lib/BridgeRoles.sol diff --git a/src/lib/BridgeRoles.sol b/src/lib/BridgeRoles.sol new file mode 100644 index 00000000..7f50aa49 --- /dev/null +++ b/src/lib/BridgeRoles.sol @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: Apache 2.0 +pragma solidity 0.8.19; + +import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol"; + +/** + * + * @title BridgeRoles.sol + * @notice BridgeRoles.sol is an abstract contract that defines the roles and permissions and pausable functionality across the root and child chain bridge contracts. + * @dev This contract uses OpenZeppelin's AccessControl and Pausable contracts. This contract is abstract and is intended to be inherited by the root and child chain bridge contracts. + */ +abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { + // Roles + /// @notice Role identifier for those who can pause functionanlity. + bytes32 public constant PAUSER_ROLE = keccak256("PAUSER"); + + /// @notice Role identifier for those who can unpause functionality. + bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER"); + + /// @notice Role identifier those who can update the cumulative IMX deposit limit. + bytes32 public constant VARIABLE_MANAGER_ROLE = keccak256("VARIABLE_MANAGER"); + + /// @notice Role identifier for those who can update the bridge adaptor. + bytes32 public constant ADAPTOR_MANAGER_ROLE = keccak256("ADAPTOR_MANAGER"); + + // Role granting functions + /** + * @notice Function to grant pauser role to an address + */ + function grantPauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(PAUSER_ROLE, account); + } + + /** + * @notice Function to grant unpauser role to an address + */ + function grantUnpauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(UNPAUSER_ROLE, account); + } + + /** + * @notice Function to grant variable manager role to an address + */ + function grantVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(VARIABLE_MANAGER_ROLE, account); + } + + /** + * @notice Function to grant adaptor manager role to an address + */ + function grantAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(ADAPTOR_MANAGER_ROLE, account); + } + + // Role revoking functions + /** + * @notice Function to revoke pauser role from an address + */ + function revokePauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(PAUSER_ROLE, account); + } + + /** + * @notice Function to revoke unpauser role from an address + */ + function revokeUnpauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(UNPAUSER_ROLE, account); + } + + /** + * @notice Function to revoke variable manager role from an address + */ + function revokeVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(VARIABLE_MANAGER_ROLE, account); + } + + /** + * @notice Function to revoke adaptor manager role from an address + */ + function revokeAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(ADAPTOR_MANAGER_ROLE, account); + } + + // Pausable functions + /** + * @notice Function to pause the contract + * @dev Only PAUSER_ROLE can call this function + */ + function pause() external onlyRole(PAUSER_ROLE) { + _pause(); + } + + /** + * @notice Function to pause the contract + * @dev Only UNPAUSER_ROLE can call this function + */ + function unpause() external onlyRole(UNPAUSER_ROLE) { + _unpause(); + } +} From d00f1b8939f7e9e9f35c4c966b21f298d50307a7 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 07:35:39 +1000 Subject: [PATCH 2/9] make common dir --- src/{lib => common}/BridgeRoles.sol | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/{lib => common}/BridgeRoles.sol (100%) diff --git a/src/lib/BridgeRoles.sol b/src/common/BridgeRoles.sol similarity index 100% rename from src/lib/BridgeRoles.sol rename to src/common/BridgeRoles.sol From 4cc81c4d30fbceb4e9d04e4f6ff910f819d71770 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 08:01:54 +1000 Subject: [PATCH 3/9] create mock bridge roles --- src/lib/BridgeRoles.sol | 101 +++++++++++++++++++++++++++ test/unit/common/MockBridgeRoles.sol | 10 +++ 2 files changed, 111 insertions(+) create mode 100644 src/lib/BridgeRoles.sol create mode 100644 test/unit/common/MockBridgeRoles.sol diff --git a/src/lib/BridgeRoles.sol b/src/lib/BridgeRoles.sol new file mode 100644 index 00000000..7f50aa49 --- /dev/null +++ b/src/lib/BridgeRoles.sol @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: Apache 2.0 +pragma solidity 0.8.19; + +import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol"; + +/** + * + * @title BridgeRoles.sol + * @notice BridgeRoles.sol is an abstract contract that defines the roles and permissions and pausable functionality across the root and child chain bridge contracts. + * @dev This contract uses OpenZeppelin's AccessControl and Pausable contracts. This contract is abstract and is intended to be inherited by the root and child chain bridge contracts. + */ +abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { + // Roles + /// @notice Role identifier for those who can pause functionanlity. + bytes32 public constant PAUSER_ROLE = keccak256("PAUSER"); + + /// @notice Role identifier for those who can unpause functionality. + bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER"); + + /// @notice Role identifier those who can update the cumulative IMX deposit limit. + bytes32 public constant VARIABLE_MANAGER_ROLE = keccak256("VARIABLE_MANAGER"); + + /// @notice Role identifier for those who can update the bridge adaptor. + bytes32 public constant ADAPTOR_MANAGER_ROLE = keccak256("ADAPTOR_MANAGER"); + + // Role granting functions + /** + * @notice Function to grant pauser role to an address + */ + function grantPauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(PAUSER_ROLE, account); + } + + /** + * @notice Function to grant unpauser role to an address + */ + function grantUnpauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(UNPAUSER_ROLE, account); + } + + /** + * @notice Function to grant variable manager role to an address + */ + function grantVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(VARIABLE_MANAGER_ROLE, account); + } + + /** + * @notice Function to grant adaptor manager role to an address + */ + function grantAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(ADAPTOR_MANAGER_ROLE, account); + } + + // Role revoking functions + /** + * @notice Function to revoke pauser role from an address + */ + function revokePauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(PAUSER_ROLE, account); + } + + /** + * @notice Function to revoke unpauser role from an address + */ + function revokeUnpauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(UNPAUSER_ROLE, account); + } + + /** + * @notice Function to revoke variable manager role from an address + */ + function revokeVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(VARIABLE_MANAGER_ROLE, account); + } + + /** + * @notice Function to revoke adaptor manager role from an address + */ + function revokeAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(ADAPTOR_MANAGER_ROLE, account); + } + + // Pausable functions + /** + * @notice Function to pause the contract + * @dev Only PAUSER_ROLE can call this function + */ + function pause() external onlyRole(PAUSER_ROLE) { + _pause(); + } + + /** + * @notice Function to pause the contract + * @dev Only UNPAUSER_ROLE can call this function + */ + function unpause() external onlyRole(UNPAUSER_ROLE) { + _unpause(); + } +} diff --git a/test/unit/common/MockBridgeRoles.sol b/test/unit/common/MockBridgeRoles.sol new file mode 100644 index 00000000..bbc197a7 --- /dev/null +++ b/test/unit/common/MockBridgeRoles.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import "../../../src/common/BridgeRoles.sol"; + +contract MockBridgeRoles is BridgeRoles { + constructor(address admin) { + _setupRole(DEFAULT_ADMIN_ROLE, admin); + } +} From 6905cee836752092bccf7bc52f5285abb02116a6 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 08:21:57 +1000 Subject: [PATCH 4/9] add bridge role tests --- src/common/BridgeRoles.sol | 2 +- src/lib/BridgeRoles.sol | 2 +- test/unit/common/BridgeRoles.t.sol | 158 +++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 test/unit/common/BridgeRoles.t.sol diff --git a/src/common/BridgeRoles.sol b/src/common/BridgeRoles.sol index 7f50aa49..81a9aaea 100644 --- a/src/common/BridgeRoles.sol +++ b/src/common/BridgeRoles.sol @@ -52,7 +52,7 @@ abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { function grantAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { grantRole(ADAPTOR_MANAGER_ROLE, account); } - + // Role revoking functions /** * @notice Function to revoke pauser role from an address diff --git a/src/lib/BridgeRoles.sol b/src/lib/BridgeRoles.sol index 7f50aa49..81a9aaea 100644 --- a/src/lib/BridgeRoles.sol +++ b/src/lib/BridgeRoles.sol @@ -52,7 +52,7 @@ abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { function grantAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { grantRole(ADAPTOR_MANAGER_ROLE, account); } - + // Role revoking functions /** * @notice Function to revoke pauser role from an address diff --git a/test/unit/common/BridgeRoles.t.sol b/test/unit/common/BridgeRoles.t.sol new file mode 100644 index 00000000..420ee44c --- /dev/null +++ b/test/unit/common/BridgeRoles.t.sol @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import "forge-std/Test.sol"; +import "@openzeppelin/contracts-upgradeable/utils/StringsUpgradeable.sol"; +import {MockBridgeRoles} from "./MockBridgeRoles.sol"; + +contract Setup is Test { + address admin = makeAddr("admin"); + address pauser = makeAddr("pauser"); + address unpauser = makeAddr("unpauser"); + address variableManager = makeAddr("variableManager"); + address adaptorManager = makeAddr("adaptorManager"); + + MockBridgeRoles mockBridgeRoles; + + function setUp() public { + mockBridgeRoles = new MockBridgeRoles(admin); + } +} + +contract BridgeRoles is Setup { + function grantRoles() internal { + vm.startPrank(admin); + mockBridgeRoles.grantPauserRole(pauser); + mockBridgeRoles.grantUnpauserRole(unpauser); + mockBridgeRoles.grantVariableManagerRole(variableManager); + mockBridgeRoles.grantAdaptorManagerRole(adaptorManager); + vm.stopPrank(); + } + + function revokeRoles() internal { + vm.startPrank(admin); + mockBridgeRoles.revokePauserRole(pauser); + mockBridgeRoles.revokeUnpauserRole(unpauser); + mockBridgeRoles.revokeVariableManagerRole(variableManager); + mockBridgeRoles.revokeAdaptorManagerRole(adaptorManager); + vm.stopPrank(); + } + + function test_grantRoles() public { + grantRoles(); + assert(mockBridgeRoles.hasRole(mockBridgeRoles.PAUSER_ROLE(), pauser)); + assert(mockBridgeRoles.hasRole(mockBridgeRoles.UNPAUSER_ROLE(), unpauser)); + assert(mockBridgeRoles.hasRole(mockBridgeRoles.VARIABLE_MANAGER_ROLE(), variableManager)); + assert(mockBridgeRoles.hasRole(mockBridgeRoles.ADAPTOR_MANAGER_ROLE(), adaptorManager)); + } + + function test_revokeRoles() public { + grantRoles(); + revokeRoles(); + assert(!mockBridgeRoles.hasRole(mockBridgeRoles.PAUSER_ROLE(), pauser)); + assert(!mockBridgeRoles.hasRole(mockBridgeRoles.UNPAUSER_ROLE(), unpauser)); + assert(!mockBridgeRoles.hasRole(mockBridgeRoles.VARIABLE_MANAGER_ROLE(), variableManager)); + assert(!mockBridgeRoles.hasRole(mockBridgeRoles.ADAPTOR_MANAGER_ROLE(), adaptorManager)); + } + + function test_pause() public { + grantRoles(); + vm.prank(pauser); + mockBridgeRoles.pause(); + assert(mockBridgeRoles.paused()); + } + + function test_unpause() public { + grantRoles(); + vm.prank(pauser); + mockBridgeRoles.pause(); + + vm.prank(unpauser); + mockBridgeRoles.unpause(); + assert(!mockBridgeRoles.paused()); + } + + function test_pause_reverts() public { + bytes32 role = mockBridgeRoles.PAUSER_ROLE(); + + // No role + vm.prank(pauser); + vm.expectRevert( + abi.encodePacked( + "AccessControl: account ", + StringsUpgradeable.toHexString(pauser), + " is missing role ", + StringsUpgradeable.toHexString(uint256(role), 32) + ) + ); + mockBridgeRoles.pause(); + + // Admin role + vm.startPrank(admin); + mockBridgeRoles.grantUnpauserRole(pauser); + vm.expectRevert( + abi.encodePacked( + "AccessControl: account ", + StringsUpgradeable.toHexString(admin), + " is missing role ", + StringsUpgradeable.toHexString(uint256(role), 32) + ) + ); + mockBridgeRoles.pause(); + vm.stopPrank(); + + // Unpauser role + vm.prank(unpauser); + vm.expectRevert( + abi.encodePacked( + "AccessControl: account ", + StringsUpgradeable.toHexString(unpauser), + " is missing role ", + StringsUpgradeable.toHexString(uint256(role), 32) + ) + ); + mockBridgeRoles.pause(); + } + + function test_unpause_reverts() public { + bytes32 role = mockBridgeRoles.UNPAUSER_ROLE(); + + // No role + vm.prank(unpauser); + vm.expectRevert( + abi.encodePacked( + "AccessControl: account ", + StringsUpgradeable.toHexString(unpauser), + " is missing role ", + StringsUpgradeable.toHexString(uint256(role), 32) + ) + ); + mockBridgeRoles.unpause(); + + // Admin role + vm.startPrank(admin); + mockBridgeRoles.grantPauserRole(unpauser); + vm.expectRevert( + abi.encodePacked( + "AccessControl: account ", + StringsUpgradeable.toHexString(admin), + " is missing role ", + StringsUpgradeable.toHexString(uint256(role), 32) + ) + ); + mockBridgeRoles.unpause(); + vm.stopPrank(); + + // Pauser role + vm.prank(unpauser); + vm.expectRevert( + abi.encodePacked( + "AccessControl: account ", + StringsUpgradeable.toHexString(unpauser), + " is missing role ", + StringsUpgradeable.toHexString(uint256(role), 32) + ) + ); + mockBridgeRoles.unpause(); + } +} From ffa101791d229d8691d08fe4f9b87058a0d800f5 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 11:21:12 +1000 Subject: [PATCH 5/9] fix tests with adding abstract contract --- src/child/ChildERC20Bridge.sol | 15 ++++----------- src/root/RootERC20Bridge.sol | 12 +++--------- test/integration/child/ChildAxelarBridge.t.sol | 2 +- test/unit/child/ChildERC20Bridge.t.sol | 6 +++--- .../withdrawals/ChildERC20BridgeWithdraw.t.sol | 2 +- .../withdrawals/ChildERC20BridgeWithdrawTo.t.sol | 2 +- test/unit/root/RootERC20Bridge.t.sol | 4 ++-- 7 files changed, 15 insertions(+), 28 deletions(-) diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 3fdcae1b..1828e01c 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/src/child/ChildERC20Bridge.sol @@ -15,6 +15,7 @@ import { import {IChildERC20BridgeAdaptor} from "../interfaces/child/IChildERC20BridgeAdaptor.sol"; import {IChildERC20} from "../interfaces/child/IChildERC20.sol"; import {IWIMX} from "../interfaces/child/IWIMX.sol"; +import {BridgeRoles} from "../common/BridgeRoles.sol"; /** * @notice RootERC20Bridge is a bridge that allows ERC20 tokens to be transferred from the root chain to the child chain. @@ -28,21 +29,14 @@ contract ChildERC20Bridge is AccessControlUpgradeable, // AccessControlUpgradeable inherits Initializable IChildERC20BridgeErrors, IChildERC20Bridge, - IChildERC20BridgeEvents + IChildERC20BridgeEvents, + BridgeRoles { using SafeERC20 for IERC20Metadata; /// @dev leave this as the first param for the integration tests mapping(address => address) public rootTokenToChildToken; - /** - * ROLES - */ - bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); - bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER_ROLE"); - bytes32 public constant VARIABLE_MANAGER_ROLE = keccak256("VARIABLE_MANAGER_ROLE"); - bytes32 public constant ADAPTOR_MANAGER_ROLE = keccak256("ADAPTOR_MANAGER_ROLE"); - bytes32 public constant MAP_TOKEN_SIG = keccak256("MAP_TOKEN"); bytes32 public constant DEPOSIT_SIG = keccak256("DEPOSIT"); bytes32 public constant WITHDRAW_SIG = keccak256("WITHDRAW"); @@ -97,8 +91,7 @@ contract ChildERC20Bridge is if ( newBridgeAdaptor == address(0) || newChildTokenTemplate == address(0) || newRootIMXToken == address(0) || newRoles.defaultAdmin == address(0) || newRoles.pauser == address(0) || newRoles.unpauser == address(0) - || newRoles.variableManager == address(0) || newRoles.adaptorManager == address(0) - || newWIMXToken == address(0) + || newRoles.variableManager == address(0) || newRoles.adaptorManager == address(0) || newWIMXToken == address(0) ) { revert ZeroAddress(); } diff --git a/src/root/RootERC20Bridge.sol b/src/root/RootERC20Bridge.sol index 5995053e..f62b770e 100644 --- a/src/root/RootERC20Bridge.sol +++ b/src/root/RootERC20Bridge.sol @@ -16,6 +16,7 @@ import { import {IRootERC20BridgeAdaptor} from "../interfaces/root/IRootERC20BridgeAdaptor.sol"; import {IChildERC20} from "../interfaces/child/IChildERC20.sol"; import {IWETH} from "../interfaces/root/IWETH.sol"; +import {BridgeRoles} from "../common/BridgeRoles.sol"; /** * @notice RootERC20Bridge is a bridge that allows ERC20 tokens to be transferred from the root chain to the child chain. @@ -29,21 +30,14 @@ contract RootERC20Bridge is AccessControlUpgradeable, // AccessControlUpgradeable inherits Initializable IRootERC20Bridge, IRootERC20BridgeEvents, - IRootERC20BridgeErrors + IRootERC20BridgeErrors, + BridgeRoles { using SafeERC20 for IERC20Metadata; /// @dev leave this as the first param for the integration tests mapping(address => address) public rootTokenToChildToken; - /** - * ROLES - */ - bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); - bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER_ROLE"); - bytes32 public constant VARIABLE_MANAGER_ROLE = keccak256("VARIABLE_MANAGER_ROLE"); - bytes32 public constant ADAPTOR_MANAGER_ROLE = keccak256("ADAPTOR_MANAGER_ROLE"); - uint256 public constant UNLIMITED_DEPOSIT = 0; bytes32 public constant MAP_TOKEN_SIG = keccak256("MAP_TOKEN"); bytes32 public constant DEPOSIT_SIG = keccak256("DEPOSIT"); diff --git a/test/integration/child/ChildAxelarBridge.t.sol b/test/integration/child/ChildAxelarBridge.t.sol index 4ed35a39..4aeba841 100644 --- a/test/integration/child/ChildAxelarBridge.t.sol +++ b/test/integration/child/ChildAxelarBridge.t.sol @@ -309,7 +309,7 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil address rootAddress = address(0x123); { // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"` - uint256 rootTokenToChildTokenMappingSlot = 151; + uint256 rootTokenToChildTokenMappingSlot = 201; address childAddress = address(444444); bytes32 slot = getMappingStorageSlotFor(rootAddress, rootTokenToChildTokenMappingSlot); bytes32 data = bytes32(uint256(uint160(childAddress))); diff --git a/test/unit/child/ChildERC20Bridge.t.sol b/test/unit/child/ChildERC20Bridge.t.sol index e25e4082..ec70df2f 100644 --- a/test/unit/child/ChildERC20Bridge.t.sol +++ b/test/unit/child/ChildERC20Bridge.t.sol @@ -65,8 +65,8 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B function test_NativeTransferFromWIMX() public { address caller = address(0x123a); payable(caller).transfer(2 ether); - - uint256 wIMXStorageSlot = 158; + // forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "wIMXToken" + uint256 wIMXStorageSlot = 208; vm.store(address(childBridge), bytes32(wIMXStorageSlot), bytes32(uint256(uint160(caller)))); vm.startPrank(caller); @@ -605,7 +605,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B address rootAddress = address(0x123); { // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"` - uint256 rootTokenToChildTokenMappingSlot = 151; + uint256 rootTokenToChildTokenMappingSlot = 201; address childAddress = address(444444); bytes32 slot = getMappingStorageSlotFor(rootAddress, rootTokenToChildTokenMappingSlot); bytes32 data = bytes32(uint256(uint160(childAddress))); diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol index 50e60253..714ab7ef 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol @@ -97,7 +97,7 @@ contract ChildERC20BridgeWithdrawUnitTest is Test, IChildERC20BridgeEvents, IChi // Slot is 2 because of the Ownable, Initializable contracts coming first. // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"` - uint256 rootTokenToChildTokenMappingSlot = 151; + uint256 rootTokenToChildTokenMappingSlot = 201; bytes32 slot = getMappingStorageSlotFor(address(0), rootTokenToChildTokenMappingSlot); bytes32 data = bytes32(uint256(uint160(address(childToken)))); diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol index 71a10a67..7d4ab6cc 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol @@ -103,7 +103,7 @@ contract ChildERC20BridgeWithdrawToUnitTest is Test, IChildERC20BridgeEvents, IC /* Then, set rootTokenToChildToken[address(0)] to the child token (to bypass the NotMapped check) */ // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"` - uint256 rootTokenToChildTokenMappingSlot = 151; + uint256 rootTokenToChildTokenMappingSlot = 201; bytes32 slot = getMappingStorageSlotFor(address(0), rootTokenToChildTokenMappingSlot); bytes32 data = bytes32(uint256(uint160(address(childToken)))); diff --git a/test/unit/root/RootERC20Bridge.t.sol b/test/unit/root/RootERC20Bridge.t.sol index fddccb87..fa8cc74c 100644 --- a/test/unit/root/RootERC20Bridge.t.sol +++ b/test/unit/root/RootERC20Bridge.t.sol @@ -90,8 +90,8 @@ contract RootERC20BridgeUnitTest is Test, IRootERC20BridgeEvents, IRootERC20Brid function test_NativeTransferFromWETH() public { address caller = address(0x123a); payable(caller).transfer(2 ether); - - uint256 wETHStorageSlot = 158; + // forge inspect src/root/RootERC20Bridge.sol:RootERC20Bridge storageLayout | grep -B3 -A5 -i "rootWETHToken" + uint256 wETHStorageSlot = 208; vm.store(address(rootBridge), bytes32(wETHStorageSlot), bytes32(uint256(uint160(caller)))); vm.startPrank(caller); From facb5efa75127b4b0edcb5b4d9b52480c59677a7 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 11:37:06 +1000 Subject: [PATCH 6/9] rm duplicate files --- src/lib/BridgeRoles.sol | 101 ---------------------------------------- 1 file changed, 101 deletions(-) delete mode 100644 src/lib/BridgeRoles.sol diff --git a/src/lib/BridgeRoles.sol b/src/lib/BridgeRoles.sol deleted file mode 100644 index 81a9aaea..00000000 --- a/src/lib/BridgeRoles.sol +++ /dev/null @@ -1,101 +0,0 @@ -// SPDX-License-Identifier: Apache 2.0 -pragma solidity 0.8.19; - -import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; -import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol"; - -/** - * - * @title BridgeRoles.sol - * @notice BridgeRoles.sol is an abstract contract that defines the roles and permissions and pausable functionality across the root and child chain bridge contracts. - * @dev This contract uses OpenZeppelin's AccessControl and Pausable contracts. This contract is abstract and is intended to be inherited by the root and child chain bridge contracts. - */ -abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { - // Roles - /// @notice Role identifier for those who can pause functionanlity. - bytes32 public constant PAUSER_ROLE = keccak256("PAUSER"); - - /// @notice Role identifier for those who can unpause functionality. - bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER"); - - /// @notice Role identifier those who can update the cumulative IMX deposit limit. - bytes32 public constant VARIABLE_MANAGER_ROLE = keccak256("VARIABLE_MANAGER"); - - /// @notice Role identifier for those who can update the bridge adaptor. - bytes32 public constant ADAPTOR_MANAGER_ROLE = keccak256("ADAPTOR_MANAGER"); - - // Role granting functions - /** - * @notice Function to grant pauser role to an address - */ - function grantPauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - grantRole(PAUSER_ROLE, account); - } - - /** - * @notice Function to grant unpauser role to an address - */ - function grantUnpauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - grantRole(UNPAUSER_ROLE, account); - } - - /** - * @notice Function to grant variable manager role to an address - */ - function grantVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - grantRole(VARIABLE_MANAGER_ROLE, account); - } - - /** - * @notice Function to grant adaptor manager role to an address - */ - function grantAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - grantRole(ADAPTOR_MANAGER_ROLE, account); - } - - // Role revoking functions - /** - * @notice Function to revoke pauser role from an address - */ - function revokePauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - revokeRole(PAUSER_ROLE, account); - } - - /** - * @notice Function to revoke unpauser role from an address - */ - function revokeUnpauserRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - revokeRole(UNPAUSER_ROLE, account); - } - - /** - * @notice Function to revoke variable manager role from an address - */ - function revokeVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - revokeRole(VARIABLE_MANAGER_ROLE, account); - } - - /** - * @notice Function to revoke adaptor manager role from an address - */ - function revokeAdaptorManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - revokeRole(ADAPTOR_MANAGER_ROLE, account); - } - - // Pausable functions - /** - * @notice Function to pause the contract - * @dev Only PAUSER_ROLE can call this function - */ - function pause() external onlyRole(PAUSER_ROLE) { - _pause(); - } - - /** - * @notice Function to pause the contract - * @dev Only UNPAUSER_ROLE can call this function - */ - function unpause() external onlyRole(UNPAUSER_ROLE) { - _unpause(); - } -} From fc74cac004bb55b7771782fa2693b5ca931afcc0 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 11:40:38 +1000 Subject: [PATCH 7/9] fix formatting --- src/child/ChildERC20Bridge.sol | 3 ++- test/unit/child/ChildERC20Bridge.t.sol | 2 +- test/unit/root/RootERC20Bridge.t.sol | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 1828e01c..11f8d658 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/src/child/ChildERC20Bridge.sol @@ -91,7 +91,8 @@ contract ChildERC20Bridge is if ( newBridgeAdaptor == address(0) || newChildTokenTemplate == address(0) || newRootIMXToken == address(0) || newRoles.defaultAdmin == address(0) || newRoles.pauser == address(0) || newRoles.unpauser == address(0) - || newRoles.variableManager == address(0) || newRoles.adaptorManager == address(0) || newWIMXToken == address(0) + || newRoles.variableManager == address(0) || newRoles.adaptorManager == address(0) + || newWIMXToken == address(0) ) { revert ZeroAddress(); } diff --git a/test/unit/child/ChildERC20Bridge.t.sol b/test/unit/child/ChildERC20Bridge.t.sol index ec70df2f..f526cfb5 100644 --- a/test/unit/child/ChildERC20Bridge.t.sol +++ b/test/unit/child/ChildERC20Bridge.t.sol @@ -65,7 +65,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B function test_NativeTransferFromWIMX() public { address caller = address(0x123a); payable(caller).transfer(2 ether); - // forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "wIMXToken" + // forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "wIMXToken" uint256 wIMXStorageSlot = 208; vm.store(address(childBridge), bytes32(wIMXStorageSlot), bytes32(uint256(uint160(caller)))); diff --git a/test/unit/root/RootERC20Bridge.t.sol b/test/unit/root/RootERC20Bridge.t.sol index fa8cc74c..76cfbf8c 100644 --- a/test/unit/root/RootERC20Bridge.t.sol +++ b/test/unit/root/RootERC20Bridge.t.sol @@ -90,7 +90,7 @@ contract RootERC20BridgeUnitTest is Test, IRootERC20BridgeEvents, IRootERC20Brid function test_NativeTransferFromWETH() public { address caller = address(0x123a); payable(caller).transfer(2 ether); - // forge inspect src/root/RootERC20Bridge.sol:RootERC20Bridge storageLayout | grep -B3 -A5 -i "rootWETHToken" + // forge inspect src/root/RootERC20Bridge.sol:RootERC20Bridge storageLayout | grep -B3 -A5 -i "rootWETHToken" uint256 wETHStorageSlot = 208; vm.store(address(rootBridge), bytes32(wETHStorageSlot), bytes32(uint256(uint160(caller)))); From 66c453dae542cc96dd5c08ffb80e3784d7db4cca Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 13:41:13 +1000 Subject: [PATCH 8/9] move variable manager out of bridgeroles --- script/InitializeChildContracts.s.sol | 1 - src/child/ChildERC20Bridge.sol | 13 +------ src/common/BridgeRoles.sol | 17 -------- src/interfaces/child/IChildERC20Bridge.sol | 1 - src/interfaces/root/IRootERC20Bridge.sol | 10 +++++ src/root/RootERC20Bridge.sol | 26 +++++++++---- .../integration/child/ChildAxelarBridge.t.sol | 1 - test/unit/child/ChildERC20Bridge.t.sol | 39 ------------------- .../ChildERC20BridgeWithdraw.t.sol | 1 - .../ChildERC20BridgeWithdrawETH.t.sol | 1 - .../ChildERC20BridgeWithdrawETHTo.t.sol | 1 - .../ChildERC20BridgeWithdrawIMX.t.sol | 1 - .../ChildERC20BridgeWithdrawIMXTo.t.sol | 1 - .../ChildERC20BridgeWithdrawTo.t.sol | 1 - .../ChildERC20BridgeWithdrawWIMX.t.sol | 1 - .../ChildERC20BridgeWithdrawWIMXTo.t.sol | 1 - test/unit/common/BridgeRoles.t.sol | 5 --- test/utils.t.sol | 1 - 18 files changed, 30 insertions(+), 92 deletions(-) diff --git a/script/InitializeChildContracts.s.sol b/script/InitializeChildContracts.s.sol index b24b6de2..daa92cba 100644 --- a/script/InitializeChildContracts.s.sol +++ b/script/InitializeChildContracts.s.sol @@ -58,7 +58,6 @@ contract InitializeChildContracts is Script { defaultAdmin: params.childAdminAddress, pauser: params.childPauserAddress, unpauser: params.childUnpauserAddress, - variableManager: params.childAdminAddress, adaptorManager: params.childAdminAddress }); params.childERC20Bridge.initialize( diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 11f8d658..5915ac34 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/src/child/ChildERC20Bridge.sol @@ -2,7 +2,6 @@ pragma solidity 0.8.19; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; @@ -25,13 +24,7 @@ import {BridgeRoles} from "../common/BridgeRoles.sol"; * @dev Because of this pattern, any checks or logic that is agnostic to the messaging protocol should be done in RootERC20Bridge. * @dev Any checks or logic that is specific to the underlying messaging protocol should be done in the bridge adaptor. */ -contract ChildERC20Bridge is - AccessControlUpgradeable, // AccessControlUpgradeable inherits Initializable - IChildERC20BridgeErrors, - IChildERC20Bridge, - IChildERC20BridgeEvents, - BridgeRoles -{ +contract ChildERC20Bridge is IChildERC20BridgeErrors, IChildERC20Bridge, IChildERC20BridgeEvents, BridgeRoles { using SafeERC20 for IERC20Metadata; /// @dev leave this as the first param for the integration tests @@ -91,8 +84,7 @@ contract ChildERC20Bridge is if ( newBridgeAdaptor == address(0) || newChildTokenTemplate == address(0) || newRootIMXToken == address(0) || newRoles.defaultAdmin == address(0) || newRoles.pauser == address(0) || newRoles.unpauser == address(0) - || newRoles.variableManager == address(0) || newRoles.adaptorManager == address(0) - || newWIMXToken == address(0) + || newRoles.adaptorManager == address(0) || newWIMXToken == address(0) ) { revert ZeroAddress(); } @@ -110,7 +102,6 @@ contract ChildERC20Bridge is _grantRole(DEFAULT_ADMIN_ROLE, newRoles.defaultAdmin); _grantRole(PAUSER_ROLE, newRoles.pauser); _grantRole(UNPAUSER_ROLE, newRoles.unpauser); - _grantRole(VARIABLE_MANAGER_ROLE, newRoles.variableManager); _grantRole(ADAPTOR_MANAGER_ROLE, newRoles.adaptorManager); rootERC20BridgeAdaptor = newRootERC20BridgeAdaptor; diff --git a/src/common/BridgeRoles.sol b/src/common/BridgeRoles.sol index 81a9aaea..6da15b5b 100644 --- a/src/common/BridgeRoles.sol +++ b/src/common/BridgeRoles.sol @@ -18,9 +18,6 @@ abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { /// @notice Role identifier for those who can unpause functionality. bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER"); - /// @notice Role identifier those who can update the cumulative IMX deposit limit. - bytes32 public constant VARIABLE_MANAGER_ROLE = keccak256("VARIABLE_MANAGER"); - /// @notice Role identifier for those who can update the bridge adaptor. bytes32 public constant ADAPTOR_MANAGER_ROLE = keccak256("ADAPTOR_MANAGER"); @@ -39,13 +36,6 @@ abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { grantRole(UNPAUSER_ROLE, account); } - /** - * @notice Function to grant variable manager role to an address - */ - function grantVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - grantRole(VARIABLE_MANAGER_ROLE, account); - } - /** * @notice Function to grant adaptor manager role to an address */ @@ -68,13 +58,6 @@ abstract contract BridgeRoles is AccessControlUpgradeable, PausableUpgradeable { revokeRole(UNPAUSER_ROLE, account); } - /** - * @notice Function to revoke variable manager role from an address - */ - function revokeVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { - revokeRole(VARIABLE_MANAGER_ROLE, account); - } - /** * @notice Function to revoke adaptor manager role from an address */ diff --git a/src/interfaces/child/IChildERC20Bridge.sol b/src/interfaces/child/IChildERC20Bridge.sol index bf6f39ab..36aaad53 100644 --- a/src/interfaces/child/IChildERC20Bridge.sol +++ b/src/interfaces/child/IChildERC20Bridge.sol @@ -9,7 +9,6 @@ interface IChildERC20Bridge { address defaultAdmin; // The address which will inherit `DEFAULT_ADMIN_ROLE`. address pauser; // The address which will inherit `PAUSER_ROLE`. address unpauser; // The address which will inherit `UNPAUSER_ROLE`. - address variableManager; // The address which will inherit `VARIABLE_MANAGER_ROLE`. address adaptorManager; // The address which will inherit `ADAPTOR_MANAGER_ROLE`. } diff --git a/src/interfaces/root/IRootERC20Bridge.sol b/src/interfaces/root/IRootERC20Bridge.sol index f7d177db..63d47bd9 100644 --- a/src/interfaces/root/IRootERC20Bridge.sol +++ b/src/interfaces/root/IRootERC20Bridge.sol @@ -12,6 +12,16 @@ interface IRootERC20Bridge { address adaptorManager; // The address which will inherit `ADAPTOR_MANAGER_ROLE`. } + /** + * @notice Function to revoke variable manager role from an address + */ + function revokeVariableManagerRole(address account) external; + + /** + * @notice Function to grant variable manager role to an address + */ + function grantVariableManagerRole(address account) external; + function childBridgeAdaptor() external view returns (string memory); /** * @notice Receives a bridge message from child chain, parsing the message type then executing. diff --git a/src/root/RootERC20Bridge.sol b/src/root/RootERC20Bridge.sol index f62b770e..22c2b822 100644 --- a/src/root/RootERC20Bridge.sol +++ b/src/root/RootERC20Bridge.sol @@ -2,7 +2,6 @@ pragma solidity 0.8.19; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; @@ -26,18 +25,15 @@ import {BridgeRoles} from "../common/BridgeRoles.sol"; * @dev Because of this pattern, any checks or logic that is agnostic to the messaging protocol should be done in RootERC20Bridge. * @dev Any checks or logic that is specific to the underlying messaging protocol should be done in the bridge adaptor. */ -contract RootERC20Bridge is - AccessControlUpgradeable, // AccessControlUpgradeable inherits Initializable - IRootERC20Bridge, - IRootERC20BridgeEvents, - IRootERC20BridgeErrors, - BridgeRoles -{ +contract RootERC20Bridge is IRootERC20Bridge, IRootERC20BridgeEvents, IRootERC20BridgeErrors, BridgeRoles { using SafeERC20 for IERC20Metadata; /// @dev leave this as the first param for the integration tests mapping(address => address) public rootTokenToChildToken; + /// @notice Role identifier those who can update the cumulative IMX deposit limit. + bytes32 public constant VARIABLE_MANAGER_ROLE = keccak256("VARIABLE_MANAGER"); + uint256 public constant UNLIMITED_DEPOSIT = 0; bytes32 public constant MAP_TOKEN_SIG = keccak256("MAP_TOKEN"); bytes32 public constant DEPOSIT_SIG = keccak256("DEPOSIT"); @@ -124,6 +120,20 @@ contract RootERC20Bridge is imxCumulativeDepositLimit = newImxCumulativeDepositLimit; } + /** + * @inheritdoc IRootERC20Bridge + */ + function revokeVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(VARIABLE_MANAGER_ROLE, account); + } + + /** + * @inheritdoc IRootERC20Bridge + */ + function grantVariableManagerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(VARIABLE_MANAGER_ROLE, account); + } + /** * @notice Updates the root bridge adaptor. * @param newRootBridgeAdaptor Address of new root bridge adaptor. diff --git a/test/integration/child/ChildAxelarBridge.t.sol b/test/integration/child/ChildAxelarBridge.t.sol index 4aeba841..b9959237 100644 --- a/test/integration/child/ChildAxelarBridge.t.sol +++ b/test/integration/child/ChildAxelarBridge.t.sol @@ -42,7 +42,6 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childERC20Bridge.initialize( diff --git a/test/unit/child/ChildERC20Bridge.t.sol b/test/unit/child/ChildERC20Bridge.t.sol index f526cfb5..268327ea 100644 --- a/test/unit/child/ChildERC20Bridge.t.sol +++ b/test/unit/child/ChildERC20Bridge.t.sol @@ -38,7 +38,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( @@ -87,7 +86,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); vm.expectRevert("Initializable: contract is already initialized"); @@ -108,7 +106,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(0), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); @@ -122,7 +119,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(0), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); @@ -136,7 +132,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(0), - variableManager: address(this), adaptorManager: address(this) }); @@ -144,41 +139,12 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B bridge.initialize(roles, address(1), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1), address(1)); } - function test_RevertIf_InitializeWithAZeroAddressVariableManager() public { - ChildERC20Bridge bridge = new ChildERC20Bridge(); - IChildERC20Bridge.InitializationRoles memory roles = IChildERC20Bridge.InitializationRoles({ - defaultAdmin: address(this), - pauser: address(this), - unpauser: address(this), - variableManager: address(0), - adaptorManager: address(this) - }); - - vm.expectRevert(ZeroAddress.selector); - bridge.initialize(roles, address(1), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1), address(1)); - } - - function test_RevertIf_InitializeWithAZeroAddressAdaptorManager() public { - ChildERC20Bridge bridge = new ChildERC20Bridge(); - IChildERC20Bridge.InitializationRoles memory roles = IChildERC20Bridge.InitializationRoles({ - defaultAdmin: address(this), - pauser: address(this), - unpauser: address(this), - variableManager: address(this), - adaptorManager: address(0) - }); - - vm.expectRevert(ZeroAddress.selector); - bridge.initialize(roles, address(1), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1), address(1)); - } - function test_RevertIf_InitializeWithAZeroAddressAdapter() public { ChildERC20Bridge bridge = new ChildERC20Bridge(); IChildERC20Bridge.InitializationRoles memory roles = IChildERC20Bridge.InitializationRoles({ defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); @@ -192,7 +158,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); @@ -206,7 +171,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); @@ -220,7 +184,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); @@ -234,7 +197,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); @@ -250,7 +212,6 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol index 714ab7ef..34a65dc7 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdraw.t.sol @@ -40,7 +40,6 @@ contract ChildERC20BridgeWithdrawUnitTest is Test, IChildERC20BridgeEvents, IChi defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge = new ChildERC20Bridge(); diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETH.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETH.t.sol index bfd58bb4..a5c3b6ec 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETH.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETH.t.sol @@ -35,7 +35,6 @@ contract ChildERC20BridgeWithdrawETHUnitTest is Test, IChildERC20BridgeEvents, I defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol index 903e5451..2e1d76a8 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol @@ -45,7 +45,6 @@ contract ChildERC20BridgeWithdrawETHToUnitTest is Test, IChildERC20BridgeEvents, defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMX.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMX.t.sol index 3317807e..71efd8dd 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMX.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMX.t.sol @@ -34,7 +34,6 @@ contract ChildERC20BridgeWithdrawIMXUnitTest is Test, IChildERC20BridgeEvents, I defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMXTo.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMXTo.t.sol index 542e5f70..762a3d22 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMXTo.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawIMXTo.t.sol @@ -38,7 +38,6 @@ contract ChildERC20BridgeWithdrawIMXToUnitTest is Test, IChildERC20BridgeEvents, defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol index 7d4ab6cc..ffc0bf23 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawTo.t.sol @@ -40,7 +40,6 @@ contract ChildERC20BridgeWithdrawToUnitTest is Test, IChildERC20BridgeEvents, IC defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge = new ChildERC20Bridge(); diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMX.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMX.t.sol index ab031e6d..5590adee 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMX.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMX.t.sol @@ -38,7 +38,6 @@ contract ChildERC20BridgeWithdrawWIMXUnitTest is Test, IChildERC20BridgeEvents, defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMXTo.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMXTo.t.sol index c9650970..36e5718e 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMXTo.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawWIMXTo.t.sol @@ -40,7 +40,6 @@ contract ChildERC20BridgeWithdrawWIMXToUnitTest is Test, IChildERC20BridgeEvents defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( diff --git a/test/unit/common/BridgeRoles.t.sol b/test/unit/common/BridgeRoles.t.sol index 420ee44c..0cb69160 100644 --- a/test/unit/common/BridgeRoles.t.sol +++ b/test/unit/common/BridgeRoles.t.sol @@ -9,7 +9,6 @@ contract Setup is Test { address admin = makeAddr("admin"); address pauser = makeAddr("pauser"); address unpauser = makeAddr("unpauser"); - address variableManager = makeAddr("variableManager"); address adaptorManager = makeAddr("adaptorManager"); MockBridgeRoles mockBridgeRoles; @@ -24,7 +23,6 @@ contract BridgeRoles is Setup { vm.startPrank(admin); mockBridgeRoles.grantPauserRole(pauser); mockBridgeRoles.grantUnpauserRole(unpauser); - mockBridgeRoles.grantVariableManagerRole(variableManager); mockBridgeRoles.grantAdaptorManagerRole(adaptorManager); vm.stopPrank(); } @@ -33,7 +31,6 @@ contract BridgeRoles is Setup { vm.startPrank(admin); mockBridgeRoles.revokePauserRole(pauser); mockBridgeRoles.revokeUnpauserRole(unpauser); - mockBridgeRoles.revokeVariableManagerRole(variableManager); mockBridgeRoles.revokeAdaptorManagerRole(adaptorManager); vm.stopPrank(); } @@ -42,7 +39,6 @@ contract BridgeRoles is Setup { grantRoles(); assert(mockBridgeRoles.hasRole(mockBridgeRoles.PAUSER_ROLE(), pauser)); assert(mockBridgeRoles.hasRole(mockBridgeRoles.UNPAUSER_ROLE(), unpauser)); - assert(mockBridgeRoles.hasRole(mockBridgeRoles.VARIABLE_MANAGER_ROLE(), variableManager)); assert(mockBridgeRoles.hasRole(mockBridgeRoles.ADAPTOR_MANAGER_ROLE(), adaptorManager)); } @@ -51,7 +47,6 @@ contract BridgeRoles is Setup { revokeRoles(); assert(!mockBridgeRoles.hasRole(mockBridgeRoles.PAUSER_ROLE(), pauser)); assert(!mockBridgeRoles.hasRole(mockBridgeRoles.UNPAUSER_ROLE(), unpauser)); - assert(!mockBridgeRoles.hasRole(mockBridgeRoles.VARIABLE_MANAGER_ROLE(), variableManager)); assert(!mockBridgeRoles.hasRole(mockBridgeRoles.ADAPTOR_MANAGER_ROLE(), adaptorManager)); } diff --git a/test/utils.t.sol b/test/utils.t.sol index 47bc1de4..7358294f 100644 --- a/test/utils.t.sol +++ b/test/utils.t.sol @@ -50,7 +50,6 @@ contract Utils is Test { defaultAdmin: address(this), pauser: address(this), unpauser: address(this), - variableManager: address(this), adaptorManager: address(this) }); childBridge.initialize( From 0ab0b199984484f83732d5f8df7991ab3618338c Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 16 Nov 2023 14:06:51 +1000 Subject: [PATCH 9/9] fix tests and add clean up --- src/child/ChildERC20Bridge.sol | 13 +++++-------- src/interfaces/child/IChildERC20Bridge.sol | 1 - src/interfaces/root/IRootERC20Bridge.sol | 13 +++++++++++++ src/root/RootERC20Bridge.sol | 11 ++++++++--- .../ChildERC20BridgeWithdrawETHTo.t.sol | 1 - test/unit/common/BridgeRoles.t.sol | 16 ++++++++-------- 6 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 5915ac34..92f1c0ba 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/src/child/ChildERC20Bridge.sol @@ -1,15 +1,13 @@ // SPDX-License-Identifier: Apache 2.0 pragma solidity 0.8.19; -import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import { IChildERC20BridgeEvents, IChildERC20BridgeErrors, - IChildERC20Bridge, - IERC20Metadata + IChildERC20Bridge } from "../interfaces/child/IChildERC20Bridge.sol"; import {IChildERC20BridgeAdaptor} from "../interfaces/child/IChildERC20BridgeAdaptor.sol"; import {IChildERC20} from "../interfaces/child/IChildERC20.sol"; @@ -17,16 +15,15 @@ import {IWIMX} from "../interfaces/child/IWIMX.sol"; import {BridgeRoles} from "../common/BridgeRoles.sol"; /** - * @notice RootERC20Bridge is a bridge that allows ERC20 tokens to be transferred from the root chain to the child chain. + * @notice ChildERC20Bridge is a bridge that handles the depositing ERC20 and native tokens to the child chain from the rootchain + * and facilates the withdrawals of ERC20 and native tokens from the child chain to the rootchain. * @dev This contract is designed to be upgradeable. - * @dev Follows a pattern of using a bridge adaptor to communicate with the child chain. This is because the underlying communication protocol may change, + * @dev Follows a pattern of using a bridge adaptor to communicate with the root chain. This is because the underlying communication protocol may change, * and also allows us to decouple vendor-specific messaging logic from the bridge logic. - * @dev Because of this pattern, any checks or logic that is agnostic to the messaging protocol should be done in RootERC20Bridge. + * @dev Because of this pattern, any checks or logic that is agnostic to the messaging protocol should be done in ChildERC20Bridge. * @dev Any checks or logic that is specific to the underlying messaging protocol should be done in the bridge adaptor. */ contract ChildERC20Bridge is IChildERC20BridgeErrors, IChildERC20Bridge, IChildERC20BridgeEvents, BridgeRoles { - using SafeERC20 for IERC20Metadata; - /// @dev leave this as the first param for the integration tests mapping(address => address) public rootTokenToChildToken; diff --git a/src/interfaces/child/IChildERC20Bridge.sol b/src/interfaces/child/IChildERC20Bridge.sol index 36aaad53..b3af5347 100644 --- a/src/interfaces/child/IChildERC20Bridge.sol +++ b/src/interfaces/child/IChildERC20Bridge.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache 2.0 pragma solidity 0.8.19; -import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import {IChildERC20} from "./IChildERC20.sol"; interface IChildERC20Bridge { diff --git a/src/interfaces/root/IRootERC20Bridge.sol b/src/interfaces/root/IRootERC20Bridge.sol index 63d47bd9..0957c545 100644 --- a/src/interfaces/root/IRootERC20Bridge.sol +++ b/src/interfaces/root/IRootERC20Bridge.sol @@ -43,6 +43,19 @@ interface IRootERC20Bridge { */ function mapToken(IERC20Metadata rootToken) external payable returns (address); + /** + * @notice Deposits `amount` of ETH to `msg.sender` on the child chain. + * @param amount The amount of ETH to deposit. + */ + function depositETH(uint256 amount) external payable; + + /** + * @notice Deposits `amount` of ETH to `receiver` on the child chain. + * @param receiver The address to deposit the ETH to. + * @param amount The amount of ETH to deposit. + */ + function depositToETH(address receiver, uint256 amount) external payable; + /** * @notice Initiate sending a deposit message to the child chain. * @custom:requires `rootToken` to already be mapped with `mapToken`. diff --git a/src/root/RootERC20Bridge.sol b/src/root/RootERC20Bridge.sol index 22c2b822..2ea00059 100644 --- a/src/root/RootERC20Bridge.sol +++ b/src/root/RootERC20Bridge.sol @@ -5,7 +5,6 @@ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; -import {IAxelarGateway} from "@axelar-cgp-solidity/contracts/interfaces/IAxelarGateway.sol"; import { IRootERC20Bridge, IERC20Metadata, @@ -13,12 +12,12 @@ import { IRootERC20BridgeErrors } from "../interfaces/root/IRootERC20Bridge.sol"; import {IRootERC20BridgeAdaptor} from "../interfaces/root/IRootERC20BridgeAdaptor.sol"; -import {IChildERC20} from "../interfaces/child/IChildERC20.sol"; import {IWETH} from "../interfaces/root/IWETH.sol"; import {BridgeRoles} from "../common/BridgeRoles.sol"; /** - * @notice RootERC20Bridge is a bridge that allows ERC20 tokens to be transferred from the root chain to the child chain. + * @notice RootERC20Bridge is a bridge that allows ERC20 and native tokens to be bridged from the root chain to the child chain + * and facilitates the withdrawals of ERC20 and native tokens from the child chain to the root chain. * @dev This contract is designed to be upgradeable. * @dev Follows a pattern of using a bridge adaptor to communicate with the child chain. This is because the underlying communication protocol may change, * and also allows us to decouple vendor-specific messaging logic from the bridge logic. @@ -221,10 +220,16 @@ contract RootERC20Bridge is IRootERC20Bridge, IRootERC20BridgeEvents, IRootERC20 return _mapToken(rootToken); } + /** + * @inheritdoc IRootERC20Bridge + */ function depositETH(uint256 amount) external payable { _depositETH(msg.sender, amount); } + /** + * @inheritdoc IRootERC20Bridge + */ function depositToETH(address receiver, uint256 amount) external payable { _depositETH(receiver, amount); } diff --git a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol index 2e1d76a8..300373b5 100644 --- a/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol +++ b/test/unit/child/withdrawals/ChildERC20BridgeWithdrawETHTo.t.sol @@ -9,7 +9,6 @@ import { ChildERC20Bridge, IChildERC20Bridge, IChildERC20BridgeEvents, - IERC20Metadata, IChildERC20BridgeErrors } from "../../../../src/child/ChildERC20Bridge.sol"; import {IChildERC20} from "../../../../src/interfaces/child/IChildERC20.sol"; diff --git a/test/unit/common/BridgeRoles.t.sol b/test/unit/common/BridgeRoles.t.sol index 0cb69160..fdc69fe0 100644 --- a/test/unit/common/BridgeRoles.t.sol +++ b/test/unit/common/BridgeRoles.t.sol @@ -71,11 +71,12 @@ contract BridgeRoles is Setup { bytes32 role = mockBridgeRoles.PAUSER_ROLE(); // No role - vm.prank(pauser); + address noRole = makeAddr("noRole"); + vm.prank(noRole); vm.expectRevert( abi.encodePacked( "AccessControl: account ", - StringsUpgradeable.toHexString(pauser), + StringsUpgradeable.toHexString(noRole), " is missing role ", StringsUpgradeable.toHexString(uint256(role), 32) ) @@ -84,7 +85,6 @@ contract BridgeRoles is Setup { // Admin role vm.startPrank(admin); - mockBridgeRoles.grantUnpauserRole(pauser); vm.expectRevert( abi.encodePacked( "AccessControl: account ", @@ -113,11 +113,12 @@ contract BridgeRoles is Setup { bytes32 role = mockBridgeRoles.UNPAUSER_ROLE(); // No role - vm.prank(unpauser); + address noRole = makeAddr("noRole"); + vm.prank(noRole); vm.expectRevert( abi.encodePacked( "AccessControl: account ", - StringsUpgradeable.toHexString(unpauser), + StringsUpgradeable.toHexString(noRole), " is missing role ", StringsUpgradeable.toHexString(uint256(role), 32) ) @@ -126,7 +127,6 @@ contract BridgeRoles is Setup { // Admin role vm.startPrank(admin); - mockBridgeRoles.grantPauserRole(unpauser); vm.expectRevert( abi.encodePacked( "AccessControl: account ", @@ -139,11 +139,11 @@ contract BridgeRoles is Setup { vm.stopPrank(); // Pauser role - vm.prank(unpauser); + vm.prank(pauser); vm.expectRevert( abi.encodePacked( "AccessControl: account ", - StringsUpgradeable.toHexString(unpauser), + StringsUpgradeable.toHexString(pauser), " is missing role ", StringsUpgradeable.toHexString(uint256(role), 32) )