From 6998958ef256ee7b3e21dc6a0459ae9d200bb225 Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Mon, 8 Jan 2024 12:53:50 +0100 Subject: [PATCH 1/4] make upgrade validation more strict --- .../contracts/upgrades/BaseZkSyncUpgrade.sol | 30 +++++++++++++++++-- .../contracts/upgrades/DefaultUpgrade.sol | 17 ----------- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol index 71e57badc..8965aaf3c 100644 --- a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol +++ b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol @@ -65,6 +65,22 @@ abstract contract BaseZkSyncUpgrade is Base { // on the L2 side would be inaccurate. The effects of this "back-dating" of L2 upgrade batches will be reduced // as the permitted delay window is reduced in the future. require(block.timestamp >= _proposedUpgrade.upgradeTimestamp, "Upgrade is not ready yet"); + + _setNewProtocolVersion(_proposedUpgrade.newProtocolVersion); + _upgradeL1Contract(_proposedUpgrade.l1ContractsUpgradeCalldata); + _upgradeVerifier(_proposedUpgrade.verifier, _proposedUpgrade.verifierParams); + _setBaseSystemContracts(_proposedUpgrade.bootloaderHash, _proposedUpgrade.defaultAccountHash); + + bytes32 txHash; + txHash = _setL2SystemContractUpgrade( + _proposedUpgrade.l2ProtocolUpgradeTx, + _proposedUpgrade.factoryDeps, + _proposedUpgrade.newProtocolVersion + ); + + _postUpgrade(_proposedUpgrade.postUpgradeCalldata); + + emit UpgradeComplete(_proposedUpgrade.newProtocolVersion, txHash, _proposedUpgrade); } /// @notice Change default account bytecode hash, that is used on L2 @@ -104,13 +120,15 @@ abstract contract BaseZkSyncUpgrade is Base { /// @notice Change the address of the verifier smart contract /// @param _newVerifier Verifier smart contract address function _setVerifier(IVerifier _newVerifier) private { + if (_newVerifier == IVerifier(address(0))) { + return; + } + // An upgrade to the verifier must be done carefully to ensure there aren't batches in the committed state // during the transition. If verifier is upgraded, it will immediately be used to prove all committed batches. // Batches committed expecting the old verifier will fail. Ensure all commited batches are finalized before the // verifier is upgraded. - if (_newVerifier == IVerifier(address(0))) { - return; - } + require(s.totalBatchesCommitted == s.totalBatchesVerified, "All committed batches must be verified first"); IVerifier oldVerifier = s.verifier; s.verifier = _newVerifier; @@ -128,6 +146,12 @@ abstract contract BaseZkSyncUpgrade is Base { return; } + // An upgrade to the verifier params must be done carefully to ensure there aren't batches in the committed state + // during the transition. If verifier verifier are upgraded, it will immediately be used to prove all committed batches. + // Batches committed expecting the old verifier params will fail. Ensure all commited batches are finalized before the + // verifier is upgraded. + require(s.totalBatchesCommitted == s.totalBatchesVerified, "All committed batches must be verified first"); + VerifierParams memory oldVerifierParams = s.verifierParams; s.verifierParams = _newVerifierParams; emit NewVerifierParams(oldVerifierParams, _newVerifierParams); diff --git a/ethereum/contracts/upgrades/DefaultUpgrade.sol b/ethereum/contracts/upgrades/DefaultUpgrade.sol index cd2bdd29f..790a9aa7d 100644 --- a/ethereum/contracts/upgrades/DefaultUpgrade.sol +++ b/ethereum/contracts/upgrades/DefaultUpgrade.sol @@ -24,23 +24,6 @@ contract DefaultUpgrade is BaseZkSyncUpgrade { /// @param _proposedUpgrade The upgrade to be executed. function upgrade(ProposedUpgrade calldata _proposedUpgrade) public override returns (bytes32) { super.upgrade(_proposedUpgrade); - - _setNewProtocolVersion(_proposedUpgrade.newProtocolVersion); - _upgradeL1Contract(_proposedUpgrade.l1ContractsUpgradeCalldata); - _upgradeVerifier(_proposedUpgrade.verifier, _proposedUpgrade.verifierParams); - _setBaseSystemContracts(_proposedUpgrade.bootloaderHash, _proposedUpgrade.defaultAccountHash); - - bytes32 txHash; - txHash = _setL2SystemContractUpgrade( - _proposedUpgrade.l2ProtocolUpgradeTx, - _proposedUpgrade.factoryDeps, - _proposedUpgrade.newProtocolVersion - ); - - _postUpgrade(_proposedUpgrade.postUpgradeCalldata); - - emit UpgradeComplete(_proposedUpgrade.newProtocolVersion, txHash, _proposedUpgrade); - return Diamond.DIAMOND_INIT_SUCCESS_RETURN_VALUE; } } From b34ca08fc6a38a25223f0e19ebe0ff11622e07b6 Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Mon, 8 Jan 2024 13:15:33 +0100 Subject: [PATCH 2/4] remove the requires --- .../contracts/upgrades/BaseZkSyncUpgrade.sol | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol index 8965aaf3c..56989c1bd 100644 --- a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol +++ b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol @@ -120,15 +120,13 @@ abstract contract BaseZkSyncUpgrade is Base { /// @notice Change the address of the verifier smart contract /// @param _newVerifier Verifier smart contract address function _setVerifier(IVerifier _newVerifier) private { - if (_newVerifier == IVerifier(address(0))) { - return; - } - // An upgrade to the verifier must be done carefully to ensure there aren't batches in the committed state // during the transition. If verifier is upgraded, it will immediately be used to prove all committed batches. // Batches committed expecting the old verifier will fail. Ensure all commited batches are finalized before the // verifier is upgraded. - require(s.totalBatchesCommitted == s.totalBatchesVerified, "All committed batches must be verified first"); + if (_newVerifier == IVerifier(address(0))) { + return; + } IVerifier oldVerifier = s.verifier; s.verifier = _newVerifier; @@ -138,6 +136,10 @@ abstract contract BaseZkSyncUpgrade is Base { /// @notice Change the verifier parameters /// @param _newVerifierParams New parameters for the verifier function _setVerifierParams(VerifierParams calldata _newVerifierParams) private { + // An upgrade to the verifier params must be done carefully to ensure there aren't batches in the committed state + // during the transition. If verifier verifier are upgraded, it will immediately be used to prove all committed batches. + // Batches committed expecting the old verifier params will fail. Ensure all commited batches are finalized before the + // verifier is upgraded. if ( _newVerifierParams.recursionNodeLevelVkHash == bytes32(0) && _newVerifierParams.recursionLeafLevelVkHash == bytes32(0) && @@ -146,12 +148,6 @@ abstract contract BaseZkSyncUpgrade is Base { return; } - // An upgrade to the verifier params must be done carefully to ensure there aren't batches in the committed state - // during the transition. If verifier verifier are upgraded, it will immediately be used to prove all committed batches. - // Batches committed expecting the old verifier params will fail. Ensure all commited batches are finalized before the - // verifier is upgraded. - require(s.totalBatchesCommitted == s.totalBatchesVerified, "All committed batches must be verified first"); - VerifierParams memory oldVerifierParams = s.verifierParams; s.verifierParams = _newVerifierParams; emit NewVerifierParams(oldVerifierParams, _newVerifierParams); From fc8f46b05e958de99b0f67bf49ca36a59247db42 Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Tue, 9 Jan 2024 22:26:34 +0100 Subject: [PATCH 3/4] fix compilation --- .../dev-contracts/test/CustomUpgradeTest.sol | 4 ++-- ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol | 12 ++++++++++++ ethereum/contracts/upgrades/DefaultUpgrade.sol | 12 ------------ 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ethereum/contracts/dev-contracts/test/CustomUpgradeTest.sol b/ethereum/contracts/dev-contracts/test/CustomUpgradeTest.sol index fcd3f5c7f..b9764336a 100644 --- a/ethereum/contracts/dev-contracts/test/CustomUpgradeTest.sol +++ b/ethereum/contracts/dev-contracts/test/CustomUpgradeTest.sol @@ -11,7 +11,7 @@ contract CustomUpgradeTest is BaseZkSyncUpgrade { /// @notice Placeholder function for custom logic for upgrading L1 contract. /// Typically this function will never be used. /// @param _customCallDataForUpgrade Custom data for upgrade, which may be interpreted differently for each upgrade. - function _upgradeL1Contract(bytes calldata _customCallDataForUpgrade) internal { + function _upgradeL1Contract(bytes calldata _customCallDataForUpgrade) internal override { emit Test(); } @@ -19,7 +19,7 @@ contract CustomUpgradeTest is BaseZkSyncUpgrade { /// Typically this function will never be used. /// @param _customCallDataForUpgrade Custom data for an upgrade, which may be interpreted differently for each /// upgrade. - function _postUpgrade(bytes calldata _customCallDataForUpgrade) internal virtual {} + function _postUpgrade(bytes calldata _customCallDataForUpgrade) internal override {} /// @notice The main function that will be called by the upgrade proxy. /// @param _proposedUpgrade The upgrade to be executed. diff --git a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol index 56989c1bd..02ba2e05d 100644 --- a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol +++ b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol @@ -248,4 +248,16 @@ abstract contract BaseZkSyncUpgrade is Base { s.protocolVersion = _newProtocolVersion; emit NewProtocolVersion(previousProtocolVersion, _newProtocolVersion); } + + /// @notice Placeholder function for custom logic for upgrading L1 contract. + /// Typically this function will never be used. + /// @param _customCallDataForUpgrade Custom data for an upgrade, which may be interpreted differently for each + /// upgrade. + function _upgradeL1Contract(bytes calldata _customCallDataForUpgrade) internal virtual {} + + /// @notice placeholder function for custom logic for post-upgrade logic. + /// Typically this function will never be used. + /// @param _customCallDataForUpgrade Custom data for an upgrade, which may be interpreted differently for each + /// upgrade. + function _postUpgrade(bytes calldata _customCallDataForUpgrade) internal virtual {} } diff --git a/ethereum/contracts/upgrades/DefaultUpgrade.sol b/ethereum/contracts/upgrades/DefaultUpgrade.sol index 790a9aa7d..1e8f8b428 100644 --- a/ethereum/contracts/upgrades/DefaultUpgrade.sol +++ b/ethereum/contracts/upgrades/DefaultUpgrade.sol @@ -8,18 +8,6 @@ import "./BaseZkSyncUpgrade.sol"; /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev contract DefaultUpgrade is BaseZkSyncUpgrade { - /// @notice Placeholder function for custom logic for upgrading L1 contract. - /// Typically this function will never be used. - /// @param _customCallDataForUpgrade Custom data for an upgrade, which may be interpreted differently for each - /// upgrade. - function _upgradeL1Contract(bytes calldata _customCallDataForUpgrade) internal virtual {} - - /// @notice placeholder function for custom logic for post-upgrade logic. - /// Typically this function will never be used. - /// @param _customCallDataForUpgrade Custom data for an upgrade, which may be interpreted differently for each - /// upgrade. - function _postUpgrade(bytes calldata _customCallDataForUpgrade) internal virtual {} - /// @notice The main function that will be called by the upgrade proxy. /// @param _proposedUpgrade The upgrade to be executed. function upgrade(ProposedUpgrade calldata _proposedUpgrade) public override returns (bytes32) { From 0dcfed1267551ce4c89cc6978f46eebdf1c3d1e3 Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Thu, 11 Jan 2024 10:54:36 +0100 Subject: [PATCH 4/4] fix typo --- ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol index 02ba2e05d..a20cc92b0 100644 --- a/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol +++ b/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol @@ -137,7 +137,7 @@ abstract contract BaseZkSyncUpgrade is Base { /// @param _newVerifierParams New parameters for the verifier function _setVerifierParams(VerifierParams calldata _newVerifierParams) private { // An upgrade to the verifier params must be done carefully to ensure there aren't batches in the committed state - // during the transition. If verifier verifier are upgraded, it will immediately be used to prove all committed batches. + // during the transition. If verifier is upgraded, it will immediately be used to prove all committed batches. // Batches committed expecting the old verifier params will fail. Ensure all commited batches are finalized before the // verifier is upgraded. if (