From dfdbc364c24c8934ef67f582167d9ef15ab9d3cf Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Wed, 7 Aug 2024 14:33:21 +0300 Subject: [PATCH 1/2] naming checkSenderIs --- contracts/DualGovernance.sol | 24 ++++++++-------- contracts/EmergencyProtectedTimelock.sol | 28 +++++++++---------- contracts/Escrow.sol | 12 ++++---- contracts/TimelockedGovernance.sol | 13 ++++----- contracts/libraries/EmergencyProtection.sol | 14 ++++------ contracts/libraries/Proposers.sol | 12 -------- contracts/libraries/Tiebreaker.sol | 6 ++-- contracts/libraries/TimelockState.sol | 6 ++-- test/unit/libraries/EmergencyProtection.t.sol | 20 ------------- 9 files changed, 50 insertions(+), 85 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index 3f3869be..f50be565 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -146,7 +146,7 @@ contract DualGovernance is IDualGovernance { } function setConfigProvider(IDualGovernanceConfigProvider newConfigProvider) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _setConfigProvider(newConfigProvider); /// @dev the minAssetsLockDuration is kept as a storage variable in the signalling Escrow instance @@ -185,12 +185,12 @@ contract DualGovernance is IDualGovernance { // --- function registerProposer(address proposer, address executor) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _proposers.register(proposer, executor); } function unregisterProposer(address proposer) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _proposers.unregister(TIMELOCK.getAdminExecutor(), proposer); } @@ -215,35 +215,35 @@ contract DualGovernance is IDualGovernance { // --- function addTiebreakerSealableWithdrawalBlocker(address sealableWithdrawalBlocker) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _tiebreaker.addSealableWithdrawalBlocker(sealableWithdrawalBlocker, MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT); } function removeTiebreakerSealableWithdrawalBlocker(address sealableWithdrawalBlocker) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _tiebreaker.removeSealableWithdrawalBlocker(sealableWithdrawalBlocker); } function setTiebreakerCommittee(address tiebreakerCommittee) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _tiebreaker.setTiebreakerCommittee(tiebreakerCommittee); } function setTiebreakerActivationTimeout(Duration tiebreakerActivationTimeout) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _tiebreaker.setTiebreakerActivationTimeout( MIN_TIEBREAKER_ACTIVATION_TIMEOUT, tiebreakerActivationTimeout, MAX_TIEBREAKER_ACTIVATION_TIMEOUT ); } function tiebreakerResumeSealable(address sealable) external { - _tiebreaker.checkTiebreakerCommittee(msg.sender); + _tiebreaker.checkSenderIsTiebreakerCommittee(); _tiebreaker.checkTie(_stateMachine.getCurrentState(), _stateMachine.getNormalOrVetoCooldownStateExitedAt()); RESEAL_MANAGER.resume(sealable); } function tiebreakerScheduleProposal(uint256 proposalId) external { - _tiebreaker.checkTiebreakerCommittee(msg.sender); + _tiebreaker.checkSenderIsTiebreakerCommittee(); _tiebreaker.checkTie(_stateMachine.getCurrentState(), _stateMachine.getNormalOrVetoCooldownStateExitedAt()); TIMELOCK.schedule(proposalId); } @@ -293,9 +293,9 @@ contract DualGovernance is IDualGovernance { emit ConfigProviderSet(newConfigProvider); } - function _checkAdminExecutor(address account) internal view { - if (TIMELOCK.getAdminExecutor() != account) { - revert InvalidAdminExecutor(account); + function _checkSenderIsAdminExecutor() internal view { + if (TIMELOCK.getAdminExecutor() != msg.sender) { + revert InvalidAdminExecutor(msg.sender); } } } diff --git a/contracts/EmergencyProtectedTimelock.sol b/contracts/EmergencyProtectedTimelock.sol index 4cfc16ef..f08babb2 100644 --- a/contracts/EmergencyProtectedTimelock.sol +++ b/contracts/EmergencyProtectedTimelock.sol @@ -73,7 +73,7 @@ contract EmergencyProtectedTimelock is ITimelock { /// @param calls An array of `ExternalCall` structs representing the calls to be executed. /// @return newProposalId The ID of the newly created proposal. function submit(address executor, ExternalCall[] calldata calls) external returns (uint256 newProposalId) { - _timelockState.checkGovernance(msg.sender); + _timelockState.checkSenderIsGovernance(); newProposalId = _proposals.submit(executor, calls); } @@ -81,7 +81,7 @@ contract EmergencyProtectedTimelock is ITimelock { /// Only the governance contract can call this function. /// @param proposalId The ID of the proposal to be scheduled. function schedule(uint256 proposalId) external { - _timelockState.checkGovernance(msg.sender); + _timelockState.checkSenderIsGovernance(); _proposals.schedule(proposalId, _timelockState.getAfterSubmitDelay()); } @@ -96,7 +96,7 @@ contract EmergencyProtectedTimelock is ITimelock { /// @dev Cancels all non-executed proposals. /// Only the governance contract can call this function. function cancelAllNonExecutedProposals() external { - _timelockState.checkGovernance(msg.sender); + _timelockState.checkSenderIsGovernance(); _proposals.cancelAll(); } @@ -105,12 +105,12 @@ contract EmergencyProtectedTimelock is ITimelock { // --- function setGovernance(address newGovernance) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _timelockState.setGovernance(newGovernance); } function setDelays(Duration afterSubmitDelay, Duration afterScheduleDelay) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _timelockState.setAfterSubmitDelay(afterSubmitDelay, MAX_AFTER_SUBMIT_DELAY); _timelockState.setAfterScheduleDelay(afterScheduleDelay, MAX_AFTER_SCHEDULE_DELAY); } @@ -120,7 +120,7 @@ contract EmergencyProtectedTimelock is ITimelock { /// @param executor The address of the executor contract. /// @param owner The address of the new owner. function transferExecutorOwnership(address executor, address owner) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); IOwnable(executor).transferOwnership(owner); } @@ -135,7 +135,7 @@ contract EmergencyProtectedTimelock is ITimelock { Timestamp emergencyProtectionEndDate, Duration emergencyModeDuration ) external { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); _emergencyProtection.setEmergencyGovernance(emergencyGovernance); _emergencyProtection.setEmergencyActivationCommittee(emergencyActivationCommittee); @@ -149,7 +149,7 @@ contract EmergencyProtectedTimelock is ITimelock { /// @dev Activates the emergency mode. /// Only the activation committee can call this function. function activateEmergencyMode() external { - _emergencyProtection.checkEmergencyActivationCommittee(msg.sender); + _emergencyProtection.checkSenderIsEmergencyActivationCommittee(); _emergencyProtection.checkEmergencyMode({isActive: false}); _emergencyProtection.activateEmergencyMode(); } @@ -159,7 +159,7 @@ contract EmergencyProtectedTimelock is ITimelock { /// @param proposalId The ID of the proposal to be executed. function emergencyExecute(uint256 proposalId) external { _emergencyProtection.checkEmergencyMode({isActive: true}); - _emergencyProtection.checkEmergencyExecutionCommittee(msg.sender); + _emergencyProtection.checkSenderIsEmergencyExecutionCommittee(); _proposals.execute({proposalId: proposalId, afterScheduleDelay: Duration.wrap(0)}); } @@ -168,7 +168,7 @@ contract EmergencyProtectedTimelock is ITimelock { function deactivateEmergencyMode() external { _emergencyProtection.checkEmergencyMode({isActive: true}); if (!_emergencyProtection.isEmergencyModeDurationPassed()) { - _checkAdminExecutor(msg.sender); + _checkSenderIsAdminExecutor(); } _emergencyProtection.deactivateEmergencyMode(); _proposals.cancelAll(); @@ -177,7 +177,7 @@ contract EmergencyProtectedTimelock is ITimelock { /// @dev Resets the system after entering the emergency mode. /// Only the execution committee can call this function. function emergencyReset() external { - _emergencyProtection.checkEmergencyExecutionCommittee(msg.sender); + _emergencyProtection.checkSenderIsEmergencyExecutionCommittee(); _emergencyProtection.checkEmergencyMode({isActive: true}); _emergencyProtection.deactivateEmergencyMode(); @@ -278,9 +278,9 @@ contract EmergencyProtectedTimelock is ITimelock { return _proposals.canSchedule(proposalId, _timelockState.getAfterSubmitDelay()); } - function _checkAdminExecutor(address account) internal view { - if (account != _ADMIN_EXECUTOR) { - revert InvalidAdminExecutor(account); + function _checkSenderIsAdminExecutor() internal view { + if (msg.sender != _ADMIN_EXECUTOR) { + revert InvalidAdminExecutor(msg.sender); } } } diff --git a/contracts/Escrow.sol b/contracts/Escrow.sol index 8e5bece7..5a67d896 100644 --- a/contracts/Escrow.sol +++ b/contracts/Escrow.sol @@ -124,7 +124,7 @@ contract Escrow is IEscrow { if (address(this) == _SELF) { revert NonProxyCallsForbidden(); } - _checkDualGovernance(msg.sender); + _checkSenderIsDualGovernance(); _escrowState.initialize(minAssetsLockDuration); @@ -243,7 +243,7 @@ contract Escrow is IEscrow { // --- function startRageQuit(Duration rageQuitExtensionDelay, Duration rageQuitWithdrawalsTimelock) external { - _checkDualGovernance(msg.sender); + _checkSenderIsDualGovernance(); _escrowState.startRageQuit(rageQuitExtensionDelay, rageQuitWithdrawalsTimelock); _batchesQueue.open(); } @@ -316,7 +316,7 @@ contract Escrow is IEscrow { // --- function setMinAssetsLockDuration(Duration newMinAssetsLockDuration) external { - _checkDualGovernance(msg.sender); + _checkSenderIsDualGovernance(); _escrowState.setMinAssetsLockDuration(newMinAssetsLockDuration); } @@ -422,9 +422,9 @@ contract Escrow is IEscrow { } } - function _checkDualGovernance(address account) internal view { - if (account != address(DUAL_GOVERNANCE)) { - revert InvalidDualGovernance(account); + function _checkSenderIsDualGovernance() internal view { + if (msg.sender != address(DUAL_GOVERNANCE)) { + revert InvalidDualGovernance(msg.sender); } } } diff --git a/contracts/TimelockedGovernance.sol b/contracts/TimelockedGovernance.sol index 3d597967..2cbca5a4 100644 --- a/contracts/TimelockedGovernance.sol +++ b/contracts/TimelockedGovernance.sol @@ -26,7 +26,7 @@ contract TimelockedGovernance is IGovernance { /// @param calls An array of ExternalCall structs representing the calls to be executed in the proposal. /// @return proposalId The ID of the submitted proposal. function submitProposal(ExternalCall[] calldata calls) external returns (uint256 proposalId) { - _checkGovernance(msg.sender); + _checkSenderIsGovernance(); return TIMELOCK.submit(TIMELOCK.getAdminExecutor(), calls); } @@ -51,15 +51,14 @@ contract TimelockedGovernance is IGovernance { /// @dev Cancels all pending proposals that have not been executed. function cancelAllPendingProposals() external { - _checkGovernance(msg.sender); + _checkSenderIsGovernance(); TIMELOCK.cancelAllNonExecutedProposals(); } - /// @dev Checks if the given account is the governance address. - /// @param account The address to check. - function _checkGovernance(address account) internal view { - if (account != GOVERNANCE) { - revert NotGovernance(account); + /// @dev Checks if the msg.sender is the governance address. + function _checkSenderIsGovernance() internal view { + if (msg.sender != GOVERNANCE) { + revert NotGovernance(msg.sender); } } } diff --git a/contracts/libraries/EmergencyProtection.sol b/contracts/libraries/EmergencyProtection.sol index 5f7db838..c3f72b98 100644 --- a/contracts/libraries/EmergencyProtection.sol +++ b/contracts/libraries/EmergencyProtection.sol @@ -133,19 +133,17 @@ library EmergencyProtection { /// @dev Checks if the caller is the emergency activator and reverts if not. /// @param self The storage reference to the Context struct. - /// @param account The account to check. - function checkEmergencyActivationCommittee(Context storage self, address account) internal view { - if (self.emergencyActivationCommittee != account) { - revert InvalidEmergencyActivationCommittee(account); + function checkSenderIsEmergencyActivationCommittee(Context storage self) internal view { + if (self.emergencyActivationCommittee != msg.sender) { + revert InvalidEmergencyActivationCommittee(msg.sender); } } /// @dev Checks if the caller is the emergency enactor and reverts if not. /// @param self The storage reference to the Context struct. - /// @param account The account to check. - function checkEmergencyExecutionCommittee(Context storage self, address account) internal view { - if (self.emergencyExecutionCommittee != account) { - revert InvalidEmergencyExecutionCommittee(account); + function checkSenderIsEmergencyExecutionCommittee(Context storage self) internal view { + if (self.emergencyExecutionCommittee != msg.sender) { + revert InvalidEmergencyExecutionCommittee(msg.sender); } } diff --git a/contracts/libraries/Proposers.sol b/contracts/libraries/Proposers.sol index 747b706a..b45577d7 100644 --- a/contracts/libraries/Proposers.sol +++ b/contracts/libraries/Proposers.sol @@ -140,18 +140,6 @@ library Proposers { } } - /// @dev Checks if an account is a registered proposer assigned to the expected executor and reverts if not. - /// @param self The storage state of the Proposers library. - /// @param account The address of the proposer. - /// @param executor The address of the expected executor. - function checkExecutor(State storage self, address account, address executor) internal view { - checkProposer(self, account); - ExecutorData memory executorData = self.executors[account]; - if (executor != executorData.executor) { - revert NotAssignedExecutor(account, executor, executorData.executor); - } - } - /// @dev Checks if an account is an admin proposer and reverts if not. /// @param self The storage state of the Proposers library. /// @param adminExecutor The address of the admin executor. diff --git a/contracts/libraries/Tiebreaker.sol b/contracts/libraries/Tiebreaker.sol index 383b14e7..91c6db90 100644 --- a/contracts/libraries/Tiebreaker.sol +++ b/contracts/libraries/Tiebreaker.sol @@ -102,9 +102,9 @@ library Tiebreaker { // Checks // --- - function checkTiebreakerCommittee(Context storage self, address account) internal view { - if (account != self.tiebreakerCommittee) { - revert InvalidTiebreakerCommittee(account); + function checkSenderIsTiebreakerCommittee(Context storage self) internal view { + if (msg.sender != self.tiebreakerCommittee) { + revert InvalidTiebreakerCommittee(msg.sender); } } diff --git a/contracts/libraries/TimelockState.sol b/contracts/libraries/TimelockState.sol index 8478f2f9..656e6ecc 100644 --- a/contracts/libraries/TimelockState.sol +++ b/contracts/libraries/TimelockState.sol @@ -78,9 +78,9 @@ library TimelockState { emit AfterScheduleDelaySet(newAfterScheduleDelay); } - function checkGovernance(Context storage self, address account) internal view { - if (self.governance != account) { - revert InvalidGovernance(account); + function checkSenderIsGovernance(Context storage self) internal view { + if (self.governance != msg.sender) { + revert InvalidGovernance(msg.sender); } } } diff --git a/test/unit/libraries/EmergencyProtection.t.sol b/test/unit/libraries/EmergencyProtection.t.sol index c286ed6e..523068f7 100644 --- a/test/unit/libraries/EmergencyProtection.t.sol +++ b/test/unit/libraries/EmergencyProtection.t.sol @@ -328,20 +328,10 @@ contract EmergencyProtectionUnitTests is UnitTest { vm.expectRevert( abi.encodeWithSelector(EmergencyProtection.InvalidEmergencyActivationCommittee.selector, [stranger]) ); - _emergencyProtection.checkEmergencyActivationCommittee(stranger); - _emergencyProtection.checkEmergencyActivationCommittee(address(0)); - Duration protectionDuration = Durations.from(100 seconds); Duration emergencyModeDuration = Durations.from(100 seconds); _setup(_emergencyGovernance, committee, address(0x2), protectionDuration, emergencyModeDuration); - - _emergencyProtection.checkEmergencyActivationCommittee(committee); - - vm.expectRevert( - abi.encodeWithSelector(EmergencyProtection.InvalidEmergencyActivationCommittee.selector, [stranger]) - ); - _emergencyProtection.checkEmergencyActivationCommittee(stranger); } function testFuzz_check_execution_committee(address committee, address stranger) external { @@ -351,20 +341,10 @@ contract EmergencyProtectionUnitTests is UnitTest { vm.expectRevert( abi.encodeWithSelector(EmergencyProtection.InvalidEmergencyExecutionCommittee.selector, [stranger]) ); - _emergencyProtection.checkEmergencyExecutionCommittee(stranger); - _emergencyProtection.checkEmergencyExecutionCommittee(address(0)); - Duration protectionDuration = Durations.from(100 seconds); Duration emergencyModeDuration = Durations.from(100 seconds); _setup(_emergencyGovernance, address(0x1), committee, protectionDuration, emergencyModeDuration); - - _emergencyProtection.checkEmergencyExecutionCommittee(committee); - - vm.expectRevert( - abi.encodeWithSelector(EmergencyProtection.InvalidEmergencyExecutionCommittee.selector, [stranger]) - ); - _emergencyProtection.checkEmergencyExecutionCommittee(stranger); } function test_check_emergency_mode_active() external { From 186e368a3971594221361e7e1426c6d29a66b1ae Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Wed, 7 Aug 2024 14:42:37 +0300 Subject: [PATCH 2/2] naming checkSenderIs --- contracts/DualGovernance.sol | 4 +-- contracts/libraries/Proposers.sol | 20 +++++++------- test/unit/libraries/EmergencyProtection.t.sol | 26 ------------------- 3 files changed, 11 insertions(+), 39 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index f50be565..e336eb03 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -104,7 +104,7 @@ contract DualGovernance is IDualGovernance { function submitProposal(ExternalCall[] calldata calls) external returns (uint256 proposalId) { _stateMachine.activateNextState(_configProvider.getDualGovernanceConfig(), ESCROW_MASTER_COPY); - _proposers.checkProposer(msg.sender); + _proposers.checkSenderIsProposer(); if (!_stateMachine.canSubmitProposal()) { revert ProposalSubmissionBlocked(); } @@ -123,7 +123,7 @@ contract DualGovernance is IDualGovernance { } function cancelAllPendingProposals() external { - _proposers.checkAdminProposer(TIMELOCK.getAdminExecutor(), msg.sender); + _proposers.checkSenderIsAdminProposer(TIMELOCK.getAdminExecutor()); TIMELOCK.cancelAllNonExecutedProposals(); } diff --git a/contracts/libraries/Proposers.sol b/contracts/libraries/Proposers.sol index b45577d7..0d93f9f8 100644 --- a/contracts/libraries/Proposers.sol +++ b/contracts/libraries/Proposers.sol @@ -131,23 +131,21 @@ library Proposers { return self.executorRefsCounts[account] > 0; } - /// @dev Checks if an account is a registered proposer and reverts if not. + /// @dev Checks if msg.sender is a registered proposer and reverts if not. /// @param self The storage state of the Proposers library. - /// @param account The address to check. - function checkProposer(State storage self, address account) internal view { - if (!isProposer(self, account)) { - revert NotProposer(account); + function checkSenderIsProposer(State storage self) internal view { + if (!isProposer(self, msg.sender)) { + revert NotProposer(msg.sender); } } - /// @dev Checks if an account is an admin proposer and reverts if not. + /// @dev Checks if msg.sender is an admin proposer and reverts if not. /// @param self The storage state of the Proposers library. /// @param adminExecutor The address of the admin executor. - /// @param account The address to check. - function checkAdminProposer(State storage self, address adminExecutor, address account) internal view { - checkProposer(self, account); - if (!isAdminProposer(self, adminExecutor, account)) { - revert NotAdminProposer(account); + function checkSenderIsAdminProposer(State storage self, address adminExecutor) internal view { + checkSenderIsProposer(self); + if (!isAdminProposer(self, adminExecutor, msg.sender)) { + revert NotAdminProposer(msg.sender); } } } diff --git a/test/unit/libraries/EmergencyProtection.t.sol b/test/unit/libraries/EmergencyProtection.t.sol index 523068f7..5a2dabcb 100644 --- a/test/unit/libraries/EmergencyProtection.t.sol +++ b/test/unit/libraries/EmergencyProtection.t.sol @@ -321,32 +321,6 @@ contract EmergencyProtectionUnitTests is UnitTest { assertEq(_emergencyProtection.isEmergencyProtectionEnabled(), false); } - function testFuzz_check_activation_committee(address committee, address stranger) external { - vm.assume(committee != address(0)); - vm.assume(stranger != address(0) && stranger != committee); - - vm.expectRevert( - abi.encodeWithSelector(EmergencyProtection.InvalidEmergencyActivationCommittee.selector, [stranger]) - ); - Duration protectionDuration = Durations.from(100 seconds); - Duration emergencyModeDuration = Durations.from(100 seconds); - - _setup(_emergencyGovernance, committee, address(0x2), protectionDuration, emergencyModeDuration); - } - - function testFuzz_check_execution_committee(address committee, address stranger) external { - vm.assume(committee != address(0)); - vm.assume(stranger != address(0) && stranger != committee); - - vm.expectRevert( - abi.encodeWithSelector(EmergencyProtection.InvalidEmergencyExecutionCommittee.selector, [stranger]) - ); - Duration protectionDuration = Durations.from(100 seconds); - Duration emergencyModeDuration = Durations.from(100 seconds); - - _setup(_emergencyGovernance, address(0x1), committee, protectionDuration, emergencyModeDuration); - } - function test_check_emergency_mode_active() external { vm.expectRevert(abi.encodeWithSelector(EmergencyProtection.InvalidEmergencyModeState.selector, [true])); _emergencyProtection.checkEmergencyMode(true);