diff --git a/packages/contracts-bedrock/interfaces/L1/IStandardValidator.sol b/packages/contracts-bedrock/interfaces/L1/IStandardValidator.sol index f9a080978f4e0..3e7951b895052 100644 --- a/packages/contracts-bedrock/interfaces/L1/IStandardValidator.sol +++ b/packages/contracts-bedrock/interfaces/L1/IStandardValidator.sol @@ -27,6 +27,11 @@ interface IStandardValidator { uint256 l2ChainID; } + struct ValidationOverrides { + address l1PAOMultisig; + address challenger; + } + function anchorStateRegistryImpl() external view returns (address); function anchorStateRegistryVersion() external pure returns (string memory); function challenger() external view returns (address); @@ -54,6 +59,14 @@ interface IStandardValidator { function systemConfigVersion() external pure returns (string memory); function withdrawalDelaySeconds() external view returns (uint256); + function validate( + ValidationInput memory _input, + bool _allowFailure, + ValidationOverrides memory _overrides + ) + external + view + returns (string memory); function validate(ValidationInput memory _input, bool _allowFailure) external view returns (string memory); function __constructor__( @@ -62,5 +75,6 @@ interface IStandardValidator { address _l1PAOMultisig, address _challenger, uint256 _withdrawalDelaySeconds - ) external; + ) + external; } diff --git a/packages/contracts-bedrock/snapshots/abi/StandardValidator.json b/packages/contracts-bedrock/snapshots/abi/StandardValidator.json index 0f32b6aabb477..627202995855b 100644 --- a/packages/contracts-bedrock/snapshots/abi/StandardValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/StandardValidator.json @@ -453,6 +453,69 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [ + { + "components": [ + { + "internalType": "contract IProxyAdmin", + "name": "proxyAdmin", + "type": "address" + }, + { + "internalType": "contract ISystemConfig", + "name": "sysCfg", + "type": "address" + }, + { + "internalType": "bytes32", + "name": "absolutePrestate", + "type": "bytes32" + }, + { + "internalType": "uint256", + "name": "l2ChainID", + "type": "uint256" + } + ], + "internalType": "struct StandardValidator.ValidationInput", + "name": "_input", + "type": "tuple" + }, + { + "internalType": "bool", + "name": "_allowFailure", + "type": "bool" + }, + { + "components": [ + { + "internalType": "address", + "name": "l1PAOMultisig", + "type": "address" + }, + { + "internalType": "address", + "name": "challenger", + "type": "address" + } + ], + "internalType": "struct StandardValidator.ValidationOverrides", + "name": "_overrides", + "type": "tuple" + } + ], + "name": "validate", + "outputs": [ + { + "internalType": "string", + "name": "", + "type": "string" + } + ], + "stateMutability": "view", + "type": "function" + }, { "inputs": [], "name": "withdrawalDelaySeconds", diff --git a/packages/contracts-bedrock/src/L1/StandardValidator.sol b/packages/contracts-bedrock/src/L1/StandardValidator.sol index 4e8624c83cac1..e899b548c3854 100644 --- a/packages/contracts-bedrock/src/L1/StandardValidator.sol +++ b/packages/contracts-bedrock/src/L1/StandardValidator.sol @@ -62,6 +62,11 @@ contract StandardValidator { uint256 l2ChainID; } + struct ValidationOverrides { + address l1PAOMultisig; + address challenger; + } + constructor( Implementations memory _implementations, ISuperchainConfig _superchainConfig, @@ -87,6 +92,35 @@ contract StandardValidator { mipsImpl = _implementations.mipsImpl; } + function getOverridesString(ValidationOverrides memory _overrides) private pure returns (string memory) { + string memory overridesError; + + if (_overrides.l1PAOMultisig != address(0)) { + overridesError = string.concat(overridesError, "OVERRIDES-L1PAOMULTISIG"); + } + + if (_overrides.challenger != address(0)) { + if (bytes(overridesError).length > 0) overridesError = string.concat(overridesError, ","); + overridesError = string.concat(overridesError, "OVERRIDES-CHALLENGER"); + } + + return overridesError; + } + + function expectedL1PAOMultisig(ValidationOverrides memory _overrides) internal view returns (address) { + if (_overrides.l1PAOMultisig != address(0)) { + return _overrides.l1PAOMultisig; + } + return l1PAOMultisig; + } + + function expectedChallenger(ValidationOverrides memory _overrides) internal view returns (address) { + if (_overrides.challenger != address(0)) { + return _overrides.challenger; + } + return challenger; + } + function systemConfigVersion() public pure returns (string memory) { return "3.2.0"; } @@ -140,8 +174,17 @@ contract StandardValidator { return _errors; } - function assertValidProxyAdmin(string memory _errors, IProxyAdmin _admin) internal view returns (string memory) { - _errors = internalRequire(_admin.owner() == l1PAOMultisig, "PROXYA-10", _errors); + function assertValidProxyAdmin( + string memory _errors, + IProxyAdmin _admin, + ValidationOverrides memory _overrides + ) + internal + view + returns (string memory) + { + address _l1PAOMultisig = expectedL1PAOMultisig(_overrides); + _errors = internalRequire(_admin.owner() == _l1PAOMultisig, "PROXYA-10", _errors); return _errors; } @@ -297,7 +340,8 @@ contract StandardValidator { function assertValidDisputeGameFactory( string memory _errors, ISystemConfig _sysCfg, - IProxyAdmin _admin + IProxyAdmin _admin, + ValidationOverrides memory _overrides ) internal view @@ -308,7 +352,9 @@ contract StandardValidator { _errors = internalRequire( _admin.getProxyImplementation(address(_factory)) == disputeGameFactoryImpl, "DF-20", _errors ); - _errors = internalRequire(_factory.owner() == l1PAOMultisig, "DF-30", _errors); + + address _l1PAOMultisig = expectedL1PAOMultisig(_overrides); + _errors = internalRequire(_factory.owner() == _l1PAOMultisig, "DF-30", _errors); return _errors; } @@ -317,7 +363,8 @@ contract StandardValidator { ISystemConfig _sysCfg, bytes32 _absolutePrestate, uint256 _l2ChainID, - IProxyAdmin _admin + IProxyAdmin _admin, + ValidationOverrides memory _overrides ) internal view @@ -343,9 +390,11 @@ contract StandardValidator { _l2ChainID, _admin, GameTypes.PERMISSIONED_CANNON, + _overrides, "PDDG" ); - _errors = internalRequire(_game.challenger() == challenger, "PDDG-120", _errors); + address _challenger = expectedChallenger(_overrides); + _errors = internalRequire(_game.challenger() == _challenger, "PDDG-120", _errors); return _errors; } @@ -355,7 +404,8 @@ contract StandardValidator { ISystemConfig _sysCfg, bytes32 _absolutePrestate, uint256 _l2ChainID, - IProxyAdmin _admin + IProxyAdmin _admin, + ValidationOverrides memory _overrides ) internal view @@ -372,7 +422,16 @@ contract StandardValidator { } _errors = assertValidDisputeGame( - _errors, _sysCfg, _game, _factory, _absolutePrestate, _l2ChainID, _admin, GameTypes.CANNON, "PLDG" + _errors, + _sysCfg, + _game, + _factory, + _absolutePrestate, + _l2ChainID, + _admin, + GameTypes.CANNON, + _overrides, + "PLDG" ); return _errors; @@ -387,6 +446,7 @@ contract StandardValidator { uint256 _l2ChainID, IProxyAdmin _admin, GameType _gameType, + ValidationOverrides memory _overrides, string memory _errorPrefix ) internal @@ -417,7 +477,7 @@ contract StandardValidator { Duration.unwrap(_game.maxClockDuration()) == 302400, string.concat(_errorPrefix, "-110"), _errors ); - _errors = assertValidDelayedWETH(_errors, _sysCfg, _game.weth(), _admin, _errorPrefix); + _errors = assertValidDelayedWETH(_errors, _sysCfg, _game.weth(), _admin, _overrides, _errorPrefix); _errors = assertValidAnchorStateRegistry(_errors, _sysCfg, _factory, _asr, _admin, _errorPrefix); // Only assert valid preimage oracle if the game VM is valid, since otherwise @@ -434,6 +494,7 @@ contract StandardValidator { ISystemConfig _sysCfg, IDelayedWETH _weth, IProxyAdmin _admin, + ValidationOverrides memory _overrides, string memory _errorPrefix ) internal @@ -449,7 +510,9 @@ contract StandardValidator { string.concat(_errorPrefix, "-20"), _errors ); - _errors = internalRequire(_weth.proxyAdminOwner() == l1PAOMultisig, string.concat(_errorPrefix, "-30"), _errors); + address _l1PAOMultisig = expectedL1PAOMultisig(_overrides); + _errors = + internalRequire(_weth.proxyAdminOwner() == _l1PAOMultisig, string.concat(_errorPrefix, "-30"), _errors); _errors = internalRequire(_weth.delay() == withdrawalDelaySeconds, string.concat(_errorPrefix, "-40"), _errors); _errors = internalRequire(_weth.systemConfig() == _sysCfg, string.concat(_errorPrefix, "-50"), _errors); return _errors; @@ -531,29 +594,57 @@ contract StandardValidator { return keccak256(bytes(_a)) == keccak256(bytes(_b)); } - function validate(ValidationInput memory _input, bool _allowFailure) public view returns (string memory) { + function validate(ValidationInput memory _input, bool _allowFailure) external view returns (string memory) { + return + validate(_input, _allowFailure, ValidationOverrides({ l1PAOMultisig: address(0), challenger: address(0) })); + } + + function validate( + ValidationInput memory _input, + bool _allowFailure, + ValidationOverrides memory _overrides + ) + public + view + returns (string memory) + { string memory _errors = ""; _errors = assertValidSuperchainConfig(_errors); - _errors = assertValidProxyAdmin(_errors, _input.proxyAdmin); + _errors = assertValidProxyAdmin(_errors, _input.proxyAdmin, _overrides); _errors = assertValidSystemConfig(_errors, _input.sysCfg, _input.proxyAdmin); _errors = assertValidL1CrossDomainMessenger(_errors, _input.sysCfg, _input.proxyAdmin); _errors = assertValidL1StandardBridge(_errors, _input.sysCfg, _input.proxyAdmin); _errors = assertValidOptimismMintableERC20Factory(_errors, _input.sysCfg, _input.proxyAdmin); _errors = assertValidL1ERC721Bridge(_errors, _input.sysCfg, _input.proxyAdmin); _errors = assertValidOptimismPortal(_errors, _input.sysCfg, _input.proxyAdmin); - _errors = assertValidDisputeGameFactory(_errors, _input.sysCfg, _input.proxyAdmin); + _errors = assertValidDisputeGameFactory(_errors, _input.sysCfg, _input.proxyAdmin, _overrides); _errors = assertValidPermissionedDisputeGame( - _errors, _input.sysCfg, _input.absolutePrestate, _input.l2ChainID, _input.proxyAdmin + _errors, _input.sysCfg, _input.absolutePrestate, _input.l2ChainID, _input.proxyAdmin, _overrides ); _errors = assertValidPermissionlessDisputeGame( - _errors, _input.sysCfg, _input.absolutePrestate, _input.l2ChainID, _input.proxyAdmin + _errors, _input.sysCfg, _input.absolutePrestate, _input.l2ChainID, _input.proxyAdmin, _overrides ); + string memory overridesString = getOverridesString(_overrides); + string memory finalErrors = _errors; + + // Handle overrides if present + if (bytes(overridesString).length > 0) { + // If we have both overrides and errors, combine them + if (bytes(_errors).length > 0) { + finalErrors = string.concat(overridesString, ",", _errors); + } else { + // If we only have overrides, use them as the final message + finalErrors = overridesString; + } + } + + // Handle validation failure if (bytes(_errors).length > 0 && !_allowFailure) { - revert(string.concat("StandardValidator: ", _errors)); + revert(string.concat("StandardValidator: ", finalErrors)); } - return _errors; + return finalErrors; } } diff --git a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol index 1ed2836fd4f13..c42d13375c152 100644 --- a/packages/contracts-bedrock/test/L1/StandardValidator.t.sol +++ b/packages/contracts-bedrock/test/L1/StandardValidator.t.sol @@ -4,9 +4,6 @@ pragma solidity 0.8.15; // Testing import { CommonTest } from "test/setup/CommonTest.sol"; -// Contracts -import { StandardValidator } from "src/L1/StandardValidator.sol"; - // Libraries import { GameTypes, Duration, Claim } from "src/dispute/lib/Types.sol"; import { DeployUtils } from "scripts/libraries/DeployUtils.sol"; @@ -34,6 +31,7 @@ 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 { IStandardValidator } from "interfaces/L1/IStandardValidator.sol"; /// @title BadDisputeGameFactoryReturner /// @notice Used to return a bad DisputeGameFactory address to the StandardValidator. Far easier @@ -41,7 +39,7 @@ import { IStandardBridge } from "interfaces/universal/IStandardBridge.sol"; /// the validation function to revert. contract BadDisputeGameFactoryReturner { /// @notice Address of the StandardValidator instance. - StandardValidator public immutable validator; + IStandardValidator public immutable validator; /// @notice Address of the real DisputeGameFactory instance. IDisputeGameFactory public immutable realDisputeGameFactory; @@ -53,7 +51,7 @@ contract BadDisputeGameFactoryReturner { /// @param _realDisputeGameFactory The real DisputeGameFactory instance. /// @param _fakeDisputeGameFactory The fake DisputeGameFactory instance. constructor( - StandardValidator _validator, + IStandardValidator _validator, IDisputeGameFactory _realDisputeGameFactory, IDisputeGameFactory _fakeDisputeGameFactory ) { @@ -76,7 +74,7 @@ contract BadDisputeGameFactoryReturner { /// @notice Base contract for StandardValidator tests, handles common setup. contract StandardValidator_TestInit is CommonTest { /// @notice StandardValidator instance, used for testing. - StandardValidator validator; + IStandardValidator validator; /// @notice Deploy input that was used to deploy the contracts being tested. IOPContractsManager.DeployInput deployInput; @@ -133,23 +131,33 @@ contract StandardValidator_TestInit is CommonTest { } // Deploy the validator. - validator = new StandardValidator( - StandardValidator.Implementations({ - systemConfigImpl: impls.systemConfigImpl, - optimismPortalImpl: impls.optimismPortalImpl, - l1CrossDomainMessengerImpl: impls.l1CrossDomainMessengerImpl, - l1StandardBridgeImpl: impls.l1StandardBridgeImpl, - l1ERC721BridgeImpl: impls.l1ERC721BridgeImpl, - optimismMintableERC20FactoryImpl: impls.optimismMintableERC20FactoryImpl, - disputeGameFactoryImpl: impls.disputeGameFactoryImpl, - mipsImpl: impls.mipsImpl, - anchorStateRegistryImpl: impls.anchorStateRegistryImpl, - delayedWETHImpl: impls.delayedWETHImpl - }), - superchainConfig, - proxyAdminOwner, - challenger, - 302400 + validator = IStandardValidator( + DeployUtils.create1({ + _name: "StandardValidator", + _args: DeployUtils.encodeConstructor( + abi.encodeCall( + IStandardValidator.__constructor__, + ( + IStandardValidator.Implementations({ + systemConfigImpl: impls.systemConfigImpl, + optimismPortalImpl: impls.optimismPortalImpl, + l1CrossDomainMessengerImpl: impls.l1CrossDomainMessengerImpl, + l1StandardBridgeImpl: impls.l1StandardBridgeImpl, + l1ERC721BridgeImpl: impls.l1ERC721BridgeImpl, + optimismMintableERC20FactoryImpl: impls.optimismMintableERC20FactoryImpl, + disputeGameFactoryImpl: impls.disputeGameFactoryImpl, + mipsImpl: impls.mipsImpl, + anchorStateRegistryImpl: impls.anchorStateRegistryImpl, + delayedWETHImpl: impls.delayedWETHImpl + }), + superchainConfig, + proxyAdminOwner, + challenger, + 302400 + ) + ) + ) + }) ); // Deploy the BadDisputeGameFactoryReturner once. @@ -199,7 +207,7 @@ contract StandardValidator_TestInit is CommonTest { /// @return The error message(s) from the validate function. function _validate(bool _allowFailure) internal view returns (string memory) { return validator.validate( - StandardValidator.ValidationInput({ + IStandardValidator.ValidationInput({ proxyAdmin: proxyAdmin, sysCfg: systemConfig, absolutePrestate: absolutePrestate.raw(), @@ -208,6 +216,33 @@ contract StandardValidator_TestInit is CommonTest { _allowFailure ); } + + /// @notice Runs the StandardValidator.validate function. + /// @param _allowFailure Whether to allow failure. + /// @return The error message(s) from the validate function. + function _validate( + bool _allowFailure, + IStandardValidator.ValidationOverrides memory _overrides + ) + internal + view + returns (string memory) + { + return validator.validate( + IStandardValidator.ValidationInput({ + proxyAdmin: proxyAdmin, + sysCfg: systemConfig, + absolutePrestate: absolutePrestate.raw(), + l2ChainID: l2ChainId + }), + _allowFailure, + _overrides + ); + } + + function _defaultValidationOverrides() internal pure returns (IStandardValidator.ValidationOverrides memory) { + return IStandardValidator.ValidationOverrides({ l1PAOMultisig: address(0), challenger: address(0) }); + } } /// @title StandardValidator_validate_Test @@ -240,6 +275,28 @@ contract StandardValidator_validate_Test is StandardValidator_TestInit { assertEq("PROXYA-10,PDDG-DWETH-30,PLDG-DWETH-30", _validate(true)); } + /// @notice Tests that the validate function successfully returns the right overrides error when the + /// ProxyAdmin owner is overriden but is correct. + function test_validate_overridenProxyAdminOwner_succeeds() public { + IStandardValidator.ValidationOverrides memory overrides = _defaultValidationOverrides(); + overrides.l1PAOMultisig = address(0xbad); + vm.mockCall(address(proxyAdmin), abi.encodeCall(IProxyAdmin.owner, ()), abi.encode(address(0xbad))); + vm.mockCall( + address(disputeGameFactory), + abi.encodeCall(IDisputeGameFactory.owner, ()), + abi.encode(overrides.l1PAOMultisig) + ); + assertEq("OVERRIDES-L1PAOMULTISIG", _validate(true, overrides)); + } + + /// @notice Tests that the validate function (with an overriden ProxyAdmin owner) successfully returns the right + /// error when the ProxyAdmin owner is not correct. + function test_validateOverrideL1PAOMultisig_invalidProxyAdminOwner_succeeds() public view { + IStandardValidator.ValidationOverrides memory overrides = _defaultValidationOverrides(); + overrides.l1PAOMultisig = address(0xbad); + assertEq("OVERRIDES-L1PAOMULTISIG,PROXYA-10,DF-30,PDDG-DWETH-30,PLDG-DWETH-30", _validate(true, overrides)); + } + /// @notice Tests that the validate function successfully returns the right error when the /// SystemConfig version is invalid. function test_validate_systemConfigInvalidVersion_succeeds() public { @@ -684,6 +741,23 @@ contract StandardValidator_validate_Test is StandardValidator_TestInit { assertEq("PDDG-120", _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 { + IStandardValidator.ValidationOverrides memory overrides = _defaultValidationOverrides(); + overrides.challenger = address(0xbad); + vm.mockCall(address(pdg), abi.encodeCall(IPermissionedDisputeGame.challenger, ()), abi.encode(address(0xbad))); + assertEq("OVERRIDES-CHALLENGER", _validate(true, overrides)); + } + + /// @notice Tests that the validate function (with an overriden PermissionedDisputeGame challenger) successfully + /// returns the right error when the PermissionedDisputeGame challenger is invalid. + function test_validateOverridesChallenger_permissionedDisputeGameInvalidChallenger_succeeds() public view { + IStandardValidator.ValidationOverrides memory overrides = _defaultValidationOverrides(); + overrides.challenger = address(0xbad); + assertEq("OVERRIDES-CHALLENGER,PDDG-120", _validate(true, overrides)); + } + /// @notice Tests that the validate function successfully returns the right error when the /// AnchorStateRegistry version is invalid. function test_validate_anchorStateRegistryInvalidVersion_succeeds() public { @@ -943,6 +1017,50 @@ contract StandardValidator_validate_Test is StandardValidator_TestInit { assertEq("L1SB-70", _validate(true)); } + /// @notice Tests that the validate function (with the L1PAOMultisig and Challenger overriden) successfully returns + /// the right error when both are invalid. + function test_validateL1PAOMultisigAndChallengerOverrides_succeeds() public view { + IStandardValidator.ValidationOverrides memory overrides = _defaultValidationOverrides(); + overrides.l1PAOMultisig = address(0xace); + overrides.challenger = address(0xbad); + assertEq( + "OVERRIDES-L1PAOMULTISIG,OVERRIDES-CHALLENGER,PROXYA-10,DF-30,PDDG-DWETH-30,PDDG-120,PLDG-DWETH-30", + _validate(true, overrides) + ); + } + + /// @notice Tests that the validate function (with the L1PAOMultisig and Challenger overriden) + /// successfully returns no error when there is none. That is, it never returns the overriden strings alone. + function test_validateOverrides_noErrors_succeeds() public { + IStandardValidator.ValidationOverrides memory overrides = + IStandardValidator.ValidationOverrides({ l1PAOMultisig: address(0xbad), challenger: address(0xc0ffee) }); + vm.mockCall(address(proxyAdmin), abi.encodeCall(IProxyAdmin.owner, ()), abi.encode(overrides.l1PAOMultisig)); + vm.mockCall( + address(disputeGameFactory), + abi.encodeCall(IDisputeGameFactory.owner, ()), + abi.encode(overrides.l1PAOMultisig) + ); + vm.mockCall( + address(pdg), abi.encodeCall(IPermissionedDisputeGame.challenger, ()), abi.encode(overrides.challenger) + ); + + assertEq("OVERRIDES-L1PAOMULTISIG,OVERRIDES-CHALLENGER", _validate(true, overrides)); + } + + /// @notice Tests that the validate function (with overrides)and allow failure set to false, returns the errors with + /// the overrides prepended. + function test_validateOverrides_notAllowFailurePrependsOverrides_succeeds() public { + IStandardValidator.ValidationOverrides memory overrides = + IStandardValidator.ValidationOverrides({ l1PAOMultisig: address(0xbad), challenger: address(0xc0ffee) }); + + vm.expectRevert( + bytes( + "StandardValidator: OVERRIDES-L1PAOMULTISIG,OVERRIDES-CHALLENGER,PROXYA-10,DF-30,PDDG-DWETH-30,PDDG-120,PLDG-DWETH-30" + ) + ); + _validate(false, overrides); + } + /// @notice Tests that the version getter functions on StandardValidator return non-empty strings. function test_versions_succeeds() public view { assertTrue(bytes(validator.systemConfigVersion()).length > 0, "systemConfigVersion empty"); diff --git a/packages/contracts-bedrock/test/universal/Specs.t.sol b/packages/contracts-bedrock/test/universal/Specs.t.sol index 2a9bad9c90af9..0be2bbeb4acbb 100644 --- a/packages/contracts-bedrock/test/universal/Specs.t.sol +++ b/packages/contracts-bedrock/test/universal/Specs.t.sol @@ -930,6 +930,10 @@ contract Specification_Test is CommonTest { _addSpec({ _name: "StandardValidator", _sel: _getSel("preimageOracleVersion()") }); _addSpec({ _name: "StandardValidator", _sel: _getSel("withdrawalDelaySeconds()") }); _addSpec({ _name: "StandardValidator", _sel: _getSel("validate((address,address,bytes32,uint256),bool)") }); + _addSpec({ + _name: "StandardValidator", + _sel: _getSel("validate((address,address,bytes32,uint256),bool,(address,address))") + }); } /// @dev Computes the selector from a function signature.