From 142d12947cc707844201c3af18b1842ebcde2c24 Mon Sep 17 00:00:00 2001 From: Chiin <77933451+0xChin@users.noreply.github.com> Date: Fri, 18 Jul 2025 05:57:03 -0300 Subject: [PATCH 01/14] fix: check hash after proposal (#447) * fix: add proposal id validation on submit upgrade proposal returned proposalId * fix: pre-pr --- .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 7 +++- .../test/governance/ProposalValidator.t.sol | 38 +++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index babaf48ae2b4d..f9436c64be41c 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xe7a93826772bf108a21923f7e45b1f46cdadb75e48b0c796e43d64f0c1d81504", - "sourceCodeHash": "0xadd8e049bf3c652af123b6c64a1d504c92be13b4797ab10b978df907b05dcf7f" + "initCodeHash": "0x06a2b713907a4a4c961061edeb8f9417f9fdf63fc5512e6794af67fcc548935d", + "sourceCodeHash": "0x98cb001a29058d9d4bdd017c73e3520af1e22e9dee15e153799196b3390923df" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index d9ffaafe21b2f..f37a40d1a7f0f 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -369,10 +369,15 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { if (_proposalType == ProposalType.MaintenanceUpgrade) { proposal.movedToVote = true; - GOVERNOR.proposeWithModule( + uint256 proposalId = GOVERNOR.proposeWithModule( votingModule, proposalVotingModuleData, _proposalDescription, uint8(_proposalType) ); + // Make sure the proposalId is the same as the proposalHash + if (proposalId != uint256(proposalHash_)) { + revert ProposalValidator_ProposalIdMismatch(); + } + emit ProposalMovedToVote(proposalHash_, msg.sender); } } diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index c41765cd03749..b538055e2c23d 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -1077,6 +1077,44 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I againstThreshold, proposalDescription, attestationUid, proposalType, CYCLE_NUMBER ); } + + function test_submitUpgradeProposal_proposalIdMismatch_reverts(uint256 proposalId) public { + uint248 againstThreshold = 5000; + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.MaintenanceUpgrade; + bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); + + // Calculate expected proposal hash + bytes memory votingModuleData = _constructOptimisticVotingModuleData(againstThreshold); + bytes32 expectedHash = validator.hashProposalWithModule( + optimisticVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) + ); + + vm.assume(proposalId != uint256(expectedHash)); // Ensure proposalId is different from expectedHash + + _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); + + _mockAndExpect( + address(governor), + abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), + abi.encode(0) + ); + + // Mock the proposeWithModule call to return a different proposalId + _mockAndExpect( + address(governor), + abi.encodeCall( + IOptimismGovernor.proposeWithModule, + (optimisticVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + ), + abi.encode(proposalId) + ); + + vm.expectRevert(ProposalValidator.ProposalValidator_ProposalIdMismatch.selector); + vm.prank(topDelegate_A); + validator.submitUpgradeProposal( + againstThreshold, proposalDescription, attestationUid, proposalType, CYCLE_NUMBER + ); + } } /// @title ProposalValidator_SubmitCouncilMemberElectionsProposal_Test From d18f80b411f43fba605e3573e9699d853131c7eb Mon Sep 17 00:00:00 2001 From: Chiin <77933451+0xChin@users.noreply.github.com> Date: Fri, 18 Jul 2025 14:10:32 -0300 Subject: [PATCH 02/14] refactor: fetch proposal types configurator externally (#448) * refactor: fetch configurator from governor * fix: pre-pr * fix: pre-pr --- .../governance/IProposalValidator.sol | 4 --- .../snapshots/abi/ProposalValidator.json | 18 ----------- .../snapshots/semver-lock.json | 4 +-- .../storageLayout/ProposalValidator.json | 15 +++------- .../src/governance/ProposalValidator.sol | 30 +++++++++---------- .../test/governance/ProposalValidator.t.sol | 9 ++++-- 6 files changed, 26 insertions(+), 54 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index 5c0c48a7be456..bf2c2e4377899 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.0; // Interfaces import {IOptimismGovernor} from './IOptimismGovernor.sol'; import { ISemver } from "interfaces/universal/ISemver.sol"; -import { IProposalTypesConfigurator } from './IProposalTypesConfigurator.sol'; /// @title IProposalValidator /// @notice Interface for the ProposalValidator contract. @@ -167,7 +166,6 @@ interface IProposalValidator is ISemver { function initialize( address _owner, - IProposalTypesConfigurator _proposalTypesConfigurator, uint256 _cycleNumber, uint256 _startingTimestamp, uint256 _duration, @@ -195,8 +193,6 @@ interface IProposalValidator is ISemver { function OPTIMISTIC_MODULE_PERCENT_DIVISOR() external view returns (uint256); - function proposalTypesConfigurator() external view returns (IProposalTypesConfigurator); - function proposalTypesData(ProposalType) external view returns (uint256 requiredApprovals, uint8 proposalVotingModule); function votingCycles(uint256) external view returns ( diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 03a0733070c7b..9eddf4e0965f8 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -134,11 +134,6 @@ "name": "_owner", "type": "address" }, - { - "internalType": "contract IProposalTypesConfigurator", - "name": "_proposalTypesConfigurator", - "type": "address" - }, { "internalType": "uint256", "name": "_cycleNumber", @@ -315,19 +310,6 @@ "stateMutability": "view", "type": "function" }, - { - "inputs": [], - "name": "proposalTypesConfigurator", - "outputs": [ - { - "internalType": "contract IProposalTypesConfigurator", - "name": "", - "type": "address" - } - ], - "stateMutability": "view", - "type": "function" - }, { "inputs": [ { diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index f9436c64be41c..7934d0b6b4852 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x06a2b713907a4a4c961061edeb8f9417f9fdf63fc5512e6794af67fcc548935d", - "sourceCodeHash": "0x98cb001a29058d9d4bdd017c73e3520af1e22e9dee15e153799196b3390923df" + "initCodeHash": "0xb612561f3182537796ec6bae6a15a4f6b9e63d839e051a165f6b81b3f6ce0807", + "sourceCodeHash": "0xbfb241a7033264d5b7097a4326a415b53514c4a4dee01430bd092fa6cbf0872b" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json b/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json index 2c01297b2b0d5..e8bbb446de6a1 100644 --- a/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json @@ -34,39 +34,32 @@ "slot": "52", "type": "uint256[49]" }, - { - "bytes": "20", - "label": "proposalTypesConfigurator", - "offset": 0, - "slot": "101", - "type": "contract IProposalTypesConfigurator" - }, { "bytes": "32", "label": "proposalDistributionThreshold", "offset": 0, - "slot": "102", + "slot": "101", "type": "uint256" }, { "bytes": "32", "label": "votingCycles", "offset": 0, - "slot": "103", + "slot": "102", "type": "mapping(uint256 => struct ProposalValidator.VotingCycleData)" }, { "bytes": "32", "label": "proposalTypesData", "offset": 0, - "slot": "104", + "slot": "103", "type": "mapping(enum ProposalValidator.ProposalType => struct ProposalValidator.ProposalTypeData)" }, { "bytes": "32", "label": "_proposals", "offset": 0, - "slot": "105", + "slot": "104", "type": "mapping(bytes32 => struct ProposalValidator.ProposalData)" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index f37a40d1a7f0f..cf839cfe8b096 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -217,9 +217,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice The Optimism Governor contract that will handle the voting phase. IOptimismGovernor public immutable GOVERNOR; - /// @notice The proposal types configurator contract. - IProposalTypesConfigurator public proposalTypesConfigurator; - /// @notice The max amount of tokens that can be distributed in a proposal. uint256 public proposalDistributionThreshold; @@ -258,7 +255,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Initializes the ProposalValidator contract. /// @param _owner The address that will own the contract. - /// @param _proposalTypesConfigurator The proposal types configurator contract address. /// @param _cycleNumber The number of the current voting cycle. /// @param _startingTimestamp The starting timestamp of the voting cycle. /// @param _duration The duration of the voting cycle. @@ -268,7 +264,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param _proposalTypesData Array of proposal type data corresponding to the proposal types. function initialize( address _owner, - IProposalTypesConfigurator _proposalTypesConfigurator, uint256 _cycleNumber, uint256 _startingTimestamp, uint256 _duration, @@ -284,7 +279,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalTypesDataLengthMismatch(); } - proposalTypesConfigurator = _proposalTypesConfigurator; _setVotingCycleData(_cycleNumber, _startingTimestamp, _duration, _votingCycleDistributionLimit); _setProposalDistributionThreshold(_proposalDistributionThreshold); @@ -338,8 +332,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(optimisticSettings); // Get the optimistic module address from configurator - address votingModule = - proposalTypesConfigurator.proposalTypes(proposalTypesData[_proposalType].proposalVotingModule).module; + address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( + proposalTypesData[_proposalType].proposalVotingModule + ).module; // Generate unique proposal hash proposalHash_ = @@ -430,7 +425,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, settings); // Get the module address from the configurator - address votingModule = proposalTypesConfigurator.proposalTypes( + address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( proposalTypesData[ProposalType.CouncilMemberElections].proposalVotingModule ).module; @@ -517,8 +512,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, settings); // Get the module address from the configurator - address votingModule = - proposalTypesConfigurator.proposalTypes(proposalTypesData[_proposalType].proposalVotingModule).module; + address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( + proposalTypesData[_proposalType].proposalVotingModule + ).module; // Generate unique proposal hash proposalHash_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); @@ -598,7 +594,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // Get the module address from the configurator ProposalType proposalType = ProposalType.ProtocolOrGovernorUpgrade; - address votingModule = proposalTypesConfigurator.proposalTypes( + address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( proposalTypesData[ProposalType.ProtocolOrGovernorUpgrade].proposalVotingModule ).module; @@ -673,8 +669,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // Get the module address from the configurator ProposalType proposalType = ProposalType.CouncilMemberElections; - address votingModule = - proposalTypesConfigurator.proposalTypes(proposalTypesData[proposalType].proposalVotingModule).module; + address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( + proposalTypesData[proposalType].proposalVotingModule + ).module; // Generate unique proposal hash proposalHash_ = @@ -771,8 +768,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, settings); // Get the module address from the configurator - address votingModule = - proposalTypesConfigurator.proposalTypes(proposalTypesData[_proposalType].proposalVotingModule).module; + address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( + proposalTypesData[_proposalType].proposalVotingModule + ).module; // Generate unique proposal hash proposalHash_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index b538055e2c23d..bf90d08d5c65f 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -464,6 +464,12 @@ contract ProposalValidator_Init is CommonTest { moduleAddress = optimisticVotingModule; } + _mockAndExpect( + address(governor), + abi.encodeCall(IOptimismGovernor.PROPOSAL_TYPES_CONFIGURATOR, ()), + abi.encode(proposalTypesConfigurator) + ); + _mockAndExpect( address(proposalTypesConfigurator), abi.encodeCall(IProposalTypesConfigurator.proposalTypes, (_votingModuleId)), @@ -501,7 +507,6 @@ contract ProposalValidator_Init is CommonTest { impl.initialize, ( owner, - proposalTypesConfigurator, CYCLE_NUMBER, START_TIMESTAMP, DURATION, @@ -619,7 +624,6 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { impl.initialize, ( owner, - proposalTypesConfigurator, CYCLE_NUMBER, START_TIMESTAMP, DURATION, @@ -691,7 +695,6 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { impl.initialize, ( owner, - proposalTypesConfigurator, CYCLE_NUMBER, START_TIMESTAMP, DURATION, From a32244fd427bad17ae5e931e222ab12dd50f2251 Mon Sep 17 00:00:00 2001 From: Chiin <77933451+0xChin@users.noreply.github.com> Date: Mon, 21 Jul 2025 06:08:59 -0300 Subject: [PATCH 03/14] fix: check for uninitialized voting modules (#446) * fix: check for uninitialized voting modules * fix: pre-pr * fix: pre-pr --- .../governance/IProposalValidator.sol | 1 + .../snapshots/abi/ProposalValidator.json | 5 ++ .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 39 +++++++--- .../test/governance/ProposalValidator.t.sol | 74 +++++++++++++++++++ 5 files changed, 112 insertions(+), 11 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index bf2c2e4377899..b932d6e246182 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -29,6 +29,7 @@ interface IProposalValidator is ISemver { error ProposalValidator_ProposalIdMismatch(); error ProposalValidator_InvalidProposer(); error ProposalValidator_InvalidProposal(); + error ProposalValidator_InvalidVotingModule(); event ProposalSubmitted( bytes32 indexed proposalHash, diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 9eddf4e0965f8..7a1bb786918db 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -853,6 +853,11 @@ "name": "ProposalValidator_InvalidVotingCycle", "type": "error" }, + { + "inputs": [], + "name": "ProposalValidator_InvalidVotingModule", + "type": "error" + }, { "inputs": [], "name": "ProposalValidator_ProposalAlreadyApproved", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 7934d0b6b4852..e4ab017c9c0e0 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xb612561f3182537796ec6bae6a15a4f6b9e63d839e051a165f6b81b3f6ce0807", - "sourceCodeHash": "0xbfb241a7033264d5b7097a4326a415b53514c4a4dee01430bd092fa6cbf0872b" + "initCodeHash": "0xd072e288107128a1b0f26f41c4b257295919777cadfc06514d2a4738fb96e1d1", + "sourceCodeHash": "0x725efcbb68cb4a23af492983ba22969ddaa5d472878fb8490a437eaaa88812a8" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index cf839cfe8b096..f8f320e46bc3b 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -86,6 +86,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Thrown when the proposal is invalid trying to move to vote. error ProposalValidator_InvalidProposal(); + /// @notice Thrown when the voting module address is invalid (zero address). + error ProposalValidator_InvalidVotingModule(); + /*////////////////////////////////////////////////////////////// EVENTS //////////////////////////////////////////////////////////////*/ @@ -332,9 +335,15 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(optimisticSettings); // Get the optimistic module address from configurator - address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( - proposalTypesData[_proposalType].proposalVotingModule - ).module; + IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( + GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() + ).proposalTypes(proposalTypesData[_proposalType].proposalVotingModule); + address votingModule = proposalTypeConfig.module; + + // Validate voting module exists + if (bytes(proposalTypeConfig.name).length == 0) { + revert ProposalValidator_InvalidVotingModule(); + } // Generate unique proposal hash proposalHash_ = @@ -425,9 +434,15 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, settings); // Get the module address from the configurator - address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( - proposalTypesData[ProposalType.CouncilMemberElections].proposalVotingModule - ).module; + IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( + GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() + ).proposalTypes(proposalTypesData[ProposalType.CouncilMemberElections].proposalVotingModule); + address votingModule = proposalTypeConfig.module; + + // Validate voting module exists + if (bytes(proposalTypeConfig.name).length == 0) { + revert ProposalValidator_InvalidVotingModule(); + } // Generate unique proposal hash proposalHash_ = @@ -512,9 +527,15 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, settings); // Get the module address from the configurator - address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( - proposalTypesData[_proposalType].proposalVotingModule - ).module; + IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( + GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() + ).proposalTypes(proposalTypesData[_proposalType].proposalVotingModule); + address votingModule = proposalTypeConfig.module; + + // Validate voting module exists + if (bytes(proposalTypeConfig.name).length == 0) { + revert ProposalValidator_InvalidVotingModule(); + } // Generate unique proposal hash proposalHash_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index bf90d08d5c65f..4bf2204b0638d 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -485,6 +485,36 @@ contract ProposalValidator_Init is CommonTest { ); } + /// @notice Helper function to mock proposal types configurator call with changed module + function _mockProposalTypesConfiguratorCallWithUninitializedModule(uint8 _votingModuleId) internal { + address moduleAddress; + if (_votingModuleId == APPROVAL_VOTING_MODULE_ID) { + moduleAddress = approvalVotingModule; + } else if (_votingModuleId == OPTIMISTIC_VOTING_MODULE_ID) { + moduleAddress = optimisticVotingModule; + } + + _mockAndExpect( + address(governor), + abi.encodeCall(IOptimismGovernor.PROPOSAL_TYPES_CONFIGURATOR, ()), + abi.encode(proposalTypesConfigurator) + ); + + _mockAndExpect( + address(proposalTypesConfigurator), + abi.encodeCall(IProposalTypesConfigurator.proposalTypes, (_votingModuleId)), + abi.encode( + IProposalTypesConfigurator.ProposalType({ + quorum: 0, + approvalThreshold: 0, + name: "", + description: "", + module: address(0) + }) + ) + ); + } + /// @notice Initializes the validator function _initializeValidator() internal virtual { ( @@ -943,6 +973,21 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I ); } + function test_submitUpgradeProposal_invalidVotingModule_reverts() public { + uint248 againstThreshold = 5000; // 50% + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; + bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); + + // Mock configurator to return uninitialized module + _mockProposalTypesConfiguratorCallWithUninitializedModule(OPTIMISTIC_VOTING_MODULE_ID); + + vm.expectRevert(ProposalValidator.ProposalValidator_InvalidVotingModule.selector); + vm.prank(topDelegate_A); + validator.submitUpgradeProposal( + againstThreshold, proposalDescription, attestationUid, proposalType, CYCLE_NUMBER + ); + } + function testFuzz_submitUpgradeProposal_duplicateProposal_reverts(uint8 proposalTypeValue) public { // Bound proposal type to only upgrade proposals (0 = ProtocolOrGovernorUpgrade, 1 = MaintenanceUpgrade) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 1)); @@ -1377,6 +1422,20 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_TestFail is Prop criteriaValue, optionDescriptions, proposalDescription, revocableAttestationUid, CYCLE_NUMBER ); } + + function test_submitCouncilMemberElectionsProposal_invalidVotingModule_reverts() public { + attestationUid = + _createApprovedProposerAttestation(topDelegate_A, ProposalValidator.ProposalType.CouncilMemberElections); + + // Mock configurator to return uninitialized module + _mockProposalTypesConfiguratorCallWithUninitializedModule(APPROVAL_VOTING_MODULE_ID); + + vm.expectRevert(ProposalValidator.ProposalValidator_InvalidVotingModule.selector); + vm.prank(topDelegate_A); + validator.submitCouncilMemberElectionsProposal( + criteriaValue, optionDescriptions, proposalDescription, attestationUid, CYCLE_NUMBER + ); + } } /// @title ProposalValidator_SubmitFundingProposal_Test @@ -1742,6 +1801,21 @@ contract ProposalValidator_SubmitFundingProposal_TestFail is ProposalValidator_I CYCLE_NUMBER ); } + + function test_submitFundingProposal_invalidVotingModule_reverts() public { + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.GovernanceFund; + (string[] memory descriptions, address[] memory recipients, uint256[] memory amounts) = + _createMinimalFundingArrays(1); + + // Mock configurator to return uninitialized module + _mockProposalTypesConfiguratorCallWithUninitializedModule(APPROVAL_VOTING_MODULE_ID); + + vm.expectRevert(ProposalValidator.ProposalValidator_InvalidVotingModule.selector); + vm.prank(user); + validator.submitFundingProposal( + FUNDING_CRITERIA_VALUE, descriptions, recipients, amounts, description, proposalType, CYCLE_NUMBER + ); + } } /// @title ProposalValidator_ApproveProposal_Test From 74a0363b9e2979505dba43f6c2482789618acd24 Mon Sep 17 00:00:00 2001 From: 0xOneTony <112496816+0xOneTony@users.noreply.github.com> Date: Mon, 21 Jul 2025 15:11:29 +0300 Subject: [PATCH 04/14] feat: add check in approve if proposal has moved to vote (#450) --- .../snapshots/semver-lock.json | 4 ++-- .../src/governance/ProposalValidator.sol | 5 +++++ .../test/governance/ProposalValidator.t.sol | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index e4ab017c9c0e0..4fc8cd4183873 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xd072e288107128a1b0f26f41c4b257295919777cadfc06514d2a4738fb96e1d1", - "sourceCodeHash": "0x725efcbb68cb4a23af492983ba22969ddaa5d472878fb8490a437eaaa88812a8" + "initCodeHash": "0x30d570ce61624852476452d9660567bae2346002878a45703cf60e44809730ca", + "sourceCodeHash": "0xe5272e8176b0ddf3ff589629f7872b5dc6c18b900f413f0f8682865bf310faa7" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index f8f320e46bc3b..6d2dc68fb9b2c 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -578,6 +578,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalAlreadyApproved(); } + // check if proposal has already moved to vote + if (proposal.movedToVote) { + revert ProposalValidator_ProposalAlreadyMovedToVote(); + } + // validate the attestation _validateTopDelegateAttestation(_attestationUid, _msgSender()); diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index 4bf2204b0638d..92f8863dfb515 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -1881,6 +1881,23 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { validator.approveProposal(_proposalHash, topDelegateAttestation_A); } + function test_approveProposal_proposalAlreadyMovedToVote_reverts( + bytes32 _proposalHash, + uint8 proposalTypeValue + ) + public + { + // Bound the proposal type to valid enum values (0-4) + proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + // set proposal data so that the proposal exists and set movedToVote to true + validator.setProposalData(_proposalHash, topDelegate_A, proposalType, true, 0, CYCLE_NUMBER); + + vm.expectRevert(IProposalValidator.ProposalValidator_ProposalAlreadyMovedToVote.selector); + vm.prank(topDelegate_A); + validator.approveProposal(_proposalHash, topDelegateAttestation_A); + } + function test_approveProposal_invalidSchema_reverts(bytes32 _proposalHash, uint8 proposalTypeValue) public { // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); From 154065c9ab24c721e2e65401ac7240cf64153834 Mon Sep 17 00:00:00 2001 From: 0xOneTony <112496816+0xOneTony@users.noreply.github.com> Date: Mon, 21 Jul 2025 15:17:29 +0300 Subject: [PATCH 05/14] fix: msg sender to be consistent (#451) --- .../contracts-bedrock/snapshots/semver-lock.json | 4 ++-- .../src/governance/ProposalValidator.sol | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 4fc8cd4183873..754d18cb890bc 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x30d570ce61624852476452d9660567bae2346002878a45703cf60e44809730ca", - "sourceCodeHash": "0xe5272e8176b0ddf3ff589629f7872b5dc6c18b900f413f0f8682865bf310faa7" + "initCodeHash": "0x0dfc44cf456909f524602c8d2b3e5793e04b59da994a8268bc59071aad567c77", + "sourceCodeHash": "0xbe31385272e8ecf4fd0bf52e768efcf9b6b4199f23707d74949c11ee6578e6de" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 6d2dc68fb9b2c..3a4659445f0f0 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -362,11 +362,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // Store proposal metadata - proposal.proposer = msg.sender; + proposal.proposer = _msgSender(); proposal.proposalType = _proposalType; proposal.votingCycle = _votingCycle; - emit ProposalSubmitted(proposalHash_, msg.sender, _proposalDescription, _proposalType); + emit ProposalSubmitted(proposalHash_, _msgSender(), _proposalDescription, _proposalType); emit ProposalVotingModuleData(proposalHash_, proposalVotingModuleData); // MaintenanceUpgrade proposals move directly to voting (atomic operation) @@ -382,7 +382,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalIdMismatch(); } - emit ProposalMovedToVote(proposalHash_, msg.sender); + emit ProposalMovedToVote(proposalHash_, _msgSender()); } } @@ -461,11 +461,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // Store proposal metadata - proposal.proposer = msg.sender; + proposal.proposer = _msgSender(); proposal.proposalType = ProposalType.CouncilMemberElections; proposal.votingCycle = _votingCycle; - emit ProposalSubmitted(proposalHash_, msg.sender, _proposalDescription, ProposalType.CouncilMemberElections); + emit ProposalSubmitted(proposalHash_, _msgSender(), _proposalDescription, ProposalType.CouncilMemberElections); emit ProposalVotingModuleData(proposalHash_, proposalVotingModuleData); } @@ -553,11 +553,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // Store proposal metadata - proposal.proposer = msg.sender; + proposal.proposer = _msgSender(); proposal.proposalType = _proposalType; proposal.votingCycle = _votingCycle; - emit ProposalSubmitted(proposalHash_, msg.sender, _description, _proposalType); + emit ProposalSubmitted(proposalHash_, _msgSender(), _description, _proposalType); emit ProposalVotingModuleData(proposalHash_, proposalVotingModuleData); } @@ -915,7 +915,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { if ( attestation.attester != owner() || attestation.schema != APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID - || approvedDelegate != msg.sender || proposalType != uint8(_expectedProposalType) + || approvedDelegate != _msgSender() || proposalType != uint8(_expectedProposalType) ) { revert ProposalValidator_InvalidAttestation(); } From 261fc72cf9f9ec8a37d68d2491e494acb7972896 Mon Sep 17 00:00:00 2001 From: 0xOneTony <112496816+0xOneTony@users.noreply.github.com> Date: Mon, 21 Jul 2025 15:35:37 +0300 Subject: [PATCH 06/14] fix: approved proposer schema (#452) --- .../governance/IProposalValidator.sol | 1 + .../snapshots/abi/ProposalValidator.json | 5 ++ .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 14 ++++-- .../test/governance/ProposalValidator.t.sol | 46 +++++++++++++++---- 5 files changed, 55 insertions(+), 15 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index b932d6e246182..7d93349984ea2 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -21,6 +21,7 @@ interface IProposalValidator is ISemver { error ProposalValidator_ExceedsDistributionThreshold(); error ProposalValidator_InvalidOptionsLength(); error ProposalValidator_AttestationRevoked(); + error ProposalValidator_AttestationExpired(); error ProposalValidator_InvalidAttestationSchema(); error ProposalValidator_InvalidCriteriaValue(); error ProposalValidator_InvalidAgainstThreshold(); diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 7a1bb786918db..2afaef89b24f9 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -788,6 +788,11 @@ "name": "VotingCycleDataSet", "type": "event" }, + { + "inputs": [], + "name": "ProposalValidator_AttestationExpired", + "type": "error" + }, { "inputs": [], "name": "ProposalValidator_AttestationRevoked", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 754d18cb890bc..fd811a1f8d457 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x0dfc44cf456909f524602c8d2b3e5793e04b59da994a8268bc59071aad567c77", - "sourceCodeHash": "0xbe31385272e8ecf4fd0bf52e768efcf9b6b4199f23707d74949c11ee6578e6de" + "initCodeHash": "0xbac284f6ec21a5d65d5b86d7e6406e0805d77e15dc4bd66397f0111701110e0e", + "sourceCodeHash": "0x58048692d1da18d17958b2dc950055ae2a58ab645385b0aed3528b289dfee21d" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 3a4659445f0f0..c871a2b9431d4 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -62,6 +62,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Thrown when an attestation is revoked. error ProposalValidator_AttestationRevoked(); + /// @notice Thrown when the attestation is expired. + error ProposalValidator_AttestationExpired(); + /// @notice Thrown when an attestation schema is invalid. error ProposalValidator_InvalidAttestationSchema(); @@ -210,7 +213,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice The schema UID for attestations in the Ethereum Attestation Service for checking if the caller /// is an approved proposer. - /// @dev Schema format: { approvedProposer: address, proposalType: uint8 } + /// @dev Schema format: { proposalType: uint8, date: string } bytes32 public immutable APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID; /// @notice The schema UID for attestations in the Ethereum Attestation Service for checking if the caller @@ -911,11 +914,16 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_AttestationRevoked(); } - (address approvedDelegate, uint8 proposalType) = abi.decode(attestation.data, (address, uint8)); + // check if the attestation is expired + if (attestation.expirationTime != 0 && attestation.expirationTime < block.timestamp) { + revert ProposalValidator_AttestationExpired(); + } + + (uint8 proposalType,) = abi.decode(attestation.data, (uint8, string)); if ( attestation.attester != owner() || attestation.schema != APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID - || approvedDelegate != _msgSender() || proposalType != uint8(_expectedProposalType) + || attestation.recipient != _msgSender() || proposalType != uint8(_expectedProposalType) ) { revert ProposalValidator_InvalidAttestation(); } diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index 92f8863dfb515..edab6a51f1a4c 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -115,6 +115,7 @@ contract ProposalValidator_Init is CommonTest { uint256 public constant OPTIMISTIC_MODULE_PERCENT_DIVISOR = 10_000; uint8 public constant APPROVAL_VOTING_MODULE_ID = 1; uint8 public constant OPTIMISTIC_VOTING_MODULE_ID = 2; + uint64 public constant ATT_EXPIRATION_TIME = 10 days; address owner; address user; @@ -561,7 +562,7 @@ contract ProposalValidator_Init is CommonTest { // Create schemas vm.prank(owner); APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID = ISchemaRegistry(Predeploys.SCHEMA_REGISTRY).register( - "address approvedAddress,uint8 proposalType", ISchemaResolver(address(0)), true + "uint8 proposalType,string date", ISchemaResolver(address(0)), true ); vm.prank(owner); @@ -588,11 +589,11 @@ contract ProposalValidator_Init is CommonTest { AttestationRequest({ schema: APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, data: AttestationRequestData({ - recipient: address(0), - expirationTime: 0, + recipient: _delegate, + expirationTime: uint64(block.timestamp + ATT_EXPIRATION_TIME), revocable: true, refUID: bytes32(0), - data: abi.encode(_delegate, _proposalType), + data: abi.encode(_proposalType, "2000-01-01"), value: 0 }) }) @@ -948,6 +949,21 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I ); } + function testFuzz_submitUpgradeProposal_attestationExpired_reverts(uint8 proposalTypeValue) public { + proposalTypeValue = uint8(bound(proposalTypeValue, 0, 1)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + uint248 againstThreshold = 5000; + bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); + + // warp the time to after the attestation expiration time + vm.warp(block.timestamp + ATT_EXPIRATION_TIME + 1); + vm.expectRevert(ProposalValidator.ProposalValidator_AttestationExpired.selector); + vm.prank(topDelegate_A); + validator.submitUpgradeProposal( + againstThreshold, proposalDescription, attestationUid, proposalType, CYCLE_NUMBER + ); + } + function test_submitUpgradeProposal_zeroAgainstThreshold_reverts() public { uint248 zeroThreshold = 0; ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; @@ -1083,11 +1099,11 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I AttestationRequest({ schema: APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, data: AttestationRequestData({ - recipient: address(0), - expirationTime: 0, + recipient: topDelegate_A, + expirationTime: uint64(block.timestamp + ATT_EXPIRATION_TIME), revocable: false, refUID: bytes32(0), - data: abi.encode(topDelegate_A, proposalType), + data: abi.encode(proposalType, "2000-01-01"), value: 0 }) }) @@ -1292,6 +1308,16 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_TestFail is Prop ); } + function testFuzz_submitCouncilMemberElectionsProposal_attestationExpired_reverts() public { + // warp the time to after the attestation expiration time + vm.warp(block.timestamp + ATT_EXPIRATION_TIME + 1); + vm.expectRevert(ProposalValidator.ProposalValidator_AttestationExpired.selector); + vm.prank(topDelegate_A); + validator.submitCouncilMemberElectionsProposal( + criteriaValue, optionDescriptions, proposalDescription, attestationUid, CYCLE_NUMBER + ); + } + function test_submitCouncilMemberElectionsProposal_zeroOptions_reverts() public { string[] memory emptyOptions = new string[](0); @@ -1370,11 +1396,11 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_TestFail is Prop AttestationRequest({ schema: APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, data: AttestationRequestData({ - recipient: address(0), - expirationTime: 0, + recipient: topDelegate_A, + expirationTime: uint64(block.timestamp + ATT_EXPIRATION_TIME), revocable: false, refUID: bytes32(0), - data: abi.encode(topDelegate_A, ProposalValidator.ProposalType.CouncilMemberElections), + data: abi.encode(ProposalValidator.ProposalType.CouncilMemberElections, "2000-01-01"), value: 0 }) }) From 6a4a6fcb7005f31ef260371b6592bddff9d66cf5 Mon Sep 17 00:00:00 2001 From: 0xOneTony <112496816+0xOneTony@users.noreply.github.com> Date: Tue, 22 Jul 2025 19:04:42 +0300 Subject: [PATCH 07/14] fix: voting cycle validity on submit (#454) * fix: voting cycle validity on submit * fix: edge case on upgrade proposals --- .../governance/IProposalValidator.sol | 2 +- .../snapshots/abi/ProposalValidator.json | 2 +- .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 25 +++++++- .../test/governance/ProposalValidator.t.sol | 59 +++++++++++++++---- 5 files changed, 74 insertions(+), 18 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index 7d93349984ea2..f270847582df6 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -107,7 +107,7 @@ interface IProposalValidator is ISemver { string memory _proposalDescription, bytes32 _attestationUid, ProposalType _proposalType, - uint256 _votingCycle + uint256 _latestVotingCycle ) external returns (bytes32 proposalHash_); function submitCouncilMemberElectionsProposal( diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 2afaef89b24f9..044e2188e955f 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -524,7 +524,7 @@ }, { "internalType": "uint256", - "name": "_votingCycle", + "name": "_latestVotingCycle", "type": "uint256" } ], diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index fd811a1f8d457..fe9a198219386 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xbac284f6ec21a5d65d5b86d7e6406e0805d77e15dc4bd66397f0111701110e0e", - "sourceCodeHash": "0x58048692d1da18d17958b2dc950055ae2a58ab645385b0aed3528b289dfee21d" + "initCodeHash": "0x33e740162106c353e5d9f550c67d47286846b80a91f042f5b600703e4ed0b42c", + "sourceCodeHash": "0xb51010203b3a894896a1e70c999f858d6d8e937c08feb301642e93f9787081e5" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index c871a2b9431d4..704bfd2653f9f 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -302,14 +302,15 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param _proposalDescription Description of the proposal. /// @param _attestationUid The UID of the attestation for the approved proposer. /// @param _proposalType The type of proposal (ProtocolOrGovernorUpgrade or MaintenanceUpgrade). - /// @param _votingCycle The voting cycle number the proposal is targetted for. + /// @param _latestVotingCycle The latest voting cycle number. Even though the upgrade proposal can be submitted + /// outside of a voting cycle, we still need the latest voting cycle number to validate top delegates attestations. /// @return proposalHash_ The hash of the submitted proposal. function submitUpgradeProposal( uint248 _againstThreshold, string memory _proposalDescription, bytes32 _attestationUid, ProposalType _proposalType, - uint256 _votingCycle + uint256 _latestVotingCycle ) external returns (bytes32 proposalHash_) @@ -320,6 +321,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_InvalidUpgradeProposalType(); } + // Validate voting cycle exists + VotingCycleData memory latestVotingCycleData = votingCycles[_latestVotingCycle]; + if (latestVotingCycleData.startingTimestamp == 0) { + revert ProposalValidator_InvalidVotingCycle(); + } + // Validate EAS attestation - must be called by owner-approved address _validateApprovedProposerAttestation(_attestationUid, _proposalType); @@ -367,7 +374,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // Store proposal metadata proposal.proposer = _msgSender(); proposal.proposalType = _proposalType; - proposal.votingCycle = _votingCycle; + proposal.votingCycle = _latestVotingCycle; emit ProposalSubmitted(proposalHash_, _msgSender(), _proposalDescription, _proposalType); emit ProposalVotingModuleData(proposalHash_, proposalVotingModuleData); @@ -407,6 +414,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { external returns (bytes32 proposalHash_) { + // Validate voting cycle exists and is not in the past + VotingCycleData memory votingCycleData = votingCycles[_votingCycle]; + if (votingCycleData.startingTimestamp == 0 || votingCycleData.startingTimestamp < block.timestamp) { + revert ProposalValidator_InvalidVotingCycle(); + } + // Validate EAS attestation - must be called by owner-approved address _validateApprovedProposerAttestation(_attestationUid, ProposalType.CouncilMemberElections); @@ -503,6 +516,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_InvalidFundingProposalType(); } + // Validate voting cycle exists and is not in the past + VotingCycleData memory votingCycleData = votingCycles[_votingCycle]; + if (votingCycleData.startingTimestamp == 0 || votingCycleData.startingTimestamp < block.timestamp) { + revert ProposalValidator_InvalidVotingCycle(); + } + // Validate input arrays have matching lengths uint256 optionsLength = _optionsDescriptions.length; if (optionsLength != _optionsRecipients.length || optionsLength != _optionsAmounts.length) { diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index edab6a51f1a4c..b70fb6e43a730 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -895,6 +895,7 @@ contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init /// @notice Sad path tests for submitUpgradeProposal function contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_Init { string proposalDescription; + uint248 againstThreshold = 5000; // 50% function setUp() public override { super.setUp(); @@ -910,7 +911,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I proposalTypeValue = uint8(bound(proposalTypeValue, 2, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); - uint248 againstThreshold = 5000; // 50% bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); vm.expectRevert(ProposalValidator.ProposalValidator_InvalidUpgradeProposalType.selector); @@ -920,8 +920,25 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I ); } + function testFuzz_submitUpgradeProposal_invalidVotingCycle_reverts( + uint8 proposalTypeValue, + uint256 votingCycle + ) + public + { + proposalTypeValue = uint8(bound(proposalTypeValue, 0, 1)); + vm.assume(votingCycle != CYCLE_NUMBER); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); + + vm.expectRevert(ProposalValidator.ProposalValidator_InvalidVotingCycle.selector); + vm.prank(topDelegate_A); + validator.submitUpgradeProposal( + againstThreshold, proposalDescription, attestationUid, proposalType, votingCycle + ); + } + function testFuzz_submitUpgradeProposal_invalidAttestation_reverts(bytes32 fuzzedAttestationUid) public { - uint248 againstThreshold = 5000; ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; bytes32 validAttestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); @@ -937,7 +954,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I function testFuzz_submitUpgradeProposal_unattestedProposer_reverts(address fuzzedProposer) public { vm.assume(fuzzedProposer != topDelegate_A); // Ensure it's different from attested proposer - uint248 againstThreshold = 5000; ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); @@ -952,7 +968,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I function testFuzz_submitUpgradeProposal_attestationExpired_reverts(uint8 proposalTypeValue) public { proposalTypeValue = uint8(bound(proposalTypeValue, 0, 1)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); - uint248 againstThreshold = 5000; bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); // warp the time to after the attestation expiration time @@ -990,7 +1005,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I } function test_submitUpgradeProposal_invalidVotingModule_reverts() public { - uint248 againstThreshold = 5000; // 50% ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); @@ -1009,7 +1023,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I proposalTypeValue = uint8(bound(proposalTypeValue, 0, 1)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); - uint248 againstThreshold = 5000; bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); // Calculate expected proposal hash @@ -1061,7 +1074,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I proposalTypeValue = uint8(bound(proposalTypeValue, 0, 1)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); - uint248 againstThreshold = 5000; bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); // Calculate expected proposal hash @@ -1090,7 +1102,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I function testFuzz_submitUpgradeProposal_attestationNotFromOwner_reverts(address fuzzedAttester) public { vm.assume(fuzzedAttester != owner); // Ensure it's not the approved owner - uint248 againstThreshold = 5000; ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; // Create attestation but don't use proper owner as attester @@ -1121,8 +1132,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I proposalTypeValue = uint8(bound(proposalTypeValue, 0, 1)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); - uint248 againstThreshold = 5000; - // Create valid attestation first (make it revocable) bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); @@ -1143,7 +1152,6 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I } function test_submitUpgradeProposal_proposalIdMismatch_reverts(uint256 proposalId) public { - uint248 againstThreshold = 5000; ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.MaintenanceUpgrade; bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); @@ -1285,6 +1293,15 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_TestFail is Prop _createApprovedProposerAttestation(topDelegate_A, ProposalValidator.ProposalType.CouncilMemberElections); } + function testFuzz_submitCouncilMemberElectionsProposal_invalidVotingCycle_reverts(uint256 votingCycle) public { + vm.assume(votingCycle != CYCLE_NUMBER); + vm.expectRevert(ProposalValidator.ProposalValidator_InvalidVotingCycle.selector); + vm.prank(topDelegate_A); + validator.submitCouncilMemberElectionsProposal( + criteriaValue, optionDescriptions, proposalDescription, attestationUid, votingCycle + ); + } + function testFuzz_submitCouncilMemberElectionsProposal_invalidAttestation_reverts(bytes32 fuzzedAttestationUid) public { @@ -1581,6 +1598,26 @@ contract ProposalValidator_SubmitFundingProposal_TestFail is ProposalValidator_I ); } + function testFuzz_submitFundingProposal_invalidVotingCycle_reverts( + uint8 proposalTypeValue, + uint256 votingCycle + ) + public + { + proposalTypeValue = uint8(bound(proposalTypeValue, 3, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + vm.assume(votingCycle != CYCLE_NUMBER); + + (string[] memory descriptions, address[] memory recipients, uint256[] memory amounts) = + _createMinimalFundingArrays(1); + + vm.expectRevert(ProposalValidator.ProposalValidator_InvalidVotingCycle.selector); + vm.prank(user); + validator.submitFundingProposal( + FUNDING_CRITERIA_VALUE, descriptions, recipients, amounts, description, proposalType, votingCycle + ); + } + function testFuzz_submitFundingProposal_mismatchedDescriptionsLength_reverts( uint8 matchingLength, uint8 mismatchedLength, From 663858467ef936bb65de4c955be436660a275ebf Mon Sep 17 00:00:00 2001 From: 0xOneTony <112496816+0xOneTony@users.noreply.github.com> Date: Tue, 22 Jul 2025 22:14:08 +0300 Subject: [PATCH 08/14] fix: budget cap dos (#453) * fix: budget cap dos * fix: invalid proposal case * fix: test * fix: tests --- .../governance/IProposalValidator.sol | 3 +- .../snapshots/abi/ProposalValidator.json | 10 +++ .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 52 +++++++++++-- .../test/governance/ProposalValidator.t.sol | 76 +++++++++++++++++-- 5 files changed, 132 insertions(+), 13 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index f270847582df6..2682c9eb5a7dd 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -31,6 +31,7 @@ interface IProposalValidator is ISemver { error ProposalValidator_InvalidProposer(); error ProposalValidator_InvalidProposal(); error ProposalValidator_InvalidVotingModule(); + error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); event ProposalSubmitted( bytes32 indexed proposalHash, @@ -130,7 +131,7 @@ interface IProposalValidator is ISemver { function approveProposal(bytes32 _proposalHash, bytes32 _attestationUid) external; - function canApproveProposal(bytes32 _attestationUid, address _delegate) external view returns (bool canApprove_); + function canApproveProposal(bytes32 _attestationUid, address _delegate, bytes32 _proposalHash) external view returns (bool canApprove_); function moveToVoteProtocolOrGovernorUpgradeProposal( uint248 _againstThreshold, diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 044e2188e955f..9c41904e7fdd9 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -101,6 +101,11 @@ "internalType": "address", "name": "_delegate", "type": "address" + }, + { + "internalType": "bytes32", + "name": "_proposalHash", + "type": "bytes32" } ], "name": "canApproveProposal", @@ -788,6 +793,11 @@ "name": "VotingCycleDataSet", "type": "event" }, + { + "inputs": [], + "name": "ProposalValidator_AttestationCreatedAfterLastVotingCycle", + "type": "error" + }, { "inputs": [], "name": "ProposalValidator_AttestationExpired", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index fe9a198219386..cef9eab6c8552 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x33e740162106c353e5d9f550c67d47286846b80a91f042f5b600703e4ed0b42c", - "sourceCodeHash": "0xb51010203b3a894896a1e70c999f858d6d8e937c08feb301642e93f9787081e5" + "initCodeHash": "0xd359b54afbf8f0bfcde0e24b648d20f1eecdc306b511d3c0cd90afa5b61382ac", + "sourceCodeHash": "0x6da6042e1bc89da33dad2ce39aaef685f2a67c04e4693f69c2693b349899d131" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 704bfd2653f9f..537d808e31434 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -92,6 +92,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Thrown when the voting module address is invalid (zero address). error ProposalValidator_InvalidVotingModule(); + /// @notice Thrown when the attestation was created after the last voting cycle. + error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); + /*////////////////////////////////////////////////////////////// EVENTS //////////////////////////////////////////////////////////////*/ @@ -591,7 +594,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { address _delegate = _msgSender(); ProposalData storage proposal = _proposals[_proposalHash]; // check if the proposal exists - if (proposal.proposer == address(0)) { + // proposal.votingCycle should never be 0, voting cycles already exist before the ProposalValidator is deployed + // and should be set by the OP Foundation + if (proposal.proposer == address(0) || proposal.votingCycle == 0) { revert ProposalValidator_ProposalDoesNotExist(); } @@ -605,8 +610,17 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalAlreadyMovedToVote(); } + // The previous voting cycle of a proposal should be the one before the + // proposal's targetted voting cycle. + uint256 previousVotingCycle = proposal.votingCycle - 1; + // Proposal or Governor Upgrade proposals are submitted with the latest voting cycle number, + // because they can be submitted outside of a voting cycle. + if (proposal.proposalType == ProposalType.ProtocolOrGovernorUpgrade) { + previousVotingCycle = proposal.votingCycle; + } + // validate the attestation - _validateTopDelegateAttestation(_attestationUid, _msgSender()); + _validateTopDelegateAttestation(_attestationUid, _msgSender(), previousVotingCycle); // store the approval proposal.delegateApprovals[_delegate] = true; @@ -618,9 +632,25 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Checks if a delegate can approve a proposal. /// @dev Helper function for UI integration. /// @param _attestationUid The UID of the attestation to check. + /// @param _delegate The delegate to check the attestation for. + /// @param _proposalHash The hash of the proposal to check the attestation for. /// @return canApprove_ True if the delegate can approve the proposal, false otherwise. - function canApproveProposal(bytes32 _attestationUid, address _delegate) external view returns (bool canApprove_) { - canApprove_ = _validateTopDelegateAttestation(_attestationUid, _delegate); + function canApproveProposal( + bytes32 _attestationUid, + address _delegate, + bytes32 _proposalHash + ) + external + view + returns (bool canApprove_) + { + // TODO: this function should be fixed in OPT-957 + ProposalData storage proposal = _proposals[_proposalHash]; + if (proposal.votingCycle == 0) { + return false; + } + + canApprove_ = _validateTopDelegateAttestation(_attestationUid, _delegate, proposal.votingCycle - 1); } /// @notice Moves a Protocol or Governor Upgrade proposal to vote by proposing it on the Governor. @@ -955,13 +985,18 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @return canApprove_ True if the attestation is valid, false otherwise. function _validateTopDelegateAttestation( bytes32 _attestationUid, - address _delegate + address _delegate, + uint256 _lastVotingCycle ) internal view returns (bool canApprove_) { Attestation memory attestation = IEAS(Predeploys.EAS).getAttestation(_attestationUid); + VotingCycleData memory previousVotingCycleData = votingCycles[_lastVotingCycle]; + if (previousVotingCycleData.startingTimestamp == 0) { + revert ProposalValidator_InvalidVotingCycle(); + } // Check if attestation exists, equivalent to calling EAS.isAttestationValid(_attestationUid) if (attestation.uid == bytes32(0)) { @@ -978,6 +1013,13 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_AttestationRevoked(); } + // since the attestations are updated daily we should only allow attestations + // created before the last voting cycle of the proposal + // check if attestation was created after the previous voting cycle + if (attestation.time > previousVotingCycleData.startingTimestamp + previousVotingCycleData.duration) { + revert ProposalValidator_AttestationCreatedAfterLastVotingCycle(); + } + (, bool _includePartialDelegation,) = abi.decode(attestation.data, (string, bool, string)); // check if the attestation includes partial delegation or the recipient is not the caller diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index b70fb6e43a730..7e88af159b627 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -1884,6 +1884,15 @@ contract ProposalValidator_SubmitFundingProposal_TestFail is ProposalValidator_I /// @title ProposalValidator_ApproveProposal_Test /// @notice Happy path tests for approveProposal function contract ProposalValidator_ApproveProposal_Test is ProposalValidator_Init { + function setUp() public override { + super.setUp(); + + // create a new voting cycle + // cycle number decreased by 1 and start time CYCLE_DURATION before the current cycle + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + } + function test_approveProposal_succeeds(bytes32 _proposalHash, uint8 proposalTypeValue) public { // Ensure the proposal hash is not 0 vm.assume(_proposalHash != bytes32(0)); @@ -1961,6 +1970,25 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { validator.approveProposal(_proposalHash, topDelegateAttestation_A); } + function test_approveProposal_invalidVotingCycle_reverts( + bytes32 _proposalHash, + uint8 proposalTypeValue, + uint256 votingCycle + ) + public + { + vm.assume(votingCycle != CYCLE_NUMBER); + // Bound the proposal type to valid enum values (0-4) + proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + // set proposal data so that the proposal exists + validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, votingCycle); + + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidVotingCycle.selector); + vm.prank(topDelegate_A); + validator.approveProposal(_proposalHash, topDelegateAttestation_A); + } + function test_approveProposal_invalidSchema_reverts(bytes32 _proposalHash, uint8 proposalTypeValue) public { // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); @@ -1990,6 +2018,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // set proposal data so that the proposal exists validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + // set the voting cycle data of the previous cycle + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestationSchema.selector); vm.prank(topDelegate_A); @@ -2002,6 +2033,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // set proposal data so that the proposal exists validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + // set the voting cycle data of the previous cycle + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); // revoke the attestation vm.prank(owner); @@ -2033,6 +2067,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + // set the voting cycle data of the previous cycle + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); // Expect the invalid attestation error to be reverted vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); @@ -2052,6 +2089,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + // set the voting cycle data of the previous cycle + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); // create an attestation with partial delegation vm.prank(owner); @@ -2091,6 +2131,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + // set the voting cycle data of the previous cycle + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); // Expect the invalid attestation error to be reverted when attestation doesn't exist vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); @@ -2102,20 +2145,34 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { /// @title ProposalValidator_CanApproveProposal_Test /// @notice Tests for the canApproveProposal function contract ProposalValidator_CanApproveProposal_Test is ProposalValidator_Init { - function test_canApproveProposal_returnTrue_succeeds() public view { + function test_canApproveProposal_returnTrue_succeeds(bytes32 _proposalHash, uint8 proposalTypeValue) public { + // Bound the proposal type to valid enum values (0-4) + proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + // set the voting cycle data of the previous cycle + validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // Attestation already created in setUp - bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A); + bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A, _proposalHash); assertTrue(canApprove); } - function test_canApproveProposal_returnFalse_succeeds(bytes32 attestationUid, address delegate) public { + function test_canApproveProposal_returnFalseRevert_succeeds( + bytes32 _attestationUid, + address _delegate, + bytes32 _proposalHash + ) + public + { // Ensure the attestation uid is not one of the top delegates - vm.assume(attestationUid != topDelegateAttestation_A); + vm.assume(_attestationUid != topDelegateAttestation_A); bool canApprove; // Expect the invalid attestation error to be reverted vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); - try validator.canApproveProposal(attestationUid, delegate) returns (bool result_) { + try validator.canApproveProposal(_attestationUid, _delegate, _proposalHash) returns (bool result_) { canApprove = result_; } catch { canApprove = false; @@ -2123,6 +2180,15 @@ contract ProposalValidator_CanApproveProposal_Test is ProposalValidator_Init { assertEq(canApprove, false); } + + function test_canApproveProposal_returnFalseProposalNotFound_reverts(bytes32 _proposalHash) public { + validator.setProposalData( + _proposalHash, topDelegate_A, ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade, false, 0, 0 + ); + + bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A, _proposalHash); + assertEq(canApprove, false); + } } /// @title ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_Test From 505ae505cc8c30d4a4003618827963d26fbef1a8 Mon Sep 17 00:00:00 2001 From: Chiin <77933451+0xChin@users.noreply.github.com> Date: Wed, 23 Jul 2025 11:33:05 -0300 Subject: [PATCH 09/14] fix: move to vote logic (#455) * refactor: improve variable naming * fix: wrong arg sent to external proposeWithModuole calls * fix: pre-pr * fix: stack too deep error * fix: pre-pr * fix: pre-pr --------- Signed-off-by: Chiin <77933451+0xChin@users.noreply.github.com> --- .../governance/IProposalValidator.sol | 6 +- .../snapshots/abi/ProposalValidator.json | 8 +- .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 100 ++++++++++-------- .../test/governance/ProposalValidator.t.sol | 76 ++++++------- 5 files changed, 97 insertions(+), 97 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index 2682c9eb5a7dd..090499667d378 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -62,7 +62,7 @@ interface IProposalValidator is ISemver { event ProposalTypeDataSet( ProposalType proposalType, uint256 requiredApprovals, - uint8 proposalVotingModule + uint8 idInConfigurator ); event ProposalVotingModuleData( @@ -85,7 +85,7 @@ interface IProposalValidator is ISemver { struct ProposalTypeData { uint256 requiredApprovals; - uint8 proposalVotingModule; + uint8 idInConfigurator; } struct VotingCycleData { @@ -196,7 +196,7 @@ interface IProposalValidator is ISemver { function OPTIMISTIC_MODULE_PERCENT_DIVISOR() external view returns (uint256); - function proposalTypesData(ProposalType) external view returns (uint256 requiredApprovals, uint8 proposalVotingModule); + function proposalTypesData(ProposalType) external view returns (uint256 requiredApprovals, uint8 idInConfigurator); function votingCycles(uint256) external view returns ( uint256 startingTimestamp, diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 9c41904e7fdd9..66b2363eda438 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -178,7 +178,7 @@ }, { "internalType": "uint8", - "name": "proposalVotingModule", + "name": "idInConfigurator", "type": "uint8" } ], @@ -332,7 +332,7 @@ }, { "internalType": "uint8", - "name": "proposalVotingModule", + "name": "idInConfigurator", "type": "uint8" } ], @@ -375,7 +375,7 @@ }, { "internalType": "uint8", - "name": "proposalVotingModule", + "name": "idInConfigurator", "type": "uint8" } ], @@ -736,7 +736,7 @@ { "indexed": false, "internalType": "uint8", - "name": "proposalVotingModule", + "name": "idInConfigurator", "type": "uint8" } ], diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index cef9eab6c8552..ae6cdf304eb92 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xd359b54afbf8f0bfcde0e24b648d20f1eecdc306b511d3c0cd90afa5b61382ac", - "sourceCodeHash": "0x6da6042e1bc89da33dad2ce39aaef685f2a67c04e4693f69c2693b349899d131" + "initCodeHash": "0xca95fa18ecb8d7d44043dd60c545719e526fdf487ea405c8321d25b92782bfd6", + "sourceCodeHash": "0xf0b71a440952d0a4a00a488f5b98da2010d1972297876ea8ade494d0885f7508" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 537d808e31434..42242128349b1 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -134,8 +134,8 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Emitted when the proposal type data is set. /// @param proposalType The type of proposal. /// @param requiredApprovals The required number of approvals. - /// @param proposalVotingModule The proposal type ID. - event ProposalTypeDataSet(ProposalType proposalType, uint256 requiredApprovals, uint8 proposalVotingModule); + /// @param idInConfigurator The proposal type ID. + event ProposalTypeDataSet(ProposalType proposalType, uint256 requiredApprovals, uint8 idInConfigurator); /// @notice Emitted with ProposalSubmitted event. /// @param proposalHash The hash of the submitted proposal. @@ -165,10 +165,10 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Struct for storing explicit data for each proposal type. /// @param requiredApprovals The number of approvals each proposal type requires in order to be able to move for /// voting. - /// @param proposalVotingModule The proposal type ID used to get the voting module from the configurator. + /// @param idInConfigurator The proposal type ID used to get the voting module from the configurator. struct ProposalTypeData { uint256 requiredApprovals; - uint8 proposalVotingModule; + uint8 idInConfigurator; } /// @notice Struct for storing voting cycle data. @@ -325,8 +325,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // Validate voting cycle exists - VotingCycleData memory latestVotingCycleData = votingCycles[_latestVotingCycle]; - if (latestVotingCycleData.startingTimestamp == 0) { + if (votingCycles[_latestVotingCycle].startingTimestamp == 0) { revert ProposalValidator_InvalidVotingCycle(); } @@ -338,24 +337,29 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_InvalidAgainstThreshold(); } - // Create OptimisticModule ProposalSettings with required parameters - IOptimisticModule.ProposalSettings memory optimisticSettings = IOptimisticModule.ProposalSettings({ - againstThreshold: _againstThreshold, - isRelativeToVotableSupply: true // MUST always be true - }); - // Optimistic proposals are signal-only, no execution targets/calldatas needed - bytes memory proposalVotingModuleData = abi.encode(optimisticSettings); + bytes memory proposalVotingModuleData = abi.encode( + IOptimisticModule.ProposalSettings({ + againstThreshold: _againstThreshold, + isRelativeToVotableSupply: true // MUST always be true + }) + ); + + // Retrieve the ID to use in the proposal type configurator + uint8 idInConfigurator = proposalTypesData[_proposalType].idInConfigurator; // Get the optimistic module address from configurator - IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( - GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() - ).proposalTypes(proposalTypesData[_proposalType].proposalVotingModule); - address votingModule = proposalTypeConfig.module; + address votingModule; + { + IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = + IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator); - // Validate voting module exists - if (bytes(proposalTypeConfig.name).length == 0) { - revert ProposalValidator_InvalidVotingModule(); + // Validate voting module exists + if (bytes(proposalTypeConfig.name).length == 0) { + revert ProposalValidator_InvalidVotingModule(); + } + + votingModule = proposalTypeConfig.module; } // Generate unique proposal hash @@ -387,7 +391,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { proposal.movedToVote = true; uint256 proposalId = GOVERNOR.proposeWithModule( - votingModule, proposalVotingModuleData, _proposalDescription, uint8(_proposalType) + votingModule, proposalVotingModuleData, _proposalDescription, idInConfigurator ); // Make sure the proposalId is the same as the proposalHash @@ -455,7 +459,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // Get the module address from the configurator IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() - ).proposalTypes(proposalTypesData[ProposalType.CouncilMemberElections].proposalVotingModule); + ).proposalTypes(proposalTypesData[ProposalType.CouncilMemberElections].idInConfigurator); address votingModule = proposalTypeConfig.module; // Validate voting module exists @@ -554,7 +558,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // Get the module address from the configurator IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() - ).proposalTypes(proposalTypesData[_proposalType].proposalVotingModule); + ).proposalTypes(proposalTypesData[_proposalType].idInConfigurator); address votingModule = proposalTypeConfig.module; // Validate voting module exists @@ -670,11 +674,13 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(settings); + // Retrieve the ID to use in the proposal type configurator + uint8 idInConfigurator = proposalTypesData[ProposalType.ProtocolOrGovernorUpgrade].idInConfigurator; + // Get the module address from the configurator ProposalType proposalType = ProposalType.ProtocolOrGovernorUpgrade; - address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( - proposalTypesData[ProposalType.ProtocolOrGovernorUpgrade].proposalVotingModule - ).module; + address votingModule = + IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator).module; // Generate unique proposal hash proposalHash_ = @@ -705,9 +711,8 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { proposal.movedToVote = true; // Propose with module on the Governor - uint256 proposalId = GOVERNOR.proposeWithModule( - votingModule, proposalVotingModuleData, _proposalDescription, uint8(proposalType) - ); + uint256 proposalId = + GOVERNOR.proposeWithModule(votingModule, proposalVotingModuleData, _proposalDescription, idInConfigurator); // Make sure the proposalId is the same as the proposalHash if (proposalId != uint256(proposalHash_)) { @@ -745,11 +750,14 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, settings); + ProposalType _proposalType = ProposalType.CouncilMemberElections; + + // Retrieve the ID to use in the proposal type configurator + uint8 idInConfigurator = proposalTypesData[_proposalType].idInConfigurator; + // Get the module address from the configurator - ProposalType proposalType = ProposalType.CouncilMemberElections; - address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( - proposalTypesData[proposalType].proposalVotingModule - ).module; + address votingModule = + IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator).module; // Generate unique proposal hash proposalHash_ = @@ -758,7 +766,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { ProposalData storage proposal = _proposals[proposalHash_]; // Proposal must exist and be valid - if (proposal.proposer == address(0) || proposal.proposalType != proposalType) { + if (proposal.proposer == address(0) || proposal.proposalType != _proposalType) { revert ProposalValidator_InvalidProposal(); } @@ -768,7 +776,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // Check if proposal has enough approvals - if (proposal.approvalCount < proposalTypesData[proposalType].requiredApprovals) { + if (proposal.approvalCount < proposalTypesData[_proposalType].requiredApprovals) { revert ProposalValidator_InsufficientApprovals(); } @@ -789,9 +797,8 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { proposal.movedToVote = true; // Propose with module on the Governor - uint256 proposalId = GOVERNOR.proposeWithModule( - votingModule, proposalVotingModuleData, _proposalDescription, uint8(proposalType) - ); + uint256 proposalId = + GOVERNOR.proposeWithModule(votingModule, proposalVotingModuleData, _proposalDescription, idInConfigurator); // Make sure the proposalId is the same as the proposalHash if (proposalId != uint256(proposalHash_)) { @@ -824,7 +831,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { external returns (bytes32 proposalHash_) { - uint256 optionsLength = _optionsDescriptions.length; // Only funding proposal types can use this function if (_proposalType != ProposalType.GovernanceFund && _proposalType != ProposalType.CouncilBudget) { revert ProposalValidator_InvalidFundingProposalType(); @@ -836,7 +842,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // Configure approval module settings IApprovalVotingModule.ProposalSettings memory settings = IApprovalVotingModule.ProposalSettings({ - maxApprovals: uint8(optionsLength), + maxApprovals: uint8(_optionsDescriptions.length), criteria: uint8(IApprovalVotingModule.PassingCriteria.Threshold), budgetToken: Predeploys.GOVERNANCE_TOKEN, criteriaValue: _criteriaValue, @@ -845,10 +851,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, settings); + // Retrieve the ID to use in the proposal type configurator + uint8 idInConfigurator = proposalTypesData[_proposalType].idInConfigurator; + // Get the module address from the configurator - address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes( - proposalTypesData[_proposalType].proposalVotingModule - ).module; + address votingModule = + IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator).module; // Generate unique proposal hash proposalHash_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); @@ -890,7 +898,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // Propose with module on the Governor uint256 proposalId = - GOVERNOR.proposeWithModule(votingModule, proposalVotingModuleData, _description, uint8(_proposalType)); + GOVERNOR.proposeWithModule(votingModule, proposalVotingModuleData, _description, idInConfigurator); // Make sure the proposalId is the same as the proposalHash if (proposalId != uint256(proposalHash_)) { @@ -1143,8 +1151,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param _proposalTypeData The data for the proposal type. function _setProposalTypeData(ProposalType _proposalType, ProposalTypeData memory _proposalTypeData) private { proposalTypesData[_proposalType] = _proposalTypeData; - emit ProposalTypeDataSet( - _proposalType, _proposalTypeData.requiredApprovals, _proposalTypeData.proposalVotingModule - ); + emit ProposalTypeDataSet(_proposalType, _proposalTypeData.requiredApprovals, _proposalTypeData.idInConfigurator); } } diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index 7e88af159b627..15eea020bd5d8 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -146,7 +146,7 @@ contract ProposalValidator_Init is CommonTest { ); event ProposalDistributionThresholdSet(uint256 newProposalDistributionThreshold); event ProposalTypeDataSet( - ProposalValidator.ProposalType proposalType, uint256 requiredApprovals, uint8 proposalVotingModule + ProposalValidator.ProposalType proposalType, uint256 requiredApprovals, uint8 idInConfigurator ); event ProposalVotingModuleData(bytes32 indexed proposalHash, bytes encodedVotingModuleData); @@ -167,9 +167,9 @@ contract ProposalValidator_Init is CommonTest { stdstore.target(address(validator)).sig("proposalTypesData(uint8)").with_key(uint256(_proposalType)).depth(0) .checked_write(_data.requiredApprovals); - // Set proposalVotingModule (depth 1) + // Set idInConfigurator (depth 1) stdstore.target(address(validator)).sig("proposalTypesData(uint8)").with_key(uint256(_proposalType)).depth(1) - .checked_write(_data.proposalVotingModule); + .checked_write(_data.idInConfigurator); } /// @notice Helper function to set CouncilMemberElections proposal type data. @@ -178,7 +178,7 @@ contract ProposalValidator_Init is CommonTest { ProposalValidator.ProposalType.CouncilMemberElections, ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: APPROVAL_VOTING_MODULE_ID + idInConfigurator: APPROVAL_VOTING_MODULE_ID }) ); } @@ -189,7 +189,7 @@ contract ProposalValidator_Init is CommonTest { ProposalValidator.ProposalType.GovernanceFund, ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: APPROVAL_VOTING_MODULE_ID + idInConfigurator: APPROVAL_VOTING_MODULE_ID }) ); } @@ -200,7 +200,7 @@ contract ProposalValidator_Init is CommonTest { ProposalValidator.ProposalType.CouncilBudget, ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: APPROVAL_VOTING_MODULE_ID + idInConfigurator: APPROVAL_VOTING_MODULE_ID }) ); } @@ -211,7 +211,7 @@ contract ProposalValidator_Init is CommonTest { ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade, ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: OPTIMISTIC_VOTING_MODULE_ID + idInConfigurator: OPTIMISTIC_VOTING_MODULE_ID }) ); } @@ -222,7 +222,7 @@ contract ProposalValidator_Init is CommonTest { ProposalValidator.ProposalType.MaintenanceUpgrade, ProposalValidator.ProposalTypeData({ requiredApprovals: 0, // MaintenanceUpgrade moves directly to voting - proposalVotingModule: OPTIMISTIC_VOTING_MODULE_ID + idInConfigurator: OPTIMISTIC_VOTING_MODULE_ID }) ); } @@ -259,27 +259,25 @@ contract ProposalValidator_Init is CommonTest { // ProtocolOrGovernorUpgrade proposalTypesData[0] = ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: OPTIMISTIC_VOTING_MODULE_ID + idInConfigurator: OPTIMISTIC_VOTING_MODULE_ID }); // MaintenanceUpgrade - proposalTypesData[1] = ProposalValidator.ProposalTypeData({ - requiredApprovals: 0, - proposalVotingModule: OPTIMISTIC_VOTING_MODULE_ID - }); + proposalTypesData[1] = + ProposalValidator.ProposalTypeData({ requiredApprovals: 0, idInConfigurator: OPTIMISTIC_VOTING_MODULE_ID }); // CouncilMemberElections proposalTypesData[2] = ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: APPROVAL_VOTING_MODULE_ID + idInConfigurator: APPROVAL_VOTING_MODULE_ID }); // GovernanceFund proposalTypesData[3] = ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: APPROVAL_VOTING_MODULE_ID + idInConfigurator: APPROVAL_VOTING_MODULE_ID }); // CouncilBudget proposalTypesData[4] = ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: APPROVAL_VOTING_MODULE_ID + idInConfigurator: APPROVAL_VOTING_MODULE_ID }); return (proposalTypes, proposalTypesData); @@ -680,7 +678,7 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { // Verify proposal type data for (uint256 i = 0; i < proposalTypes.length; i++) { - (uint256 requiredApprovals, uint8 proposalVotingModule) = validator.proposalTypesData(proposalTypes[i]); + (uint256 requiredApprovals, uint8 idInConfigurator) = validator.proposalTypesData(proposalTypes[i]); if (proposalTypes[i] == ProposalValidator.ProposalType.MaintenanceUpgrade) { assertEq(requiredApprovals, 0); } else { @@ -693,10 +691,10 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { || proposalTypes[i] == ProposalValidator.ProposalType.CouncilBudget || proposalTypes[i] == ProposalValidator.ProposalType.CouncilMemberElections ) { - assertEq(proposalVotingModule, APPROVAL_VOTING_MODULE_ID); + assertEq(idInConfigurator, APPROVAL_VOTING_MODULE_ID); } else { // ProtocolOrGovernorUpgrade and MaintenanceUpgrade use OPTIMISTIC_VOTING_MODULE_ID - assertEq(proposalVotingModule, OPTIMISTIC_VOTING_MODULE_ID); + assertEq(idInConfigurator, OPTIMISTIC_VOTING_MODULE_ID); } } } @@ -709,14 +707,10 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { // Create mismatched array with different length ProposalValidator.ProposalTypeData[] memory proposalTypesData = new ProposalValidator.ProposalTypeData[](2); - proposalTypesData[0] = ProposalValidator.ProposalTypeData({ - requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: 0 - }); - proposalTypesData[1] = ProposalValidator.ProposalTypeData({ - requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, - proposalVotingModule: 1 - }); + proposalTypesData[0] = + ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, idInConfigurator: 0 }); + proposalTypesData[1] = + ProposalValidator.ProposalTypeData({ requiredApprovals: PROPOSAL_REQUIRED_APPROVALS, idInConfigurator: 1 }); vm.prank(owner); vm.expectRevert("Proxy: delegatecall to new implementation contract failed"); @@ -790,7 +784,7 @@ contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (optimisticVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), abi.encode(uint256(expectedHash)) ); @@ -1046,7 +1040,7 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (optimisticVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), abi.encode(uint256(expectedHash)) ); @@ -1176,7 +1170,7 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (optimisticVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), abi.encode(proposalId) ); @@ -2216,7 +2210,7 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_Test is P address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (optimisticVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), abi.encode(uint256(expectedHash)) ); @@ -2309,7 +2303,7 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_TestFail address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (optimisticVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), abi.encode(uint256(_randomHash)) ); @@ -2348,7 +2342,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_Test is Prop address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (approvalVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (approvalVotingModule, votingModuleData, proposalDescription, APPROVAL_VOTING_MODULE_ID) ), abi.encode(uint256(expectedHash)) ); @@ -2455,7 +2449,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (approvalVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (approvalVotingModule, votingModuleData, proposalDescription, APPROVAL_VOTING_MODULE_ID) ), abi.encode(uint256(_randomHash)) ); @@ -2530,7 +2524,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I approvalVotingModule, governanceFundVotingModuleData, governanceFundProposalDescription, - uint8(governanceFundProposalType) + APPROVAL_VOTING_MODULE_ID ) ), abi.encode(uint256(expectedGovernanceFundHash)) @@ -2570,7 +2564,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I approvalVotingModule, councilBudgetVotingModuleData, councilBudgetProposalDescription, - uint8(councilBudgetProposalType) + APPROVAL_VOTING_MODULE_ID ) ), abi.encode(uint256(expectedCouncilBudgetHash)) @@ -2905,7 +2899,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat address(governor), abi.encodeCall( IOptimismGovernor.proposeWithModule, - (approvalVotingModule, votingModuleData, proposalDescription, uint8(proposalType)) + (approvalVotingModule, votingModuleData, proposalDescription, APPROVAL_VOTING_MODULE_ID) ), abi.encode(uint256(_randomHash)) ); @@ -3012,7 +3006,7 @@ contract ProposalValidator_Setters_Test is ProposalValidator_Init { ProposalValidator.ProposalTypeData memory newData = ProposalValidator.ProposalTypeData({ requiredApprovals: newRequiredApprovals, - proposalVotingModule: newProposalTypeId + idInConfigurator: newProposalTypeId }); // Expect the ProposalTypeDataSet event to be emitted @@ -3022,16 +3016,16 @@ contract ProposalValidator_Setters_Test is ProposalValidator_Init { vm.prank(owner); validator.setProposalTypeData(proposalType, newData); - (uint256 requiredApprovals, uint8 proposalVotingModule) = validator.proposalTypesData(proposalType); + (uint256 requiredApprovals, uint8 idInConfigurator) = validator.proposalTypesData(proposalType); assertEq(requiredApprovals, newRequiredApprovals); - assertEq(proposalVotingModule, newProposalTypeId); + assertEq(idInConfigurator, newProposalTypeId); } function testFuzz_setProposalTypeData_notOwner_reverts(address caller) public { vm.assume(caller != owner); ProposalValidator.ProposalTypeData memory newData = - ProposalValidator.ProposalTypeData({ requiredApprovals: 4, proposalVotingModule: 0 }); + ProposalValidator.ProposalTypeData({ requiredApprovals: 4, idInConfigurator: 0 }); vm.prank(caller); vm.expectRevert("Ownable: caller is not the owner"); From 253cc6e8ff2e5132ac14ef7102479f991adf4060 Mon Sep 17 00:00:00 2001 From: Chiin <77933451+0xChin@users.noreply.github.com> Date: Thu, 24 Jul 2025 09:54:55 -0300 Subject: [PATCH 10/14] fix: normalize validate functions (#456) * refactor: remove canApproveProposal and normalize validate functions * fix(test): handle invalid voting cycle edge case * fix: pre-pr --- .../governance/IProposalValidator.sol | 2 - .../snapshots/abi/ProposalValidator.json | 29 ----------- .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 43 ++-------------- .../test/governance/ProposalValidator.t.sol | 51 +------------------ 5 files changed, 7 insertions(+), 122 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index 090499667d378..ec63c98eaffd1 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -131,8 +131,6 @@ interface IProposalValidator is ISemver { function approveProposal(bytes32 _proposalHash, bytes32 _attestationUid) external; - function canApproveProposal(bytes32 _attestationUid, address _delegate, bytes32 _proposalHash) external view returns (bool canApprove_); - function moveToVoteProtocolOrGovernorUpgradeProposal( uint248 _againstThreshold, string memory _proposalDescription diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 66b2363eda438..dc516cfa49b44 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -90,35 +90,6 @@ "stateMutability": "nonpayable", "type": "function" }, - { - "inputs": [ - { - "internalType": "bytes32", - "name": "_attestationUid", - "type": "bytes32" - }, - { - "internalType": "address", - "name": "_delegate", - "type": "address" - }, - { - "internalType": "bytes32", - "name": "_proposalHash", - "type": "bytes32" - } - ], - "name": "canApproveProposal", - "outputs": [ - { - "internalType": "bool", - "name": "canApprove_", - "type": "bool" - } - ], - "stateMutability": "view", - "type": "function" - }, { "inputs": [], "name": "initVersion", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index ae6cdf304eb92..3a11df4c0a069 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xca95fa18ecb8d7d44043dd60c545719e526fdf487ea405c8321d25b92782bfd6", - "sourceCodeHash": "0xf0b71a440952d0a4a00a488f5b98da2010d1972297876ea8ade494d0885f7508" + "initCodeHash": "0x6bcced3e1050048ecc63fd4d9a86ec72e756de4cb328d29c357cc31c87834341", + "sourceCodeHash": "0x9380a09eeb5f12304baf0d45ea14e7bbd1b046d805429bfe0de970a7dfccd176" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 42242128349b1..30702c95efa85 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -624,7 +624,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // validate the attestation - _validateTopDelegateAttestation(_attestationUid, _msgSender(), previousVotingCycle); + _validateTopDelegateAttestation(_attestationUid, previousVotingCycle); // store the approval proposal.delegateApprovals[_delegate] = true; @@ -633,30 +633,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { emit ProposalApproved(_proposalHash, _delegate); } - /// @notice Checks if a delegate can approve a proposal. - /// @dev Helper function for UI integration. - /// @param _attestationUid The UID of the attestation to check. - /// @param _delegate The delegate to check the attestation for. - /// @param _proposalHash The hash of the proposal to check the attestation for. - /// @return canApprove_ True if the delegate can approve the proposal, false otherwise. - function canApproveProposal( - bytes32 _attestationUid, - address _delegate, - bytes32 _proposalHash - ) - external - view - returns (bool canApprove_) - { - // TODO: this function should be fixed in OPT-957 - ProposalData storage proposal = _proposals[_proposalHash]; - if (proposal.votingCycle == 0) { - return false; - } - - canApprove_ = _validateTopDelegateAttestation(_attestationUid, _delegate, proposal.votingCycle - 1); - } - /// @notice Moves a Protocol or Governor Upgrade proposal to vote by proposing it on the Governor. /// @param _againstThreshold The threshold for the proposal to be against the total supply. /// @param _proposalDescription Description of the proposal. @@ -989,17 +965,8 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Validates the attestation data for a delegate that tries to approve a proposal. /// @dev Only acceptes attestations that does NOT include partial delegation. /// @param _attestationUid The UID of the attestation to validate. - /// @param _delegate The delegate to validate the attestation for. - /// @return canApprove_ True if the attestation is valid, false otherwise. - function _validateTopDelegateAttestation( - bytes32 _attestationUid, - address _delegate, - uint256 _lastVotingCycle - ) - internal - view - returns (bool canApprove_) - { + /// @param _lastVotingCycle The last voting cycle to validate against. + function _validateTopDelegateAttestation(bytes32 _attestationUid, uint256 _lastVotingCycle) internal view { Attestation memory attestation = IEAS(Predeploys.EAS).getAttestation(_attestationUid); VotingCycleData memory previousVotingCycleData = votingCycles[_lastVotingCycle]; if (previousVotingCycleData.startingTimestamp == 0) { @@ -1031,11 +998,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { (, bool _includePartialDelegation,) = abi.decode(attestation.data, (string, bool, string)); // check if the attestation includes partial delegation or the recipient is not the caller - if (_includePartialDelegation || attestation.recipient != _delegate) { + if (_includePartialDelegation || attestation.recipient != _msgSender()) { revert ProposalValidator_InvalidAttestation(); } - - canApprove_ = true; } /// @notice Internal function to build proposal options with optional execution data. diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index 15eea020bd5d8..1407a1dc6017a 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -1971,7 +1971,7 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { ) public { - vm.assume(votingCycle != CYCLE_NUMBER); + vm.assume(votingCycle != CYCLE_NUMBER && votingCycle != 0); // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); @@ -2136,55 +2136,6 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { } } -/// @title ProposalValidator_CanApproveProposal_Test -/// @notice Tests for the canApproveProposal function -contract ProposalValidator_CanApproveProposal_Test is ProposalValidator_Init { - function test_canApproveProposal_returnTrue_succeeds(bytes32 _proposalHash, uint8 proposalTypeValue) public { - // Bound the proposal type to valid enum values (0-4) - proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); - ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); - // set the voting cycle data of the previous cycle - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); - vm.prank(owner); - validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); - - // Attestation already created in setUp - bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A, _proposalHash); - assertTrue(canApprove); - } - - function test_canApproveProposal_returnFalseRevert_succeeds( - bytes32 _attestationUid, - address _delegate, - bytes32 _proposalHash - ) - public - { - // Ensure the attestation uid is not one of the top delegates - vm.assume(_attestationUid != topDelegateAttestation_A); - - bool canApprove; - // Expect the invalid attestation error to be reverted - vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); - try validator.canApproveProposal(_attestationUid, _delegate, _proposalHash) returns (bool result_) { - canApprove = result_; - } catch { - canApprove = false; - } - - assertEq(canApprove, false); - } - - function test_canApproveProposal_returnFalseProposalNotFound_reverts(bytes32 _proposalHash) public { - validator.setProposalData( - _proposalHash, topDelegate_A, ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade, false, 0, 0 - ); - - bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A, _proposalHash); - assertEq(canApprove, false); - } -} - /// @title ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_Test /// @notice Happy path tests for moveToVoteProtocolOrGovernorUpgradeProposal function contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_Test is ProposalValidator_Init { From 332b1f3e49177773406b71855c37af7e46e6d173 Mon Sep 17 00:00:00 2001 From: Chiin <77933451+0xChin@users.noreply.github.com> Date: Thu, 24 Jul 2025 12:07:02 -0300 Subject: [PATCH 11/14] chore: use fixed variable for contract version (#457) --- .../snapshots/abi/ProposalValidator.json | 2 +- packages/contracts-bedrock/snapshots/semver-lock.json | 4 ++-- .../src/governance/ProposalValidator.sol | 10 ++++------ 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index dc516cfa49b44..c1f63a7b24dca 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -538,7 +538,7 @@ "type": "string" } ], - "stateMutability": "pure", + "stateMutability": "view", "type": "function" }, { diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 3a11df4c0a069..215f92c69a8a4 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x6bcced3e1050048ecc63fd4d9a86ec72e756de4cb328d29c357cc31c87834341", - "sourceCodeHash": "0x9380a09eeb5f12304baf0d45ea14e7bbd1b046d805429bfe0de970a7dfccd176" + "initCodeHash": "0x8f58c80a20e9f5e631d9451d86b2b5478a60879e58420f6788e75629f5863430", + "sourceCodeHash": "0x1d4ce0d6f868bd36418e7e760c81ecb033c15720e6a35f088ea135620fcae91a" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 30702c95efa85..19500f835f048 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -206,6 +206,10 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { CONSTANTS //////////////////////////////////////////////////////////////*/ + /// @notice Semantic version. + /// @custom:semver 1.0.0 + string public constant version = "1.0.0"; + /// @notice The divisor used for percentage calculations in optimistic voting modules. /// @dev Represents 100% in basis points (10,000 = 100%). uint256 public constant OPTIMISTIC_MODULE_PERCENT_DIVISOR = 10_000; @@ -238,12 +242,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Mapping of proposal hash to their corresponding proposal data. mapping(bytes32 => ProposalData) internal _proposals; - /// @notice Semantic version. - /// @custom:semver 1.0.0 - function version() public pure virtual returns (string memory) { - return "1.0.0"; - } - /// @notice Constructs the ProposalValidator contract. /// @param _approvedProposerAttestationSchemaUid The schema UID for attestations in EAS for submitting proposals. /// @param _topDelegatesAttestationSchemaUid The schema UID for attestations in EAS for checking if the caller From 2499ac1078068604b65e64ca2cd0dddfcc32c84d Mon Sep 17 00:00:00 2001 From: Chiin <77933451+0xChin@users.noreply.github.com> Date: Thu, 24 Jul 2025 12:57:29 -0300 Subject: [PATCH 12/14] fix: pp minors (#458) * chore: use fixed variable for contract version * refactor: rename proposalHash to proposalId for governor consistency * chore: move attestation schemas uids to initialize function * chore: specify target modules in submit functions * refactor: proper naming for modules settings * fix: pre-pr --- .../governance/IProposalValidator.sol | 30 +- .../snapshots/abi/ProposalValidator.json | 134 +++--- .../snapshots/semver-lock.json | 4 +- .../storageLayout/ProposalValidator.json | 24 +- .../src/governance/ProposalValidator.sol | 202 +++++---- .../test/governance/ProposalValidator.t.sol | 413 +++++++++--------- 6 files changed, 399 insertions(+), 408 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index ec63c98eaffd1..291ba4833b063 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -34,19 +34,19 @@ interface IProposalValidator is ISemver { error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); event ProposalSubmitted( - bytes32 indexed proposalHash, + uint256 indexed proposalId, address indexed proposer, string description, ProposalType proposalType ); event ProposalApproved( - bytes32 indexed proposalHash, + uint256 indexed proposalId, address indexed approver ); event ProposalMovedToVote( - bytes32 indexed proposalHash, + uint256 indexed proposalId, address indexed executor ); @@ -66,7 +66,7 @@ interface IProposalValidator is ISemver { ); event ProposalVotingModuleData( - bytes32 indexed proposalHash, + uint256 indexed proposalId, bytes encodedVotingModuleData ); @@ -109,7 +109,7 @@ interface IProposalValidator is ISemver { bytes32 _attestationUid, ProposalType _proposalType, uint256 _latestVotingCycle - ) external returns (bytes32 proposalHash_); + ) external returns (uint256 proposalId_); function submitCouncilMemberElectionsProposal( uint128 _criteriaValue, @@ -117,7 +117,7 @@ interface IProposalValidator is ISemver { string memory _proposalDescription, bytes32 _attestationUid, uint256 _votingCycle - ) external returns (bytes32 proposalHash_); + ) external returns (uint256 proposalId_); function submitFundingProposal( uint128 _criteriaValue, @@ -127,20 +127,20 @@ interface IProposalValidator is ISemver { string memory _description, ProposalType _proposalType, uint256 _votingCycle - ) external returns (bytes32 proposalHash_); + ) external returns (uint256 proposalId_); - function approveProposal(bytes32 _proposalHash, bytes32 _attestationUid) external; + function approveProposal(uint256 _proposalId, bytes32 _attestationUid) external; function moveToVoteProtocolOrGovernorUpgradeProposal( uint248 _againstThreshold, string memory _proposalDescription - ) external returns (bytes32 proposalHash_); + ) external returns (uint256 proposalId_); function moveToVoteCouncilMemberElectionsProposal( uint128 _criteriaValue, string[] memory _optionsDescriptions, string memory _proposalDescription - ) external returns (bytes32 proposalHash_); + ) external returns (uint256 proposalId_); function moveToVoteFundingProposal( uint128 _criteriaValue, @@ -149,7 +149,7 @@ interface IProposalValidator is ISemver { uint256[] memory _optionsAmounts, string memory _description, ProposalType _proposalType - ) external returns (bytes32 proposalHash_); + ) external returns (uint256 proposalId_); function setVotingCycleData( uint256 _cycleNumber, @@ -172,6 +172,8 @@ interface IProposalValidator is ISemver { uint256 _duration, uint256 _votingCycleDistributionLimit, uint256 _proposalDistributionThreshold, + bytes32 _approvedProposerAttestationSchemaUid, + bytes32 _topDelegatesAttestationSchemaUid, ProposalType[] memory _proposalTypes, ProposalTypeData[] memory _proposalTypesData ) external; @@ -188,9 +190,9 @@ interface IProposalValidator is ISemver { function initVersion() external view returns (uint8); - function APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID() external view returns (bytes32); + function approvedProposerAttestationSchemaUid() external view returns (bytes32); - function TOP_DELEGATES_ATTESTATION_SCHEMA_UID() external view returns (bytes32); + function topDelegatesAttestationSchemaUid() external view returns (bytes32); function OPTIMISTIC_MODULE_PERCENT_DIVISOR() external view returns (uint256); @@ -204,8 +206,6 @@ interface IProposalValidator is ISemver { ); function __constructor__( - bytes32 _approvedProposerAttestationSchemaUid, - bytes32 _topDelegatesAttestationSchemaUid, IOptimismGovernor _governor ) external; } diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index c1f63a7b24dca..b26b264755756 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -1,16 +1,6 @@ [ { "inputs": [ - { - "internalType": "bytes32", - "name": "_approvedProposerAttestationSchemaUid", - "type": "bytes32" - }, - { - "internalType": "bytes32", - "name": "_topDelegatesAttestationSchemaUid", - "type": "bytes32" - }, { "internalType": "contract IOptimismGovernor", "name": "_governor", @@ -20,19 +10,6 @@ "stateMutability": "nonpayable", "type": "constructor" }, - { - "inputs": [], - "name": "APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID", - "outputs": [ - { - "internalType": "bytes32", - "name": "", - "type": "bytes32" - } - ], - "stateMutability": "view", - "type": "function" - }, { "inputs": [], "name": "GOVERNOR", @@ -60,34 +37,34 @@ "type": "function" }, { - "inputs": [], - "name": "TOP_DELEGATES_ATTESTATION_SCHEMA_UID", - "outputs": [ + "inputs": [ + { + "internalType": "uint256", + "name": "_proposalId", + "type": "uint256" + }, { "internalType": "bytes32", - "name": "", + "name": "_attestationUid", "type": "bytes32" } ], - "stateMutability": "view", + "name": "approveProposal", + "outputs": [], + "stateMutability": "nonpayable", "type": "function" }, { - "inputs": [ - { - "internalType": "bytes32", - "name": "_proposalHash", - "type": "bytes32" - }, + "inputs": [], + "name": "approvedProposerAttestationSchemaUid", + "outputs": [ { "internalType": "bytes32", - "name": "_attestationUid", + "name": "", "type": "bytes32" } ], - "name": "approveProposal", - "outputs": [], - "stateMutability": "nonpayable", + "stateMutability": "view", "type": "function" }, { @@ -135,6 +112,16 @@ "name": "_proposalDistributionThreshold", "type": "uint256" }, + { + "internalType": "bytes32", + "name": "_approvedProposerAttestationSchemaUid", + "type": "bytes32" + }, + { + "internalType": "bytes32", + "name": "_topDelegatesAttestationSchemaUid", + "type": "bytes32" + }, { "internalType": "enum ProposalValidator.ProposalType[]", "name": "_proposalTypes", @@ -184,9 +171,9 @@ "name": "moveToVoteCouncilMemberElectionsProposal", "outputs": [ { - "internalType": "bytes32", - "name": "proposalHash_", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId_", + "type": "uint256" } ], "stateMutability": "nonpayable", @@ -228,9 +215,9 @@ "name": "moveToVoteFundingProposal", "outputs": [ { - "internalType": "bytes32", - "name": "proposalHash_", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId_", + "type": "uint256" } ], "stateMutability": "nonpayable", @@ -252,9 +239,9 @@ "name": "moveToVoteProtocolOrGovernorUpgradeProposal", "outputs": [ { - "internalType": "bytes32", - "name": "proposalHash_", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId_", + "type": "uint256" } ], "stateMutability": "nonpayable", @@ -419,9 +406,9 @@ "name": "submitCouncilMemberElectionsProposal", "outputs": [ { - "internalType": "bytes32", - "name": "proposalHash_", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId_", + "type": "uint256" } ], "stateMutability": "nonpayable", @@ -468,9 +455,9 @@ "name": "submitFundingProposal", "outputs": [ { - "internalType": "bytes32", - "name": "proposalHash_", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId_", + "type": "uint256" } ], "stateMutability": "nonpayable", @@ -505,14 +492,27 @@ } ], "name": "submitUpgradeProposal", + "outputs": [ + { + "internalType": "uint256", + "name": "proposalId_", + "type": "uint256" + } + ], + "stateMutability": "nonpayable", + "type": "function" + }, + { + "inputs": [], + "name": "topDelegatesAttestationSchemaUid", "outputs": [ { "internalType": "bytes32", - "name": "proposalHash_", + "name": "", "type": "bytes32" } ], - "stateMutability": "nonpayable", + "stateMutability": "view", "type": "function" }, { @@ -612,9 +612,9 @@ "inputs": [ { "indexed": true, - "internalType": "bytes32", - "name": "proposalHash", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId", + "type": "uint256" }, { "indexed": true, @@ -644,9 +644,9 @@ "inputs": [ { "indexed": true, - "internalType": "bytes32", - "name": "proposalHash", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId", + "type": "uint256" }, { "indexed": true, @@ -663,9 +663,9 @@ "inputs": [ { "indexed": true, - "internalType": "bytes32", - "name": "proposalHash", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId", + "type": "uint256" }, { "indexed": true, @@ -719,9 +719,9 @@ "inputs": [ { "indexed": true, - "internalType": "bytes32", - "name": "proposalHash", - "type": "bytes32" + "internalType": "uint256", + "name": "proposalId", + "type": "uint256" }, { "indexed": false, diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 215f92c69a8a4..70d743998b67e 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x8f58c80a20e9f5e631d9451d86b2b5478a60879e58420f6788e75629f5863430", - "sourceCodeHash": "0x1d4ce0d6f868bd36418e7e760c81ecb033c15720e6a35f088ea135620fcae91a" + "initCodeHash": "0x4171ca5a66e60a2a3715dc3108d76f985eb16937d71197c74ee0465461e8f9fe", + "sourceCodeHash": "0xa95d144dad3bb25ef0ac0e4ae9039b1ee5304e86e9ef7c1726d263f63598f864" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json b/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json index e8bbb446de6a1..2b847ae39937b 100644 --- a/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json @@ -36,30 +36,44 @@ }, { "bytes": "32", - "label": "proposalDistributionThreshold", + "label": "approvedProposerAttestationSchemaUid", "offset": 0, "slot": "101", + "type": "bytes32" + }, + { + "bytes": "32", + "label": "topDelegatesAttestationSchemaUid", + "offset": 0, + "slot": "102", + "type": "bytes32" + }, + { + "bytes": "32", + "label": "proposalDistributionThreshold", + "offset": 0, + "slot": "103", "type": "uint256" }, { "bytes": "32", "label": "votingCycles", "offset": 0, - "slot": "102", + "slot": "104", "type": "mapping(uint256 => struct ProposalValidator.VotingCycleData)" }, { "bytes": "32", "label": "proposalTypesData", "offset": 0, - "slot": "103", + "slot": "105", "type": "mapping(enum ProposalValidator.ProposalType => struct ProposalValidator.ProposalTypeData)" }, { "bytes": "32", "label": "_proposals", "offset": 0, - "slot": "104", - "type": "mapping(bytes32 => struct ProposalValidator.ProposalData)" + "slot": "106", + "type": "mapping(uint256 => struct ProposalValidator.ProposalData)" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 19500f835f048..61db34720bba9 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -80,7 +80,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Thrown when the trying to move a proposal to vote outside of the accepted voting cycle. error ProposalValidator_InvalidVotingCycle(); - /// @notice Thrown when the proposalId returned by the Governor is not the same as the proposalHash. + /// @notice Thrown when the proposalId returned by the Governor does not match the expected proposalId. error ProposalValidator_ProposalIdMismatch(); /// @notice Thrown when the caller is not the proposer. @@ -100,23 +100,23 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { //////////////////////////////////////////////////////////////*/ /// @notice Emitted when a new proposal is submitted. - /// @param proposalHash The hash of the submitted proposal. + /// @param proposalId The ID of the submitted proposal. /// @param proposer The address that submitted the proposal. /// @param description Description of the proposal. /// @param proposalType Type of the proposal. event ProposalSubmitted( - bytes32 indexed proposalHash, address indexed proposer, string description, ProposalType proposalType + uint256 indexed proposalId, address indexed proposer, string description, ProposalType proposalType ); /// @notice Emitted when a delegate approves a proposal. - /// @param proposalHash The hash of the approved proposal. + /// @param proposalId The ID of the approved proposal. /// @param approver The address of the delegate who approved the proposal. - event ProposalApproved(bytes32 indexed proposalHash, address indexed approver); + event ProposalApproved(uint256 indexed proposalId, address indexed approver); /// @notice Emitted when a proposal is moved to the voting phase in the governor contract. - /// @param proposalHash The hash of the proposal moved to vote. + /// @param proposalId The ID of the proposal moved to vote. /// @param executor The address that executed the move to vote. - event ProposalMovedToVote(bytes32 indexed proposalHash, address indexed executor); + event ProposalMovedToVote(uint256 indexed proposalId, address indexed executor); /// @notice Emitted when the voting cycle data is set. /// @param cycleNumber The number of the voting cycle. @@ -138,9 +138,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { event ProposalTypeDataSet(ProposalType proposalType, uint256 requiredApprovals, uint8 idInConfigurator); /// @notice Emitted with ProposalSubmitted event. - /// @param proposalHash The hash of the submitted proposal. + /// @param proposalId The ID of the submitted proposal. /// @param encodedVotingModuleData The encoded voting module data. - event ProposalVotingModuleData(bytes32 indexed proposalHash, bytes encodedVotingModuleData); + event ProposalVotingModuleData(uint256 indexed proposalId, bytes encodedVotingModuleData); /*////////////////////////////////////////////////////////////// STRUCTS @@ -166,6 +166,10 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param requiredApprovals The number of approvals each proposal type requires in order to be able to move for /// voting. /// @param idInConfigurator The proposal type ID used to get the voting module from the configurator. + /// @dev Based on the spec document, funding and council member elections proposals are + /// configured for the ApprovalVotingModule, while the upgrade proposals are configured for the + /// OptimisticVotingModule. + /// Any change on the module used for proposals would require the Validator to be upgraded. struct ProposalTypeData { uint256 requiredApprovals; uint8 idInConfigurator; @@ -218,17 +222,17 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { STATE VARIABLES //////////////////////////////////////////////////////////////*/ + /// @notice The Optimism Governor contract that will handle the voting phase. + IOptimismGovernor public immutable GOVERNOR; + /// @notice The schema UID for attestations in the Ethereum Attestation Service for checking if the caller /// is an approved proposer. /// @dev Schema format: { proposalType: uint8, date: string } - bytes32 public immutable APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID; + bytes32 public approvedProposerAttestationSchemaUid; /// @notice The schema UID for attestations in the Ethereum Attestation Service for checking if the caller /// is part of the top100 delegates. - bytes32 public immutable TOP_DELEGATES_ATTESTATION_SCHEMA_UID; - - /// @notice The Optimism Governor contract that will handle the voting phase. - IOptimismGovernor public immutable GOVERNOR; + bytes32 public topDelegatesAttestationSchemaUid; /// @notice The max amount of tokens that can be distributed in a proposal. uint256 public proposalDistributionThreshold; @@ -239,23 +243,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Mapping of proposal types to their corresponding data. mapping(ProposalType => ProposalTypeData) public proposalTypesData; - /// @notice Mapping of proposal hash to their corresponding proposal data. - mapping(bytes32 => ProposalData) internal _proposals; + /// @notice Mapping of proposal ID to their corresponding proposal data. + mapping(uint256 => ProposalData) internal _proposals; /// @notice Constructs the ProposalValidator contract. - /// @param _approvedProposerAttestationSchemaUid The schema UID for attestations in EAS for submitting proposals. - /// @param _topDelegatesAttestationSchemaUid The schema UID for attestations in EAS for checking if the caller - /// is part of the top100 delegates. /// @param _governor The Optimism Governor contract address. - constructor( - bytes32 _approvedProposerAttestationSchemaUid, - bytes32 _topDelegatesAttestationSchemaUid, - IOptimismGovernor _governor - ) - ReinitializableBase(1) - { - APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID = _approvedProposerAttestationSchemaUid; - TOP_DELEGATES_ATTESTATION_SCHEMA_UID = _topDelegatesAttestationSchemaUid; + constructor(IOptimismGovernor _governor) ReinitializableBase(1) { GOVERNOR = _governor; _disableInitializers(); } @@ -276,6 +269,8 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 _duration, uint256 _votingCycleDistributionLimit, uint256 _proposalDistributionThreshold, + bytes32 _approvedProposerAttestationSchemaUid, + bytes32 _topDelegatesAttestationSchemaUid, ProposalType[] memory _proposalTypes, ProposalTypeData[] memory _proposalTypesData ) @@ -286,6 +281,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalTypesDataLengthMismatch(); } + approvedProposerAttestationSchemaUid = _approvedProposerAttestationSchemaUid; + topDelegatesAttestationSchemaUid = _topDelegatesAttestationSchemaUid; + _setVotingCycleData(_cycleNumber, _startingTimestamp, _duration, _votingCycleDistributionLimit); _setProposalDistributionThreshold(_proposalDistributionThreshold); @@ -305,7 +303,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param _proposalType The type of proposal (ProtocolOrGovernorUpgrade or MaintenanceUpgrade). /// @param _latestVotingCycle The latest voting cycle number. Even though the upgrade proposal can be submitted /// outside of a voting cycle, we still need the latest voting cycle number to validate top delegates attestations. - /// @return proposalHash_ The hash of the submitted proposal. + /// @return proposalId_ The ID of the submitted proposal. function submitUpgradeProposal( uint248 _againstThreshold, string memory _proposalDescription, @@ -314,7 +312,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 _latestVotingCycle ) external - returns (bytes32 proposalHash_) + returns (uint256 proposalId_) { // Validate proposal type is valid for upgrade proposals if (_proposalType != ProposalType.ProtocolOrGovernorUpgrade && _proposalType != ProposalType.MaintenanceUpgrade) @@ -360,11 +358,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { votingModule = proposalTypeConfig.module; } - // Generate unique proposal hash - proposalHash_ = + // Generate unique proposal ID + proposalId_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_proposalDescription))); - ProposalData storage proposal = _proposals[proposalHash_]; + ProposalData storage proposal = _proposals[proposalId_]; // Prevent duplicate proposals if (proposal.proposer != address(0)) { @@ -372,7 +370,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // Check if proposal already exists in OptimismGovernor - if (GOVERNOR.proposalSnapshot(uint256(proposalHash_)) != 0) { + if (GOVERNOR.proposalSnapshot(proposalId_) != 0) { revert ProposalValidator_ProposalAlreadySubmitted(); } @@ -381,8 +379,8 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { proposal.proposalType = _proposalType; proposal.votingCycle = _latestVotingCycle; - emit ProposalSubmitted(proposalHash_, _msgSender(), _proposalDescription, _proposalType); - emit ProposalVotingModuleData(proposalHash_, proposalVotingModuleData); + emit ProposalSubmitted(proposalId_, _msgSender(), _proposalDescription, _proposalType); + emit ProposalVotingModuleData(proposalId_, proposalVotingModuleData); // MaintenanceUpgrade proposals move directly to voting (atomic operation) if (_proposalType == ProposalType.MaintenanceUpgrade) { @@ -392,12 +390,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { votingModule, proposalVotingModuleData, _proposalDescription, idInConfigurator ); - // Make sure the proposalId is the same as the proposalHash - if (proposalId != uint256(proposalHash_)) { + // Make sure the proposalId matches + if (proposalId != proposalId_) { revert ProposalValidator_ProposalIdMismatch(); } - emit ProposalMovedToVote(proposalHash_, _msgSender()); + emit ProposalMovedToVote(proposalId_, _msgSender()); } } @@ -408,7 +406,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param _proposalDescription Description of the proposal. /// @param _attestationUid The UID of the attestation for the approved proposer. /// @param _votingCycle The voting cycle number the proposal is targetted for. - /// @return proposalHash_ The hash of the submitted proposal. + /// @return proposalId_ The ID of the submitted proposal. function submitCouncilMemberElectionsProposal( uint128 _criteriaValue, string[] memory _optionDescriptions, @@ -417,7 +415,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 _votingCycle ) external - returns (bytes32 proposalHash_) + returns (uint256 proposalId_) { // Validate voting cycle exists and is not in the past VotingCycleData memory votingCycleData = votingCycles[_votingCycle]; @@ -444,7 +442,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { _buildApprovalModuleOptions(_optionDescriptions, new address[](0), new uint256[](0)); // Configure approval voting settings with TopChoices criteria - IApprovalVotingModule.ProposalSettings memory settings = IApprovalVotingModule.ProposalSettings({ + IApprovalVotingModule.ProposalSettings memory approvalSettings = IApprovalVotingModule.ProposalSettings({ maxApprovals: uint8(optionsLength), criteria: uint8(IApprovalVotingModule.PassingCriteria.TopChoices), budgetToken: address(0), // No budget token for elections @@ -452,7 +450,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { budgetAmount: 0 // No budget amount for elections }); - bytes memory proposalVotingModuleData = abi.encode(options, settings); + bytes memory proposalVotingModuleData = abi.encode(options, approvalSettings); // Get the module address from the configurator IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( @@ -465,19 +463,19 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_InvalidVotingModule(); } - // Generate unique proposal hash - proposalHash_ = + // Generate unique proposal ID + proposalId_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_proposalDescription))); - ProposalData storage proposal = _proposals[proposalHash_]; + ProposalData storage proposal = _proposals[proposalId_]; - // Prevent duplicate proposals with same hash + // Prevent duplicate proposals with same ID if (proposal.proposer != address(0)) { revert ProposalValidator_ProposalAlreadySubmitted(); } // Check if proposal already exists in OptimismGovernor - if (GOVERNOR.proposalSnapshot(uint256(proposalHash_)) != 0) { + if (GOVERNOR.proposalSnapshot(proposalId_) != 0) { revert ProposalValidator_ProposalAlreadySubmitted(); } @@ -486,8 +484,8 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { proposal.proposalType = ProposalType.CouncilMemberElections; proposal.votingCycle = _votingCycle; - emit ProposalSubmitted(proposalHash_, _msgSender(), _proposalDescription, ProposalType.CouncilMemberElections); - emit ProposalVotingModuleData(proposalHash_, proposalVotingModuleData); + emit ProposalSubmitted(proposalId_, _msgSender(), _proposalDescription, ProposalType.CouncilMemberElections); + emit ProposalVotingModuleData(proposalId_, proposalVotingModuleData); } /// @notice Submits a GovernanceFund or CouncilBudget proposal type that transfers OP tokens for approval and @@ -503,7 +501,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param _description Description of the proposal. /// @param _proposalType The type of proposal (must be GovernanceFund or CouncilBudget). /// @param _votingCycle The voting cycle number the proposal is targetted for. - /// @return proposalHash_ The hash of the submitted proposal. + /// @return proposalId_ The ID of the submitted proposal. function submitFundingProposal( uint128 _criteriaValue, string[] memory _optionsDescriptions, @@ -514,7 +512,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 _votingCycle ) external - returns (bytes32 proposalHash_) + returns (uint256 proposalId_) { // Only funding proposal types can use this function if (_proposalType != ProposalType.GovernanceFund && _proposalType != ProposalType.CouncilBudget) { @@ -543,7 +541,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { _buildApprovalModuleOptions(_optionsDescriptions, _optionsRecipients, _optionsAmounts); // Configure approval voting settings - IApprovalVotingModule.ProposalSettings memory settings = IApprovalVotingModule.ProposalSettings({ + IApprovalVotingModule.ProposalSettings memory approvalSettings = IApprovalVotingModule.ProposalSettings({ maxApprovals: uint8(optionsLength), criteria: uint8(IApprovalVotingModule.PassingCriteria.Threshold), budgetToken: Predeploys.GOVERNANCE_TOKEN, @@ -551,7 +549,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { budgetAmount: uint128(totalBudget) }); - bytes memory proposalVotingModuleData = abi.encode(options, settings); + bytes memory proposalVotingModuleData = abi.encode(options, approvalSettings); // Get the module address from the configurator IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( @@ -564,18 +562,18 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_InvalidVotingModule(); } - // Generate unique proposal hash - proposalHash_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); + // Generate unique proposal ID + proposalId_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); - ProposalData storage proposal = _proposals[proposalHash_]; + ProposalData storage proposal = _proposals[proposalId_]; - // Prevent duplicate proposals with same hash + // Prevent duplicate proposals with same ID if (proposal.proposer != address(0)) { revert ProposalValidator_ProposalAlreadySubmitted(); } // Check if proposal already exists in OptimismGovernor - if (GOVERNOR.proposalSnapshot(uint256(proposalHash_)) != 0) { + if (GOVERNOR.proposalSnapshot(proposalId_) != 0) { revert ProposalValidator_ProposalAlreadySubmitted(); } @@ -584,17 +582,17 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { proposal.proposalType = _proposalType; proposal.votingCycle = _votingCycle; - emit ProposalSubmitted(proposalHash_, _msgSender(), _description, _proposalType); - emit ProposalVotingModuleData(proposalHash_, proposalVotingModuleData); + emit ProposalSubmitted(proposalId_, _msgSender(), _description, _proposalType); + emit ProposalVotingModuleData(proposalId_, proposalVotingModuleData); } /// @notice Approves a proposal before being moved for voting. /// @dev This function should only be called by the top delegates. - /// @param _proposalHash The hash of the proposal to approve + /// @param _proposalId The ID of the proposal to approve /// @param _attestationUid The UID of the attestation for the delegate to approve the proposal - function approveProposal(bytes32 _proposalHash, bytes32 _attestationUid) external { + function approveProposal(uint256 _proposalId, bytes32 _attestationUid) external { address _delegate = _msgSender(); - ProposalData storage proposal = _proposals[_proposalHash]; + ProposalData storage proposal = _proposals[_proposalId]; // check if the proposal exists // proposal.votingCycle should never be 0, voting cycles already exist before the ProposalValidator is deployed // and should be set by the OP Foundation @@ -628,25 +626,25 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { proposal.delegateApprovals[_delegate] = true; proposal.approvalCount++; - emit ProposalApproved(_proposalHash, _delegate); + emit ProposalApproved(_proposalId, _delegate); } /// @notice Moves a Protocol or Governor Upgrade proposal to vote by proposing it on the Governor. /// @param _againstThreshold The threshold for the proposal to be against the total supply. /// @param _proposalDescription Description of the proposal. - /// @return proposalHash_ The hash of the submitted proposal. + /// @return proposalId_ The ID of the submitted proposal. function moveToVoteProtocolOrGovernorUpgradeProposal( uint248 _againstThreshold, string memory _proposalDescription ) external - returns (bytes32 proposalHash_) + returns (uint256 proposalId_) { // Configure optimistic proposal settings - IOptimisticModule.ProposalSettings memory settings = + IOptimisticModule.ProposalSettings memory optimisticSettings = IOptimisticModule.ProposalSettings({ againstThreshold: _againstThreshold, isRelativeToVotableSupply: true }); - bytes memory proposalVotingModuleData = abi.encode(settings); + bytes memory proposalVotingModuleData = abi.encode(optimisticSettings); // Retrieve the ID to use in the proposal type configurator uint8 idInConfigurator = proposalTypesData[ProposalType.ProtocolOrGovernorUpgrade].idInConfigurator; @@ -656,11 +654,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator).module; - // Generate unique proposal hash - proposalHash_ = + // Generate unique proposal ID + proposalId_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_proposalDescription))); - ProposalData storage proposal = _proposals[proposalHash_]; + ProposalData storage proposal = _proposals[proposalId_]; // Proposal must exist and be valid if (proposal.proposer == address(0) || proposal.proposalType != proposalType) { @@ -688,33 +686,33 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 proposalId = GOVERNOR.proposeWithModule(votingModule, proposalVotingModuleData, _proposalDescription, idInConfigurator); - // Make sure the proposalId is the same as the proposalHash - if (proposalId != uint256(proposalHash_)) { + // Make sure the proposalId matches + if (proposalId != proposalId_) { revert ProposalValidator_ProposalIdMismatch(); } - emit ProposalMovedToVote(proposalHash_, _msgSender()); + emit ProposalMovedToVote(proposalId_, _msgSender()); } /// @notice Moves a council member elections proposal to vote by proposing it on the Governor. /// @param _criteriaValue The number of top choices that can pass the voting. /// @param _optionsDescriptions The strings of the different options that can be voted. /// @param _proposalDescription Description of the proposal. - /// @return proposalHash_ The hash of the submitted proposal. + /// @return proposalId_ The ID of the submitted proposal. function moveToVoteCouncilMemberElectionsProposal( uint128 _criteriaValue, string[] memory _optionsDescriptions, string memory _proposalDescription ) external - returns (bytes32 proposalHash_) + returns (uint256 proposalId_) { // Configure approval module options (IApprovalVotingModule.ProposalOption[] memory options,) = _buildApprovalModuleOptions(_optionsDescriptions, new address[](0), new uint256[](0)); // Configure approval module settings - IApprovalVotingModule.ProposalSettings memory settings = IApprovalVotingModule.ProposalSettings({ + IApprovalVotingModule.ProposalSettings memory approvalSettings = IApprovalVotingModule.ProposalSettings({ maxApprovals: uint8(_optionsDescriptions.length), criteria: uint8(IApprovalVotingModule.PassingCriteria.TopChoices), budgetToken: address(0), @@ -722,7 +720,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { budgetAmount: 0 }); - bytes memory proposalVotingModuleData = abi.encode(options, settings); + bytes memory proposalVotingModuleData = abi.encode(options, approvalSettings); ProposalType _proposalType = ProposalType.CouncilMemberElections; @@ -733,11 +731,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator).module; - // Generate unique proposal hash - proposalHash_ = + // Generate unique proposal ID + proposalId_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_proposalDescription))); - ProposalData storage proposal = _proposals[proposalHash_]; + ProposalData storage proposal = _proposals[proposalId_]; // Proposal must exist and be valid if (proposal.proposer == address(0) || proposal.proposalType != _proposalType) { @@ -774,12 +772,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 proposalId = GOVERNOR.proposeWithModule(votingModule, proposalVotingModuleData, _proposalDescription, idInConfigurator); - // Make sure the proposalId is the same as the proposalHash - if (proposalId != uint256(proposalHash_)) { + // Make sure the proposalId matches + if (proposalId != proposalId_) { revert ProposalValidator_ProposalIdMismatch(); } - emit ProposalMovedToVote(proposalHash_, _msgSender()); + emit ProposalMovedToVote(proposalId_, _msgSender()); } /// @notice Moves a funding proposal to vote by proposing it on the Governor. @@ -793,7 +791,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @param _optionsAmounts The amount to transfer for each option in case the option passes the voting. /// @param _description Description of the proposal. /// @param _proposalType The type of proposal (must be GovernanceFund or CouncilBudget). - /// @return proposalHash_ The hash of the submitted proposal. + /// @return proposalId_ The ID of the submitted proposal. function moveToVoteFundingProposal( uint128 _criteriaValue, string[] memory _optionsDescriptions, @@ -803,7 +801,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { ProposalType _proposalType ) external - returns (bytes32 proposalHash_) + returns (uint256 proposalId_) { // Only funding proposal types can use this function if (_proposalType != ProposalType.GovernanceFund && _proposalType != ProposalType.CouncilBudget) { @@ -815,7 +813,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { _buildApprovalModuleOptions(_optionsDescriptions, _optionsRecipients, _optionsAmounts); // Configure approval module settings - IApprovalVotingModule.ProposalSettings memory settings = IApprovalVotingModule.ProposalSettings({ + IApprovalVotingModule.ProposalSettings memory approvalSettings = IApprovalVotingModule.ProposalSettings({ maxApprovals: uint8(_optionsDescriptions.length), criteria: uint8(IApprovalVotingModule.PassingCriteria.Threshold), budgetToken: Predeploys.GOVERNANCE_TOKEN, @@ -823,7 +821,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { budgetAmount: uint128(totalBudget) }); - bytes memory proposalVotingModuleData = abi.encode(options, settings); + bytes memory proposalVotingModuleData = abi.encode(options, approvalSettings); // Retrieve the ID to use in the proposal type configurator uint8 idInConfigurator = proposalTypesData[_proposalType].idInConfigurator; @@ -832,10 +830,10 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { address votingModule = IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator).module; - // Generate unique proposal hash - proposalHash_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); + // Generate unique proposal ID + proposalId_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_description))); - ProposalData storage proposal = _proposals[proposalHash_]; + ProposalData storage proposal = _proposals[proposalId_]; // Proposal must exist if (proposal.proposer == address(0) || proposal.proposalType != _proposalType) { @@ -874,12 +872,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 proposalId = GOVERNOR.proposeWithModule(votingModule, proposalVotingModuleData, _description, idInConfigurator); - // Make sure the proposalId is the same as the proposalHash - if (proposalId != uint256(proposalHash_)) { + // Make sure the proposalId matches + if (proposalId != proposalId_) { revert ProposalValidator_ProposalIdMismatch(); } - emit ProposalMovedToVote(proposalHash_, _msgSender()); + emit ProposalMovedToVote(proposalId_, _msgSender()); } /// @notice Sets the data of a voting cycle. @@ -953,7 +951,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { (uint8 proposalType,) = abi.decode(attestation.data, (uint8, string)); if ( - attestation.attester != owner() || attestation.schema != APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID + attestation.attester != owner() || attestation.schema != approvedProposerAttestationSchemaUid || attestation.recipient != _msgSender() || proposalType != uint8(_expectedProposalType) ) { revert ProposalValidator_InvalidAttestation(); @@ -961,7 +959,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } /// @notice Validates the attestation data for a delegate that tries to approve a proposal. - /// @dev Only acceptes attestations that does NOT include partial delegation. + /// @dev Only accepts attestations that do NOT include partial delegation. /// @param _attestationUid The UID of the attestation to validate. /// @param _lastVotingCycle The last voting cycle to validate against. function _validateTopDelegateAttestation(bytes32 _attestationUid, uint256 _lastVotingCycle) internal view { @@ -977,7 +975,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // check if the schema is correct - if (attestation.schema != TOP_DELEGATES_ATTESTATION_SCHEMA_UID) { + if (attestation.schema != topDelegatesAttestationSchemaUid) { revert ProposalValidator_InvalidAttestationSchema(); } @@ -1057,11 +1055,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } } - /// @notice Calculate `proposalId` hashing similarly to `hashProposal` but based on `module` and `proposalData`. + /// @notice Calculate `proposalId` based on `module`, `proposalData` and `descriptionHash`. /// @param _module The address of the voting module to use for this proposal. /// @param _proposalData The proposal data to pass to the voting module. /// @param _descriptionHash The hash of the proposal description. - /// @return The hash of the proposal. + /// @return The proposal ID as uint256. function _hashProposalWithModule( address _module, bytes memory _proposalData, @@ -1069,9 +1067,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { ) internal view - returns (bytes32) + returns (uint256) { - return keccak256(abi.encode(address(GOVERNOR), _module, _proposalData, _descriptionHash)); + return uint256(keccak256(abi.encode(address(GOVERNOR), _module, _proposalData, _descriptionHash))); } /// @notice Private function to set the voting cycle data and emit event. diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index 1407a1dc6017a..a307afbb91869 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -33,15 +33,9 @@ import { stdStorage, StdStorage } from "forge-std/Test.sol"; import { CommonTest } from "test/setup/CommonTest.sol"; /// @title ProposalValidatorForTest -/// @notice A test contract that exposes the private _hashProposal function +/// @notice A test contract that exposes the private _hashProposalWithModule function contract ProposalValidatorForTest is ProposalValidator { - constructor( - bytes32 _approvedProposerAttestationSchemaUid, - bytes32 _topDelegatesAttestationSchemaUid, - IOptimismGovernor _governor - ) - ProposalValidator(_approvedProposerAttestationSchemaUid, _topDelegatesAttestationSchemaUid, _governor) - { } + constructor(IOptimismGovernor _governor) ProposalValidator(_governor) { } function hashProposalWithModule( address _module, @@ -50,13 +44,13 @@ contract ProposalValidatorForTest is ProposalValidator { ) public view - returns (bytes32) + returns (uint256) { return _hashProposalWithModule(_module, _proposalData, _descriptionHash); } /// @notice Exposes proposal data for testing - function getProposalData(bytes32 _proposalHash) + function getProposalData(uint256 _proposalId) public view returns ( @@ -67,14 +61,14 @@ contract ProposalValidatorForTest is ProposalValidator { uint256 votingCycle_ ) { - ProposalData storage proposal = _proposals[_proposalHash]; + ProposalData storage proposal = _proposals[_proposalId]; return ( proposal.proposer, proposal.proposalType, proposal.movedToVote, proposal.approvalCount, proposal.votingCycle ); } function setProposalData( - bytes32 _proposalHash, + uint256 _proposalId, address _proposer, ProposalType _proposalType, bool _movedToVote, @@ -83,20 +77,20 @@ contract ProposalValidatorForTest is ProposalValidator { ) public { - _proposals[_proposalHash].proposer = _proposer; - _proposals[_proposalHash].proposalType = _proposalType; - _proposals[_proposalHash].movedToVote = _movedToVote; - _proposals[_proposalHash].approvalCount = _approvalCount; - _proposals[_proposalHash].votingCycle = _votingCycle; + _proposals[_proposalId].proposer = _proposer; + _proposals[_proposalId].proposalType = _proposalType; + _proposals[_proposalId].movedToVote = _movedToVote; + _proposals[_proposalId].approvalCount = _approvalCount; + _proposals[_proposalId].votingCycle = _votingCycle; } - function mockApproveProposal(bytes32 _proposalHash, address _delegate) public { - _proposals[_proposalHash].delegateApprovals[_delegate] = true; + function mockApproveProposal(uint256 _proposalId, address _delegate) public { + _proposals[_proposalId].delegateApprovals[_delegate] = true; } /// @notice Check if a delegate has approved a proposal - function hasDelegateApproved(bytes32 _proposalHash, address _delegate) public view returns (bool hasApproved_) { - return _proposals[_proposalHash].delegateApprovals[_delegate]; + function hasDelegateApproved(uint256 _proposalId, address _delegate) public view returns (bool hasApproved_) { + return _proposals[_proposalId].delegateApprovals[_delegate]; } } @@ -116,6 +110,8 @@ contract ProposalValidator_Init is CommonTest { uint8 public constant APPROVAL_VOTING_MODULE_ID = 1; uint8 public constant OPTIMISTIC_VOTING_MODULE_ID = 2; uint64 public constant ATT_EXPIRATION_TIME = 10 days; + bytes32 public APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID; + bytes32 public TOP_DELEGATES_ATTESTATION_SCHEMA_UID; address owner; address user; @@ -129,17 +125,15 @@ contract ProposalValidator_Init is CommonTest { ProposalValidatorForTest public impl; IOptimismGovernor public governor; IProposalTypesConfigurator public proposalTypesConfigurator; - bytes32 public APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID; - bytes32 public TOP_DELEGATES_ATTESTATION_SCHEMA_UID; event ProposalSubmitted( - bytes32 indexed proposalHash, + uint256 indexed proposalId, address indexed proposer, string description, ProposalValidator.ProposalType proposalType ); - event ProposalApproved(bytes32 indexed proposalHash, address indexed approver); - event ProposalMovedToVote(bytes32 indexed proposalHash, address indexed executor); + event ProposalApproved(uint256 indexed proposalId, address indexed approver); + event ProposalMovedToVote(uint256 indexed proposalId, address indexed executor); event MinimumVotingPowerSet(uint256 newMinimumVotingPower); event VotingCycleDataSet( uint256 cycleNumber, uint256 startingTimestamp, uint256 duration, uint256 votingCycleDistributionLimit @@ -148,7 +142,7 @@ contract ProposalValidator_Init is CommonTest { event ProposalTypeDataSet( ProposalValidator.ProposalType proposalType, uint256 requiredApprovals, uint8 idInConfigurator ); - event ProposalVotingModuleData(bytes32 indexed proposalHash, bytes encodedVotingModuleData); + event ProposalVotingModuleData(uint256 indexed proposalId, bytes encodedVotingModuleData); /// @notice Helper function to setup a mock and expect a call to it. function _mockAndExpect(address _receiver, bytes memory _calldata, bytes memory _returned) internal { @@ -321,7 +315,7 @@ contract ProposalValidator_Init is CommonTest { } // Construct ProposalSettings - IApprovalVotingModule.ProposalSettings memory settings = IApprovalVotingModule.ProposalSettings({ + IApprovalVotingModule.ProposalSettings memory approvalSettings = IApprovalVotingModule.ProposalSettings({ maxApprovals: uint8(descriptions.length), criteria: uint8(IApprovalVotingModule.PassingCriteria.Threshold), budgetToken: Predeploys.GOVERNANCE_TOKEN, @@ -329,7 +323,7 @@ contract ProposalValidator_Init is CommonTest { budgetAmount: uint128(totalBudget) }); - return abi.encode(options, settings); + return abi.encode(options, approvalSettings); } /// @notice Helper function to construct voting module data for council elections @@ -360,7 +354,7 @@ contract ProposalValidator_Init is CommonTest { } // Construct ProposalSettings with TopChoices criteria - IApprovalVotingModule.ProposalSettings memory settings = IApprovalVotingModule.ProposalSettings({ + IApprovalVotingModule.ProposalSettings memory approvalSettings = IApprovalVotingModule.ProposalSettings({ maxApprovals: uint8(descriptions.length), criteria: uint8(IApprovalVotingModule.PassingCriteria.TopChoices), budgetToken: address(0), @@ -368,15 +362,15 @@ contract ProposalValidator_Init is CommonTest { budgetAmount: 0 }); - return abi.encode(options, settings); + return abi.encode(options, approvalSettings); } /// @notice Helper function to construct voting module data for upgrade proposals function _constructOptimisticVotingModuleData(uint248 againstThreshold) internal pure returns (bytes memory) { - IOptimisticModule.ProposalSettings memory settings = + IOptimisticModule.ProposalSettings memory optimisticSettings = IOptimisticModule.ProposalSettings({ againstThreshold: againstThreshold, isRelativeToVotableSupply: true }); - return abi.encode(settings); + return abi.encode(optimisticSettings); } /// @notice Helper function to create a proposal for move to vote @@ -386,17 +380,17 @@ contract ProposalValidator_Init is CommonTest { string memory proposalDescription ) internal - returns (bytes32 proposalHash_, bytes memory votingModuleData_) + returns (uint256 proposalId_, bytes memory votingModuleData_) { - // Calculate expected proposal hash + // Calculate expected proposal ID votingModuleData_ = _constructOptimisticVotingModuleData(againstThreshold); - proposalHash_ = validator.hashProposalWithModule( + proposalId_ = validator.hashProposalWithModule( optimisticVotingModule, votingModuleData_, keccak256(bytes(proposalDescription)) ); // 1 vote as default for being able to move to vote validator.setProposalData( - proposalHash_, + proposalId_, proposer, ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade, false, @@ -413,15 +407,15 @@ contract ProposalValidator_Init is CommonTest { string memory proposalDescription ) internal - returns (bytes32 proposalHash_, bytes memory votingModuleData_) + returns (uint256 proposalId_, bytes memory votingModuleData_) { votingModuleData_ = _constructCouncilElectionVotingModuleData(optionsDescriptions, criteriaValue); - proposalHash_ = validator.hashProposalWithModule( + proposalId_ = validator.hashProposalWithModule( approvalVotingModule, votingModuleData_, keccak256(bytes(proposalDescription)) ); validator.setProposalData( - proposalHash_, + proposalId_, proposer, ProposalValidator.ProposalType.CouncilMemberElections, false, @@ -441,17 +435,15 @@ contract ProposalValidator_Init is CommonTest { ProposalValidator.ProposalType proposalType ) internal - returns (bytes32 proposalHash_, bytes memory votingModuleData_) + returns (uint256 proposalId_, bytes memory votingModuleData_) { votingModuleData_ = _constructFundingVotingModuleData(optionsDescriptions, optionsRecipients, optionsAmounts, criteriaValue); - proposalHash_ = validator.hashProposalWithModule( + proposalId_ = validator.hashProposalWithModule( approvalVotingModule, votingModuleData_, keccak256(bytes(proposalDescription)) ); - validator.setProposalData( - proposalHash_, proposer, proposalType, false, PROPOSAL_REQUIRED_APPROVALS, CYCLE_NUMBER - ); + validator.setProposalData(proposalId_, proposer, proposalType, false, PROPOSAL_REQUIRED_APPROVALS, CYCLE_NUMBER); } /// @notice Helper function to setup proposal types configurator mocks @@ -524,9 +516,7 @@ contract ProposalValidator_Init is CommonTest { // Create mock addresses proposalTypesConfigurator = IProposalTypesConfigurator(makeAddr("proposalTypesConfigurator")); - impl = new ProposalValidatorForTest( - APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, TOP_DELEGATES_ATTESTATION_SCHEMA_UID, governor - ); + impl = new ProposalValidatorForTest(governor); validator = ProposalValidatorForTest(address(new Proxy(owner))); vm.prank(owner); @@ -541,6 +531,8 @@ contract ProposalValidator_Init is CommonTest { DURATION, DISTRIBUTION_LIMIT, DISTRIBUTION_THRESHOLD, + APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, + TOP_DELEGATES_ATTESTATION_SCHEMA_UID, proposalTypes, proposalTypesData ) @@ -634,9 +626,7 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { // Create mock addresses proposalTypesConfigurator = IProposalTypesConfigurator(makeAddr("proposalTypesConfigurator")); - impl = new ProposalValidatorForTest( - APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, TOP_DELEGATES_ATTESTATION_SCHEMA_UID, governor - ); + impl = new ProposalValidatorForTest(governor); validator = ProposalValidatorForTest(address(new Proxy(owner))); } @@ -658,6 +648,8 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { DURATION, DISTRIBUTION_LIMIT, DISTRIBUTION_THRESHOLD, + APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, + TOP_DELEGATES_ATTESTATION_SCHEMA_UID, proposalTypes, proposalTypesData ) @@ -725,6 +717,8 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { DURATION, DISTRIBUTION_LIMIT, DISTRIBUTION_THRESHOLD, + APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, + TOP_DELEGATES_ATTESTATION_SCHEMA_UID, proposalTypes, proposalTypesData ) @@ -764,17 +758,15 @@ contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init // Create attestation for the proposal bytes32 attestationUid = _createApprovedProposerAttestation(proposer, proposalType); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructOptimisticVotingModuleData(againstThreshold); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( optimisticVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); // Mock proposalSnapshot to return 0 (proposal doesn't exist in governor) _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); @@ -786,25 +778,25 @@ contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init IOptimismGovernor.proposeWithModule, (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), - abi.encode(uint256(expectedHash)) + abi.encode(expectedId) ); // For MaintenanceUpgrade, events are: ProposalSubmitted, ProposalVotingModuleData, ProposalMovedToVote vm.expectEmit(address(validator)); - emit ProposalSubmitted(expectedHash, proposer, proposalDescription, proposalType); + emit ProposalSubmitted(expectedId, proposer, proposalDescription, proposalType); vm.expectEmit(address(validator)); - emit ProposalVotingModuleData(expectedHash, votingModuleData); + emit ProposalVotingModuleData(expectedId, votingModuleData); vm.expectEmit(address(validator)); - emit ProposalMovedToVote(expectedHash, proposer); + emit ProposalMovedToVote(expectedId, proposer); vm.prank(proposer); - bytes32 proposalHash = validator.submitUpgradeProposal( + uint256 proposalId = validator.submitUpgradeProposal( againstThreshold, proposalDescription, attestationUid, proposalType, CYCLE_NUMBER ); - assertEq(proposalHash, expectedHash); + assertEq(proposalId, expectedId); // Verify proposal data was stored correctly ( @@ -813,7 +805,7 @@ contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init bool movedToVote, uint256 approvalCount, uint256 votingCycle - ) = validator.getProposalData(proposalHash); + ) = validator.getProposalData(proposalId); assertEq(storedProposer, proposer, "Proposer should match input"); assertEq(uint8(storedProposalType), uint8(proposalType), "Proposal type should match input"); @@ -839,34 +831,32 @@ contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init // Create attestation for the proposal bytes32 attestationUid = _createApprovedProposerAttestation(proposer, proposalType); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructOptimisticVotingModuleData(againstThreshold); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( optimisticVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); // Mock proposalSnapshot to return 0 (proposal doesn't exist in governor) _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); // For ProtocolOrGovernorUpgrade, only ProposalSubmitted and ProposalVotingModuleData events vm.expectEmit(address(validator)); - emit ProposalSubmitted(expectedHash, proposer, proposalDescription, proposalType); + emit ProposalSubmitted(expectedId, proposer, proposalDescription, proposalType); vm.expectEmit(address(validator)); - emit ProposalVotingModuleData(expectedHash, votingModuleData); + emit ProposalVotingModuleData(expectedId, votingModuleData); vm.prank(proposer); - bytes32 proposalHash = validator.submitUpgradeProposal( + uint256 proposalId = validator.submitUpgradeProposal( againstThreshold, proposalDescription, attestationUid, proposalType, CYCLE_NUMBER ); - assertEq(proposalHash, expectedHash); + assertEq(proposalId, expectedId); // Verify proposal data was stored correctly ( @@ -875,7 +865,7 @@ contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init bool movedToVote, uint256 approvalCount, uint256 votingCycle - ) = validator.getProposalData(proposalHash); + ) = validator.getProposalData(proposalId); assertEq(storedProposer, proposer, "Proposer should match input"); assertEq(uint8(storedProposalType), uint8(proposalType), "Proposal type should match input"); @@ -1019,17 +1009,15 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructOptimisticVotingModuleData(againstThreshold); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( optimisticVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); // Mock proposalSnapshot to return 0 for first submission _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); @@ -1042,7 +1030,7 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I IOptimismGovernor.proposeWithModule, (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), - abi.encode(uint256(expectedHash)) + abi.encode(expectedId) ); } @@ -1070,16 +1058,16 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructOptimisticVotingModuleData(againstThreshold); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( optimisticVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); // Mock proposalSnapshot to return non-zero (proposal already exists in governor) _mockAndExpect( address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), + abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(1000) // Non-zero indicates proposal exists ); @@ -1149,20 +1137,18 @@ contract ProposalValidator_SubmitUpgradeProposal_TestFail is ProposalValidator_I ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.MaintenanceUpgrade; bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, proposalType); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructOptimisticVotingModuleData(againstThreshold); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( optimisticVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); - vm.assume(proposalId != uint256(expectedHash)); // Ensure proposalId is different from expectedHash + vm.assume(proposalId != expectedId); // Ensure proposalId is different from expectedId _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); // Mock the proposeWithModule call to return a different proposalId @@ -1210,37 +1196,35 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_Test is Proposal bytes32 attestationUid = _createApprovedProposerAttestation(topDelegate_A, ProposalValidator.ProposalType.CouncilMemberElections); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructCouncilElectionVotingModuleData(optionDescriptions, criteriaValue); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( approvalVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); // Mock proposalSnapshot to return 0 (proposal doesn't exist in governor) _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); // Expect ProposalSubmitted event vm.expectEmit(address(validator)); emit ProposalSubmitted( - expectedHash, topDelegate_A, proposalDescription, ProposalValidator.ProposalType.CouncilMemberElections + expectedId, topDelegate_A, proposalDescription, ProposalValidator.ProposalType.CouncilMemberElections ); // Expect ProposalVotingModuleData event vm.expectEmit(address(validator)); - emit ProposalVotingModuleData(expectedHash, votingModuleData); + emit ProposalVotingModuleData(expectedId, votingModuleData); _mockProposalTypesConfiguratorCall(APPROVAL_VOTING_MODULE_ID); vm.prank(topDelegate_A); - bytes32 proposalHash = validator.submitCouncilMemberElectionsProposal( + uint256 proposalId = validator.submitCouncilMemberElectionsProposal( criteriaValue, optionDescriptions, proposalDescription, attestationUid, CYCLE_NUMBER ); - assertEq(proposalHash, expectedHash); + assertEq(proposalId, expectedId); // Verify proposal data was stored correctly ( @@ -1249,7 +1233,7 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_Test is Proposal bool movedToVote, uint256 approvalCount, uint256 votingCycle - ) = validator.getProposalData(proposalHash); + ) = validator.getProposalData(proposalId); assertEq(proposer, topDelegate_A, "Proposer should be topDelegate_A"); assertEq( @@ -1340,17 +1324,15 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_TestFail is Prop } function test_submitCouncilMemberElectionsProposal_duplicateProposal_reverts() public { - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructCouncilElectionVotingModuleData(optionDescriptions, criteriaValue); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( approvalVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); // Mock proposalSnapshot to return 0 for first submission _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); _mockProposalTypesConfiguratorCall(APPROVAL_VOTING_MODULE_ID); @@ -1373,16 +1355,16 @@ contract ProposalValidator_SubmitCouncilMemberElectionsProposal_TestFail is Prop } function test_submitCouncilMemberElectionsProposal_proposalExistsInGovernor_reverts() public { - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructCouncilElectionVotingModuleData(optionDescriptions, criteriaValue); - bytes32 expectedHash = validator.hashProposalWithModule( + uint256 expectedId = validator.hashProposalWithModule( approvalVotingModule, votingModuleData, keccak256(bytes(proposalDescription)) ); // Mock proposalSnapshot to return non-zero (proposal already exists in governor) _mockAndExpect( address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), + abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(1000) // Non-zero indicates proposal exists ); @@ -1517,35 +1499,33 @@ contract ProposalValidator_SubmitFundingProposal_Test is ProposalValidator_Init amounts[i] = amount; } - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructFundingVotingModuleData(descriptions, recipients, amounts, criteriaValue); - bytes32 expectedHash = + uint256 expectedId = validator.hashProposalWithModule(approvalVotingModule, votingModuleData, keccak256(bytes(description))); // Mock proposalSnapshot to return 0 (proposal doesn't exist in governor) _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); // Expect ProposalSubmitted event vm.expectEmit(address(validator)); - emit ProposalSubmitted(expectedHash, proposer, description, proposalType); + emit ProposalSubmitted(expectedId, proposer, description, proposalType); // Expect ProposalVotingModuleData event vm.expectEmit(address(validator)); - emit ProposalVotingModuleData(expectedHash, votingModuleData); + emit ProposalVotingModuleData(expectedId, votingModuleData); _mockProposalTypesConfiguratorCall(APPROVAL_VOTING_MODULE_ID); vm.prank(proposer); - bytes32 proposalHash = validator.submitFundingProposal( + uint256 proposalId = validator.submitFundingProposal( criteriaValue, descriptions, recipients, amounts, description, proposalType, CYCLE_NUMBER ); - assertEq(proposalHash, expectedHash); + assertEq(proposalId, expectedId); // Verify proposal data was stored correctly ( @@ -1554,7 +1534,7 @@ contract ProposalValidator_SubmitFundingProposal_Test is ProposalValidator_Init bool movedToVote, uint256 approvalCount, uint256 votingCycle - ) = validator.getProposalData(proposalHash); + ) = validator.getProposalData(proposalId); assertEq(storedProposer, proposer, "Proposer should match input"); assertEq(uint8(storedProposalType), uint8(proposalType), "Proposal type should match input"); @@ -1747,17 +1727,15 @@ contract ProposalValidator_SubmitFundingProposal_TestFail is ProposalValidator_I (string[] memory descriptions, address[] memory recipients, uint256[] memory amounts) = _createMinimalFundingArrays(1); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructFundingVotingModuleData(descriptions, recipients, amounts, FUNDING_CRITERIA_VALUE); - bytes32 expectedHash = + uint256 expectedId = validator.hashProposalWithModule(approvalVotingModule, votingModuleData, keccak256(bytes(description))); // Mock proposalSnapshot to return 0 for first submission _mockAndExpect( - address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), - abi.encode(0) + address(governor), abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(0) ); _mockProposalTypesConfiguratorCall(APPROVAL_VOTING_MODULE_ID); @@ -1787,16 +1765,16 @@ contract ProposalValidator_SubmitFundingProposal_TestFail is ProposalValidator_I (string[] memory descriptions, address[] memory recipients, uint256[] memory amounts) = _createMinimalFundingArrays(1); - // Calculate expected proposal hash + // Calculate expected proposal ID bytes memory votingModuleData = _constructFundingVotingModuleData(descriptions, recipients, amounts, FUNDING_CRITERIA_VALUE); - bytes32 expectedHash = + uint256 expectedId = validator.hashProposalWithModule(approvalVotingModule, votingModuleData, keccak256(bytes(description))); // Mock proposalSnapshot to return non-zero (proposal already exists in governor) _mockAndExpect( address(governor), - abi.encodeCall(IOptimismGovernor.proposalSnapshot, (uint256(expectedHash))), + abi.encodeCall(IOptimismGovernor.proposalSnapshot, (expectedId)), abi.encode(1000) // Non-zero indicates proposal exists ); @@ -1887,29 +1865,29 @@ contract ProposalValidator_ApproveProposal_Test is ProposalValidator_Init { validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); } - function test_approveProposal_succeeds(bytes32 _proposalHash, uint8 proposalTypeValue) public { - // Ensure the proposal hash is not 0 - vm.assume(_proposalHash != bytes32(0)); + function test_approveProposal_succeeds(uint256 _proposalId, uint8 proposalTypeValue) public { + // Ensure the proposal ID is not 0 + vm.assume(_proposalId != 0); // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // Set mock proposal data of a random proposal in the validator contract - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); // Expect event to be emitted when approving vm.expectEmit(address(validator)); - emit ProposalApproved(_proposalHash, topDelegate_A); + emit ProposalApproved(_proposalId, topDelegate_A); // Approve the proposal, use the attestation of the top delegate that was created in setUp vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, topDelegateAttestation_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); // Check that the proposal data has been updated - assertTrue(validator.hasDelegateApproved(_proposalHash, topDelegate_A)); + assertTrue(validator.hasDelegateApproved(_proposalId, topDelegate_A)); - (,,, uint256 approvalCount,) = validator.getProposalData(_proposalHash); + (,,, uint256 approvalCount,) = validator.getProposalData(_proposalId); assertEq(approvalCount, 1); } } @@ -1921,15 +1899,15 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { super.setUp(); } - function test_approveProposal_proposalDoesNotExist_reverts(bytes32 _proposalHash) public { + function test_approveProposal_proposalDoesNotExist_reverts(uint256 _proposalId) public { // There is no stored proposal data so this will revert vm.expectRevert(IProposalValidator.ProposalValidator_ProposalDoesNotExist.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, topDelegateAttestation_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); } function test_approveProposal_proposalAlreadyApproved_reverts( - bytes32 _proposalHash, + uint256 _proposalId, uint8 proposalTypeValue ) public @@ -1938,17 +1916,17 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // set proposal data so that the proposal exists - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); // Mock the proposal as already approved by the top delegate - validator.mockApproveProposal(_proposalHash, topDelegate_A); + validator.mockApproveProposal(_proposalId, topDelegate_A); vm.expectRevert(IProposalValidator.ProposalValidator_ProposalAlreadyApproved.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, topDelegateAttestation_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); } function test_approveProposal_proposalAlreadyMovedToVote_reverts( - bytes32 _proposalHash, + uint256 _proposalId, uint8 proposalTypeValue ) public @@ -1957,33 +1935,36 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // set proposal data so that the proposal exists and set movedToVote to true - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, true, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, true, 0, CYCLE_NUMBER); vm.expectRevert(IProposalValidator.ProposalValidator_ProposalAlreadyMovedToVote.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, topDelegateAttestation_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); } function test_approveProposal_invalidVotingCycle_reverts( - bytes32 _proposalHash, + uint256 _proposalId, uint8 proposalTypeValue, uint256 votingCycle ) public { vm.assume(votingCycle != CYCLE_NUMBER && votingCycle != 0); + vm.assume(votingCycle != CYCLE_NUMBER + 1); // Avoid existing cycle + // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + // set proposal data so that the proposal exists - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, votingCycle); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, votingCycle); vm.expectRevert(IProposalValidator.ProposalValidator_InvalidVotingCycle.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, topDelegateAttestation_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); } - function test_approveProposal_invalidSchema_reverts(bytes32 _proposalHash, uint8 proposalTypeValue) public { + function test_approveProposal_invalidSchema_reverts(uint256 _proposalId, uint8 proposalTypeValue) public { // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); @@ -2011,22 +1992,22 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { ); // set proposal data so that the proposal exists - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); // set the voting cycle data of the previous cycle vm.prank(owner); validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestationSchema.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, _invalidAttestationUid); + validator.approveProposal(_proposalId, _invalidAttestationUid); } - function test_approveProposal_attestationRevoked_reverts(bytes32 _proposalHash, uint8 proposalTypeValue) public { + function test_approveProposal_attestationRevoked_reverts(uint256 _proposalId, uint8 proposalTypeValue) public { // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // set proposal data so that the proposal exists - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); // set the voting cycle data of the previous cycle vm.prank(owner); validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); @@ -2042,11 +2023,11 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { vm.expectRevert(IProposalValidator.ProposalValidator_AttestationRevoked.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, topDelegateAttestation_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); } function test_approveProposal_invalidAttestationCaller_reverts( - bytes32 _proposalHash, + uint256 _proposalId, uint8 proposalTypeValue, address _caller ) @@ -2060,7 +2041,7 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { vm.assume(_caller != topDelegate_A); // Set mock proposal data of a random proposal in the validator contract - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); // set the voting cycle data of the previous cycle vm.prank(owner); validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); @@ -2068,11 +2049,11 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Expect the invalid attestation error to be reverted vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); vm.prank(_caller); - validator.approveProposal(_proposalHash, topDelegateAttestation_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); } function test_approveProposal_invalidAttestationPartialDelegation_reverts( - bytes32 _proposalHash, + uint256 _proposalId, uint8 proposalTypeValue ) public @@ -2082,7 +2063,7 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // Set mock proposal data of a random proposal in the validator contract - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); // set the voting cycle data of the previous cycle vm.prank(owner); validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); @@ -2106,11 +2087,11 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Expect the invalid attestation error to be reverted vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, _attestationUidWithPartialDelegation); + validator.approveProposal(_proposalId, _attestationUidWithPartialDelegation); } function test_approveProposal_nonExistentAttestation_reverts( - bytes32 _proposalHash, + uint256 _proposalId, uint8 proposalTypeValue, bytes32 _nonExistentAttestationUid ) @@ -2124,7 +2105,7 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { vm.assume(_nonExistentAttestationUid != topDelegateAttestation_A); // Set mock proposal data of a random proposal in the validator contract - validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); // set the voting cycle data of the previous cycle vm.prank(owner); validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); @@ -2132,7 +2113,7 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Expect the invalid attestation error to be reverted when attestation doesn't exist vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); vm.prank(topDelegate_A); - validator.approveProposal(_proposalHash, _nonExistentAttestationUid); + validator.approveProposal(_proposalId, _nonExistentAttestationUid); } } @@ -2143,12 +2124,12 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_Test is P string proposalDescription = "Test proposal"; ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; bytes votingModuleData; - bytes32 expectedHash; + uint256 expectedId; function setUp() public override { super.setUp(); - (expectedHash, votingModuleData) = + (expectedId, votingModuleData) = _createUpgradeProposalForMoveToVote(approvedProposer, againstThreshold, proposalDescription); } @@ -2163,19 +2144,19 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_Test is P IOptimismGovernor.proposeWithModule, (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), - abi.encode(uint256(expectedHash)) + abi.encode(expectedId) ); // Expect the ProposalMovedToVote event to be emitted vm.expectEmit(address(validator)); - emit ProposalMovedToVote(expectedHash, approvedProposer); + emit ProposalMovedToVote(expectedId, approvedProposer); // Move to vote vm.prank(approvedProposer); validator.moveToVoteProtocolOrGovernorUpgradeProposal(againstThreshold, proposalDescription); // Check that the proposal is in voting - (,, bool movedToVote,,) = validator.getProposalData(expectedHash); + (,, bool movedToVote,,) = validator.getProposalData(expectedId); assertTrue(movedToVote, "Proposal should be in voting"); } } @@ -2185,12 +2166,12 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_TestFail string proposalDescription = "Test proposal"; ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade; bytes votingModuleData; - bytes32 expectedHash; + uint256 expectedId; function setUp() public override { super.setUp(); - (expectedHash, votingModuleData) = + (expectedId, votingModuleData) = _createUpgradeProposalForMoveToVote(approvedProposer, againstThreshold, proposalDescription); } @@ -2208,7 +2189,7 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_TestFail function test_moveToVoteProtocolOrGovernorUpgradeProposal_invalidProposal_reverts(uint248 _againstThreshold) public { - // This will generate a different proposal hash which will make the proposal type wrong + // This will generate a different proposal ID which will make the proposal type wrong vm.assume(_againstThreshold != againstThreshold); // Mock the proposal types configurator call @@ -2221,7 +2202,7 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_TestFail function test_moveToVoteProtocolOrGovernorUpgradeProposal_insufficientApprovals_reverts() public { // Set proposal data approved count to 0 since it is 1 by the approval on the setUp - validator.setProposalData(expectedHash, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(expectedId, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); // Mock the proposal types configurator call _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); @@ -2236,15 +2217,15 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_TestFail _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); // Set proposal data movedToVote to true - validator.setProposalData(expectedHash, approvedProposer, proposalType, true, 1, CYCLE_NUMBER); + validator.setProposalData(expectedId, approvedProposer, proposalType, true, 1, CYCLE_NUMBER); vm.expectRevert(IProposalValidator.ProposalValidator_ProposalAlreadyMovedToVote.selector); vm.prank(approvedProposer); validator.moveToVoteProtocolOrGovernorUpgradeProposal(againstThreshold, proposalDescription); } - function test_moveToVoteProtocolOrGovernorUpgradeProposal_proposalIdMismatch_reverts(bytes32 _randomHash) public { - vm.assume(_randomHash != expectedHash); + function test_moveToVoteProtocolOrGovernorUpgradeProposal_proposalIdMismatch_reverts(uint256 _randomId) public { + vm.assume(_randomId != expectedId); // Mock the proposal types configurator call _mockProposalTypesConfiguratorCall(OPTIMISTIC_VOTING_MODULE_ID); @@ -2256,7 +2237,7 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_TestFail IOptimismGovernor.proposeWithModule, (optimisticVotingModule, votingModuleData, proposalDescription, OPTIMISTIC_VOTING_MODULE_ID) ), - abi.encode(uint256(_randomHash)) + abi.encode(uint256(_randomId)) ); vm.expectRevert(IProposalValidator.ProposalValidator_ProposalIdMismatch.selector); @@ -2268,7 +2249,7 @@ contract ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_TestFail contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_Test is ProposalValidator_Init { ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType.CouncilMemberElections; uint128 criteriaValue = 1; - bytes32 expectedHash; + uint256 expectedId; bytes votingModuleData; string proposalDescription = "Test proposal"; string[] optionsDescriptions = new string[](2); @@ -2279,7 +2260,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_Test is Prop // Create a proposal for move to vote with 1 top choice and 2 options optionsDescriptions[0] = "Option 1"; optionsDescriptions[1] = "Option 2"; - (expectedHash, votingModuleData) = _createCouncilElectionProposalForMoveToVote( + (expectedId, votingModuleData) = _createCouncilElectionProposalForMoveToVote( approvedProposer, criteriaValue, optionsDescriptions, proposalDescription ); } @@ -2295,12 +2276,12 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_Test is Prop IOptimismGovernor.proposeWithModule, (approvalVotingModule, votingModuleData, proposalDescription, APPROVAL_VOTING_MODULE_ID) ), - abi.encode(uint256(expectedHash)) + abi.encode(expectedId) ); // Expect the ProposalMovedToVote event to be emitted vm.expectEmit(address(validator)); - emit ProposalMovedToVote(expectedHash, approvedProposer); + emit ProposalMovedToVote(expectedId, approvedProposer); // Move to vote vm.warp(START_TIMESTAMP + 1); @@ -2308,7 +2289,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_Test is Prop validator.moveToVoteCouncilMemberElectionsProposal(criteriaValue, optionsDescriptions, proposalDescription); // Check that the proposal is in voting - (,, bool movedToVote,,) = validator.getProposalData(expectedHash); + (,, bool movedToVote,,) = validator.getProposalData(expectedId); assertTrue(movedToVote, "Proposal should be in voting"); } } @@ -2318,7 +2299,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is uint128 criteriaValue = 1; string proposalDescription = "Test proposal"; string[] optionsDescriptions = new string[](2); - bytes32 expectedHash; + uint256 expectedId; bytes votingModuleData; function setUp() public override { @@ -2327,7 +2308,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is // Create a proposal for move to vote with 1 top choice and 2 options optionsDescriptions[0] = "Option 1"; optionsDescriptions[1] = "Option 2"; - (expectedHash, votingModuleData) = _createCouncilElectionProposalForMoveToVote( + (expectedId, votingModuleData) = _createCouncilElectionProposalForMoveToVote( approvedProposer, criteriaValue, optionsDescriptions, proposalDescription ); } @@ -2344,7 +2325,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is } function test_moveToVoteCouncilMemberElectionsProposal_invalidProposal_reverts() public { - // This will generate a different proposal hash which will make the proposal type wrong + // This will generate a different proposal ID which will make the proposal type wrong uint128 _criteriaValue = 2; // we use 2 since it is the max based on the created proposal in setUp // Mock the proposal types configurator call @@ -2357,7 +2338,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is function test_moveToVoteCouncilMemberElectionsProposal_insufficientApprovals_reverts() public { // Set proposal data approved count to 0 since it is 1 by the approval on the setUp - validator.setProposalData(expectedHash, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(expectedId, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); // Mock the proposal types configurator call _mockProposalTypesConfiguratorCall(APPROVAL_VOTING_MODULE_ID); @@ -2369,7 +2350,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is function test_moveToVoteCouncilMemberElectionsProposal_proposalAlreadyMovedToVote_reverts() public { // Set proposal data movedToVote to true - validator.setProposalData(expectedHash, approvedProposer, proposalType, true, 2, CYCLE_NUMBER); + validator.setProposalData(expectedId, approvedProposer, proposalType, true, 2, CYCLE_NUMBER); // Mock the proposal types configurator call _mockProposalTypesConfiguratorCall(APPROVAL_VOTING_MODULE_ID); @@ -2389,8 +2370,8 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is validator.moveToVoteCouncilMemberElectionsProposal(criteriaValue, optionsDescriptions, proposalDescription); } - function test_moveToVoteCouncilMemberElectionsProposal_proposalIdMismatch_reverts(bytes32 _randomHash) public { - vm.assume(_randomHash != expectedHash); + function test_moveToVoteCouncilMemberElectionsProposal_proposalIdMismatch_reverts(uint256 _randomId) public { + vm.assume(_randomId != expectedId); // Mock the proposal types configurator call _mockProposalTypesConfiguratorCall(APPROVAL_VOTING_MODULE_ID); @@ -2402,7 +2383,7 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is IOptimismGovernor.proposeWithModule, (approvalVotingModule, votingModuleData, proposalDescription, APPROVAL_VOTING_MODULE_ID) ), - abi.encode(uint256(_randomHash)) + abi.encode(uint256(_randomId)) ); vm.expectRevert(IProposalValidator.ProposalValidator_ProposalIdMismatch.selector); @@ -2421,8 +2402,8 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I string[] optionsDescriptions = new string[](2); address[] optionsRecipients = new address[](2); uint256[] optionsAmounts = new uint256[](2); - bytes32 expectedGovernanceFundHash; - bytes32 expectedCouncilBudgetHash; + uint256 expectedGovernanceFundId; + uint256 expectedCouncilBudgetId; bytes governanceFundVotingModuleData; bytes councilBudgetVotingModuleData; @@ -2442,7 +2423,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I optionsAmounts[1] = 200 ether; // Create one proposal for each type - (expectedGovernanceFundHash, governanceFundVotingModuleData) = _createFundingProposalForMoveToVote( + (expectedGovernanceFundId, governanceFundVotingModuleData) = _createFundingProposalForMoveToVote( approvedProposer, criteriaValue, optionsDescriptions, @@ -2451,7 +2432,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I governanceFundProposalDescription, governanceFundProposalType ); - (expectedCouncilBudgetHash, councilBudgetVotingModuleData) = _createFundingProposalForMoveToVote( + (expectedCouncilBudgetId, councilBudgetVotingModuleData) = _createFundingProposalForMoveToVote( approvedProposer, criteriaValue, optionsDescriptions, @@ -2478,12 +2459,12 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I APPROVAL_VOTING_MODULE_ID ) ), - abi.encode(uint256(expectedGovernanceFundHash)) + abi.encode(uint256(expectedGovernanceFundId)) ); // Expect the ProposalMovedToVote event to be emitted vm.expectEmit(address(validator)); - emit ProposalMovedToVote(expectedGovernanceFundHash, approvedProposer); + emit ProposalMovedToVote(expectedGovernanceFundId, approvedProposer); // Move to vote vm.warp(START_TIMESTAMP + 1); @@ -2498,7 +2479,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I ); // Check that the proposal is in voting - (,, bool movedToVote,,) = validator.getProposalData(expectedGovernanceFundHash); + (,, bool movedToVote,,) = validator.getProposalData(expectedGovernanceFundId); assertTrue(movedToVote, "Proposal should be in voting"); } @@ -2518,12 +2499,12 @@ contract ProposalValidator_MoveToVoteFundingProposal_Test is ProposalValidator_I APPROVAL_VOTING_MODULE_ID ) ), - abi.encode(uint256(expectedCouncilBudgetHash)) + abi.encode(uint256(expectedCouncilBudgetId)) ); // Expect the ProposalMovedToVote event to be emitted vm.expectEmit(address(validator)); - emit ProposalMovedToVote(expectedCouncilBudgetHash, approvedProposer); + emit ProposalMovedToVote(expectedCouncilBudgetId, approvedProposer); // Move to vote vm.warp(START_TIMESTAMP + 1); @@ -2548,8 +2529,8 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat string[] optionsDescriptions; address[] optionsRecipients; uint256[] optionsAmounts; - bytes32 governanceFundExpectedHash; - bytes32 councilBudgetExpectedHash; + uint256 governanceFundExpectedId; + uint256 councilBudgetExpectedId; bytes governanceFundVotingModuleData; bytes councilBudgetVotingModuleData; @@ -2557,7 +2538,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat super.setUp(); (optionsDescriptions, optionsRecipients, optionsAmounts) = _createMinimalFundingArrays(1); - (governanceFundExpectedHash, governanceFundVotingModuleData) = _createFundingProposalForMoveToVote( + (governanceFundExpectedId, governanceFundVotingModuleData) = _createFundingProposalForMoveToVote( approvedProposer, criteriaValue, optionsDescriptions, @@ -2566,7 +2547,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat governanceFundProposalDescription, governanceFundProposalType ); - (councilBudgetExpectedHash, councilBudgetVotingModuleData) = _createFundingProposalForMoveToVote( + (councilBudgetExpectedId, councilBudgetVotingModuleData) = _createFundingProposalForMoveToVote( approvedProposer, criteriaValue, optionsDescriptions, @@ -2600,7 +2581,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat ) public { - // Ensure the criteria value is not the same as the one in setUp so when calculating the proposal hash it will + // Ensure the criteria value is not the same as the one in setUp so when calculating the proposal ID it will // not find the proposal vm.assume(_criteriaValue != criteriaValue); @@ -2642,13 +2623,13 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat if (validProposalType == governanceFundProposalType) { // Set proposal data proposal type to a different value validator.setProposalData( - governanceFundExpectedHash, approvedProposer, wrongProposalType, false, 0, CYCLE_NUMBER + governanceFundExpectedId, approvedProposer, wrongProposalType, false, 0, CYCLE_NUMBER ); proposalDescription = governanceFundProposalDescription; } else { // Set proposal data proposal type to a different value validator.setProposalData( - councilBudgetExpectedHash, approvedProposer, wrongProposalType, false, 0, CYCLE_NUMBER + councilBudgetExpectedId, approvedProposer, wrongProposalType, false, 0, CYCLE_NUMBER ); proposalDescription = councilBudgetProposalDescription; } @@ -2676,13 +2657,11 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat string memory proposalDescription; if (proposalType == governanceFundProposalType) { // Set proposal data approved count to 0 since it is 1 by the approval on the setUp - validator.setProposalData( - governanceFundExpectedHash, approvedProposer, proposalType, false, 0, CYCLE_NUMBER - ); + validator.setProposalData(governanceFundExpectedId, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); proposalDescription = governanceFundProposalDescription; } else { // Set proposal data approved count to 0 since it is 1 by the approval on the setUp - validator.setProposalData(councilBudgetExpectedHash, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); + validator.setProposalData(councilBudgetExpectedId, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); proposalDescription = councilBudgetProposalDescription; } @@ -2704,11 +2683,11 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat string memory proposalDescription; if (proposalType == governanceFundProposalType) { // Set proposal data movedToVote to true - validator.setProposalData(governanceFundExpectedHash, approvedProposer, proposalType, true, 1, CYCLE_NUMBER); + validator.setProposalData(governanceFundExpectedId, approvedProposer, proposalType, true, 1, CYCLE_NUMBER); proposalDescription = governanceFundProposalDescription; } else { // Set proposal data movedToVote to true - validator.setProposalData(councilBudgetExpectedHash, approvedProposer, proposalType, true, 1, CYCLE_NUMBER); + validator.setProposalData(councilBudgetExpectedId, approvedProposer, proposalType, true, 1, CYCLE_NUMBER); proposalDescription = councilBudgetProposalDescription; } @@ -2822,11 +2801,11 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat function test_moveToVoteFundingProposal_proposalIdMismatch_reverts( uint8 _proposalTypeValue, - bytes32 _randomHash + uint256 _randomId ) public { - vm.assume(_randomHash != governanceFundExpectedHash && _randomHash != councilBudgetExpectedHash); + vm.assume(_randomId != governanceFundExpectedId && _randomId != councilBudgetExpectedId); // Valid funding proposal types are GovernanceFund (3) and CouncilBudget (4) _proposalTypeValue = uint8(bound(_proposalTypeValue, 3, 4)); @@ -2852,7 +2831,7 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat IOptimismGovernor.proposeWithModule, (approvalVotingModule, votingModuleData, proposalDescription, APPROVAL_VOTING_MODULE_ID) ), - abi.encode(uint256(_randomHash)) + abi.encode(uint256(_randomId)) ); vm.expectRevert(IProposalValidator.ProposalValidator_ProposalIdMismatch.selector); @@ -2995,11 +2974,11 @@ contract ProposalValidator_HashProposalWithModule_Test is ProposalValidator_Init public view { - bytes32 hash = validator.hashProposalWithModule(module, proposalData, descriptionHash); - bytes32 expectedHash = - keccak256(abi.encode(address(validator.GOVERNOR()), module, proposalData, descriptionHash)); + uint256 id = validator.hashProposalWithModule(module, proposalData, descriptionHash); + uint256 expectedId = + uint256(keccak256(abi.encode(address(validator.GOVERNOR()), module, proposalData, descriptionHash))); - assertEq(hash, expectedHash); + assertEq(id, expectedId); } function test_hashProposalWithModule_differentInputs_succeeds() public { @@ -3008,9 +2987,9 @@ contract ProposalValidator_HashProposalWithModule_Test is ProposalValidator_Init bytes memory data = abi.encode("data"); bytes32 descHash = keccak256("desc"); - bytes32 hash1 = validator.hashProposalWithModule(module1, data, descHash); - bytes32 hash2 = validator.hashProposalWithModule(module2, data, descHash); + uint256 id1 = validator.hashProposalWithModule(module1, data, descHash); + uint256 id2 = validator.hashProposalWithModule(module2, data, descHash); - assertTrue(hash1 != hash2); + assertTrue(id1 != id2); } } From 39b6ca9e13be417b59d715e95fb9fba6c7aa025e Mon Sep 17 00:00:00 2001 From: 0xOneTony <112496816+0xOneTony@users.noreply.github.com> Date: Mon, 28 Jul 2025 20:20:27 +0300 Subject: [PATCH 13/14] fix: descrepancies (#464) * fix: improve comment * fix: make schemas immutable again * fix: improve code * fix: validation check order * fix: add move to vote safety check * fix: pre-pr --- .../governance/IProposalValidator.sol | 10 +- .../snapshots/abi/ProposalValidator.json | 72 +++++++------- .../snapshots/semver-lock.json | 4 +- .../storageLayout/ProposalValidator.json | 22 +---- .../src/governance/ProposalValidator.sol | 99 ++++++++++++------- .../test/governance/ProposalValidator.t.sol | 57 +++++++++-- 6 files changed, 160 insertions(+), 104 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index 291ba4833b063..84518b450dc63 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -172,8 +172,6 @@ interface IProposalValidator is ISemver { uint256 _duration, uint256 _votingCycleDistributionLimit, uint256 _proposalDistributionThreshold, - bytes32 _approvedProposerAttestationSchemaUid, - bytes32 _topDelegatesAttestationSchemaUid, ProposalType[] memory _proposalTypes, ProposalTypeData[] memory _proposalTypesData ) external; @@ -190,9 +188,9 @@ interface IProposalValidator is ISemver { function initVersion() external view returns (uint8); - function approvedProposerAttestationSchemaUid() external view returns (bytes32); + function APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID() external view returns (bytes32); - function topDelegatesAttestationSchemaUid() external view returns (bytes32); + function TOP_DELEGATES_ATTESTATION_SCHEMA_UID() external view returns (bytes32); function OPTIMISTIC_MODULE_PERCENT_DIVISOR() external view returns (uint256); @@ -206,6 +204,8 @@ interface IProposalValidator is ISemver { ); function __constructor__( - IOptimismGovernor _governor + IOptimismGovernor _governor, + bytes32 _approvedProposerAttestationSchemaUid, + bytes32 _topDelegatesAttestationSchemaUid ) external; } diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index b26b264755756..f7c9631976afa 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -5,11 +5,34 @@ "internalType": "contract IOptimismGovernor", "name": "_governor", "type": "address" + }, + { + "internalType": "bytes32", + "name": "_approvedProposerAttestationSchemaUid", + "type": "bytes32" + }, + { + "internalType": "bytes32", + "name": "_topDelegatesAttestationSchemaUid", + "type": "bytes32" } ], "stateMutability": "nonpayable", "type": "constructor" }, + { + "inputs": [], + "name": "APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID", + "outputs": [ + { + "internalType": "bytes32", + "name": "", + "type": "bytes32" + } + ], + "stateMutability": "view", + "type": "function" + }, { "inputs": [], "name": "GOVERNOR", @@ -36,6 +59,19 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [], + "name": "TOP_DELEGATES_ATTESTATION_SCHEMA_UID", + "outputs": [ + { + "internalType": "bytes32", + "name": "", + "type": "bytes32" + } + ], + "stateMutability": "view", + "type": "function" + }, { "inputs": [ { @@ -54,19 +90,6 @@ "stateMutability": "nonpayable", "type": "function" }, - { - "inputs": [], - "name": "approvedProposerAttestationSchemaUid", - "outputs": [ - { - "internalType": "bytes32", - "name": "", - "type": "bytes32" - } - ], - "stateMutability": "view", - "type": "function" - }, { "inputs": [], "name": "initVersion", @@ -112,16 +135,6 @@ "name": "_proposalDistributionThreshold", "type": "uint256" }, - { - "internalType": "bytes32", - "name": "_approvedProposerAttestationSchemaUid", - "type": "bytes32" - }, - { - "internalType": "bytes32", - "name": "_topDelegatesAttestationSchemaUid", - "type": "bytes32" - }, { "internalType": "enum ProposalValidator.ProposalType[]", "name": "_proposalTypes", @@ -502,19 +515,6 @@ "stateMutability": "nonpayable", "type": "function" }, - { - "inputs": [], - "name": "topDelegatesAttestationSchemaUid", - "outputs": [ - { - "internalType": "bytes32", - "name": "", - "type": "bytes32" - } - ], - "stateMutability": "view", - "type": "function" - }, { "inputs": [ { diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 70d743998b67e..5d029bed2315b 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x4171ca5a66e60a2a3715dc3108d76f985eb16937d71197c74ee0465461e8f9fe", - "sourceCodeHash": "0xa95d144dad3bb25ef0ac0e4ae9039b1ee5304e86e9ef7c1726d263f63598f864" + "initCodeHash": "0xd96122c73104bff67f8493e0e0d9d7aeeb6a631ecd52d5572a8a21b724591ad9", + "sourceCodeHash": "0x857b3e452c59b2263d812868916e20aafb81380f1438616d913141ae45a6b806" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json b/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json index 2b847ae39937b..167b206ea9ced 100644 --- a/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/storageLayout/ProposalValidator.json @@ -34,46 +34,32 @@ "slot": "52", "type": "uint256[49]" }, - { - "bytes": "32", - "label": "approvedProposerAttestationSchemaUid", - "offset": 0, - "slot": "101", - "type": "bytes32" - }, - { - "bytes": "32", - "label": "topDelegatesAttestationSchemaUid", - "offset": 0, - "slot": "102", - "type": "bytes32" - }, { "bytes": "32", "label": "proposalDistributionThreshold", "offset": 0, - "slot": "103", + "slot": "101", "type": "uint256" }, { "bytes": "32", "label": "votingCycles", "offset": 0, - "slot": "104", + "slot": "102", "type": "mapping(uint256 => struct ProposalValidator.VotingCycleData)" }, { "bytes": "32", "label": "proposalTypesData", "offset": 0, - "slot": "105", + "slot": "103", "type": "mapping(enum ProposalValidator.ProposalType => struct ProposalValidator.ProposalTypeData)" }, { "bytes": "32", "label": "_proposals", "offset": 0, - "slot": "106", + "slot": "104", "type": "mapping(uint256 => struct ProposalValidator.ProposalData)" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 61db34720bba9..01c6e0d28c38f 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -134,7 +134,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Emitted when the proposal type data is set. /// @param proposalType The type of proposal. /// @param requiredApprovals The required number of approvals. - /// @param idInConfigurator The proposal type ID. + /// @param idInConfigurator The proposal type ID in the ProposalTypesConfigurator contract. event ProposalTypeDataSet(ProposalType proposalType, uint256 requiredApprovals, uint8 idInConfigurator); /// @notice Emitted with ProposalSubmitted event. @@ -228,11 +228,11 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice The schema UID for attestations in the Ethereum Attestation Service for checking if the caller /// is an approved proposer. /// @dev Schema format: { proposalType: uint8, date: string } - bytes32 public approvedProposerAttestationSchemaUid; + bytes32 public immutable APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID; /// @notice The schema UID for attestations in the Ethereum Attestation Service for checking if the caller /// is part of the top100 delegates. - bytes32 public topDelegatesAttestationSchemaUid; + bytes32 public immutable TOP_DELEGATES_ATTESTATION_SCHEMA_UID; /// @notice The max amount of tokens that can be distributed in a proposal. uint256 public proposalDistributionThreshold; @@ -248,8 +248,22 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Constructs the ProposalValidator contract. /// @param _governor The Optimism Governor contract address. - constructor(IOptimismGovernor _governor) ReinitializableBase(1) { + /// @param _approvedProposerAttestationSchemaUid The schema UID for attestations in the Ethereum Attestation Service + /// for checking if the caller + /// is an approved proposer. + /// @param _topDelegatesAttestationSchemaUid The schema UID for attestations in the Ethereum Attestation Service for + /// checking if the caller + /// is part of the top100 delegates. + constructor( + IOptimismGovernor _governor, + bytes32 _approvedProposerAttestationSchemaUid, + bytes32 _topDelegatesAttestationSchemaUid + ) + ReinitializableBase(1) + { GOVERNOR = _governor; + APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID = _approvedProposerAttestationSchemaUid; + TOP_DELEGATES_ATTESTATION_SCHEMA_UID = _topDelegatesAttestationSchemaUid; _disableInitializers(); } @@ -269,8 +283,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { uint256 _duration, uint256 _votingCycleDistributionLimit, uint256 _proposalDistributionThreshold, - bytes32 _approvedProposerAttestationSchemaUid, - bytes32 _topDelegatesAttestationSchemaUid, ProposalType[] memory _proposalTypes, ProposalTypeData[] memory _proposalTypesData ) @@ -281,9 +293,6 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalTypesDataLengthMismatch(); } - approvedProposerAttestationSchemaUid = _approvedProposerAttestationSchemaUid; - topDelegatesAttestationSchemaUid = _topDelegatesAttestationSchemaUid; - _setVotingCycleData(_cycleNumber, _startingTimestamp, _duration, _votingCycleDistributionLimit); _setProposalDistributionThreshold(_proposalDistributionThreshold); @@ -452,17 +461,20 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, approvalSettings); + // Retrieve the ID to use in the proposal type configurator + uint8 idInConfigurator = proposalTypesData[ProposalType.CouncilMemberElections].idInConfigurator; + // Get the module address from the configurator - IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( - GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() - ).proposalTypes(proposalTypesData[ProposalType.CouncilMemberElections].idInConfigurator); - address votingModule = proposalTypeConfig.module; + IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = + IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator); // Validate voting module exists if (bytes(proposalTypeConfig.name).length == 0) { revert ProposalValidator_InvalidVotingModule(); } + address votingModule = proposalTypeConfig.module; + // Generate unique proposal ID proposalId_ = _hashProposalWithModule(votingModule, proposalVotingModuleData, keccak256(bytes(_proposalDescription))); @@ -551,15 +563,21 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { bytes memory proposalVotingModuleData = abi.encode(options, approvalSettings); + // Retrieve the ID to use in the proposal type configurator + uint8 idInConfigurator = proposalTypesData[_proposalType].idInConfigurator; + // Get the module address from the configurator - IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = IProposalTypesConfigurator( - GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR() - ).proposalTypes(proposalTypesData[_proposalType].idInConfigurator); - address votingModule = proposalTypeConfig.module; + address votingModule; + { + IProposalTypesConfigurator.ProposalType memory proposalTypeConfig = + IProposalTypesConfigurator(GOVERNOR.PROPOSAL_TYPES_CONFIGURATOR()).proposalTypes(idInConfigurator); - // Validate voting module exists - if (bytes(proposalTypeConfig.name).length == 0) { - revert ProposalValidator_InvalidVotingModule(); + // Validate voting module exists + if (bytes(proposalTypeConfig.name).length == 0) { + revert ProposalValidator_InvalidVotingModule(); + } + + votingModule = proposalTypeConfig.module; } // Generate unique proposal ID @@ -707,13 +725,18 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { external returns (uint256 proposalId_) { + uint256 optionsLength = _optionsDescriptions.length; + // Validate options length bounds + if (optionsLength == 0 || optionsLength > type(uint8).max) { + revert ProposalValidator_InvalidOptionsLength(); + } // Configure approval module options (IApprovalVotingModule.ProposalOption[] memory options,) = _buildApprovalModuleOptions(_optionsDescriptions, new address[](0), new uint256[](0)); // Configure approval module settings IApprovalVotingModule.ProposalSettings memory approvalSettings = IApprovalVotingModule.ProposalSettings({ - maxApprovals: uint8(_optionsDescriptions.length), + maxApprovals: uint8(optionsLength), criteria: uint8(IApprovalVotingModule.PassingCriteria.TopChoices), budgetToken: address(0), criteriaValue: _criteriaValue, @@ -808,6 +831,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_InvalidFundingProposalType(); } + uint256 optionsLength = _optionsDescriptions.length; + // Validate options length bounds + if (optionsLength == 0 || optionsLength > type(uint8).max) { + revert ProposalValidator_InvalidOptionsLength(); + } + // Configure approval module options (IApprovalVotingModule.ProposalOption[] memory options, uint256 totalBudget) = _buildApprovalModuleOptions(_optionsDescriptions, _optionsRecipients, _optionsAmounts); @@ -850,18 +879,20 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalAlreadyMovedToVote(); } - // Check if proposal can be moved to vote - VotingCycleData memory votingCycleData = votingCycles[proposal.votingCycle]; - if ( - votingCycleData.startingTimestamp > block.timestamp - || votingCycleData.startingTimestamp + votingCycleData.duration < block.timestamp - ) { - revert ProposalValidator_InvalidVotingCycle(); - } + { + // Check if proposal can be moved to vote + VotingCycleData memory votingCycleData = votingCycles[proposal.votingCycle]; + if ( + votingCycleData.startingTimestamp > block.timestamp + || votingCycleData.startingTimestamp + votingCycleData.duration < block.timestamp + ) { + revert ProposalValidator_InvalidVotingCycle(); + } - // Check if total budget is within the voting cycle distribution limit - if (votingCycleData.movedToVoteTokenCount + totalBudget > votingCycleData.votingCycleDistributionLimit) { - revert ProposalValidator_ExceedsDistributionThreshold(); + // Check if total budget is within the voting cycle distribution limit + if (votingCycleData.movedToVoteTokenCount + totalBudget > votingCycleData.votingCycleDistributionLimit) { + revert ProposalValidator_ExceedsDistributionThreshold(); + } } // Move proposal to vote @@ -951,7 +982,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { (uint8 proposalType,) = abi.decode(attestation.data, (uint8, string)); if ( - attestation.attester != owner() || attestation.schema != approvedProposerAttestationSchemaUid + attestation.attester != owner() || attestation.schema != APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID || attestation.recipient != _msgSender() || proposalType != uint8(_expectedProposalType) ) { revert ProposalValidator_InvalidAttestation(); @@ -975,7 +1006,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { } // check if the schema is correct - if (attestation.schema != topDelegatesAttestationSchemaUid) { + if (attestation.schema != TOP_DELEGATES_ATTESTATION_SCHEMA_UID) { revert ProposalValidator_InvalidAttestationSchema(); } diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index a307afbb91869..81d24cb0c2720 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -35,7 +35,13 @@ import { CommonTest } from "test/setup/CommonTest.sol"; /// @title ProposalValidatorForTest /// @notice A test contract that exposes the private _hashProposalWithModule function contract ProposalValidatorForTest is ProposalValidator { - constructor(IOptimismGovernor _governor) ProposalValidator(_governor) { } + constructor( + IOptimismGovernor _governor, + bytes32 _approvedProposerAttestationSchemaUid, + bytes32 _topDelegatesAttestationSchemaUid + ) + ProposalValidator(_governor, _approvedProposerAttestationSchemaUid, _topDelegatesAttestationSchemaUid) + { } function hashProposalWithModule( address _module, @@ -516,7 +522,9 @@ contract ProposalValidator_Init is CommonTest { // Create mock addresses proposalTypesConfigurator = IProposalTypesConfigurator(makeAddr("proposalTypesConfigurator")); - impl = new ProposalValidatorForTest(governor); + impl = new ProposalValidatorForTest( + governor, APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, TOP_DELEGATES_ATTESTATION_SCHEMA_UID + ); validator = ProposalValidatorForTest(address(new Proxy(owner))); vm.prank(owner); @@ -531,8 +539,6 @@ contract ProposalValidator_Init is CommonTest { DURATION, DISTRIBUTION_LIMIT, DISTRIBUTION_THRESHOLD, - APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, - TOP_DELEGATES_ATTESTATION_SCHEMA_UID, proposalTypes, proposalTypesData ) @@ -626,7 +632,9 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { // Create mock addresses proposalTypesConfigurator = IProposalTypesConfigurator(makeAddr("proposalTypesConfigurator")); - impl = new ProposalValidatorForTest(governor); + impl = new ProposalValidatorForTest( + governor, APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, TOP_DELEGATES_ATTESTATION_SCHEMA_UID + ); validator = ProposalValidatorForTest(address(new Proxy(owner))); } @@ -648,8 +656,6 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { DURATION, DISTRIBUTION_LIMIT, DISTRIBUTION_THRESHOLD, - APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, - TOP_DELEGATES_ATTESTATION_SCHEMA_UID, proposalTypes, proposalTypesData ) @@ -717,8 +723,6 @@ contract ProposalValidator_Initialize_Test is ProposalValidator_Init { DURATION, DISTRIBUTION_LIMIT, DISTRIBUTION_THRESHOLD, - APPROVED_PROPOSER_ATTESTATION_SCHEMA_UID, - TOP_DELEGATES_ATTESTATION_SCHEMA_UID, proposalTypes, proposalTypesData ) @@ -2336,6 +2340,16 @@ contract ProposalValidator_MoveToVoteCouncilMemberElectionsProposal_TestFail is validator.moveToVoteCouncilMemberElectionsProposal(_criteriaValue, optionsDescriptions, proposalDescription); } + function test_moveToVoteCouncilMemberElectionsProposal_invalidOptionsLength_reverts() public { + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidOptionsLength.selector); + vm.prank(approvedProposer); + validator.moveToVoteCouncilMemberElectionsProposal(criteriaValue, new string[](0), proposalDescription); + + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidOptionsLength.selector); + vm.prank(approvedProposer); + validator.moveToVoteCouncilMemberElectionsProposal(criteriaValue, new string[](256), proposalDescription); + } + function test_moveToVoteCouncilMemberElectionsProposal_insufficientApprovals_reverts() public { // Set proposal data approved count to 0 since it is 1 by the approval on the setUp validator.setProposalData(expectedId, approvedProposer, proposalType, false, 0, CYCLE_NUMBER); @@ -2649,6 +2663,31 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat ); } + function test_moveToVoteFundingProposal_invalidOptionsLength_reverts(uint8 _proposalTypeValue) public { + // Valid funding proposal types are GovernanceFund (3) and CouncilBudget (4) + _proposalTypeValue = uint8(bound(_proposalTypeValue, 3, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(_proposalTypeValue); + + string memory proposalDescription; + if (proposalType == governanceFundProposalType) { + proposalDescription = governanceFundProposalDescription; + } else { + proposalDescription = councilBudgetProposalDescription; + } + + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidOptionsLength.selector); + vm.prank(approvedProposer); + validator.moveToVoteFundingProposal( + criteriaValue, new string[](0), optionsRecipients, optionsAmounts, proposalDescription, proposalType + ); + + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidOptionsLength.selector); + vm.prank(approvedProposer); + validator.moveToVoteFundingProposal( + criteriaValue, new string[](256), optionsRecipients, optionsAmounts, proposalDescription, proposalType + ); + } + function test_moveToVoteFundingProposal_insufficientApprovals_reverts(uint8 _proposalTypeValue) public { // Valid funding proposal types are GovernanceFund (3) and CouncilBudget (4) _proposalTypeValue = uint8(bound(_proposalTypeValue, 3, 4)); From 96371dd6053b620e6a3ecc24261872bb6f26c1f9 Mon Sep 17 00:00:00 2001 From: OneTony Date: Mon, 28 Jul 2025 22:03:34 +0300 Subject: [PATCH 14/14] fix: add total budget overflow check --- .../governance/IProposalValidator.sol | 1 + .../snapshots/abi/ProposalValidator.json | 5 ++ .../snapshots/semver-lock.json | 4 +- .../src/governance/ProposalValidator.sol | 9 +++- .../test/governance/ProposalValidator.t.sol | 49 +++++++++++++++++++ 5 files changed, 65 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index 84518b450dc63..5208757dfc49c 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -31,6 +31,7 @@ interface IProposalValidator is ISemver { error ProposalValidator_InvalidProposer(); error ProposalValidator_InvalidProposal(); error ProposalValidator_InvalidVotingModule(); + error ProposalValidator_InvalidTotalBudget(); error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); event ProposalSubmitted( diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index f7c9631976afa..f2305002325f9 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -829,6 +829,11 @@ "name": "ProposalValidator_InvalidProposer", "type": "error" }, + { + "inputs": [], + "name": "ProposalValidator_InvalidTotalBudget", + "type": "error" + }, { "inputs": [], "name": "ProposalValidator_InvalidUpgradeProposalType", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 5d029bed2315b..f7ab1bedf50db 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xd96122c73104bff67f8493e0e0d9d7aeeb6a631ecd52d5572a8a21b724591ad9", - "sourceCodeHash": "0x857b3e452c59b2263d812868916e20aafb81380f1438616d913141ae45a6b806" + "initCodeHash": "0xfd63a8de6795e33cba8b47ce2cab24f09a003065292d74bdbe236161335b02ba", + "sourceCodeHash": "0xa774418504002f9f2aaff0333a534c65a46e8bb9d9d1e9408fbe1b3204b10462" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index 01c6e0d28c38f..a1d9a76d09cc6 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -89,9 +89,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Thrown when the proposal is invalid trying to move to vote. error ProposalValidator_InvalidProposal(); - /// @notice Thrown when the voting module address is invalid (zero address). + /// @notice Thrown when the voting module address is invalid. error ProposalValidator_InvalidVotingModule(); + /// @notice Thrown when the total budget is invalid (must be > 0 and <= uint128 max). + error ProposalValidator_InvalidTotalBudget(); + /// @notice Thrown when the attestation was created after the last voting cycle. error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); @@ -1084,6 +1087,10 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { description: _optionDescriptions[i] }); } + + if (totalBudget_ > type(uint128).max) { + revert ProposalValidator_InvalidTotalBudget(); + } } /// @notice Calculate `proposalId` based on `module`, `proposalData` and `descriptionHash`. diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index 81d24cb0c2720..be1305762c1ff 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -1855,6 +1855,26 @@ contract ProposalValidator_SubmitFundingProposal_TestFail is ProposalValidator_I FUNDING_CRITERIA_VALUE, descriptions, recipients, amounts, description, proposalType, CYCLE_NUMBER ); } + + function test_submitFundingProposal_invalidTotalBudget_reverts(uint8 proposalTypeValue, uint256 _amount) public { + _amount = bound(_amount, type(uint136).max, type(uint192).max); + // Bound proposal type to only GovernanceFund (3) or CouncilBudget (4) + proposalTypeValue = uint8(bound(proposalTypeValue, 3, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + + (string[] memory descriptions, address[] memory recipients, uint256[] memory amounts) = + _createMinimalFundingArrays(1); + + vm.prank(owner); + validator.setProposalDistributionThreshold(type(uint256).max); + + amounts[0] = _amount; + vm.expectRevert(ProposalValidator.ProposalValidator_InvalidTotalBudget.selector); + vm.prank(user); + validator.submitFundingProposal( + FUNDING_CRITERIA_VALUE, descriptions, recipients, amounts, description, proposalType, CYCLE_NUMBER + ); + } } /// @title ProposalValidator_ApproveProposal_Test @@ -2688,6 +2708,35 @@ contract ProposalValidator_MoveToVoteFundingProposal_TestFail is ProposalValidat ); } + function test_moveToVoteFundingProposal_invalidTotalBudget_reverts( + uint8 _proposalTypeValue, + uint256 _amount + ) + public + { + _amount = bound(_amount, type(uint136).max, type(uint192).max); + // Valid funding proposal types are GovernanceFund (3) and CouncilBudget (4) + _proposalTypeValue = uint8(bound(_proposalTypeValue, 3, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(_proposalTypeValue); + + string memory proposalDescription; + if (proposalType == governanceFundProposalType) { + proposalDescription = governanceFundProposalDescription; + } else { + proposalDescription = councilBudgetProposalDescription; + } + + vm.prank(owner); + validator.setProposalDistributionThreshold(type(uint256).max); + + optionsAmounts[0] = _amount; + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidTotalBudget.selector); + vm.prank(approvedProposer); + validator.moveToVoteFundingProposal( + criteriaValue, optionsDescriptions, optionsRecipients, optionsAmounts, proposalDescription, proposalType + ); + } + function test_moveToVoteFundingProposal_insufficientApprovals_reverts(uint8 _proposalTypeValue) public { // Valid funding proposal types are GovernanceFund (3) and CouncilBudget (4) _proposalTypeValue = uint8(bound(_proposalTypeValue, 3, 4));