diff --git a/packages/contracts-bedrock/src/Safe/LivenessModule.sol b/packages/contracts-bedrock/src/Safe/LivenessModule.sol index a8c4f136d0c81995ab24c3574814f964b221977a..66d802306adb3215b27a54d02ae2f82b242e1b9f 100644 --- a/packages/contracts-bedrock/src/Safe/LivenessModule.sol +++ b/packages/contracts-bedrock/src/Safe/LivenessModule.sol @@ -98,6 +98,14 @@ contract LivenessModule is ISemver { fallbackOwner_ = FALLBACK_OWNER; } + /// @notice Checks if the owner can be removed + /// @param _owner The owner to be removed + /// @return canRemove_ bool indicating if the owner can be removed + function canRemove(address _owner) public view returns (bool canRemove_) { + require(SAFE.isOwner(_owner), "LivenessModule: the owner to remove must be an owner of the Safe"); + canRemove_ = LIVENESS_GUARD.lastLive(_owner) + LIVENESS_INTERVAL < block.timestamp; + } + /// @notice This function can be called by anyone to remove a set of owners that have not signed a transaction /// during the liveness interval. If the number of owners drops below the minimum, then all owners /// must be removed. @@ -111,7 +119,7 @@ contract LivenessModule is ISemver { for (uint256 i = 0; i < _previousOwners.length; i++) { ownersCount--; if (ownersCount >= MIN_OWNERS) { - require(_canRemove(_ownersToRemove[i]), "LivenessModule: the owner to remove has signed recently"); + require(canRemove(_ownersToRemove[i]), "LivenessModule: the owner to remove has signed recently"); } _removeOwner({ @@ -170,13 +178,6 @@ contract LivenessModule is ISemver { ); } - /// @notice Checks if the owner can be removed - /// @param _owner The owner to be removed - /// @return canRemove_ bool indicating if the owner can be removed - function _canRemove(address _owner) internal view returns (bool canRemove_) { - canRemove_ = LIVENESS_GUARD.lastLive(_owner) + LIVENESS_INTERVAL < block.timestamp; - } - /// @notice A FREI-PI invariant check enforcing requirements on number of owners and threshold. function _verifyFinalState() internal view { address[] memory owners = SAFE.getOwners(); diff --git a/packages/contracts-bedrock/test/LivenessModule.t.sol b/packages/contracts-bedrock/test/LivenessModule.t.sol index 71734cd9a80dbdb8e5afebd97cc22cd1918966a9..de0cb36d85c5c2597f722234ea66673cb19270df 100644 --- a/packages/contracts-bedrock/test/LivenessModule.t.sol +++ b/packages/contracts-bedrock/test/LivenessModule.t.sol @@ -105,6 +105,24 @@ contract LivenessModule_Getters_Test is LivenessModule_TestInit { } } +contract LivenessModule_CanRemove_TestFail is LivenessModule_TestInit { + /// @dev Tests if canRemove work correctly + function test_canRemove_notSafeOwner_reverts() external { + address nonOwner = makeAddr("nonOwner"); + vm.expectRevert("LivenessModule: the owner to remove must be an owner of the Safe"); + livenessModule.canRemove(nonOwner); + } +} + +contract LivenessModule_CanRemove_Test is LivenessModule_TestInit { + /// @dev Tests if canRemove work correctly + function test_canRemove_works() external { + _warpPastLivenessInterval(); + bool canRemove = livenessModule.canRemove(safeInstance.owners[0]); + assertTrue(canRemove); + } +} + contract LivenessModule_Get75PercentThreshold_Test is LivenessModule_TestInit { /// @dev check the return values of the get75PercentThreshold function against manually /// calculated values.