-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Smr 1910 rbac #23
Smr 1910 rbac #23
Changes from 10 commits
296f99e
546cf41
f0b2622
a512367
e09c8f8
c4b61a2
828ef3b
bb7bd6e
1da1d50
6fd7756
5ea316d
460442e
caf5d21
45fd048
de3aad3
f41ecc9
e08b7e2
7546445
c4b0252
17ab7d4
dc5d718
b963861
822246a
635b110
7695c60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,23 @@ | ||
ROOT_ADMIN_ADDRESS= | ||
ROOT_PAUSER_ADDRESS= | ||
ROOT_UNPAUSER_ADDRESS= | ||
ROOT_RPC_URL= | ||
CHILD_RPC_URL= | ||
ROOT_CHAIN_ID= | ||
CHILD_CHAIN_ID= | ||
ROOT_PRIVATE_KEY= | ||
CHILD_PRIVATE_KEY= | ||
ROOT_GATEWAY_ADDRESS= | ||
CHILD_GATEWAY_ADDRESS= | ||
ROOT_GAS_SERVICE_ADDRESS= | ||
CHILD_GAS_SERVICE_ADDRESS= | ||
ROOT_CHAIN_NAME= | ||
CHILD_CHAIN_NAME= | ||
ROOT_IMX_ADDRESS= | ||
ROOT_WETH_ADDRESS= | ||
INITIAL_IMX_CUMULATIVE_DEPOSIT_LIMIT= # 0 for unlimited | ||
|
||
CHILD_ADMIN_ADDRESS= | ||
CHILD_PAUSER_ADDRESS= | ||
CHILD_UNPAUSER_ADDRESS= | ||
CHILD_RPC_URL= | ||
CHILD_CHAIN_ID= | ||
CHILD_PRIVATE_KEY= | ||
CHILD_GATEWAY_ADDRESS= | ||
CHILD_GAS_SERVICE_ADDRESS= | ||
CHILD_CHAIN_NAME= | ||
ENVIRONMENT= |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,42 +1,76 @@ | ||
// SPDX-License-Identifier: Apache 2.0 | ||
pragma solidity ^0.8.21; | ||
pragma solidity 0.8.19; | ||
|
||
import {Script, console2} from "forge-std/Script.sol"; | ||
|
||
import {ChildERC20Bridge} from "../src/child/ChildERC20Bridge.sol"; | ||
import {ChildERC20Bridge, IChildERC20Bridge} from "../src/child/ChildERC20Bridge.sol"; | ||
import {ChildAxelarBridgeAdaptor} from "../src/child/ChildAxelarBridgeAdaptor.sol"; | ||
import {Utils} from "./Utils.sol"; | ||
|
||
// TODO update private key usage to be more secure: https://book.getfoundry.sh/reference/forge/forge-script#wallet-options---raw | ||
|
||
struct InitializeChildContractsParams { | ||
address childAdminAddress; | ||
address childPauserAddress; | ||
address childUnpauserAddress; | ||
ChildERC20Bridge childERC20Bridge; | ||
ChildAxelarBridgeAdaptor childAxelarBridgeAdaptor; | ||
address childTokenTemplate; | ||
address rootERC20BridgeAdaptor; | ||
string rootChainName; | ||
address rootIMXToken; | ||
string childRpcUrl; | ||
uint256 deployerPrivateKey; | ||
address childGasService; | ||
} | ||
|
||
contract InitializeChildContracts is Script { | ||
function run() public { | ||
uint256 deployerPrivateKey = vm.envUint("CHILD_PRIVATE_KEY"); | ||
ChildERC20Bridge childERC20Bridge = ChildERC20Bridge(vm.envAddress("CHILD_ERC20_BRIDGE")); | ||
ChildAxelarBridgeAdaptor childAxelarBridgeAdaptor = | ||
ChildAxelarBridgeAdaptor(vm.envAddress("CHILD_BRIDGE_ADAPTOR")); | ||
address childTokenTemplate = vm.envAddress("CHILDCHAIN_CHILD_TOKEN_TEMPLATE"); | ||
address rootERC20BridgeAdaptor = vm.envAddress("ROOT_BRIDGE_ADAPTOR"); | ||
string memory childRpcUrl = vm.envString("CHILD_RPC_URL"); | ||
string memory rootChainName = vm.envString("ROOT_CHAIN_NAME"); | ||
address rootIMXToken = vm.envAddress("ROOT_IMX_ADDRESS"); | ||
address childGasService = vm.envAddress("CHILD_GAS_SERVICE_ADDRESS"); // Not yet used. | ||
InitializeChildContractsParams memory params = InitializeChildContractsParams({ | ||
childAdminAddress: vm.envAddress("CHILD_ADMIN_ADDRESS"), | ||
childPauserAddress: vm.envAddress("CHILD_PAUSER_ADDRESS"), | ||
childUnpauserAddress: vm.envAddress("CHILD_UNPAUSER_ADDRESS"), | ||
childERC20Bridge: ChildERC20Bridge(vm.envAddress("CHILD_ERC20_BRIDGE")), | ||
childAxelarBridgeAdaptor: ChildAxelarBridgeAdaptor(vm.envAddress("CHILD_BRIDGE_ADAPTOR")), | ||
childTokenTemplate: vm.envAddress("CHILDCHAIN_CHILD_TOKEN_TEMPLATE"), | ||
rootERC20BridgeAdaptor: vm.envAddress("ROOT_BRIDGE_ADAPTOR"), | ||
rootChainName: vm.envString("ROOT_CHAIN_NAME"), | ||
rootIMXToken: vm.envAddress("ROOT_IMX_ADDRESS"), | ||
childRpcUrl: vm.envString("CHILD_RPC_URL"), | ||
deployerPrivateKey: vm.envUint("CHILD_PRIVATE_KEY"), | ||
childGasService: vm.envAddress("CHILD_GAS_SERVICE_ADDRESS") | ||
}); | ||
|
||
/** | ||
* INITIALIZE CHILD CONTRACTS | ||
*/ | ||
string[] memory checksumInputs = Utils.getChecksumInputs(rootERC20BridgeAdaptor); | ||
string[] memory checksumInputs = Utils.getChecksumInputs(params.rootERC20BridgeAdaptor); | ||
bytes memory checksumOutput = vm.ffi(checksumInputs); | ||
string memory rootBridgeAdaptorString = string(Utils.removeZeroByteValues(checksumOutput)); | ||
|
||
vm.createSelectFork(childRpcUrl); | ||
vm.startBroadcast(deployerPrivateKey); | ||
vm.createSelectFork(params.childRpcUrl); | ||
vm.startBroadcast(params.deployerPrivateKey); | ||
|
||
childERC20Bridge.initialize( | ||
address(childAxelarBridgeAdaptor), rootBridgeAdaptorString, childTokenTemplate, rootChainName, rootIMXToken | ||
// TODO update | ||
IChildERC20Bridge.InitializationRoles memory roles = IChildERC20Bridge.InitializationRoles({ | ||
defaultAdmin: params.childAdminAddress, | ||
pauser: params.childPauserAddress, | ||
unpauser: params.childUnpauserAddress, | ||
variableManager: params.childAdminAddress, | ||
adaptorManager: params.childAdminAddress | ||
Comment on lines
+59
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't look right. The variable manager and adaptor manager is same as the childAdmin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The script isn't looking good and looks pretty outdated, so worth creating a ticket to update the script in a separate PR. |
||
}); | ||
params.childERC20Bridge.initialize( | ||
roles, | ||
address(params.childAxelarBridgeAdaptor), | ||
rootBridgeAdaptorString, | ||
params.childTokenTemplate, | ||
params.rootChainName, | ||
params.rootIMXToken | ||
); | ||
|
||
childAxelarBridgeAdaptor.initialize(rootChainName, address(childERC20Bridge), childGasService); | ||
params.childAxelarBridgeAdaptor.initialize( | ||
params.rootChainName, address(params.childERC20Bridge), params.childGasService | ||
); | ||
|
||
vm.stopBroadcast(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
// SPDX-License-Identifier: Apache 2.0 | ||
pragma solidity ^0.8.21; | ||
pragma solidity 0.8.19; | ||
|
||
import {Script, console2} from "forge-std/Script.sol"; | ||
|
||
import {ChildERC20} from "../src/child/ChildERC20.sol"; | ||
import {RootERC20Bridge} from "../src/root/RootERC20Bridge.sol"; | ||
import {RootERC20Bridge, IRootERC20Bridge} from "../src/root/RootERC20Bridge.sol"; | ||
import {RootAxelarBridgeAdaptor} from "../src/root/RootAxelarBridgeAdaptor.sol"; | ||
import {ChildERC20Bridge} from "../src/child/ChildERC20Bridge.sol"; | ||
import {ChildAxelarBridgeAdaptor} from "../src/child/ChildAxelarBridgeAdaptor.sol"; | ||
|
@@ -14,6 +14,9 @@ import {Utils} from "./Utils.sol"; | |
// TODO update private key usage to be more secure: https://book.getfoundry.sh/reference/forge/forge-script#wallet-options---raw | ||
|
||
struct InitializeRootContractsParams { | ||
address rootAdminAddress; | ||
address rootPauserAddress; | ||
address rootUnpauserAddress; | ||
RootERC20Bridge rootERC20Bridge; | ||
RootAxelarBridgeAdaptor rootBridgeAdaptor; | ||
address rootChainChildTokenTemplate; | ||
|
@@ -31,6 +34,9 @@ struct InitializeRootContractsParams { | |
contract InitializeRootContracts is Script { | ||
function run() public { | ||
InitializeRootContractsParams memory params = InitializeRootContractsParams({ | ||
rootAdminAddress: vm.envAddress("ROOT_ADMIN_ADDRESS"), | ||
rootPauserAddress: vm.envAddress("ROOT_PAUSER_ADDRESS"), | ||
rootUnpauserAddress: vm.envAddress("ROOT_UNPAUSER_ADDRESS"), | ||
rootERC20Bridge: RootERC20Bridge(payable(vm.envAddress("ROOT_ERC20_BRIDGE"))), | ||
rootBridgeAdaptor: RootAxelarBridgeAdaptor(vm.envAddress("ROOT_BRIDGE_ADAPTOR")), | ||
rootChainChildTokenTemplate: vm.envAddress("ROOTCHAIN_CHILD_TOKEN_TEMPLATE"), | ||
|
@@ -54,7 +60,18 @@ contract InitializeRootContracts is Script { | |
vm.createSelectFork(params.rootRpcUrl); | ||
vm.startBroadcast(params.rootPrivateKey); | ||
|
||
// TODO add pauser, unpauser roles. variable manager and Adaptor manager will be privileged transaction multisg | ||
|
||
IRootERC20Bridge.InitializationRoles memory roles = IRootERC20Bridge.InitializationRoles({ | ||
defaultAdmin: params.rootAdminAddress, | ||
pauser: params.rootPauserAddress, | ||
unpauser: params.rootUnpauserAddress, | ||
variableManager: params.rootAdminAddress, | ||
adaptorManager: params.rootAdminAddress | ||
Comment on lines
+69
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, is it intended that the variableManager and adaptorManager is the rootAdmin? |
||
}); | ||
|
||
params.rootERC20Bridge.initialize( | ||
roles, | ||
address(params.rootBridgeAdaptor), | ||
params.childERC20Bridge, | ||
childBridgeAdaptorChecksum, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import for |
||
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"; | ||
import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol"; | ||
import { | ||
IChildERC20BridgeEvents, | ||
IChildERC20BridgeErrors, | ||
|
@@ -25,8 +25,7 @@ import {IChildERC20} from "../interfaces/child/IChildERC20.sol"; | |
* @dev Any checks or logic that is specific to the underlying messaging protocol should be done in the bridge adaptor. | ||
*/ | ||
contract ChildERC20Bridge is | ||
Ownable2Step, | ||
Initializable, | ||
AccessControlUpgradeable, // AccessControlUpgradeable inherits Initializable | ||
IChildERC20BridgeErrors, | ||
IChildERC20Bridge, | ||
IChildERC20BridgeEvents | ||
|
@@ -36,6 +35,14 @@ contract ChildERC20Bridge is | |
/// @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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see these same roles are also defined in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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"); | ||
|
@@ -56,7 +63,8 @@ contract ChildERC20Bridge is | |
address public childETHToken; | ||
|
||
/** | ||
* @notice Initilization function for RootERC20Bridge. | ||
* @notice Initialization function for RootERC20Bridge. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect comment, should be |
||
* @param newRoles Struct containing addresses of roles. | ||
* @param newBridgeAdaptor Address of StateSender to send deposit information to. | ||
* @param newRootERC20BridgeAdaptor Stringified address of root ERC20 bridge adaptor to communicate with. | ||
* @param newChildTokenTemplate Address of child token template to clone. | ||
|
@@ -65,13 +73,18 @@ contract ChildERC20Bridge is | |
* @dev Can only be called once. | ||
*/ | ||
function initialize( | ||
InitializationRoles memory newRoles, | ||
address newBridgeAdaptor, | ||
string memory newRootERC20BridgeAdaptor, | ||
address newChildTokenTemplate, | ||
string memory newRootChain, | ||
address newRootIMXToken | ||
) public initializer { | ||
if (newBridgeAdaptor == address(0) || newChildTokenTemplate == address(0) || newRootIMXToken == address(0)) { | ||
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) | ||
) { | ||
revert ZeroAddress(); | ||
} | ||
|
||
|
@@ -83,6 +96,12 @@ contract ChildERC20Bridge is | |
revert InvalidRootChain(); | ||
} | ||
|
||
_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); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Best practice to call initializer of inherited contracts, i.e.
In this case its empty, so not critical but its good to get the habit |
||
rootERC20BridgeAdaptor = newRootERC20BridgeAdaptor; | ||
childTokenTemplate = newChildTokenTemplate; | ||
bridgeAdaptor = IChildERC20BridgeAdaptor(newBridgeAdaptor); | ||
|
@@ -281,7 +300,10 @@ contract ChildERC20Bridge is | |
} | ||
} | ||
|
||
function updateBridgeAdaptor(address newBridgeAdaptor) external override onlyOwner { | ||
function updateBridgeAdaptor(address newBridgeAdaptor) external override /*onlyOwner*/ { | ||
wcgcyx marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing |
||
if (!(hasRole(VARIABLE_MANAGER_ROLE, msg.sender))) { | ||
revert NotVariableManager(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be worth adding the caller address in AccessControl default error messages will also return the account being checked for role in the error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good |
||
} | ||
if (newBridgeAdaptor == address(0)) { | ||
revert ZeroAddress(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker but in general better to avoid the precompile addresses for things