Smr 1910 rbac #23

Benjimmutable · 2023-11-07T03:20:19Z

This adds roles, but does not add some things that roles will be used for, e.g.:

…to smr-1910-rbac

update solc version to 0.8.19

src/child/ChildERC20Bridge.sol

rzmahmood · 2023-11-08T04:57:26Z

src/child/ChildERC20Bridge.sol

@@ -281,7 +300,10 @@ contract ChildERC20Bridge is
        }
    }

-    function updateBridgeAdaptor(address newBridgeAdaptor) external override onlyOwner {
+    function updateBridgeAdaptor(address newBridgeAdaptor) external override /*onlyOwner*/ {


Missing inheritdoc

rzmahmood · 2023-11-08T04:59:42Z

src/child/ChildERC20Bridge.sol

-    function updateBridgeAdaptor(address newBridgeAdaptor) external override onlyOwner {
+    function updateBridgeAdaptor(address newBridgeAdaptor) external override /*onlyOwner*/ {
+        if (!(hasRole(VARIABLE_MANAGER_ROLE, msg.sender))) {
+            revert NotVariableManager();


Might be worth adding the caller address in NotVariableManager, i.e. NotVariableManager(msg.sender).

AccessControl default error messages will also return the account being checked for role in the error

Sounds good

rzmahmood · 2023-11-08T05:22:27Z

script/DeployChildContracts.s.sol

@@ -27,8 +27,16 @@ contract DeployChildContracts is Script {
        ChildERC20 childTokenTemplate = new ChildERC20();
        childTokenTemplate.initialize(address(123), "TEMPLATE", "TPT", 18);

+        IChildERC20Bridge.InitializationRoles memory roles = IChildERC20Bridge.InitializationRoles({


not a blocker but in general better to avoid the precompile addresses for things

rzmahmood · 2023-11-08T05:35:19Z

src/child/ChildERC20Bridge.sol

@@ -1,12 +1,12 @@
 // SPDX-License-Identifier: Apache 2.0
-pragma solidity ^0.8.21;
+pragma solidity 0.8.19;

 import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";


Import for Initializable can be removed

rzmahmood · 2023-11-08T05:40:41Z

src/child/ChildERC20Bridge.sol

+        _grantRole(UNPAUSER_ROLE, newRoles.unpauser);
+        _grantRole(VARIABLE_MANAGER_ROLE, newRoles.variableManager);
+        _grantRole(ADAPTOR_MANAGER_ROLE, newRoles.adaptorManager);
+


Best practice to call initializer of inherited contracts, i.e.

__AccessControl_init()

In this case its empty, so not critical but its good to get the habit

rzmahmood · 2023-11-08T05:45:03Z

src/interfaces/child/IChildERC20Bridge.sol

@@ -55,6 +63,8 @@ interface IChildERC20BridgeEvents {

 // TODO add parameters to errors if it makes sense
 interface IChildERC20BridgeErrors {
+    /// @notice Error when the caller is not the variable manager role.
+    error NotVariableManager();


See above about adding the msg.sender

rzmahmood · 2023-11-08T05:51:57Z

script/InitializeChildContracts.s.sol

+            variableManager: params.childAdminAddress,
+            adaptorManager: params.childAdminAddress


This doesn't look right. The variable manager and adaptor manager is same as the childAdmin?

The script isn't looking good and looks pretty outdated, so worth creating a ticket to update the script in a separate PR.

rzmahmood · 2023-11-08T05:53:50Z

script/InitializeRootContracts.s.sol

+            variableManager: params.rootAdminAddress,
+            adaptorManager: params.rootAdminAddress


Same here, is it intended that the variableManager and adaptorManager is the rootAdmin?

rzmahmood · 2023-11-09T00:24:24Z

src/interfaces/root/IRootERC20Bridge.sol

@@ -83,6 +91,8 @@ interface IRootERC20BridgeEvents {
 }

 interface IRootERC20BridgeErrors {
+    /// @notice Error when the caller is not the variable manager role.
+    error NotVariableManager();


See comment about adding caller as argument

rzmahmood · 2023-11-09T00:53:51Z

src/root/RootERC20Bridge.sol

@@ -1,12 +1,12 @@
 // SPDX-License-Identifier: Apache 2.0
-pragma solidity ^0.8.21;
+pragma solidity 0.8.19;

 import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";


Initializable line is not required

rzmahmood · 2023-11-09T00:54:52Z

src/root/RootERC20Bridge.sol


 import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 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";


Address is imported twice

rzmahmood · 2023-11-09T00:56:01Z

src/root/RootERC20Bridge.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 {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {IAxelarGateway} from "@axelar-cgp-solidity/contracts/interfaces/IAxelarGateway.sol";
 import {IRootERC20Bridge, IERC20Metadata} from "../interfaces/root/IRootERC20Bridge.sol";


Two separate imports from the same IRootERC20Bridge.sol?

rzmahmood · 2023-11-09T02:56:50Z

src/root/RootERC20Bridge.sol

+        _grantRole(UNPAUSER_ROLE, newRoles.unpauser);
+        _grantRole(VARIABLE_MANAGER_ROLE, newRoles.variableManager);
+        _grantRole(ADAPTOR_MANAGER_ROLE, newRoles.adaptorManager);
+


better practice to add __AccessControl_init()

rzmahmood · 2023-11-09T02:57:58Z

src/root/RootERC20Bridge.sol

@@ -115,7 +131,11 @@ contract RootERC20Bridge is
     * @param newRootBridgeAdaptor Address of new root bridge adaptor.
     * @dev Can only be called by owner.


outdated comment. There is no owner

rzmahmood · 2023-11-09T02:59:41Z

src/root/RootERC20Bridge.sol

@@ -131,7 +151,11 @@ contract RootERC20Bridge is
     * @dev Can only be called by owner.


outdated comment. There is no owner

rzmahmood · 2023-11-09T03:23:31Z

src/root/RootERC20Bridge.sol

@@ -115,7 +131,11 @@ contract RootERC20Bridge is
     * @param newRootBridgeAdaptor Address of new root bridge adaptor.
     * @dev Can only be called by owner.
     */
-    function updateRootBridgeAdaptor(address newRootBridgeAdaptor) external onlyOwner {
+    function updateRootBridgeAdaptor(address newRootBridgeAdaptor) external {
+        if (!hasRole(VARIABLE_MANAGER_ROLE, msg.sender)) {


Should be ADAPTOR_MANAGER, not variable_manager

You are right

rzmahmood · 2023-11-09T05:38:30Z

src/root/RootERC20Bridge.sol

+        if (!hasRole(VARIABLE_MANAGER_ROLE, msg.sender)) {
+            revert NotVariableManager();
+        }
+
        if (
            newImxCumulativeDepositLimit != NO_DEPOSIT_LIMIT


Avoid double negatives as it is confusing. Better to change NO_DEPOSIT_LIMIT to DEPOSIT_LIMIT

This constant represents no limit in deposit. How about UNLIMITED_DEPOSIT?

unlimited feels better

rzmahmood · 2023-11-09T05:56:44Z

test/integration/root/RootERC20Bridge.t.sol

@@ -19,7 +19,7 @@ contract RootERC20BridgeIntegrationTest is Test, IRootERC20BridgeEvents, IRootAx
    address constant IMX_TOKEN_ADDRESS = address(0xccc);
    address constant NATIVE_ETH = address(0xeee);
    address constant WRAPPED_ETH = address(0xddd);
-    uint256 constant unlimitedDepositLimit = 0;
+    uint256 constant UNLIMITED_DEPOSIT_LIMIT = 0;


Why is this named differently to imxCumulativeDepositLimit? It makes it seem like UNLIMITED_DEPOSIT_LIMIT is false, when reality the deposit limit is unlimited.

I think this constant represents no limit in the deposit while any other positive number in imxCumulativeDepositLimit represents a limit in-place.

rzmahmood · 2023-11-09T06:01:07Z

test/unit/child/ChildERC20Bridge.t.sol

+        });
+
+        vm.expectRevert(ZeroAddress.selector);
+        bridge.initialize(roles, address(0), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1));


This will revert because newBridgeAdaptor is 0, not because defaultAdmin is 0

You are correct, fixed

rzmahmood · 2023-11-09T06:01:30Z

test/unit/child/ChildERC20Bridge.t.sol

+        });
+
+        vm.expectRevert(ZeroAddress.selector);
+        bridge.initialize(roles, address(0), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1));


This will revert because newBridgeAdaptor is 0. Same as above

rzmahmood · 2023-11-09T06:01:56Z

test/unit/child/ChildERC20Bridge.t.sol

+        });
+
+        vm.expectRevert(ZeroAddress.selector);
+        bridge.initialize(roles, address(0), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1));


This will revert because newBridgeAdaptor is 0. Same as above

rzmahmood · 2023-11-09T06:02:05Z

test/unit/child/ChildERC20Bridge.t.sol

+        });
+
+        vm.expectRevert(ZeroAddress.selector);
+        bridge.initialize(roles, address(0), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1));


This will revert because newBridgeAdaptor is 0. Same as above

rzmahmood · 2023-11-09T06:02:10Z

test/unit/child/ChildERC20Bridge.t.sol

+        });
+
+        vm.expectRevert(ZeroAddress.selector);
+        bridge.initialize(roles, address(0), ROOT_BRIDGE_ADAPTOR, address(1), ROOT_CHAIN_NAME, address(1));


This will revert because newBridgeAdaptor is 0. Same as above

rzmahmood · 2023-11-09T06:04:37Z

test/unit/child/ChildERC20Bridge.t.sol

@@ -220,7 +353,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B

    function test_RevertIf_updateBridgeAdaptorCalledByNonOwner() public {


Test name outdated

rzmahmood · 2023-11-09T06:05:41Z

test/unit/child/ChildERC20Bridge.t.sol

@@ -220,7 +353,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B

    function test_RevertIf_updateBridgeAdaptorCalledByNonOwner() public {
        vm.prank(address(0xf00f00));
-        vm.expectRevert("Ownable: caller is not the owner");
+        vm.expectRevert(NotVariableManager.selector);


Should be testing for AdaptorManager

tsnewnami · 2023-11-12T22:26:11Z

src/child/ChildERC20Bridge.sol

+    /**
+     * ROLES
+     */
+    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");


I see these same roles are also defined in RootERC20Bridge.sol. Possibly worth moving to another contract for DRY, consistency, less error-prone and for prosperity reasons when we support bridging of other assets?

Yes, not just this. The root and child bridge share a lot of common constants, errors, etc, so it would be nice to create an issue and do that all in a separate PR.

tsnewnami · 2023-11-12T22:27:23Z

src/child/ChildERC20Bridge.sol

@@ -56,7 +63,8 @@ contract ChildERC20Bridge is
    address public childETHToken;

    /**
-     * @notice Initilization function for RootERC20Bridge.
+     * @notice Initialization function for RootERC20Bridge.


Incorrect comment, should be ChildERC20Bridge?

tsnewnami · 2023-11-12T22:30:45Z

src/root/RootERC20Bridge.sol

+    /**
+     * ROLES
+     */
+    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");


See above comment regarding role definitions in contract.

rzmahmood

See comments

Co-authored-by: Zoraiz Mahmood <[email protected]>

src/root/RootERC20Bridge.sol

Co-authored-by: Zoraiz Mahmood <[email protected]>

Benjimmutable added 7 commits November 6, 2023 15:11

Roles in RootERC20Bridge

296f99e

Lint

546cf41

Child chain roles

f0b2622

lint

a512367

Merge branch 'main' of github.com:immutable/zkevm-bridge-contracts in…

e09c8f8

…to smr-1910-rbac

Add env vars

c4b61a2

Lint

828ef3b

Benjimmutable self-assigned this Nov 7, 2023

Benjimmutable and others added 3 commits November 7, 2023 14:34

Merge branch 'main' of github.com:immutable/zkevm-bridge-contracts in…

bb7bd6e

…to smr-1910-rbac

update solc version to 0.8.19

1da1d50

Merge pull request #25 from immutable/update-solc-0-8-19

6fd7756

update solc version to 0.8.19

rzmahmood reviewed Nov 8, 2023

View reviewed changes

src/child/ChildERC20Bridge.sol Outdated Show resolved Hide resolved

rzmahmood reviewed Nov 8, 2023

View reviewed changes

rzmahmood reviewed Nov 9, 2023

View reviewed changes

tsnewnami reviewed Nov 12, 2023

View reviewed changes

rzmahmood suggested changes Nov 12, 2023

View reviewed changes

wcgcyx and others added 14 commits November 13, 2023 09:24

Update src/child/ChildERC20Bridge.sol

5ea316d

Co-authored-by: Zoraiz Mahmood <[email protected]>

Update ChildERC20Bridge.sol

460442e

Update DeployChildContracts.s.sol

caf5d21

Update ChildERC20Bridge.sol

45fd048

Update

de3aad3

Update RootERC20Bridge.sol

f41ecc9

Update RootERC20Bridge.sol

e08b7e2

Update RootERC20Bridge.sol

7546445

Update

c4b0252

Fix test

17ab7d4

Update

dc5d718

Fix test issue

b963861

Update ChildERC20Bridge.sol

822246a

Update RootERC20Bridge.sol

635b110

rzmahmood reviewed Nov 13, 2023

View reviewed changes

src/root/RootERC20Bridge.sol Outdated Show resolved Hide resolved

Update src/root/RootERC20Bridge.sol

7695c60

Co-authored-by: Zoraiz Mahmood <[email protected]>

rzmahmood approved these changes Nov 13, 2023

View reviewed changes

wcgcyx merged commit 81697c0 into main Nov 13, 2023
3 checks passed

ermyas deleted the smr-1910-rbac branch April 12, 2024 05:13

		variableManager: params.childAdminAddress,
		adaptorManager: params.childAdminAddress

		variableManager: params.rootAdminAddress,
		adaptorManager: params.rootAdminAddress

		@@ -115,7 +131,11 @@ contract RootERC20Bridge is
		* @param newRootBridgeAdaptor Address of new root bridge adaptor.
		* @dev Can only be called by owner.

		@@ -131,7 +151,11 @@ contract RootERC20Bridge is
		* @dev Can only be called by owner.

		@@ -220,7 +353,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B

		function test_RevertIf_updateBridgeAdaptorCalledByNonOwner() public {

Smr 1910 rbac #23

Smr 1910 rbac #23

Conversation

Benjimmutable commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzmahmood Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzmahmood Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzmahmood Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzmahmood left a comment

Choose a reason for hiding this comment

rzmahmood Nov 8, 2023 •

edited

Loading

rzmahmood Nov 9, 2023 •

edited

Loading

rzmahmood Nov 9, 2023 •

edited

Loading