diff --git a/.gas-snapshot b/.gas-snapshot index 4b10097b..7e8791a1 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,32 +1,32 @@ -AIP1Point2ActionTest:testAction() (gas: 629328) +AIP1Point2ActionTest:testAction() (gas: 629408) AIPNovaFeeRoutingActionTest:testAction() (gas: 3074) ArbitrumDAOConstitutionTest:testConstructor() (gas: 259383) ArbitrumDAOConstitutionTest:testMonOwnerCannotSetHash() (gas: 262836) ArbitrumDAOConstitutionTest:testOwnerCanSetHash() (gas: 261148) ArbitrumDAOConstitutionTest:testOwnerCanSetHashTwice() (gas: 263824) -ArbitrumFoundationVestingWalletTest:testBeneficiaryCanSetBeneficiary() (gas: 16332093) -ArbitrumFoundationVestingWalletTest:testMigrateEthToNewWalletWithSlowerVesting() (gas: 19243747) -ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithFasterVesting() (gas: 19247090) -ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithSlowerVesting() (gas: 19247035) -ArbitrumFoundationVestingWalletTest:testMigrationTargetMustBeContract() (gas: 16335426) -ArbitrumFoundationVestingWalletTest:testOnlyBeneficiaryCanRelease() (gas: 16327408) -ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16329757) -ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16332176) -ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16337546) -ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16329656) -ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16451131) +ArbitrumFoundationVestingWalletTest:testBeneficiaryCanSetBeneficiary() (gas: 16711185) +ArbitrumFoundationVestingWalletTest:testMigrateEthToNewWalletWithSlowerVesting() (gas: 19622838) +ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithFasterVesting() (gas: 19626181) +ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithSlowerVesting() (gas: 19626126) +ArbitrumFoundationVestingWalletTest:testMigrationTargetMustBeContract() (gas: 16714518) +ArbitrumFoundationVestingWalletTest:testOnlyBeneficiaryCanRelease() (gas: 16706500) +ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16708849) +ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16711268) +ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16716637) +ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16708748) +ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16830223) ArbitrumVestingWalletFactoryTest:testDeploy() (gas: 4589688) ArbitrumVestingWalletFactoryTest:testOnlyOwnerCanCreateWallets() (gas: 1504286) -ArbitrumVestingWalletTest:testCastVote() (gas: 16201584) -ArbitrumVestingWalletTest:testCastVoteFailsForNonBeneficiary() (gas: 16151341) -ArbitrumVestingWalletTest:testClaim() (gas: 16007768) -ArbitrumVestingWalletTest:testClaimFailsForNonBeneficiary() (gas: 15967955) -ArbitrumVestingWalletTest:testDelegate() (gas: 16081106) -ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16008435) -ArbitrumVestingWalletTest:testDoesDeploy() (gas: 15971342) -ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16008649) -ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16074917) -E2E:testE2E() (gas: 84680100) +ArbitrumVestingWalletTest:testCastVote() (gas: 16602853) +ArbitrumVestingWalletTest:testCastVoteFailsForNonBeneficiary() (gas: 16552681) +ArbitrumVestingWalletTest:testClaim() (gas: 16386861) +ArbitrumVestingWalletTest:testClaimFailsForNonBeneficiary() (gas: 16347048) +ArbitrumVestingWalletTest:testDelegate() (gas: 16460199) +ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16387528) +ArbitrumVestingWalletTest:testDoesDeploy() (gas: 16350435) +ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16387742) +ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16454010) +E2E:testE2E() (gas: 84680209) FixedDelegateErc20WalletTest:testInit() (gas: 5822575) FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 5816805) FixedDelegateErc20WalletTest:testTransfer() (gas: 5932218) @@ -60,13 +60,13 @@ L1GovernanceFactoryTest:testL1GovernanceFactory() (gas: 10771109) L1GovernanceFactoryTest:testSetMinDelay() (gas: 10746003) L1GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 10798958) L2AddressRegistryTest:testAddressRegistryAddress() (gas: 54658) -L2ArbitrumGovernorTest:testCantReinit() (gas: 13669489) -L2ArbitrumGovernorTest:testExecutorPermissions() (gas: 13706483) -L2ArbitrumGovernorTest:testExecutorPermissionsFail() (gas: 13679135) -L2ArbitrumGovernorTest:testPastCirculatingSupply() (gas: 13673238) -L2ArbitrumGovernorTest:testPastCirculatingSupplyExclude() (gas: 13812715) -L2ArbitrumGovernorTest:testPastCirculatingSupplyMint() (gas: 13737218) -L2ArbitrumGovernorTest:testProperlyInitialized() (gas: 13664706) +L2ArbitrumGovernorTest:testCantReinit() (gas: 14048626) +L2ArbitrumGovernorTest:testExecutorPermissions() (gas: 14085537) +L2ArbitrumGovernorTest:testExecutorPermissionsFail() (gas: 14058208) +L2ArbitrumGovernorTest:testPastCirculatingSupply() (gas: 14052375) +L2ArbitrumGovernorTest:testPastCirculatingSupplyExclude() (gas: 14191829) +L2ArbitrumGovernorTest:testPastCirculatingSupplyMint() (gas: 14116355) +L2ArbitrumGovernorTest:testProperlyInitialized() (gas: 14043756) L2ArbitrumTokenTest:testCanBurn() (gas: 4066835) L2ArbitrumTokenTest:testCanMint2Percent() (gas: 4101512) L2ArbitrumTokenTest:testCanMintLessThan2Percent() (gas: 4101514) @@ -85,15 +85,15 @@ L2ArbitrumTokenTest:testDoesNotInitialiseZeroL1Token() (gas: 3800726) L2ArbitrumTokenTest:testDoesNotInitialiseZeroOwner() (gas: 3800739) L2ArbitrumTokenTest:testIsInitialised() (gas: 4072777) L2ArbitrumTokenTest:testNoLogicContractInit() (gas: 2693127) -L2GovernanceFactoryTest:testContractsDeployed() (gas: 28359365) -L2GovernanceFactoryTest:testContractsInitialized() (gas: 28396315) -L2GovernanceFactoryTest:testDeploySteps() (gas: 28370874) -L2GovernanceFactoryTest:testProxyAdminOwnership() (gas: 28368375) -L2GovernanceFactoryTest:testRoles() (gas: 28391450) -L2GovernanceFactoryTest:testSanityCheckValues() (gas: 28415658) -L2GovernanceFactoryTest:testSetMinDelay() (gas: 28364371) -L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 28417242) -L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 28657360) +L2GovernanceFactoryTest:testContractsDeployed() (gas: 29117182) +L2GovernanceFactoryTest:testContractsInitialized() (gas: 29154197) +L2GovernanceFactoryTest:testDeploySteps() (gas: 29128691) +L2GovernanceFactoryTest:testProxyAdminOwnership() (gas: 29126192) +L2GovernanceFactoryTest:testRoles() (gas: 29149267) +L2GovernanceFactoryTest:testSanityCheckValues() (gas: 29173498) +L2GovernanceFactoryTest:testSetMinDelay() (gas: 29122188) +L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 29175059) +L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 29437526) L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 30524001) L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 30528232) L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 25659644) @@ -101,7 +101,7 @@ L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 30526232) L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 30545325) NomineeGovernorV2UpgradeActionTest:testAction() (gas: 8153) OfficeHoursActionTest:testConstructor() (gas: 9050) -OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 258, μ: 317071, ~: 317184) +OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317064, ~: 317184) OfficeHoursActionTest:testInvalidConstructorParameters() (gas: 235740) OfficeHoursActionTest:testPerformBeforeMinimumTimestamp() (gas: 8646) OfficeHoursActionTest:testPerformDuringOfficeHours() (gas: 9140) @@ -153,7 +153,7 @@ SecurityCouncilMemberElectionGovernorTest:testOnlyNomineeElectionGovernorCanProp SecurityCouncilMemberElectionGovernorTest:testProperInitialization() (gas: 49388) SecurityCouncilMemberElectionGovernorTest:testProposeReverts() (gas: 32916) SecurityCouncilMemberElectionGovernorTest:testRelay() (gas: 42229) -SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 258, μ: 339983, ~: 339822) +SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340112, ~: 339983) SecurityCouncilMemberElectionGovernorTest:testSelectTopNomineesFails() (gas: 273335) SecurityCouncilMemberElectionGovernorTest:testSetFullWeightDuration() (gas: 34951) SecurityCouncilMemberElectionGovernorTest:testVotesToWeight() (gas: 152898) @@ -205,7 +205,7 @@ SecurityCouncilNomineeElectionGovernorTest:testSetNomineeVetter() (gas: 39905) SequencerActionsTest:testAddAndRemoveSequencer() (gas: 483532) SequencerActionsTest:testCantAddZeroAddress() (gas: 235614) SetInitialGovParamsActionTest:testL1() (gas: 259904) -SetInitialGovParamsActionTest:testL2() (gas: 688888) +SetInitialGovParamsActionTest:testL2() (gas: 688702) SetSequencerInboxMaxTimeVariationAction:testSetMaxTimeVariation() (gas: 374262) SwitchManagerRolesActionTest:testAction() (gas: 6313) TokenDistributorTest:testClaim() (gas: 5742744) diff --git a/src/L2ArbitrumGovernor.sol b/src/L2ArbitrumGovernor.sol index f08e2cb1..74916c74 100644 --- a/src/L2ArbitrumGovernor.sol +++ b/src/L2ArbitrumGovernor.sol @@ -41,6 +41,9 @@ contract L2ArbitrumGovernor is /// Note that Excluded Address is a readable name with no code of PK associated with it, and thus can't vote. address public constant EXCLUDE_ADDRESS = address(0xA4b86); + /// @notice Get the proposer of a proposal + mapping(uint256 => address) public proposalProposer; + constructor() { _disableInitializers(); } @@ -111,6 +114,37 @@ contract L2ArbitrumGovernor is AddressUpgradeable.functionCallWithValue(target, data, value); } + /** + * @notice Cancel a proposal. Can only be called by the proposer. + */ + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public virtual returns (uint256 proposalId) { + proposalId = hashProposal(targets, values, calldatas, descriptionHash); + require(state(proposalId) == ProposalState.Pending, "L2ArbitrumGovernor: too late to cancel"); + require(_msgSender() == proposalProposer[proposalId], "L2ArbitrumGovernor: not proposer"); + _cancel(targets, values, calldatas, descriptionHash); + } + + /** + * @dev See {IGovernor-propose}. This function has opt-in frontrunning protection, described in {_isValidDescriptionForProposer}. + */ + function propose( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + string memory description + ) public virtual override(IGovernorUpgradeable, GovernorUpgradeable) returns (uint256) { + address _proposer = _msgSender(); + require(_isValidDescriptionForProposer(_proposer, description), "L2ArbitrumGovernor: proposer restricted"); + uint256 proposalId = GovernorUpgradeable.propose(targets, values, calldatas, description); + proposalProposer[proposalId] = _proposer; + return proposalId; + } + /// @notice returns l2 executor address; used internally for onlyGovernance check function _executor() internal @@ -227,7 +261,101 @@ contract L2ArbitrumGovernor is override(GovernorUpgradeable, GovernorTimelockControlUpgradeable) returns (bool) { - return GovernorTimelockControlUpgradeable.supportsInterface(interfaceId); + bytes4 governorCancelId = this.cancel.selector ^ this.proposalProposer.selector; + return interfaceId == governorCancelId || GovernorTimelockControlUpgradeable.supportsInterface(interfaceId); + } + + /** + * @dev This project uses OZ v4.7.3, which has a vulnerability in proposal cancellation that was patched in v4.9.1. + * + * For details on the vulnerability, see here: + * https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-5h3x-9wvq-w4m2 + * + * _isValidDescriptionForProposer and its usage in propose is a backport of the fix in OZ v4.9.1, which can be found here: + * https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/commit/66f390fa516b550838e2c2f65132b5bc2afe1ced + * Check if the proposer is authorized to submit a proposal with the given description. + * + * If the proposal description ends with `#proposer=0x???`, where `0x???` is an address written as a hex string + * (case insensitive), then the submission of this proposal will only be authorized to said address. + * + * This is used for frontrunning protection. By adding this pattern at the end of their proposal, one can ensure + * that no other address can submit the same proposal. An attacker would have to either remove or change that part, + * which would result in a different proposal id. + * + * If the description does not match this pattern, it is unrestricted and anyone can submit it. This includes: + * - If the `0x???` part is not a valid hex string. + * - If the `0x???` part is a valid hex string, but does not contain exactly 40 hex digits. + * - If it ends with the expected suffix followed by newlines or other whitespace. + * - If it ends with some other similar suffix, e.g. `#other=abc`. + * - If it does not end with any such suffix. + */ + function _isValidDescriptionForProposer( + address proposer_, + string memory description + ) internal view virtual returns (bool) { + uint256 len = bytes(description).length; + + // Length is too short to contain a valid proposer suffix + if (len < 52) { + return true; + } + + // Extract what would be the `#proposer=0x` marker beginning the suffix + bytes12 marker; + assembly { + // - Start of the string contents in memory = description + 32 + // - First character of the marker = len - 52 + // - Length of "#proposer=0x0000000000000000000000000000000000000000" = 52 + // - We read the memory word starting at the first character of the marker: + // - (description + 32) + (len - 52) = description + (len - 20) + // - Note: Solidity will ignore anything past the first 12 bytes + marker := mload(add(description, sub(len, 20))) + } + + // If the marker is not found, there is no proposer suffix to check + if (marker != bytes12("#proposer=0x")) { + return true; + } + + // Parse the 40 characters following the marker as uint160 + uint160 recovered = 0; + for (uint256 i = len - 40; i < len; ++i) { + (bool isHex, uint8 value) = _tryHexToUint(bytes(description)[i]); + // If any of the characters is not a hex digit, ignore the suffix entirely + if (!isHex) { + return true; + } + recovered = (recovered << 4) | value; + } + + return recovered == uint160(proposer_); + } + + /** + * from https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/commit/66f390fa516b550838e2c2f65132b5bc2afe1ced + * @dev Try to parse a character from a string as a hex value. Returns `(true, value)` if the char is in + * `[0-9a-fA-F]` and `(false, 0)` otherwise. Value is guaranteed to be in the range `0 <= value < 16` + */ + function _tryHexToUint(bytes1 char) private pure returns (bool, uint8) { + uint8 c = uint8(char); + unchecked { + // Case 0-9 + if (47 < c && c < 58) { + return (true, c - 48); + } + // Case A-F + else if (64 < c && c < 71) { + return (true, c - 55); + } + // Case a-f + else if (96 < c && c < 103) { + return (true, c - 87); + } + // Else: not a hex char + else { + return (false, 0); + } + } } /** @@ -235,5 +363,5 @@ contract L2ArbitrumGovernor is * variables without shifting down storage in the inheritance chain. * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps */ - uint256[50] private __gap; + uint256[49] private __gap; } diff --git a/src/interfaces/IL2ArbitrumGovernor.sol b/src/interfaces/IL2ArbitrumGovernor.sol index 75cdf721..96379e72 100644 --- a/src/interfaces/IL2ArbitrumGovernor.sol +++ b/src/interfaces/IL2ArbitrumGovernor.sol @@ -16,4 +16,11 @@ interface IL2ArbitrumGoverner { function owner() external view returns (address); function proposalThreshold() external view returns (uint256); function setProposalThreshold(uint256 newProposalThreshold) external; + function proposalProposer(uint256 proposalId) external view returns (address); + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) external; } diff --git a/test/signatures/L2ArbitrumGovernor b/test/signatures/L2ArbitrumGovernor index 630d36fe..5796fd15 100644 --- a/test/signatures/L2ArbitrumGovernor +++ b/test/signatures/L2ArbitrumGovernor @@ -3,6 +3,7 @@ "COUNTING_MODE()": "dd4e2ba5", "EXCLUDE_ADDRESS()": "5e12ebbd", "EXTENDED_BALLOT_TYPEHASH()": "2fe3e261", + "cancel(address[],uint256[],bytes[],bytes32)": "452115d6", "castVote(uint256,uint8)": "56781388", "castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)": "3bccf4fd", "castVoteWithReason(uint256,uint8,string)": "7b3c71d3", @@ -23,6 +24,7 @@ "owner()": "8da5cb5b", "proposalDeadline(uint256)": "c01f9e37", "proposalEta(uint256)": "ab58fb8e", + "proposalProposer(uint256)": "143489d0", "proposalSnapshot(uint256)": "2d63f693", "proposalThreshold()": "b58131b0", "proposalVotes(uint256)": "544ffc9c", diff --git a/test/storage/L2ArbitrumGovernor b/test/storage/L2ArbitrumGovernor index e6965aa8..34288fe3 100644 --- a/test/storage/L2ArbitrumGovernor +++ b/test/storage/L2ArbitrumGovernor @@ -32,4 +32,5 @@ | __gap | uint256[48] | 556 | 0 | 1536 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor | | _owner | address | 604 | 0 | 20 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor | | __gap | uint256[49] | 605 | 0 | 1568 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor | -| __gap | uint256[50] | 654 | 0 | 1600 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor | +| proposalProposer | mapping(uint256 => address) | 654 | 0 | 32 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor | +| __gap | uint256[49] | 655 | 0 | 1568 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |