Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addContender by signature only #232

Merged
merged 15 commits into from
Dec 28, 2023
45 changes: 23 additions & 22 deletions .gas-snapshot
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
AIP1Point2ActionTest:testAction() (gas: 629328)
AIPXActionTest:testAction() (gas: 8153)
ArbitrumDAOConstitutionTest:testConstructor() (gas: 259383)
ArbitrumDAOConstitutionTest:testMonOwnerCannotSetHash() (gas: 262836)
ArbitrumDAOConstitutionTest:testOwnerCanSetHash() (gas: 261148)
Expand All @@ -25,7 +26,7 @@ ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16008435)
ArbitrumVestingWalletTest:testDoesDeploy() (gas: 15971342)
ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16008649)
ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16074917)
E2E:testE2E() (gas: 83632522)
E2E:testE2E() (gas: 84736756)
FixedDelegateErc20WalletTest:testInit() (gas: 5822575)
FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 5816805)
FixedDelegateErc20WalletTest:testTransfer() (gas: 5932218)
Expand Down Expand Up @@ -93,11 +94,11 @@ L2GovernanceFactoryTest:testSanityCheckValues() (gas: 28415658)
L2GovernanceFactoryTest:testSetMinDelay() (gas: 28364371)
L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 28417242)
L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 28657360)
L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 30363122)
L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 30367353)
L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 25498765)
L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 30365353)
L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 30384446)
L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 30534838)
L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 30539025)
L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 25670482)
L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 30537069)
L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 30556162)
OutboxActionsTest:testAddOutbxesAction() (gas: 651591)
OutboxActionsTest:testCantAddEOA() (gas: 969161)
OutboxActionsTest:testCantReAddOutbox() (gas: 974559)
Expand Down Expand Up @@ -142,7 +143,7 @@ SecurityCouncilMemberElectionGovernorTest:testOnlyNomineeElectionGovernorCanProp
SecurityCouncilMemberElectionGovernorTest:testProperInitialization() (gas: 49388)
SecurityCouncilMemberElectionGovernorTest:testProposeReverts() (gas: 32916)
SecurityCouncilMemberElectionGovernorTest:testRelay() (gas: 42229)
SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340004, ~: 339432)
SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 339963, ~: 339326)
SecurityCouncilMemberElectionGovernorTest:testSelectTopNomineesFails() (gas: 273335)
SecurityCouncilMemberElectionGovernorTest:testSetFullWeightDuration() (gas: 34951)
SecurityCouncilMemberElectionGovernorTest:testVotesToWeight() (gas: 152898)
Expand Down Expand Up @@ -176,21 +177,21 @@ SecurityCouncilMemberSyncActionTest:testRemoveOne() (gas: 7930546)
SecurityCouncilMemberSyncActionTest:testUpdateCohort() (gas: 8171934)
SecurityCouncilMemberSyncActionTest:testUpdateCohort() (gas: 8172795)
SecurityCouncilMgmtUtilsTests:testIsInArray() (gas: 2102)
SecurityCouncilNomineeElectionGovernorTest:testAddContender() (gas: 215127)
SecurityCouncilNomineeElectionGovernorTest:testCastBySig() (gas: 318348)
SecurityCouncilNomineeElectionGovernorTest:testCastBySigTwice() (gas: 281704)
SecurityCouncilNomineeElectionGovernorTest:testCastVoteReverts() (gas: 35255)
SecurityCouncilNomineeElectionGovernorTest:testCountVote() (gas: 542695)
SecurityCouncilNomineeElectionGovernorTest:testCreateElection() (gas: 253049)
SecurityCouncilNomineeElectionGovernorTest:testExcludeNominee() (gas: 431856)
SecurityCouncilNomineeElectionGovernorTest:testExecute() (gas: 671552)
SecurityCouncilNomineeElectionGovernorTest:testForceSupport() (gas: 176798)
SecurityCouncilNomineeElectionGovernorTest:testIncludeNominee() (gas: 644396)
SecurityCouncilNomineeElectionGovernorTest:testInvalidInit() (gas: 7095881)
SecurityCouncilNomineeElectionGovernorTest:testAddContender() (gas: 314166)
SecurityCouncilNomineeElectionGovernorTest:testCastBySig() (gas: 334321)
SecurityCouncilNomineeElectionGovernorTest:testCastBySigTwice() (gas: 297176)
SecurityCouncilNomineeElectionGovernorTest:testCastVoteReverts() (gas: 35256)
SecurityCouncilNomineeElectionGovernorTest:testCountVote() (gas: 584101)
SecurityCouncilNomineeElectionGovernorTest:testCreateElection() (gas: 253095)
SecurityCouncilNomineeElectionGovernorTest:testExcludeNominee() (gas: 457070)
SecurityCouncilNomineeElectionGovernorTest:testExecute() (gas: 677169)
SecurityCouncilNomineeElectionGovernorTest:testForceSupport() (gas: 195298)
SecurityCouncilNomineeElectionGovernorTest:testIncludeNominee() (gas: 674563)
SecurityCouncilNomineeElectionGovernorTest:testInvalidInit() (gas: 7267578)
SecurityCouncilNomineeElectionGovernorTest:testProperInitialization() (gas: 78091)
SecurityCouncilNomineeElectionGovernorTest:testProposeFails() (gas: 19744)
SecurityCouncilNomineeElectionGovernorTest:testRelay() (gas: 42365)
SecurityCouncilNomineeElectionGovernorTest:testSetNomineeVetter() (gas: 40019)
SecurityCouncilNomineeElectionGovernorTest:testProposeFails() (gas: 19811)
SecurityCouncilNomineeElectionGovernorTest:testRelay() (gas: 42431)
SecurityCouncilNomineeElectionGovernorTest:testSetNomineeVetter() (gas: 39909)
SequencerActionsTest:testAddAndRemoveSequencer() (gas: 483356)
SequencerActionsTest:testCantAddZeroAddress() (gas: 235614)
SetInitialGovParamsActionTest:testL1() (gas: 259904)
Expand Down Expand Up @@ -231,7 +232,7 @@ TokenDistributorTest:testZeroDelegateTo() (gas: 4132733)
TokenDistributorTest:testZeroOwner() (gas: 4132646)
TokenDistributorTest:testZeroReceiver() (gas: 4132675)
TokenDistributorTest:testZeroToken() (gas: 71889)
TopNomineesGasTest:testTopNomineesGas() (gas: 4502786)
TopNomineesGasTest:testTopNomineesGas() (gas: 4502996)
UpgradeExecRouteBuilderTest:testAIP1Point2() (gas: 1322645)
UpgradeExecRouteBuilderTest:testRouteBuilderErrors() (gas: 1127374)
UpgradeExecutorTest:testAdminCanChangeExecutor() (gas: 2583801)
Expand Down
2 changes: 1 addition & 1 deletion foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ via_ir = false
solc_version = '0.8.16'

