diff --git a/.gas-snapshot b/.gas-snapshot index 3e19288e..07f6545b 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -27,7 +27,7 @@ ArbitrumVestingWalletTest:testDoesDeploy() (gas: 15971357) ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16008664) ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16074932) CancelTimelockAndRemoveMemberActionTest:testAction() (gas: 8159) -E2E:testE2E() (gas: 85744277) +E2E:testE2E() (gas: 86029203) FixedDelegateErc20WalletTest:testInit() (gas: 5822585) FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 5816815) FixedDelegateErc20WalletTest:testTransfer() (gas: 5932228) @@ -95,14 +95,14 @@ L2GovernanceFactoryTest:testSanityCheckValues() (gas: 28415658) L2GovernanceFactoryTest:testSetMinDelay() (gas: 28364371) L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 28417242) L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 28657360) -L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 31486267) -L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 31490498) -L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 26573276) -L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 31488498) -L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 31509665) +L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 31757348) +L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 31761579) +L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 26844357) +L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 31759579) +L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 31781044) NomineeGovernorV2UpgradeActionTest:testAction() (gas: 8153) OfficeHoursActionTest:testConstructor() (gas: 9050) -OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317068, ~: 317184) +OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317076, ~: 317184) OfficeHoursActionTest:testInvalidConstructorParameters() (gas: 235740) OfficeHoursActionTest:testPerformBeforeMinimumTimestamp() (gas: 8646) OfficeHoursActionTest:testPerformDuringOfficeHours() (gas: 9140) @@ -120,32 +120,34 @@ ProxyUpgradeAndCallActionTest:testUpgradeAndCall() (gas: 143042) RotateMembersUpgradeActionTest:testAction() (gas: 8153) SecurityCouncilManagerTest:testAddMemberAffordances() (gas: 253582) SecurityCouncilManagerTest:testAddMemberSpecialAddresses() (gas: 20770) -SecurityCouncilManagerTest:testAddMemberToFirstCohort() (gas: 348673) -SecurityCouncilManagerTest:testAddMemberToSecondCohort() (gas: 352108) +SecurityCouncilManagerTest:testAddMemberToFirstCohort() (gas: 348540) +SecurityCouncilManagerTest:testAddMemberToSecondCohort() (gas: 351975) SecurityCouncilManagerTest:testAddSC() (gas: 118742) SecurityCouncilManagerTest:testAddSCAffordances() (gas: 112296) -SecurityCouncilManagerTest:testCantUpdateCohortWithADup() (gas: 136614) +SecurityCouncilManagerTest:testCantUpdateCohortWithADup() (gas: 148550) SecurityCouncilManagerTest:testCohortMethods() (gas: 137958) -SecurityCouncilManagerTest:testInitialization() (gas: 206439) -SecurityCouncilManagerTest:testPostUpgradeInit() (gas: 4985090) -SecurityCouncilManagerTest:testRemoveMember() (gas: 217184) -SecurityCouncilManagerTest:testRemoveMemberAffordances() (gas: 101567) -SecurityCouncilManagerTest:testRemoveMemberRotated() (gas: 400541) -SecurityCouncilManagerTest:testRemoveSCAffordances() (gas: 81441) -SecurityCouncilManagerTest:testRemoveSeC() (gas: 38400) -SecurityCouncilManagerTest:testReplaceMemberAffordances() (gas: 216359) -SecurityCouncilManagerTest:testReplaceMemberInFirstCohort() (gas: 266389) -SecurityCouncilManagerTest:testReplaceMemberInFirstCohortAfterRotation() (gas: 448472) -SecurityCouncilManagerTest:testReplaceMemberInSecondCohort() (gas: 455694) -SecurityCouncilManagerTest:testReplaceMemberInSecondCohortAfterRotation() (gas: 269958) -SecurityCouncilManagerTest:testRotateMember() (gas: 606894) -SecurityCouncilManagerTest:testRotateMemberNotContender() (gas: 3815426) -SecurityCouncilManagerTest:testSetMinRotationPeriod() (gas: 65880) -SecurityCouncilManagerTest:testUpdateCohortAffordances() (gas: 83211) -SecurityCouncilManagerTest:testUpdateFirstCohort() (gas: 313581) -SecurityCouncilManagerTest:testUpdateRouter() (gas: 76385) -SecurityCouncilManagerTest:testUpdateRouterAffordances() (gas: 112474) -SecurityCouncilManagerTest:testUpdateSecondCohort() (gas: 313675) +SecurityCouncilManagerTest:testInitialization() (gas: 206820) +SecurityCouncilManagerTest:testPostUpgradeInit() (gas: 5256512) +SecurityCouncilManagerTest:testRemoveMember() (gas: 217162) +SecurityCouncilManagerTest:testRemoveMemberAffordances() (gas: 101612) +SecurityCouncilManagerTest:testRemoveMemberRotated() (gas: 422948) +SecurityCouncilManagerTest:testRemoveSCAffordances() (gas: 81486) +SecurityCouncilManagerTest:testRemoveSeC() (gas: 38435) +SecurityCouncilManagerTest:testReplaceCohortRotatingTo() (gas: 962400) +SecurityCouncilManagerTest:testReplaceMemberAffordances() (gas: 216337) +SecurityCouncilManagerTest:testReplaceMemberInFirstCohort() (gas: 266256) +SecurityCouncilManagerTest:testReplaceMemberInFirstCohortAfterRotation() (gas: 471153) +SecurityCouncilManagerTest:testReplaceMemberInSecondCohort() (gas: 478397) +SecurityCouncilManagerTest:testReplaceMemberInSecondCohortAfterRotation() (gas: 269847) +SecurityCouncilManagerTest:testRotateMember() (gas: 1014991) +SecurityCouncilManagerTest:testRotateMemberNotContender() (gas: 3868444) +SecurityCouncilManagerTest:testSetMinRotationPeriod() (gas: 65924) +SecurityCouncilManagerTest:testSetRotatingTo() (gas: 113076) +SecurityCouncilManagerTest:testUpdateCohortAffordances() (gas: 83230) +SecurityCouncilManagerTest:testUpdateFirstCohort() (gas: 327400) +SecurityCouncilManagerTest:testUpdateRouter() (gas: 76429) +SecurityCouncilManagerTest:testUpdateRouterAffordances() (gas: 112452) +SecurityCouncilManagerTest:testUpdateSecondCohort() (gas: 327504) SecurityCouncilMemberElectionGovernorTest:testCannotUseMoreVotesThanAvailable() (gas: 247018) SecurityCouncilMemberElectionGovernorTest:testCastBySig() (gas: 302873) SecurityCouncilMemberElectionGovernorTest:testCastBySigTwice() (gas: 266265) @@ -161,7 +163,7 @@ SecurityCouncilMemberElectionGovernorTest:testOnlyNomineeElectionGovernorCanProp SecurityCouncilMemberElectionGovernorTest:testProperInitialization() (gas: 49388) SecurityCouncilMemberElectionGovernorTest:testProposeReverts() (gas: 32916) SecurityCouncilMemberElectionGovernorTest:testRelay() (gas: 42229) -SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340116, ~: 339918) +SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340069, ~: 339927) SecurityCouncilMemberElectionGovernorTest:testSelectTopNomineesFails() (gas: 273467) SecurityCouncilMemberElectionGovernorTest:testSetFullWeightDuration() (gas: 34951) SecurityCouncilMemberElectionGovernorTest:testVotesToWeight() (gas: 152898) diff --git a/src/security-council-mgmt/SecurityCouncilManager.sol b/src/security-council-mgmt/SecurityCouncilManager.sol index ec35295d..ce5e44b8 100644 --- a/src/security-council-mgmt/SecurityCouncilManager.sol +++ b/src/security-council-mgmt/SecurityCouncilManager.sol @@ -38,6 +38,7 @@ contract SecurityCouncilManager is event MemberRemoved(address indexed member, Cohort indexed cohort); event MemberReplaced(address indexed replacedMember, address indexed newMember, Cohort cohort); event MemberRotated(address indexed replacedAddress, address indexed newAddress, Cohort cohort); + event RotatingToSet(address indexed replacedAddress, address indexed newAddress); event SecurityCouncilAdded( address indexed securityCouncil, address indexed updateAction, @@ -90,6 +91,13 @@ contract SecurityCouncilManager is /// @inheritdoc ISecurityCouncilManager uint256 public minRotationPeriod; + /// @notice Store the address to be rotated to for new members in the future + /// @dev `rotatingTo[X] = Y` means if X is installed as a new member, Y will be installed instead + mapping(address => address) public rotatingTo; + + /// @notice Nonce used when setting rotatingTo or rotatedTo + mapping(address => uint256) public rotationNonce; + /// @notice The 712 name hash bytes32 public constant NAME_HASH = keccak256(bytes("SecurityCouncilManager")); /// @notice The 712 version hash @@ -109,8 +117,10 @@ contract SecurityCouncilManager is bytes32 public constant DOMAIN_TYPE_HASH = keccak256( "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" ); - bytes32 public constant TYPE_HASH = + bytes32 public constant ROTATE_MEMBER_TYPE_HASH = keccak256(bytes("rotateMember(address from, uint256 nonce)")); + bytes32 public constant SET_ROTATING_TO_TYPE_HASH = + keccak256(bytes("setRotatingTo(address from, uint256 nonce)")); constructor() { _disableInitializers(); @@ -205,8 +215,20 @@ contract SecurityCouncilManager is // delete the old cohort _cohort == Cohort.FIRST ? delete firstCohort : delete secondCohort; + address[] storage otherCohort = _cohort == Cohort.FIRST ? secondCohort : firstCohort; for (uint256 i = 0; i < _newCohort.length; i++) { + // we have to change the array so correct _newCohort can be emitted + address rotatingAddress = rotatingTo[_newCohort[i]]; + if (rotatingAddress != address(0)) { + // only replace if there is no clash + if ( + !SecurityCouncilMgmtUtils.isInArray(rotatingAddress, _newCohort) + && !SecurityCouncilMgmtUtils.isInArray(rotatingAddress, otherCohort) + ) { + _newCohort[i] = rotatingAddress; + } + } _addMemberToCohortArray(_newCohort[i], _cohort); } @@ -290,7 +312,14 @@ contract SecurityCouncilManager is /// @inheritdoc ISecurityCouncilManager function getRotateMemberHash(address from, uint256 nonce) public view returns (bytes32) { return ECDSAUpgradeable.toTypedDataHash( - _domainSeparatorV4(), keccak256(abi.encode(TYPE_HASH, from, nonce)) + _domainSeparatorV4(), keccak256(abi.encode(ROTATE_MEMBER_TYPE_HASH, from, nonce)) + ); + } + + /// @inheritdoc ISecurityCouncilManager + function getSetRotatingToHash(address from, uint256 nonce) public view returns (bytes32) { + return ECDSAUpgradeable.toTypedDataHash( + _domainSeparatorV4(), keccak256(abi.encode(SET_ROTATING_TO_TYPE_HASH, from, nonce)) ); } @@ -305,11 +334,12 @@ contract SecurityCouncilManager is { revert RotationTooSoon(msg.sender, lastRotatedTimestamp + minRotationPeriod); } - // we enforce that a the new address is an eoa in the same way do // in NomineeGovernor.addContender by requiring a signature - bytes32 digest = getRotateMemberHash(msg.sender, updateNonce); - address newAddress = ECDSAUpgradeable.recover(digest, signature); + uint256 currentRotationNonce = rotationNonce[msg.sender]; + address newAddress = ECDSAUpgradeable.recover( + getRotateMemberHash(msg.sender, currentRotationNonce), signature + ); // we safety check the new member address is the one that we expect to replace here // this isn't strictly necessary but it guards agains the case where the wrong sig is accidentally used if (newAddress != newMemberAddress) { @@ -372,10 +402,30 @@ contract SecurityCouncilManager is lastRotated[newAddress] = block.timestamp; rotatedTo[msg.sender] = newAddress; + rotationNonce[msg.sender] = currentRotationNonce + 1; Cohort cohort = _swapMembers(msg.sender, newAddress); emit MemberRotated({replacedAddress: msg.sender, newAddress: newAddress, cohort: cohort}); } + /// @inheritdoc ISecurityCouncilManager + function setRotatingTo(address newMemberAddress, bytes calldata signature) external { + uint256 currentRotationNonce = rotationNonce[msg.sender]; + // we enforce that a the new address is an eoa in the same way do + // in NomineeGovernor.addContender by requiring a signature + bytes32 digest = getSetRotatingToHash(msg.sender, currentRotationNonce); + address newAddress = ECDSAUpgradeable.recover(digest, signature); + // we safety check the new member address is the one that we expect to replace here + // this isn't strictly necessary but it guards against the case where the wrong sig is accidentally used + if (newAddress != newMemberAddress) { + revert InvalidNewAddress(newAddress); + } + + rotatingTo[msg.sender] = newAddress; + rotationNonce[msg.sender] = currentRotationNonce + 1; + + emit RotatingToSet({replacedAddress: msg.sender, newAddress: newAddress}); + } + function _swapMembers(address _addressToRemove, address _addressToAdd) internal returns (Cohort) @@ -608,5 +658,5 @@ contract SecurityCouncilManager is * variables without shifting down storage in the inheritance chain. * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps */ - uint256[40] private __gap; + uint256[38] private __gap; } diff --git a/src/security-council-mgmt/interfaces/ISecurityCouncilManager.sol b/src/security-council-mgmt/interfaces/ISecurityCouncilManager.sol index f42d3625..8376b884 100644 --- a/src/security-council-mgmt/interfaces/ISecurityCouncilManager.sol +++ b/src/security-council-mgmt/interfaces/ISecurityCouncilManager.sol @@ -46,8 +46,14 @@ interface ISecurityCouncilManager { error GovernorNotReplacer(); error NewMemberIsContender(uint256 proposalId, address newMember); error NewMemberIsNominee(uint256 proposalId, address newMember); + error NewMemberIsRotating(address newMember); + error NewMemberIsRotatingTarget(address newMember); error InvalidNewAddress(address newAddress); + function rotatedTo(address) external view returns (address); + function rotatingTo(address) external view returns (address); + function rotationNonce(address) external view returns (uint256); + /// @notice There is a minimum period between when an address can be rotated /// This is to ensure a single member cannot do many rotations in a row function minRotationPeriod() external view returns (uint256); @@ -109,24 +115,34 @@ interface ISecurityCouncilManager { /// @param _memberToReplace Security Council member to remove /// @param _newMember Security Council member to add in their place function replaceMember(address _memberToReplace, address _newMember) external; - /// @notice Get the hash to be signed for member rotation + /// @notice Get the hash to be signed for an existing member rotation /// @param from The address that will be rotated out. Included in the hash so that other members cant use this message to rotate their address - /// @param nonce The message nonce. Must be equal to the update nonce in the contract at the time of execution + /// @param nonce The message nonce. Must be equal to the rotationNonce for the member being rotated out function getRotateMemberHash(address from, uint256 nonce) external view returns (bytes32); /// @notice Security council member can rotate out their address for a new one /// @dev Initiates cross chain messages to update the individual Security Councils. /// Cannot rotate to a contender in an ongoing election, as this could cause a clash that would stop the election result executing - /// Since the signature is over the update nonce, it is understood that other updates can invalidate the signed message, however since - /// other updates are either from the council itself (trusted), the election (infrequent) or another member rotation (also infrequent due - /// to the minRotationPeriod) the invalidation cannot occur often and in those cases the member should sign a new rotation message /// @param newMemberAddress The new member address to be rotated to /// @param memberElectionGovernor The current member election governor - must have the COHORT_REPLACER_ROLE role - /// @param signature A signature from the new member address over the 712 addMember hash + /// @param signature A signature from the new member address over the 712 rotateMember hash function rotateMember( address newMemberAddress, address memberElectionGovernor, bytes calldata signature ) external; + /// @notice Get the hash to be signed for future member rotation + /// @param from The address that will be rotated out. This is included in the hash so that other members cant use this message to rotate their address + /// @param nonce The message nonce. Must be the from address's current rotationNonce + function getSetRotatingToHash(address from, uint256 nonce) external view returns (bytes32); + /// @notice Set an address to be rotated to if the sender is ever elected as a member + /// This enables unelected members to decide where their election address will update to. When a member is elected to the council they + /// are expected to have a high level of security on their member key. Election candidates may not have set up that high level of security before + /// registering their election key, so this method allows them to set up a new key that will be actually installed as the member upon election. + /// If this future rotation causes a clash, the rotation will not be executed and the original address will be installed + /// This rotation only applies to future replaceCohort, mainly used by the member election governor + /// @param newMemberAddress The new member address to be rotated to + /// @param signature A signature from the new member address over the 712 setRotatingTo hash + function setRotatingTo(address newMemberAddress, bytes calldata signature) external; /// @notice Is the account a member of the first cohort function firstCohortIncludes(address account) external view returns (bool); /// @notice Is the account a member of the second cohort diff --git a/test/gov-actions/CancelTimelockAndRemoveMemberActionTest.t.sol b/test/gov-actions/CancelTimelockAndRemoveMemberActionTest.t.sol index 3637dc02..aef54115 100644 --- a/test/gov-actions/CancelTimelockAndRemoveMemberActionTest.t.sol +++ b/test/gov-actions/CancelTimelockAndRemoveMemberActionTest.t.sol @@ -53,7 +53,7 @@ contract CancelTimelockAndRemoveMemberActionTest is Test { bytes memory sig; { (uint8 v, bytes32 r, bytes32 s) = - vm.sign(memberInKey, scm.getRotateMemberHash(memberOut, scm.updateNonce())); + vm.sign(memberInKey, scm.getRotateMemberHash(memberOut, scm.rotationNonce(memberOut))); sig = abi.encodePacked(r, s, v); } diff --git a/test/security-council-mgmt/SecurityCouncilManager.t.sol b/test/security-council-mgmt/SecurityCouncilManager.t.sol index da6f0a37..dc3f757a 100644 --- a/test/security-council-mgmt/SecurityCouncilManager.t.sol +++ b/test/security-council-mgmt/SecurityCouncilManager.t.sol @@ -41,17 +41,22 @@ contract MockArbitrumTimelock { } contract SecurityCouncilManagerTest is Test { + uint256 pknc1 = 9772; + address pkncAddr1 = vm.addr(pknc1); + uint256 pknc2 = 9773; + address pkncAddr2 = vm.addr(pknc2); + address[] firstCohort = new address[](6); address[6] _firstCohort = [address(1111), address(1112), address(1113), address(1114), address(1115), address(1116)]; address[] secondCohort = new address[](6); address[6] _secondCohort = - [address(2221), address(2222), address(2223), address(2224), address(2225), address(2226)]; + [address(2221), address(2222), address(2223), pkncAddr2, address(2225), address(2226)]; address[] newCohort = new address[](6); address[6] _newCohort = - [address(3331), address(3332), address(3333), address(3334), address(3335), address(3336)]; + [address(3331), address(3332), address(3333), address(3334), pkncAddr1, address(3336)]; address[] newCohortWithADup = new address[](6); address dup = address(3335); @@ -248,7 +253,7 @@ contract SecurityCouncilManagerTest is Test { function testRemoveMemberRotated() public { address memberToRemove = firstCohort[0]; - bytes32 digest = scm.getRotateMemberHash(memberToRemove, scm.updateNonce()); + bytes32 digest = scm.getRotateMemberHash(memberToRemove, scm.rotationNonce(memberToRemove)); bytes memory signature = sign(pk1, digest); vm.prank(memberToRemove); scm.rotateMember(memberToRotate1, memberElectionGovernor, signature); @@ -396,7 +401,7 @@ contract SecurityCouncilManagerTest is Test { } function testReplaceMemberInFirstCohortAfterRotation() public { - bytes32 digest = scm.getRotateMemberHash(firstCohort[0], scm.updateNonce()); + bytes32 digest = scm.getRotateMemberHash(firstCohort[0], scm.rotationNonce(firstCohort[0])); bytes memory signature = sign(pk1, digest); vm.prank(firstCohort[0]); scm.rotateMember(memberToRotate1, memberElectionGovernor, signature); @@ -423,7 +428,8 @@ contract SecurityCouncilManagerTest is Test { } function testReplaceMemberInSecondCohort() public { - bytes32 digest = scm.getRotateMemberHash(secondCohort[0], scm.updateNonce()); + bytes32 digest = + scm.getRotateMemberHash(secondCohort[0], scm.rotationNonce(secondCohort[0])); bytes memory signature = sign(pk2, digest); vm.prank(secondCohort[0]); scm.rotateMember(memberToRotate2, memberElectionGovernor, signature); @@ -492,18 +498,18 @@ contract SecurityCouncilManagerTest is Test { } function testRotateMember() public { - address originalMember = secondCohort[1]; + address originalMember = secondCohort[3]; uint256 startTime = 5678; vm.warp(startTime); - bytes32 digest = scm.getRotateMemberHash(originalMember, scm.updateNonce()); + bytes32 digest = scm.getRotateMemberHash(originalMember, scm.rotationNonce(originalMember)); bytes memory signature = sign(pk1, digest); - uint256 startNonce = scm.updateNonce(); + uint256 startNonce = scm.rotationNonce(originalMember); vm.expectRevert( abi.encodeWithSelector( ISecurityCouncilManager.InvalidNewAddress.selector, - 0x00e6008973a133b0e603275498f18321534c3721f3 + 0xf1ba0050017D0A16690e3Be7B4A2Ab75F22CFFA2 ) ); vm.prank(secondCohort[2]); @@ -518,16 +524,17 @@ contract SecurityCouncilManagerTest is Test { vm.recordLogs(); vm.prank(originalMember); scm.rotateMember(memberToRotate1, memberElectionGovernor, signature); - assertEq(startNonce + 1, scm.updateNonce(), "nonce 1"); + assertEq(startNonce + 1, scm.rotationNonce(originalMember), "nonce 1"); checkScheduleWasCalled(); - checkCohortChange(memberToRotate1, 1, secondCohort, Cohort.SECOND); + checkCohortChange(memberToRotate1, 3, secondCohort, Cohort.SECOND); assertTrue( TestUtil.areUniqueAddressArraysEqual(firstCohort, scm.getFirstCohort()), "first cohort untouched" ); assertEq(scm.lastRotated(memberToRotate1), startTime, "Member 1 last rotated"); - bytes32 digest1 = scm.getRotateMemberHash(memberToRotate1, scm.updateNonce()); + bytes32 digest1 = + scm.getRotateMemberHash(memberToRotate1, scm.rotationNonce(memberToRotate1)); bytes memory signature1 = sign(pk2, digest1); vm.expectRevert( @@ -554,12 +561,13 @@ contract SecurityCouncilManagerTest is Test { vm.warp(startTime + minRotationPeriod); + startNonce = scm.rotationNonce(memberToRotate1); vm.recordLogs(); vm.prank(memberToRotate1); scm.rotateMember(memberToRotate2, memberElectionGovernor, signature1); - assertEq(startNonce + 2, scm.updateNonce(), "nonce 2"); + assertEq(startNonce + 1, scm.rotationNonce(memberToRotate1), "nonce 2"); checkScheduleWasCalled(); - checkCohortChange(memberToRotate2, 1, secondCohort, Cohort.SECOND); + checkCohortChange(memberToRotate2, 3, secondCohort, Cohort.SECOND); assertTrue( TestUtil.areUniqueAddressArraysEqual(firstCohort, scm.getFirstCohort()), "first cohort untouched 2" @@ -567,6 +575,41 @@ contract SecurityCouncilManagerTest is Test { assertEq( scm.lastRotated(memberToRotate2), startTime + minRotationPeriod, "Member 2 last rotated" ); + + // now do another rotation from the original address - we rotate back to it then away from it - this tests the nonce updates + signature1 = sign( + pknc2, scm.getRotateMemberHash(memberToRotate2, scm.rotationNonce(memberToRotate2)) + ); + vm.warp(startTime + 2 * minRotationPeriod); + vm.recordLogs(); + vm.prank(memberToRotate2); + scm.rotateMember(pkncAddr2, memberElectionGovernor, signature1); + checkScheduleWasCalled(); + + assertTrue( + TestUtil.areUniqueAddressArraysEqual(firstCohort, scm.getFirstCohort()), + "first cohort untouched 1" + ); + assertTrue( + TestUtil.areUniqueAddressArraysEqual(secondCohort, scm.getSecondCohort()), + "first cohort back to original" + ); + + signature1 = + sign(pk1, scm.getRotateMemberHash(originalMember, scm.rotationNonce(originalMember))); + vm.warp(startTime + 3 * minRotationPeriod); + startNonce = scm.rotationNonce(originalMember); + vm.recordLogs(); + vm.prank(originalMember); + scm.rotateMember(memberToRotate1, memberElectionGovernor, signature1); + checkScheduleWasCalled(); + assertEq(startNonce + 1, scm.rotationNonce(originalMember), "nonce 3"); + + assertTrue( + TestUtil.areUniqueAddressArraysEqual(firstCohort, scm.getFirstCohort()), + "first cohort untouched 1" + ); + checkCohortChange(memberToRotate1, 3, secondCohort, Cohort.SECOND); } function addAllContendersAndVote(uint256 proposalId) public { @@ -613,20 +656,19 @@ contract SecurityCouncilManagerTest is Test { vm.warp(startTime); // start an election and add a contender - SigUtils sigUtils = new SigUtils(nomineeElectionGovernor); uint256 proposalId = SecurityCouncilNomineeElectionGovernor( payable(nomineeElectionGovernor) ).createElection(); - bytes memory sig = sigUtils.signAddContenderMessage(proposalId, pk1); SecurityCouncilNomineeElectionGovernor(payable(nomineeElectionGovernor)).addContender( - proposalId, sig + proposalId, + new SigUtils(nomineeElectionGovernor).signAddContenderMessage(proposalId, pk1) ); - bytes32 digest = scm.getRotateMemberHash(originalMember, scm.updateNonce()); - bytes memory signature = sign(pk1, digest); + bytes memory signature = + sign(pk1, scm.getRotateMemberHash(originalMember, scm.rotationNonce(originalMember))); bytes memory signature2 = - sign(pk2, scm.getRotateMemberHash(originalMember, scm.updateNonce())); - uint256 startNonce = scm.updateNonce(); + sign(pk2, scm.getRotateMemberHash(originalMember, scm.rotationNonce(originalMember))); + uint256 startNonce = scm.rotationNonce(originalMember); // replace in other cohort in ongoing election does not work vm.expectRevert( @@ -686,20 +728,22 @@ contract SecurityCouncilManagerTest is Test { // replacing that member with one in the same cohort does work bytes memory signatureA = - sign(pk1, scm.getRotateMemberHash(firstCohort[1], scm.updateNonce())); + sign(pk1, scm.getRotateMemberHash(firstCohort[1], scm.rotationNonce(firstCohort[1]))); + startNonce = scm.rotationNonce(firstCohort[1]); vm.prank(firstCohort[1]); scm.rotateMember(memberToRotate1, memberElectionGovernor, signatureA); - assertEq(startNonce + 1, scm.updateNonce(), "nonce 1"); + assertEq(startNonce + 1, scm.rotationNonce(firstCohort[1]), "nonce 1"); checkCohortChange(memberToRotate1, 1, firstCohort, Cohort.FIRST); vm.revertTo(snap); bytes memory signature1 = - sign(pk2, scm.getRotateMemberHash(originalMember, scm.updateNonce())); + sign(pk2, scm.getRotateMemberHash(originalMember, scm.rotationNonce(originalMember))); + startNonce = scm.rotationNonce(originalMember); vm.recordLogs(); vm.prank(originalMember); scm.rotateMember(memberToRotate2, memberElectionGovernor, signature1); - assertEq(startNonce + 1, scm.updateNonce(), "nonce 1"); + assertEq(startNonce + 1, scm.rotationNonce(originalMember), "nonce 1"); checkScheduleWasCalled(); checkCohortChange(memberToRotate2, 1, secondCohort, Cohort.SECOND); assertTrue( @@ -877,6 +921,127 @@ contract SecurityCouncilManagerTest is Test { scm.replaceCohort(newCohortWithADup, Cohort.SECOND); } + function testReplaceCohortRotatingTo() public { + // set a rotatingTo for a member of the first cohort + address[] memory newCohortCopy = newCohort; + address rotatingFrom = newCohortCopy[1]; + bytes32 digest = scm.getSetRotatingToHash(rotatingFrom, scm.rotationNonce(rotatingFrom)); + bytes memory signature = sign(pk1, digest); + vm.prank(rotatingFrom); + scm.setRotatingTo(memberToRotate1, signature); + + vm.startPrank(roles.cohortUpdator); + vm.recordLogs(); + scm.replaceCohort(newCohortCopy, Cohort.FIRST); + checkScheduleWasCalled(); + vm.stopPrank(); + + newCohortCopy[1] = memberToRotate1; + + assertTrue( + TestUtil.areUniqueAddressArraysEqual(newCohortCopy, scm.getFirstCohort()), + "first cohort updated" + ); + + assertTrue( + TestUtil.areUniqueAddressArraysEqual(secondCohort, scm.getSecondCohort()), + "second cohort untouched" + ); + + rotatingFrom = newCohortCopy[2]; + digest = scm.getSetRotatingToHash(rotatingFrom, scm.rotationNonce(rotatingFrom)); + signature = sign(pknc1, digest); + vm.prank(rotatingFrom); + scm.setRotatingTo(pkncAddr1, signature); + + // set rotation to a member of the incoming cohort + vm.startPrank(roles.cohortUpdator); + vm.recordLogs(); + scm.replaceCohort(newCohortCopy, Cohort.FIRST); + checkScheduleWasCalled(); + vm.stopPrank(); + + // should still just equal the newcohort copy + assertTrue( + TestUtil.areUniqueAddressArraysEqual(newCohortCopy, scm.getFirstCohort()), + "first cohort updated" + ); + assertTrue( + TestUtil.areUniqueAddressArraysEqual(secondCohort, scm.getSecondCohort()), + "second cohort untouched" + ); + + // now try rotate to a member of the other cohort + rotatingFrom = newCohortCopy[2]; + digest = scm.getSetRotatingToHash(rotatingFrom, scm.rotationNonce(rotatingFrom)); + signature = sign(pknc2, digest); + vm.prank(rotatingFrom); + scm.setRotatingTo(pkncAddr2, signature); + + // set rotation to a member of the incoming cohort + vm.startPrank(roles.cohortUpdator); + vm.recordLogs(); + scm.replaceCohort(newCohortCopy, Cohort.FIRST); + checkScheduleWasCalled(); + vm.stopPrank(); + + // should still just equal the newcohort copy + assertTrue( + TestUtil.areUniqueAddressArraysEqual(newCohortCopy, scm.getFirstCohort()), + "first cohort updated" + ); + assertTrue( + TestUtil.areUniqueAddressArraysEqual(secondCohort, scm.getSecondCohort()), + "second cohort untouched" + ); + + // put it back to how we found it + vm.startPrank(roles.cohortUpdator); + vm.recordLogs(); + scm.replaceCohort(firstCohort, Cohort.FIRST); + checkScheduleWasCalled(); + vm.stopPrank(); + } + + event RotatingToSet(address indexed replacedAddress, address indexed newAddress); + + function testSetRotatingTo() public { + address testAddr = vm.addr(97_990); + + uint256 testPk1 = 45_678; + address addr1 = vm.addr(testPk1); + uint256 testPk2 = 45_679; + address addr2 = vm.addr(testPk2); + + assertEq(scm.rotatingTo(testAddr), address(0)); + assertEq(scm.rotationNonce(testAddr), 0); + + bytes32 digest = scm.getSetRotatingToHash(testAddr, scm.rotationNonce(testAddr)); + bytes memory signature = sign(testPk1, digest); + + vm.prank(testAddr); + vm.expectRevert( + abi.encodeWithSelector(ISecurityCouncilManager.InvalidNewAddress.selector, addr1) + ); + scm.setRotatingTo(address(1), signature); + + vm.prank(testAddr); + vm.expectEmit(true, true, true, true); + emit RotatingToSet({replacedAddress: testAddr, newAddress: addr1}); + scm.setRotatingTo(addr1, signature); + assertEq(scm.rotatingTo(testAddr), addr1); + assertEq(scm.rotationNonce(testAddr), 1); + + digest = scm.getSetRotatingToHash(testAddr, scm.rotationNonce(testAddr)); + signature = sign(testPk2, digest); + vm.prank(testAddr); + vm.expectEmit(true, true, true, true); + emit RotatingToSet({replacedAddress: testAddr, newAddress: addr2}); + scm.setRotatingTo(addr2, signature); + assertEq(scm.rotatingTo(testAddr), addr2); + assertEq(scm.rotationNonce(testAddr), 2); + } + function testUpdateRouterAffordances() public { UpgradeExecRouteBuilder newRouter = UpgradeExecRouteBuilder(TestUtil.deployStubContract()); vm.prank(rando); diff --git a/test/signatures/SecurityCouncilManager b/test/signatures/SecurityCouncilManager index 1d01fd5d..23616709 100644 --- a/test/signatures/SecurityCouncilManager +++ b/test/signatures/SecurityCouncilManager @@ -10,7 +10,8 @@ "MIN_ROTATION_PERIOD_SETTER_ROLE()": "5db9bf4e", "NAME_HASH()": "04622c2e", "RETRYABLE_TICKET_MAGIC()": "3994073d", - "TYPE_HASH()": "64d4c819", + "ROTATE_MEMBER_TYPE_HASH()": "aea6b1e7", + "SET_ROTATING_TO_TYPE_HASH()": "ff57aaed", "VERSION_HASH()": "9e4e7318", "addMember(address,uint8)": "62d0d1c3", "addSecurityCouncil((address,address,uint256))": "6eaff79e", @@ -24,6 +25,7 @@ "getRotateMemberHash(address,uint256)": "09af9e5f", "getScheduleUpdateInnerData(uint256)": "8bbd5149", "getSecondCohort()": "bdc9f17c", + "getSetRotatingToHash(address,uint256)": "2bf9dbfe", "grantRole(bytes32,address)": "2f2ff15d", "hasRole(bytes32,address)": "91d14854", "initialize(address[],address[],(address,address,uint256)[],(address,address,address,address[],address,address,address),address,address,uint256)": "caa33e24", @@ -39,11 +41,14 @@ "revokeRole(bytes32,address)": "d547741f", "rotateMember(address,address,bytes)": "02ea6df4", "rotatedTo(address)": "86bc77a3", + "rotatingTo(address)": "cd6150a4", + "rotationNonce(address)": "ac823694", "router()": "f887ea40", "secondCohortIncludes(address)": "e9d9f048", "securityCouncils(uint256)": "bef3f745", "securityCouncilsLength()": "7889acb2", "setMinRotationPeriod(uint256)": "d4c271b2", + "setRotatingTo(address,bytes)": "62cd078d", "setUpgradeExecRouteBuilder(address)": "0e5e43d7", "supportsInterface(bytes4)": "01ffc9a7", "updateNonce()": "0feca68a" diff --git a/test/storage/SecurityCouncilManager b/test/storage/SecurityCouncilManager index e60ff252..ffd9b0e5 100644 --- a/test/storage/SecurityCouncilManager +++ b/test/storage/SecurityCouncilManager @@ -34,6 +34,10 @@ |-------------------+--------------------------------------------------------------+------+--------+-------+-----------------------------------------------------------------------------| | minRotationPeriod | uint256 | 160 | 0 | 32 | src/security-council-mgmt/SecurityCouncilManager.sol:SecurityCouncilManager | |-------------------+--------------------------------------------------------------+------+--------+-------+-----------------------------------------------------------------------------| -| __gap | uint256[40] | 161 | 0 | 1280 | src/security-council-mgmt/SecurityCouncilManager.sol:SecurityCouncilManager | +| rotatingTo | mapping(address => address) | 161 | 0 | 32 | src/security-council-mgmt/SecurityCouncilManager.sol:SecurityCouncilManager | +|-------------------+--------------------------------------------------------------+------+--------+-------+-----------------------------------------------------------------------------| +| rotationNonce | mapping(address => uint256) | 162 | 0 | 32 | src/security-council-mgmt/SecurityCouncilManager.sol:SecurityCouncilManager | +|-------------------+--------------------------------------------------------------+------+--------+-------+-----------------------------------------------------------------------------| +| __gap | uint256[38] | 163 | 0 | 1216 | src/security-council-mgmt/SecurityCouncilManager.sol:SecurityCouncilManager | ╰-------------------+--------------------------------------------------------------+------+--------+-------+-----------------------------------------------------------------------------╯