Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

merging main #824

Merged
merged 3 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion gas-bound-caller/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"ethers": "^5.7.0",
"fast-glob": "^3.3.2",
"hardhat": "=2.22.2",
"preprocess": "^3.2.0"
"preprocess": "^3.2.0",
"zksync-ethers": "^5.9.0"
},
"devDependencies": {
"@matterlabs/hardhat-zksync-chai-matchers": "^0.2.0",
Expand Down
53 changes: 52 additions & 1 deletion l1-contracts/contracts/bridge/L1SharedBridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@
/// NOTE: this function may be removed in the future, don't rely on it!
mapping(uint256 chainId => mapping(address l1Token => uint256 balance)) public chainBalance;

/// @dev Admin has the ability to register new chains within the shared bridge.
address public admin;

/// @dev The pending admin, i.e. the candidate to the admin role.
address public pendingAdmin;

/// @notice Checks that the message sender is the bridgehub.
modifier onlyBridgehub() {
if (msg.sender != address(BRIDGE_HUB)) {
Expand Down Expand Up @@ -122,6 +128,12 @@
_;
}

/// @notice Checks that the message sender is either the owner or admin.
modifier onlyOwnerOrAdmin() {
require(msg.sender == owner() || msg.sender == admin, "ShB not owner or admin");

Check failure on line 133 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 133 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 133 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements
_;
}

/// @dev Contract is expected to be used as proxy implementation.
/// @dev Initialize the implementation to prevent Parity hack.
constructor(
Expand All @@ -147,6 +159,31 @@
_transferOwnership(_owner);
}

/// @inheritdoc IL1SharedBridge
/// @dev Please note, if the owner wants to enforce the admin change it must execute both `setPendingAdmin` and
/// `acceptAdmin` atomically. Otherwise `admin` can set different pending admin and so fail to accept the admin rights.
function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin {
// Save previous value into the stack to put it into the event later
address oldPendingAdmin = pendingAdmin;
// Change pending admin
pendingAdmin = _newPendingAdmin;
emit NewPendingAdmin(oldPendingAdmin, _newPendingAdmin);
}

/// @inheritdoc IL1SharedBridge
/// @notice Accepts transfer of admin rights. Only pending admin can accept the role.
function acceptAdmin() external {
address currentPendingAdmin = pendingAdmin;
require(msg.sender == currentPendingAdmin, "ShB not pending admin"); // Only proposed by current admin address can claim the admin rights

Check failure on line 177 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 177 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 177 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just in case, do you want to switch to using custom errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably but i wasnt sure how many changes we wanted to make before going to deploy/what the scope is as theres a lot of non custom error reverts
cc: @vladbochok


address previousAdmin = admin;
admin = currentPendingAdmin;
delete pendingAdmin;

emit NewPendingAdmin(currentPendingAdmin, address(0));
emit NewAdmin(previousAdmin, currentPendingAdmin);
}

/// @dev This sets the first post diamond upgrade batch for era, used to check old eth withdrawals
/// @param _eraPostDiamondUpgradeFirstBatch The first batch number on the ZKsync Era Diamond Proxy that was settled after diamond proxy upgrade.
function setEraPostDiamondUpgradeFirstBatch(uint256 _eraPostDiamondUpgradeFirstBatch) external onlyOwner {
Expand Down Expand Up @@ -236,7 +273,21 @@
}

/// @dev Initializes the l2Bridge address by governance for a specific chain.
function initializeChainGovernance(uint256 _chainId, address _l2BridgeAddress) external onlyOwner {
/// @param _chainId The chain ID for which the l2Bridge address is being initialized.
/// @param _l2BridgeAddress The address of the L2 bridge contract.
function initializeChainGovernance(uint256 _chainId, address _l2BridgeAddress) external onlyOwnerOrAdmin {
require(l2BridgeAddress[_chainId] == address(0), "ShB: l2 bridge already set");

Check failure on line 279 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 279 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 279 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements
require(_l2BridgeAddress != address(0), "ShB: l2 bridge 0");

Check failure on line 280 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 280 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 280 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements
l2BridgeAddress[_chainId] = _l2BridgeAddress;
}

/// @dev Reinitializes the l2Bridge address by governance for a specific chain.
/// @dev Only accessible to the owner of the bridge to prevent malicious admin from changing the bridge address for
/// an existing chain.
/// @param _chainId The chain ID for which the l2Bridge address is being initialized.
/// @param _l2BridgeAddress The address of the L2 bridge contract.
function reinitializeChainGovernance(uint256 _chainId, address _l2BridgeAddress) external onlyOwner {
require(l2BridgeAddress[_chainId] != address(0), "ShB: l2 bridge not yet set");

Check failure on line 290 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 290 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 290 in l1-contracts/contracts/bridge/L1SharedBridge.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements
l2BridgeAddress[_chainId] = _l2BridgeAddress;
}

Expand Down
15 changes: 15 additions & 0 deletions l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ import {IL1ERC20Bridge} from "./IL1ERC20Bridge.sol";
/// @author Matter Labs
/// @custom:security-contact [email protected]
interface IL1SharedBridge {
/// @notice pendingAdmin is changed
/// @dev Also emitted when new admin is accepted and in this case, `newPendingAdmin` would be zero address
event NewPendingAdmin(address indexed oldPendingAdmin, address indexed newPendingAdmin);

/// @notice Admin changed
event NewAdmin(address indexed oldAdmin, address indexed newAdmin);

event LegacyDepositInitiated(
uint256 indexed chainId,
bytes32 indexed l2DepositTxHash,
Expand Down Expand Up @@ -151,4 +158,12 @@ interface IL1SharedBridge {
function bridgehubConfirmL2Transaction(uint256 _chainId, bytes32 _txDataHash, bytes32 _txHash) external;

function receiveEth(uint256 _chainId) external payable;

/// @notice Starts the transfer of admin rights. Only the current admin can propose a new pending one.
/// @notice New admin can accept admin rights by calling `acceptAdmin` function.
/// @param _newPendingAdmin Address of the new admin
function setPendingAdmin(address _newPendingAdmin) external;

/// @notice Accepts transfer of admin rights. Only pending admin can accept the role.
function acceptAdmin() external;
}
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridgehub/Bridgehub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus
}

/// @notice token can be any contract with the appropriate interface/functionality
function addToken(address _token) external onlyOwner {
function addToken(address _token) external onlyOwnerOrAdmin {
if (tokenIsRegistered[_token]) {
revert TokenAlreadyRegistered(_token);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ l1_shared_bridge = "0x2ae37d8130b82c7e79b3863a39027178e073eedb"
bridgehub = "0xea785a9c91a07ed69b83eb165f4ce2c30ecb4c0b"
governance = "0x6a08d69675af7755569a1a25ef37e795493473a1"
erc20_bridge = "0x84fbda16bd5f2d66d7fbaec5e8d816e7b7014595"
consensus_registry_owner = "0xD64e136566a9E04eb05B30184fF577F52682D182"
4 changes: 4 additions & 0 deletions l1-contracts/deploy-scripts/DeployL1.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -593,12 +593,15 @@ contract DeployL1Script is Script {

Bridgehub bridgehub = Bridgehub(addresses.bridgehub.bridgehubProxy);
bridgehub.transferOwnership(addresses.governance);
bridgehub.setPendingAdmin(addresses.chainAdmin);

L1SharedBridge sharedBridge = L1SharedBridge(addresses.bridges.sharedBridgeProxy);
sharedBridge.transferOwnership(addresses.governance);
sharedBridge.setPendingAdmin(addresses.chainAdmin);

StateTransitionManager stm = StateTransitionManager(addresses.stateTransition.stateTransitionProxy);
stm.transferOwnership(addresses.governance);
stm.setPendingAdmin(addresses.chainAdmin);

vm.stopBroadcast();
console.log("Owners updated");
Expand Down Expand Up @@ -709,6 +712,7 @@ contract DeployL1Script is Script {
addresses.blobVersionedHashRetriever
);
vm.serializeAddress("deployed_addresses", "validator_timelock_addr", addresses.validatorTimelock);
vm.serializeAddress("deployed_addresses", "chain_admin", addresses.chainAdmin);
vm.serializeString("deployed_addresses", "bridgehub", bridgehub);
vm.serializeString("deployed_addresses", "state_transition", stateTransition);
string memory deployedAddresses = vm.serializeString("deployed_addresses", "bridges", bridges);
Expand Down
Loading
Loading