[profile.sec_council_mgmt]
optimizer_runs = 1500
optimizer_runs = 750

[fmt]
number_underscore = 'thousands'
Expand Down
2 changes: 1 addition & 1 deletion hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const solidityProfiles = {
settings: {
optimizer: {
enabled: true,
runs: 1500
runs: 750
},
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ contract SecurityCouncilNomineeElectionGovernor is
error QuorumNumeratorTooLow(uint256 quorumNumeratorValue);
error CastVoteDisabled();
error LastMemberElectionNotExecuted(uint256 prevProposalId);
error InvalidSignature();
error Deprecated(string message);

constructor() {
_disableInitializers();
Expand Down Expand Up @@ -208,18 +210,22 @@ contract SecurityCouncilNomineeElectionGovernor is
}
}

/// @notice Put `msg.sender` up for nomination. Must be called before a contender can receive votes.
/// Contenders are expected to control an address than can create a signature that would be a
/// recognised by a Gnosis Safe. They need to be able to do this with this same address on each of the
/// chains where the Security Council is active. It is expected that the nominee vetter will check this
/// during the vetting phase and exclude any contenders which dont meet this criteria.
/// @notice Put `contender` up for nomination. Must be called before a contender can receive votes.
/// @param proposalId The id of the proposal
/// @param contender The address of the contender
/// @param signature EIP712 `AddContenderMessage(uint256 proposalId,address contender)` signed by `contender`
/// @dev Can be called only while a proposal is pending (after proposal created but before voting phase)
/// A contender cannot be a member of the opposite cohort.
function addContender(uint256 proposalId) external {
DZGoldman marked this conversation as resolved.
Show resolved Hide resolved
function addContender(uint256 proposalId, address contender, bytes calldata signature) external {
address signer = recoverAddContenderMessage(proposalId, contender, signature);
if (signer == address(0) || signer != contender) {
revert InvalidSignature();
}

ElectionInfo storage election = _elections[proposalId];

if (election.isContender[msg.sender]) {
revert AlreadyContender(msg.sender);
if (election.isContender[signer]) {
revert AlreadyContender(signer);
}

ProposalState state_ = state(proposalId);
Expand All @@ -233,13 +239,13 @@ contract SecurityCouncilNomineeElectionGovernor is
// this check then is only a relevant check when the elections are running as expected - one at a time,
// every 6 months. Updates to the sec council manager using methods other than replaceCohort can effect this check
// and it's expected that the entity making those updates understands this.
if (securityCouncilManager.cohortIncludes(otherCohort(), msg.sender)) {
revert AccountInOtherCohort(otherCohort(), msg.sender);
if (securityCouncilManager.cohortIncludes(otherCohort(), signer)) {
revert AccountInOtherCohort(otherCohort(), signer);
}

election.isContender[msg.sender] = true;
election.isContender[signer] = true;

emit ContenderAdded(proposalId, msg.sender);
emit ContenderAdded(proposalId, signer);
}

/// @notice Allows the owner to change the nomineeVetter
Expand Down Expand Up @@ -423,6 +429,16 @@ contract SecurityCouncilNomineeElectionGovernor is
return _elections[proposalId].isContender[possibleContender];
}

/// @notice Recover EIP712 signature for `AddContenderMessage`
function recoverAddContenderMessage(uint256 proposalId, address contender, bytes calldata signature) public view returns (address) {
bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(
keccak256("AddContenderMessage(uint256 proposalId,address contender)"),
proposalId,
contender
)));
return ECDSAUpgradeable.recover(digest, signature);
}

/// @notice Always reverts.
/// @dev `GovernorUpgradeable` function to create a proposal overridden to just revert.
/// We only want proposals to be created via `createElection`.
Expand Down Expand Up @@ -475,6 +491,13 @@ contract SecurityCouncilNomineeElectionGovernor is
);
}

