feat: ZKChain Upgrades and Libraries Diff Audit merged fixes (#1050)

Co-authored-by: vladbochok <[email protected]> Co-authored-by: Stanislav Breadless <[email protected]> Co-authored-by: kelemeno <[email protected]> Co-authored-by: Raid Ateir <[email protected]>
matter-labs · Dec 12, 2024 · 431d076 · 431d076
1 parent 5b57c7f
commit 431d076
Show file tree

Hide file tree

Showing 31 changed files with 135 additions and 115 deletions.
diff --git a/l1-contracts/contracts/bridge/L2SharedBridgeLegacy.sol b/l1-contracts/contracts/bridge/L2SharedBridgeLegacy.sol
@@ -7,7 +7,7 @@ import {UpgradeableBeacon} from "@openzeppelin/contracts-v4/proxy/beacon/Upgrade
 
 import {BridgedStandardERC20} from "./BridgedStandardERC20.sol";
 
-import {DEPLOYER_SYSTEM_CONTRACT, L2_ASSET_ROUTER_ADDR, L2_NATIVE_TOKEN_VAULT_ADDR} from "../common/L2ContractAddresses.sol";
+import {L2_DEPLOYER_SYSTEM_CONTRACT_ADDR, L2_ASSET_ROUTER_ADDR, L2_NATIVE_TOKEN_VAULT_ADDR} from "../common/L2ContractAddresses.sol";
 import {SystemContractsCaller} from "../common/libraries/SystemContractsCaller.sol";
 import {L2ContractHelper, IContractDeployer} from "../common/libraries/L2ContractHelper.sol";
 
@@ -143,7 +143,7 @@ contract L2SharedBridgeLegacy is IL2SharedBridgeLegacy, Initializable {
     function deployBeaconProxy(bytes32 salt) external onlyNTV returns (address proxy) {
         (bool success, bytes memory returndata) = SystemContractsCaller.systemCallWithReturndata(
             uint32(gasleft()),
-            DEPLOYER_SYSTEM_CONTRACT,
+            L2_DEPLOYER_SYSTEM_CONTRACT_ADDR,
             0,
             abi.encodeCall(
                 IContractDeployer.create2,

diff --git a/l1-contracts/contracts/bridge/interfaces/IL2Bridge.sol b/l1-contracts/contracts/bridge/interfaces/IL2Bridge.sol
diff --git a/l1-contracts/contracts/bridge/ntv/L2NativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/L2NativeTokenVault.sol
@@ -16,7 +16,7 @@ import {NativeTokenVault} from "./NativeTokenVault.sol";
 import {IL2SharedBridgeLegacy} from "../interfaces/IL2SharedBridgeLegacy.sol";
 import {BridgedStandardERC20} from "../BridgedStandardERC20.sol";
 
-import {DEPLOYER_SYSTEM_CONTRACT, L2_ASSET_ROUTER_ADDR} from "../../common/L2ContractAddresses.sol";
+import {L2_DEPLOYER_SYSTEM_CONTRACT_ADDR, L2_ASSET_ROUTER_ADDR} from "../../common/L2ContractAddresses.sol";
 import {L2ContractHelper, IContractDeployer} from "../../common/libraries/L2ContractHelper.sol";
 
 import {SystemContractsCaller} from "../../common/libraries/SystemContractsCaller.sol";
@@ -136,7 +136,7 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault {
 
             (bool success, bytes memory returndata) = SystemContractsCaller.systemCallWithReturndata(
                 uint32(gasleft()),
-                DEPLOYER_SYSTEM_CONTRACT,
+                L2_DEPLOYER_SYSTEM_CONTRACT_ADDR,
                 0,
                 abi.encodeCall(
                     IContractDeployer.create2,

diff --git a/l1-contracts/contracts/bridgehub/IMessageRoot.sol b/l1-contracts/contracts/bridgehub/IMessageRoot.sol
@@ -4,8 +4,11 @@ pragma solidity 0.8.24;
 
 import {IBridgehub} from "./IBridgehub.sol";
 
-/// @author Matter Labs
-/// @custom:security-contact [email protected]
+/**
+ * @author Matter Labs
+ * @notice MessageRoot contract is responsible for storing and aggregating the roots of the batches from different chains into the MessageRoot.
+ * @custom:security-contact [email protected]
+ */
 interface IMessageRoot {
     function BRIDGE_HUB() external view returns (IBridgehub);
 

diff --git a/l1-contracts/contracts/common/L1ContractErrors.sol b/l1-contracts/contracts/common/L1ContractErrors.sol
@@ -138,8 +138,6 @@ error HashedLogIsDefault();
 error HashMismatch(bytes32 expected, bytes32 actual);
 // 0xb615c2b1
 error ZKChainLimitReached();
-//
-error InsufficientAllowance(uint256 providedAllowance, uint256 requiredAmount);
 // 0xdd381a4c
 error IncorrectBridgeHubAddress(address bridgehub);
 // 0x826fb11e
@@ -154,8 +152,6 @@ error InvalidChainId();
 error InvalidDelay();
 // 0x0af806e0
 error InvalidHash();
-//
-error InvalidInput();
 // 0xc1780bd6
 error InvalidLogSender(address sender, uint256 logKey);
 // 0xd8e9405c
@@ -358,14 +354,10 @@ error UndefinedDiamondCutAction();
 error UnexpectedNumberOfFactoryDeps();
 // 0x6aa39880
 error UnexpectedSystemLog(uint256 logKey);
-//
-error UnimplementedMessage(string);
 // 0xf093c2e5
 error UpgradeBatchNumberIsNotZero();
 //
 error UnsupportedEncodingVersion();
-//
-error UnsupportedPaymasterFlow();
 // 0x47b3b145
 error ValidateTxnNotEnoughGas();
 // 0x626ade30
@@ -388,8 +380,6 @@ error ZeroAddress();
 error ZeroBalance();
 // 0xc84885d4
 error ZeroChainId();
-// 0x520aa59c
-error PubdataIsEmpty();
 // 0x99d8fec9
 error EmptyData();
 // 0xc99a8360

diff --git a/l1-contracts/contracts/common/L2ContractAddresses.sol b/l1-contracts/contracts/common/L2ContractAddresses.sol
@@ -69,15 +69,12 @@ interface IL2Messenger {
 /// if the assetId can be calculated with this address then it is in fact an NTV asset
 address constant L2_NATIVE_TOKEN_VAULT_ADDR = address(0x10004);
 
-/// @dev the address of the l2 asse3t router.
+/// @dev the address of the l2 asset router.
 address constant L2_MESSAGE_ROOT_ADDR = address(0x10005);
 
 /// @dev the offset for the system contracts
 uint160 constant SYSTEM_CONTRACTS_OFFSET = 0x8000; // 2^15
 
-/// @dev the address of the deployer system contract
-address constant DEPLOYER_SYSTEM_CONTRACT = address(SYSTEM_CONTRACTS_OFFSET + 0x06);
-
 /// @dev the address of the l2 messenger system contract
 IL2Messenger constant L2_MESSENGER = IL2Messenger(address(SYSTEM_CONTRACTS_OFFSET + 0x08));
 

diff --git a/l1-contracts/contracts/common/interfaces/IL2ContractDeployer.sol b/l1-contracts/contracts/common/interfaces/IL2ContractDeployer.sol
@@ -4,7 +4,9 @@ pragma solidity ^0.8.21;
 
 /**
  * @author Matter Labs
- * @notice System smart contract that is responsible for deploying other smart contracts on a ZK chain.
+ * @custom:security-contact [email protected]
+ * @notice Interface for the contract that is used to deploy contracts on L2.
+ * @dev Identical interface is defined in L2ContractHelper library. Duplication is for testing purposes only.
  */
 interface IL2ContractDeployer {
     /// @notice A struct that describes a forced deployment on an address.

diff --git a/l1-contracts/contracts/common/libraries/DataEncoding.sol b/l1-contracts/contracts/common/libraries/DataEncoding.sol
@@ -120,12 +120,16 @@ library DataEncoding {
         }
     }
 
-    /// @notice Decodes the token data by combining chain id, asset deployment tracker and asset data.
+    /// @notice Decodes the token data
+    /// @dev Note that all the returned metadata of the token is ABI encoded.
+    /// @return chainId The chainId of the origin of the token
+    /// @return name The name of the token.
+    /// @return symbol The symbol of the token.
+    /// @return decimals The decimals of the token.
     function decodeTokenData(
         bytes calldata _tokenData
     ) internal pure returns (uint256 chainId, bytes memory name, bytes memory symbol, bytes memory decimals) {
         bytes1 encodingVersion = _tokenData[0];
-        // kl todo check correct
         if (encodingVersion == LEGACY_ENCODING_VERSION) {
             (name, symbol, decimals) = abi.decode(_tokenData, (bytes, bytes, bytes));
         } else if (encodingVersion == NEW_ENCODING_VERSION) {
@@ -135,7 +139,8 @@ library DataEncoding {
         }
     }
 
-    /// @notice Encodes the token data by combining chain id, asset deployment tracker and asset data.
+    /// @notice Encodes the token data by combining chain id, and its metadata.
+    /// @dev Note that all the metadata of the token is expected to be ABI encoded.
     /// @param _chainId The id of the chain token is native to.
     /// @param _name The name of the token.
     /// @param _symbol The symbol of the token.

diff --git a/l1-contracts/contracts/common/libraries/SystemContractsCaller.sol b/l1-contracts/contracts/common/libraries/SystemContractsCaller.sol
@@ -48,7 +48,7 @@ library SystemContractsCaller {
         assembly {
             dataStart := add(data, 0x20)
         }
-        uint32 dataLength = uint32(Utils.safeCastToU32(data.length));
+        uint32 dataLength = Utils.safeCastToU32(data.length);
 
         uint256 farCallAbi = getFarCallABI({
             dataOffset: 0,

diff --git a/l1-contracts/contracts/common/libraries/UnsafeBytes.sol b/l1-contracts/contracts/common/libraries/UnsafeBytes.sol
@@ -30,13 +30,6 @@ library UnsafeBytes {
         }
     }
 
-    function readUint128(bytes memory _bytes, uint256 _start) internal pure returns (uint128 result, uint256 offset) {
-        assembly {
-            offset := add(_start, 16)
-            result := mload(add(_bytes, offset))
-        }
-    }
-
     function readUint256(bytes memory _bytes, uint256 _start) internal pure returns (uint256 result, uint256 offset) {
         assembly {
             offset := add(_start, 32)

diff --git a/l1-contracts/contracts/dev-contracts/test/CustomUpgradeTest.sol b/l1-contracts/contracts/dev-contracts/test/CustomUpgradeTest.sol
@@ -25,7 +25,7 @@ contract CustomUpgradeTest is BaseZkSyncUpgrade {
     /// upgrade.
     function _postUpgrade(bytes calldata _customCallDataForUpgrade) internal override {}
 
-    /// @notice The main function that will be called by the upgrade proxy.
+    /// @notice The main function that will be delegate-called by the chain.
     /// @param _proposedUpgrade The upgrade to be executed.
     function upgrade(ProposedUpgrade calldata _proposedUpgrade) public override returns (bytes32) {
         (uint32 newMinorVersion, bool isPatchOnly) = _setNewProtocolVersion(_proposedUpgrade.newProtocolVersion);

diff --git a/l1-contracts/contracts/dev-contracts/test/DummySharedBridge.sol b/l1-contracts/contracts/dev-contracts/test/DummySharedBridge.sol
@@ -10,7 +10,6 @@ import {TWO_BRIDGES_MAGIC_VALUE, ETH_TOKEN_ADDRESS} from "../../common/Config.so
 import {IL1NativeTokenVault} from "../../bridge/ntv/L1NativeTokenVault.sol";
 import {L2_NATIVE_TOKEN_VAULT_ADDR} from "../../common/L2ContractAddresses.sol";
 import {SafeERC20} from "@openzeppelin/contracts-v4/token/ERC20/utils/SafeERC20.sol";
-import {IL2Bridge} from "../../bridge/interfaces/IL2Bridge.sol";
 import {IL2SharedBridgeLegacy} from "../../bridge/interfaces/IL2SharedBridgeLegacy.sol";
 import {IL2SharedBridgeLegacyFunctions} from "../../bridge/interfaces/IL2SharedBridgeLegacyFunctions.sol";
 

diff --git a/l1-contracts/contracts/governance/PermanentRestriction.sol b/l1-contracts/contracts/governance/PermanentRestriction.sol
@@ -235,7 +235,7 @@ contract PermanentRestriction is IRestriction, IPermanentRestriction, Ownable2St
 
         // Note, that we do not use an explicit call here to ensure that the function does not panic in case of
         // incorrect `_chain` address.
-        (bool success, bytes memory data) = _chain.staticcall(abi.encodeWithSelector(IGetters.getChainId.selector));
+        (bool success, bytes memory data) = _chain.staticcall(abi.encodeCall(IGetters.getChainId, ()));
         if (!success || data.length < 32) {
             revert NotAHyperchain(_chain);
         }

diff --git a/l1-contracts/contracts/transactionFilterer/GatewayTransactionFilterer.sol b/l1-contracts/contracts/transactionFilterer/GatewayTransactionFilterer.sol
@@ -8,7 +8,6 @@ import {ReentrancyGuard} from "../common/ReentrancyGuard.sol";
 import {AlreadyWhitelisted, InvalidSelector, NotWhitelisted, ZeroAddress} from "../common/L1ContractErrors.sol";
 import {ITransactionFilterer} from "../state-transition/chain-interfaces/ITransactionFilterer.sol";
 import {IBridgehub} from "../bridgehub/IBridgehub.sol";
-import {IL2Bridge} from "../bridge/interfaces/IL2Bridge.sol";
 import {IAssetRouterBase} from "../bridge/asset-router/IAssetRouterBase.sol";
 
 /// @author Matter Labs
@@ -82,16 +81,13 @@ contract GatewayTransactionFilterer is ITransactionFilterer, ReentrancyGuard, Ow
     ) external view returns (bool) {
         if (sender == L1_ASSET_ROUTER) {
             bytes4 l2TxSelector = bytes4(l2Calldata[:4]);
-            if (
-                (IAssetRouterBase.finalizeDeposit.selector != l2TxSelector) &&
-                (IL2Bridge.finalizeDeposit.selector != l2TxSelector)
-            ) {
+            if (IAssetRouterBase.finalizeDeposit.selector != l2TxSelector) {
                 revert InvalidSelector(l2TxSelector);
             }
 
             (, bytes32 decodedAssetId, ) = abi.decode(l2Calldata[4:], (uint256, bytes32, bytes));
-            address stmAddress = BRIDGE_HUB.ctmAssetIdToAddress(decodedAssetId);
-            return (stmAddress != address(0));
+            address ctmAddress = BRIDGE_HUB.ctmAssetIdToAddress(decodedAssetId);
+            return (ctmAddress != address(0));
         }
 
         return whitelistedSenders[sender];

diff --git a/l1-contracts/contracts/upgrades/BaseZkSyncUpgrade.sol b/l1-contracts/contracts/upgrades/BaseZkSyncUpgrade.sol
@@ -63,7 +63,7 @@ abstract contract BaseZkSyncUpgrade is ZKChainBase {
     /// @notice Notifies about complete upgrade
     event UpgradeComplete(uint256 indexed newProtocolVersion, bytes32 indexed l2UpgradeTxHash, ProposedUpgrade upgrade);
 
-    /// @notice The main function that will be provided by the upgrade proxy
+    /// @notice The main function that will be delegate-called by the chain.
     /// @dev This is a virtual function and should be overridden by custom upgrade implementations.
     /// @param _proposedUpgrade The upgrade to be executed.
     /// @return txHash The hash of the L2 system contract upgrade transaction.

diff --git a/l1-contracts/contracts/upgrades/DefaultUpgrade.sol b/l1-contracts/contracts/upgrades/DefaultUpgrade.sol
@@ -8,7 +8,7 @@ import {BaseZkSyncUpgrade, ProposedUpgrade} from "./BaseZkSyncUpgrade.sol";
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 contract DefaultUpgrade is BaseZkSyncUpgrade {
-    /// @notice The main function that will be called by the upgrade proxy.
+    /// @notice The main function that will be delegate-called by the chain.
     /// @param _proposedUpgrade The upgrade to be executed.
     function upgrade(ProposedUpgrade calldata _proposedUpgrade) public override returns (bytes32) {
         super.upgrade(_proposedUpgrade);

diff --git a/l1-contracts/contracts/upgrades/GatewayUpgrade.sol b/l1-contracts/contracts/upgrades/GatewayUpgrade.sol
@@ -22,13 +22,15 @@ contract GatewayUpgrade is BaseZkSyncUpgrade {
     using PriorityQueue for PriorityQueue.Queue;
     using PriorityTree for PriorityTree.Tree;
 
+    /// @notice The address of this contract.
+    /// @dev needed as this address is delegateCalled, and we delegateCall it again.
     address public immutable THIS_ADDRESS;
 
     constructor() {
         THIS_ADDRESS = address(this);
     }
 
-    /// @notice The main function that will be called by the upgrade proxy.
+    /// @notice The main function that will be delegate-called by the chain.
     /// @param _proposedUpgrade The upgrade to be executed.
     /// @dev Doesn't require any access-control restrictions as the contract is used in the delegate call.
     function upgrade(ProposedUpgrade calldata _proposedUpgrade) public override returns (bytes32) {
@@ -48,9 +50,7 @@ contract GatewayUpgrade is BaseZkSyncUpgrade {
             l2TxDataFinish
         );
         // slither-disable-next-line controlled-delegatecall
-        (bool success, ) = THIS_ADDRESS.delegatecall(
-            abi.encodeWithSelector(IGatewayUpgrade.upgradeExternal.selector, proposedUpgrade)
-        );
+        (bool success, ) = THIS_ADDRESS.delegatecall(abi.encodeCall(IGatewayUpgrade.upgradeExternal, proposedUpgrade));
         // solhint-disable-next-line gas-custom-errors
         require(success, "GatewayUpgrade: upgrade failed");
         return Diamond.DIAMOND_INIT_SUCCESS_RETURN_VALUE;

diff --git a/l1-contracts/contracts/upgrades/IGatewayUpgrade.sol b/l1-contracts/contracts/upgrades/IGatewayUpgrade.sol
@@ -4,6 +4,13 @@ pragma solidity 0.8.24;
 
 import {ProposedUpgrade} from "./BaseZkSyncUpgrade.sol";
 
+/**
+ * @author Matter Labs
+ * @notice Gateway upgrade interface. Used for the protocol upgrade that introduces the Gateway.
+ */
 interface IGatewayUpgrade {
+    /// @notice The upgrade function called from within this same contract
+    /// @dev This is needed for memory -> calldata conversion of the _upgrade arg.
+    /// @param _upgrade The upgrade to be executed.
     function upgradeExternal(ProposedUpgrade calldata _upgrade) external returns (bytes32);
 }
diff --git a/l1-contracts/contracts/upgrades/IL1GenesisUpgrade.sol b/l1-contracts/contracts/upgrades/IL1GenesisUpgrade.sol
@@ -4,15 +4,31 @@ pragma solidity 0.8.24;
 
 import {L2CanonicalTransaction} from "../common/Messaging.sol";
 
+/**
+ * @author Matter Labs
+ * @notice L1 genesis upgrade interface. Every chain has to process an upgrade txs at its genesis.
+ * @notice This is needed to set system params like the chainId and to deploy some system contracts.
+ */
 interface IL1GenesisUpgrade {
     /// @dev emitted when a chain registers and a GenesisUpgrade happens
+    /// @param _zkChain the address of the zk chain
+    /// @param _l2Transaction the l2 genesis upgrade transaction
+    /// @param _protocolVersion the current protocol version
+    /// @param _factoryDeps the factory dependencies needed for the upgrade
     event GenesisUpgrade(
         address indexed _zkChain,
         L2CanonicalTransaction _l2Transaction,
         uint256 indexed _protocolVersion,
         bytes[] _factoryDeps
     );
 
+    /// @notice The main function that will be called by the Admin facet at genesis.
+    /// @param _l1GenesisUpgrade the address of the l1 genesis upgrade
+    /// @param _chainId the chain id
+    /// @param _protocolVersion the current protocol version
+    /// @param _l1CtmDeployerAddress the address of the l1 ctm deployer
+    /// @param _forceDeployments the force deployments
+    /// @param _factoryDeps the factory dependencies
     function genesisUpgrade(
         address _l1GenesisUpgrade,
         uint256 _chainId,

diff --git a/l1-contracts/contracts/upgrades/L1GenesisUpgrade.sol b/l1-contracts/contracts/upgrades/L1GenesisUpgrade.sol
@@ -21,7 +21,13 @@ import {L2ContractHelper} from "../common/libraries/L2ContractHelper.sol";
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 contract L1GenesisUpgrade is IL1GenesisUpgrade, BaseZkSyncUpgradeGenesis {
-    /// @notice The main function that will be called by the upgrade proxy.
+    /// @notice The main function that will be delegate-called by the chain Admin facet.
+    /// @param _l1GenesisUpgrade the address of the l1 genesis upgrade
+    /// @param _chainId the chain id
+    /// @param _protocolVersion the current protocol version
+    /// @param _l1CtmDeployerAddress the address of the l1 ctm deployer
+    /// @param _forceDeploymentsData the force deployments data
+    /// @param _factoryDeps the factory dependencies
     function genesisUpgrade(
         address _l1GenesisUpgrade,
         uint256 _chainId,

diff --git a/l1-contracts/test/foundry/l2/unit/utils/L2Utils.sol b/l1-contracts/test/foundry/l2/unit/utils/L2Utils.sol
@@ -4,7 +4,7 @@ pragma solidity ^0.8.20;
 
 import {Vm} from "forge-std/Vm.sol";
 
-import {DEPLOYER_SYSTEM_CONTRACT, L2_ASSET_ROUTER_ADDR, L2_NATIVE_TOKEN_VAULT_ADDR} from "contracts/common/L2ContractAddresses.sol";
+import {L2_DEPLOYER_SYSTEM_CONTRACT_ADDR, L2_ASSET_ROUTER_ADDR, L2_NATIVE_TOKEN_VAULT_ADDR} from "contracts/common/L2ContractAddresses.sol";
 import {IContractDeployer, L2ContractHelper} from "contracts/common/libraries/L2ContractHelper.sol";
 
 import {L2AssetRouter} from "contracts/bridge/asset-router/L2AssetRouter.sol";
@@ -55,7 +55,7 @@ library L2Utils {
      */
     function initSystemContracts() internal {
         bytes memory contractDeployerBytecode = readSystemContractsBytecode("ContractDeployer");
-        vm.etch(DEPLOYER_SYSTEM_CONTRACT, contractDeployerBytecode);
+        vm.etch(L2_DEPLOYER_SYSTEM_CONTRACT_ADDR, contractDeployerBytecode);
     }
 
     /// @notice Deploys the L2AssetRouter contract.
@@ -89,7 +89,7 @@ library L2Utils {
         });
 
         vm.prank(L2_FORCE_DEPLOYER_ADDR);
-        IContractDeployer(DEPLOYER_SYSTEM_CONTRACT).forceDeployOnAddresses(deployments);
+        IContractDeployer(L2_DEPLOYER_SYSTEM_CONTRACT_ADDR).forceDeployOnAddresses(deployments);
     }
 
     /// @notice Deploys the L2NativeTokenVault contract.
@@ -146,7 +146,7 @@ library L2Utils {
         });
 
         vm.prank(L2_FORCE_DEPLOYER_ADDR);
-        IContractDeployer(DEPLOYER_SYSTEM_CONTRACT).forceDeployOnAddresses(deployments);
+        IContractDeployer(L2_DEPLOYER_SYSTEM_CONTRACT_ADDR).forceDeployOnAddresses(deployments);
     }
 
     /// @notice Encodes the token data.

diff --git a/l1-contracts/test/foundry/l2/unit/weth/WETH.t.sol b/l1-contracts/test/foundry/l2/unit/weth/WETH.t.sol
@@ -7,7 +7,7 @@ import {Test} from "forge-std/Test.sol";
 import {L2WrappedBaseToken} from "contracts/bridge/L2WrappedBaseToken.sol";
 import {TransparentUpgradeableProxy} from "@openzeppelin/contracts-v4/proxy/transparent/TransparentUpgradeableProxy.sol";
 
-import {Unauthorized, UnimplementedMessage, BridgeMintNotImplemented} from "contracts/common/L1ContractErrors.sol";
+import {Unauthorized, BridgeMintNotImplemented} from "contracts/common/L1ContractErrors.sol";
 
 contract WethTest is Test {
     L2WrappedBaseToken internal weth;