From 7d506993441c583343257ea9f56d5c85baaa5b36 Mon Sep 17 00:00:00 2001 From: Xi Lin Date: Fri, 25 Aug 2023 10:45:35 +0800 Subject: [PATCH] fix(contracts): OZ-L07 Potentially Misleading Verifier Event (#849) Co-authored-by: zimpha Co-authored-by: HAOYUatHZ <37070449+HAOYUatHZ@users.noreply.github.com> --- common/version/version.go | 2 +- .../foundry/InitializeL1BridgeContracts.s.sol | 4 +++ contracts/src/L1/rollup/IScrollChain.sol | 3 ++ .../rollup/MultipleVersionRollupVerifier.sol | 12 +++++++ contracts/src/L1/rollup/ScrollChain.sol | 4 +-- .../test/MultipleVersionRollupVerifier.t.sol | 34 ++++++++++++++++++- contracts/src/test/mocks/MockScrollChain.sol | 15 ++------ 7 files changed, 57 insertions(+), 17 deletions(-) diff --git a/common/version/version.go b/common/version/version.go index 96d7202a9..a981d852e 100644 --- a/common/version/version.go +++ b/common/version/version.go @@ -7,7 +7,7 @@ import ( "strings" ) -var tag = "v4.1.108" +var tag = "v4.1.109" var commit = func() string { if info, ok := debug.ReadBuildInfo(); ok { diff --git a/contracts/scripts/foundry/InitializeL1BridgeContracts.s.sol b/contracts/scripts/foundry/InitializeL1BridgeContracts.s.sol index 02e3622b3..28d8a5cc3 100644 --- a/contracts/scripts/foundry/InitializeL1BridgeContracts.s.sol +++ b/contracts/scripts/foundry/InitializeL1BridgeContracts.s.sol @@ -12,6 +12,7 @@ import {L1ScrollMessenger} from "../../src/L1/L1ScrollMessenger.sol"; import {L1StandardERC20Gateway} from "../../src/L1/gateways/L1StandardERC20Gateway.sol"; import {L1WETHGateway} from "../../src/L1/gateways/L1WETHGateway.sol"; import {L1DAIGateway} from "../../src/L1/gateways/L1DAIGateway.sol"; +import {MultipleVersionRollupVerifier} from "../../src/L1/rollup/MultipleVersionRollupVerifier.sol"; import {ScrollChain} from "../../src/L1/rollup/ScrollChain.sol"; import {L1MessageQueue} from "../../src/L1/rollup/L1MessageQueue.sol"; import {L2GasPriceOracle} from "../../src/L1/rollup/L2GasPriceOracle.sol"; @@ -71,6 +72,9 @@ contract InitializeL1BridgeContracts is Script { ScrollChain(L1_SCROLL_CHAIN_PROXY_ADDR).addSequencer(L1_COMMIT_SENDER_ADDRESS); ScrollChain(L1_SCROLL_CHAIN_PROXY_ADDR).addProver(L1_FINALIZE_SENDER_ADDRESS); + // initialize MultipleVersionRollupVerifier + MultipleVersionRollupVerifier(L1_MULTIPLE_VERSION_ROLLUP_VERIFIER_ADDR).initialize(L1_SCROLL_CHAIN_PROXY_ADDR); + // initialize L2GasPriceOracle L2GasPriceOracle(L2_GAS_PRICE_ORACLE_PROXY_ADDR).initialize( 21000, // _txGas diff --git a/contracts/src/L1/rollup/IScrollChain.sol b/contracts/src/L1/rollup/IScrollChain.sol index 1adb62a2e..9f86c2046 100644 --- a/contracts/src/L1/rollup/IScrollChain.sol +++ b/contracts/src/L1/rollup/IScrollChain.sol @@ -28,6 +28,9 @@ interface IScrollChain { * Public View Functions * *************************/ + /// @notice The latest finalized batch index. + function lastFinalizedBatchIndex() external view returns (uint256); + /// @notice Return the batch hash of a committed batch. /// @param batchIndex The index of the batch. function committedBatches(uint256 batchIndex) external view returns (bytes32); diff --git a/contracts/src/L1/rollup/MultipleVersionRollupVerifier.sol b/contracts/src/L1/rollup/MultipleVersionRollupVerifier.sol index 393051301..3eda7b86e 100644 --- a/contracts/src/L1/rollup/MultipleVersionRollupVerifier.sol +++ b/contracts/src/L1/rollup/MultipleVersionRollupVerifier.sol @@ -4,6 +4,7 @@ pragma solidity =0.8.16; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {IScrollChain} from "./IScrollChain.sol"; import {IRollupVerifier} from "../../libraries/verifier/IRollupVerifier.sol"; import {IZkEvmVerifier} from "../../libraries/verifier/IZkEvmVerifier.sol"; @@ -38,6 +39,9 @@ contract MultipleVersionRollupVerifier is IRollupVerifier, Ownable { /// @notice The lastest used zkevm verifier. Verifier public latestVerifier; + /// @notice The address of ScrollChain contract. + address public scrollChain; + /*************** * Constructor * ***************/ @@ -48,6 +52,12 @@ contract MultipleVersionRollupVerifier is IRollupVerifier, Ownable { latestVerifier.verifier = _verifier; } + function initialize(address _scrollChain) external onlyOwner { + require(scrollChain == address(0), "initialized"); + + scrollChain = _scrollChain; + } + /************************* * Public View Functions * *************************/ @@ -101,6 +111,8 @@ contract MultipleVersionRollupVerifier is IRollupVerifier, Ownable { /// @param _startBatchIndex The start batch index when the verifier will be used. /// @param _verifier The address of new verifier. function updateVerifier(uint64 _startBatchIndex, address _verifier) external onlyOwner { + require(_startBatchIndex > IScrollChain(scrollChain).lastFinalizedBatchIndex(), "start batch index finalized"); + Verifier memory _latestVerifier = latestVerifier; require(_startBatchIndex >= _latestVerifier.startBatchIndex, "start batch index too small"); require(_verifier != address(0), "zero verifier address"); diff --git a/contracts/src/L1/rollup/ScrollChain.sol b/contracts/src/L1/rollup/ScrollChain.sol index d274050f9..24185a7a4 100644 --- a/contracts/src/L1/rollup/ScrollChain.sol +++ b/contracts/src/L1/rollup/ScrollChain.sol @@ -67,8 +67,8 @@ contract ScrollChain is OwnableUpgradeable, PausableUpgradeable, IScrollChain { /// @notice Whether an account is a prover. mapping(address => bool) public isProver; - /// @notice The latest finalized batch index. - uint256 public lastFinalizedBatchIndex; + /// @inheritdoc IScrollChain + uint256 public override lastFinalizedBatchIndex; /// @inheritdoc IScrollChain mapping(uint256 => bytes32) public override committedBatches; diff --git a/contracts/src/test/MultipleVersionRollupVerifier.t.sol b/contracts/src/test/MultipleVersionRollupVerifier.t.sol index eba1b8b9d..3e5558ee7 100644 --- a/contracts/src/test/MultipleVersionRollupVerifier.t.sol +++ b/contracts/src/test/MultipleVersionRollupVerifier.t.sol @@ -7,6 +7,7 @@ import {DSTestPlus} from "solmate/test/utils/DSTestPlus.sol"; import {L1MessageQueue} from "../L1/rollup/L1MessageQueue.sol"; import {MultipleVersionRollupVerifier} from "../L1/rollup/MultipleVersionRollupVerifier.sol"; +import {MockScrollChain} from "./mocks/MockScrollChain.sol"; import {MockZkEvmVerifier} from "./mocks/MockZkEvmVerifier.sol"; contract MultipleVersionRollupVerifierTest is DSTestPlus { @@ -17,27 +18,54 @@ contract MultipleVersionRollupVerifierTest is DSTestPlus { MockZkEvmVerifier private v0; MockZkEvmVerifier private v1; MockZkEvmVerifier private v2; + MockScrollChain private chain; function setUp() external { v0 = new MockZkEvmVerifier(); v1 = new MockZkEvmVerifier(); v2 = new MockZkEvmVerifier(); + chain = new MockScrollChain(); verifier = new MultipleVersionRollupVerifier(address(v0)); } + function testInitialize(address _chain) external { + hevm.assume(_chain != address(0)); + + // set by non-owner, should revert + hevm.startPrank(address(1)); + hevm.expectRevert("Ownable: caller is not the owner"); + verifier.initialize(_chain); + hevm.stopPrank(); + + // succeed + assertEq(verifier.scrollChain(), address(0)); + verifier.initialize(_chain); + assertEq(verifier.scrollChain(), _chain); + + // initialized, revert + hevm.expectRevert("initialized"); + verifier.initialize(_chain); + } + function testUpdateVerifier(address _newVerifier) external { hevm.assume(_newVerifier != address(0)); + verifier.initialize(address(chain)); + // set by non-owner, should revert hevm.startPrank(address(1)); hevm.expectRevert("Ownable: caller is not the owner"); verifier.updateVerifier(0, address(0)); hevm.stopPrank(); + // start batch index finalized, revert + hevm.expectRevert("start batch index finalized"); + verifier.updateVerifier(0, address(1)); + // zero verifier address, revert hevm.expectRevert("zero verifier address"); - verifier.updateVerifier(0, address(0)); + verifier.updateVerifier(1, address(0)); // change to random operator assertEq(verifier.legacyVerifiersLength(), 0); @@ -65,6 +93,8 @@ contract MultipleVersionRollupVerifierTest is DSTestPlus { } function testGetVerifier() external { + verifier.initialize(address(chain)); + verifier.updateVerifier(100, address(v1)); verifier.updateVerifier(300, address(v2)); @@ -80,6 +110,8 @@ contract MultipleVersionRollupVerifierTest is DSTestPlus { } function testVerifyAggregateProof() external { + verifier.initialize(address(chain)); + verifier.updateVerifier(100, address(v1)); verifier.updateVerifier(300, address(v2)); diff --git a/contracts/src/test/mocks/MockScrollChain.sol b/contracts/src/test/mocks/MockScrollChain.sol index f4631f124..b9c5700d7 100644 --- a/contracts/src/test/mocks/MockScrollChain.sol +++ b/contracts/src/test/mocks/MockScrollChain.sol @@ -7,18 +7,7 @@ import {ScrollChain} from "../../L1/rollup/ScrollChain.sol"; contract MockScrollChain is ScrollChain { constructor() ScrollChain(0) {} - /* - function computePublicInputHash(uint64 accTotalL1Messages, Batch memory batch) - external - view - returns ( - bytes32, - uint64, - uint64, - uint64 - ) - { - return _computePublicInputHash(accTotalL1Messages, batch); + function setLastFinalizedBatchIndex(uint256 _lastFinalizedBatchIndex) external { + lastFinalizedBatchIndex = _lastFinalizedBatchIndex; } - */ }