/// @notice Deprecated, use `addContender(uint256 proposalId, bytes calldata signature)` instead
/// @dev This function is deprecated because contenders should only be EOA's that can produce signatures.
/// If a security council member's address is not an EOA, then they may be unable to sign on all relevant chains.
function addContender(uint256) external pure {
DZGoldman marked this conversation as resolved.
Show resolved Hide resolved
revert Deprecated("addContender(uint256 proposalId) has been deprecated. Use addContender(uint256 proposalId, bytes calldata signature) instead");
}

/**
* @dev This empty reserved space is put in place to allow future versions to add new
* variables without shifting down storage in the inheritance chain.
Expand Down
44 changes: 24 additions & 20 deletions test/security-council-mgmt/E2E.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import "@gnosis.pm/safe-contracts/contracts/proxies/GnosisSafeProxyFactory.sol";
import "../../src/gov-action-contracts/address-registries/L2AddressRegistry.sol";
import "../util/DeployGnosisWithModule.sol";
import "../../src/security-council-mgmt/Common.sol";
import "./governors/SecurityCouncilNomineeElectionGovernor.t.sol";

contract ArbSysMock {
event ArbSysL2ToL1Tx(address from, address to, uint256 value, bytes data);
Expand Down Expand Up @@ -94,24 +95,26 @@ contract E2E is Test, DeployGnosisWithModule {
L2ArbitrumToken l2TokenLogic = new L2ArbitrumToken();

// owners
address member1 = address(637);
address member2 = address(638);
address member3 = address(639);
address member4 = address(640);
address member5 = address(641);
address member6 = address(642);
address member7 = address(643);
address member8 = address(644);
address member9 = address(645);
address member10 = address(646);
address member11 = address(647);
address member12 = address(648);
address member13 = address(649);
address member14 = address(650);
address member15 = address(651);
address member16 = address(652);
address member17 = address(653);
address member18 = address(654);
address member1 = vm.addr(637);
address member2 = vm.addr(638);
address member3 = vm.addr(639);
address member4 = vm.addr(640);
address member5 = vm.addr(641);
address member6 = vm.addr(642);
address member7 = vm.addr(643);
address member8 = vm.addr(644);
address member9 = vm.addr(645);
address member10 = vm.addr(646);
address member11 = vm.addr(647);
address member12 = vm.addr(648);
address member13 = vm.addr(649);
address member14 = vm.addr(650);
address member15 = vm.addr(651);
address member16 = vm.addr(652);
address member17 = vm.addr(653);
address member18 = vm.addr(654);



address[] members = [
member1,
Expand Down Expand Up @@ -449,9 +452,10 @@ contract E2E is Test, DeployGnosisWithModule {
uint256 propId = vars.secDeployedContracts.nomineeElectionGovernor.createElection();

// put contenders up for election
SigUtils sigUtils = new SigUtils(address(vars.secDeployedContracts.nomineeElectionGovernor));
for (uint256 i = 0; i < newCohort1.length; i++) {
vm.prank(newCohort1[i]);
vars.secDeployedContracts.nomineeElectionGovernor.addContender(propId);
uint256 pk = 649 + i; // member 13 - 18 priv keys
vars.secDeployedContracts.nomineeElectionGovernor.addContender(propId, vm.addr(pk), sigUtils.signAddContenderMessage(propId, pk));
}

// vote for them
Expand Down
Loading