From 23bb5fb674a26d8880971a92fc01ec0426ee64e4 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Wed, 14 May 2025 14:38:51 +0100 Subject: [PATCH 1/6] [U16] Update Anchor Root Validation approach --- .../src/L1/StandardValidator.sol | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/contracts-bedrock/src/L1/StandardValidator.sol b/packages/contracts-bedrock/src/L1/StandardValidator.sol index 4e8624c83cac1..6b9f33332ba73 100644 --- a/packages/contracts-bedrock/src/L1/StandardValidator.sol +++ b/packages/contracts-bedrock/src/L1/StandardValidator.sol @@ -6,6 +6,7 @@ import { GameType, Claim, GameTypes } from "src/dispute/lib/Types.sol"; import { Duration } from "src/dispute/lib/LibUDT.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; import { Constants } from "src/libraries/Constants.sol"; +import { Hash } from "src/dispute/lib/Types.sol"; // Interfaces import { ISystemConfig } from "interfaces/L1/ISystemConfig.sol"; @@ -347,6 +348,16 @@ contract StandardValidator { ); _errors = internalRequire(_game.challenger() == challenger, "PDDG-120", _errors); + (Hash anchorRoot,) = _game.anchorStateRegistry().getAnchorRoot(); + bytes32 _anchorRoot = Hash.unwrap(anchorRoot); + _errors = internalRequire(_anchorRoot != bytes32(0), "PDDG-130", _errors); + + if (_anchorRoot == bytes32(0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead)) { + _errors = internalRequire(_game.l2SequenceNumber() == 0, "PDDG-140", _errors); + } else { + _errors = internalRequire(_game.l2SequenceNumber() != 0, "PDDG-150", _errors); + } + return _errors; } @@ -375,6 +386,16 @@ contract StandardValidator { _errors, _sysCfg, _game, _factory, _absolutePrestate, _l2ChainID, _admin, GameTypes.CANNON, "PLDG" ); + (Hash anchorRoot,) = _game.anchorStateRegistry().getAnchorRoot(); + bytes32 _anchorRoot = Hash.unwrap(anchorRoot); + _errors = internalRequire( + _anchorRoot != bytes32(0) + || _anchorRoot != bytes32(0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead), + "PLDG-130", + _errors + ); + _errors = internalRequire(_game.l2SequenceNumber() != 0, "PLDG-140", _errors); + return _errors; } From 206f7939ec3745a1f3b4a25b9cbc7993c1f08c74 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Wed, 14 May 2025 15:00:00 +0100 Subject: [PATCH 2/6] Update packages/contracts-bedrock/src/L1/StandardValidator.sol Co-authored-by: Adrian Sutton --- packages/contracts-bedrock/src/L1/StandardValidator.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts-bedrock/src/L1/StandardValidator.sol b/packages/contracts-bedrock/src/L1/StandardValidator.sol index 6b9f33332ba73..7354da30472ba 100644 --- a/packages/contracts-bedrock/src/L1/StandardValidator.sol +++ b/packages/contracts-bedrock/src/L1/StandardValidator.sol @@ -390,7 +390,7 @@ contract StandardValidator { bytes32 _anchorRoot = Hash.unwrap(anchorRoot); _errors = internalRequire( _anchorRoot != bytes32(0) - || _anchorRoot != bytes32(0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead), + && _anchorRoot != bytes32(0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead), "PLDG-130", _errors ); From 9cbfa3c6c8193a79f88a822a1dbc8dfb29229f85 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Wed, 14 May 2025 15:24:38 +0100 Subject: [PATCH 3/6] fix tests --- .../test/L1/StandardValidator.t.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol index 1ed2836fd4f13..631b1df3b5a44 100644 --- a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol +++ b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol @@ -34,11 +34,13 @@ import { IPreimageOracle } from "interfaces/cannon/IPreimageOracle.sol"; import { IL1StandardBridge } from "interfaces/L1/IL1StandardBridge.sol"; import { IProxyAdminOwnedBase } from "interfaces/L1/IProxyAdminOwnedBase.sol"; import { IStandardBridge } from "interfaces/universal/IStandardBridge.sol"; +import { IDisputeGame } from "interfaces/dispute/IDisputeGame.sol"; /// @title BadDisputeGameFactoryReturner /// @notice Used to return a bad DisputeGameFactory address to the StandardValidator. Far easier /// than the alternative ways of mocking this value since the normal vm.mockCall will cause /// the validation function to revert. + contract BadDisputeGameFactoryReturner { /// @notice Address of the StandardValidator instance. StandardValidator public immutable validator; @@ -191,6 +193,17 @@ contract StandardValidator_TestInit is CommonTest { // Add the FaultDisputeGame to the DisputeGameFactory. vm.prank(disputeGameFactory.owner()); disputeGameFactory.setImplementation(GameTypes.CANNON, IDisputeGame(address(fdg))); + + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.PERMISSIONED_CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(1) + ); + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(1) + ); } } From 9728357d50165b636beded29dffe98e24689a4ee Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Wed, 14 May 2025 16:10:48 +0100 Subject: [PATCH 4/6] fixes --- packages/contracts-bedrock/src/L1/StandardValidator.sol | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/contracts-bedrock/src/L1/StandardValidator.sol b/packages/contracts-bedrock/src/L1/StandardValidator.sol index 7354da30472ba..664be71573aaa 100644 --- a/packages/contracts-bedrock/src/L1/StandardValidator.sol +++ b/packages/contracts-bedrock/src/L1/StandardValidator.sol @@ -352,7 +352,7 @@ contract StandardValidator { bytes32 _anchorRoot = Hash.unwrap(anchorRoot); _errors = internalRequire(_anchorRoot != bytes32(0), "PDDG-130", _errors); - if (_anchorRoot == bytes32(0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead)) { + if (_anchorRoot == bytes32(hex"dead")) { _errors = internalRequire(_game.l2SequenceNumber() == 0, "PDDG-140", _errors); } else { _errors = internalRequire(_game.l2SequenceNumber() != 0, "PDDG-150", _errors); @@ -388,12 +388,7 @@ contract StandardValidator { (Hash anchorRoot,) = _game.anchorStateRegistry().getAnchorRoot(); bytes32 _anchorRoot = Hash.unwrap(anchorRoot); - _errors = internalRequire( - _anchorRoot != bytes32(0) - && _anchorRoot != bytes32(0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead), - "PLDG-130", - _errors - ); + _errors = internalRequire(_anchorRoot != bytes32(0) && _anchorRoot != bytes32(hex"dead"), "PLDG-130", _errors); _errors = internalRequire(_game.l2SequenceNumber() != 0, "PLDG-140", _errors); return _errors; From 45e7df49bf42c6e9e8295a70525897d3483f6e61 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Wed, 14 May 2025 21:13:49 +0100 Subject: [PATCH 5/6] fix failing tests --- .../test/L1/StandardValidator.t.sol | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol index e775f43538b67..ef24f4164ad67 100644 --- a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol +++ b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol @@ -200,18 +200,18 @@ contract StandardValidator_TestInit is CommonTest { // Add the FaultDisputeGame to the DisputeGameFactory. vm.prank(disputeGameFactory.owner()); disputeGameFactory.setImplementation(GameTypes.CANNON, IDisputeGame(address(fdg))); - - vm.mockCall( - address(disputeGameFactory.gameImpls(GameTypes.PERMISSIONED_CANNON)), - abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), - abi.encode(1) - ); - vm.mockCall( - address(disputeGameFactory.gameImpls(GameTypes.CANNON)), - abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), - abi.encode(1) - ); } + + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.PERMISSIONED_CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(1) + ); + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(1) + ); } /// @notice Runs the StandardValidator.validate function. From ab757a2e4a9aed914f4d76a2826ed773c93a8c8c Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Wed, 14 May 2025 22:19:51 +0100 Subject: [PATCH 6/6] improve, add tests --- .../src/L1/StandardValidator.sol | 7 +- .../test/L1/StandardValidator.t.sol | 66 +++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/src/L1/StandardValidator.sol b/packages/contracts-bedrock/src/L1/StandardValidator.sol index 2165d319a6f01..5731468c7c45b 100644 --- a/packages/contracts-bedrock/src/L1/StandardValidator.sol +++ b/packages/contracts-bedrock/src/L1/StandardValidator.sol @@ -399,8 +399,8 @@ contract StandardValidator { (Hash anchorRoot,) = _game.anchorStateRegistry().getAnchorRoot(); bytes32 _anchorRoot = Hash.unwrap(anchorRoot); - _errors = internalRequire(_anchorRoot != bytes32(0), "PDDG-130", _errors); + _errors = internalRequire(_anchorRoot != bytes32(0), "PDDG-130", _errors); if (_anchorRoot == bytes32(hex"dead")) { _errors = internalRequire(_game.l2SequenceNumber() == 0, "PDDG-140", _errors); } else { @@ -447,8 +447,9 @@ contract StandardValidator { (Hash anchorRoot,) = _game.anchorStateRegistry().getAnchorRoot(); bytes32 _anchorRoot = Hash.unwrap(anchorRoot); - _errors = internalRequire(_anchorRoot != bytes32(0) && _anchorRoot != bytes32(hex"dead"), "PLDG-130", _errors); - _errors = internalRequire(_game.l2SequenceNumber() != 0, "PLDG-140", _errors); + _errors = internalRequire(_anchorRoot != bytes32(0), "PLDG-130", _errors); + _errors = internalRequire(_anchorRoot != bytes32(hex"dead"), "PLDG-140", _errors); + _errors = internalRequire(_game.l2SequenceNumber() != 0, "PLDG-150", _errors); return _errors; } diff --git a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol index ef24f4164ad67..7d25659322f69 100644 --- a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol +++ b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol @@ -753,6 +753,72 @@ contract StandardValidator_validate_Test is StandardValidator_TestInit { assertEq("PDDG-120", _validate(true)); } + /// @notice Tests that the validate function successfully returns the right error when the anchor root is + /// bytes32(hex"dead") but the l2 sequence number is not 0. + function test_validate_0xdeadAnchorRootAndNonZeroSequenceNumber_succeeds() public { + uint256 randomSequenceNumber = 1; // this does not correspond to the sequence number of the games. + vm.mockCall( + address(anchorStateRegistry), + abi.encodeCall(IAnchorStateRegistry.getAnchorRoot, ()), + abi.encode(bytes32(hex"dead"), randomSequenceNumber) + ); + assertEq("PDDG-140,PLDG-140", _validate(true)); + } + + function test_validate_0xdeadAnchorRootAndZeroSequenceNumber_succeeds() public { + uint256 randomSequenceNumber = 1; // this does not correspond to the sequence number of the games. + vm.mockCall( + address(anchorStateRegistry), + abi.encodeCall(IAnchorStateRegistry.getAnchorRoot, ()), + abi.encode(bytes32(hex"dead"), randomSequenceNumber) + ); + + // Set the sequence number returned by the games to be 0 + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.PERMISSIONED_CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(0) + ); + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(0) + ); + assertEq("PLDG-140,PLDG-150", _validate(true)); + } + + function test_validate_0x00AnchorRoot_succeeds() public { + vm.mockCall( + address(anchorStateRegistry), + abi.encodeCall(IAnchorStateRegistry.getAnchorRoot, ()), + abi.encode(bytes32(hex"00"), 1) + ); + assertEq("PDDG-130,PLDG-130", _validate(true)); + } + + function test_validate_nonZeroOrDeadAnchorRootAndZeroSequenceNumber_succeeds() public { + uint256 randomSequenceNumber = 1; // this does not correspond to the sequence number of the games. + vm.mockCall( + address(anchorStateRegistry), + abi.encodeCall(IAnchorStateRegistry.getAnchorRoot, ()), + abi.encode(bytes32(hex"01"), randomSequenceNumber) + ); + + // Set the sequence number returned by the games to be 0 + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.PERMISSIONED_CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(0) + ); + vm.mockCall( + address(disputeGameFactory.gameImpls(GameTypes.CANNON)), + abi.encodeCall(IDisputeGame.l2SequenceNumber, ()), + abi.encode(0) + ); + + assertEq("PDDG-150,PLDG-150", _validate(true)); + } + /// @notice Tests that the validate function successfully returns the right overrides error when the /// PermissionedDisputeGame challenger is overriden but is correct. function test_validate_overridenPermissionedDisputeGameChallenger_succeeds() public {