From bc9a6d2b3a6c83e9cbf60dccdabf7922a9300068 Mon Sep 17 00:00:00 2001 From: Benjamin Halimic Date: Fri, 20 Jan 2023 11:07:37 +0100 Subject: [PATCH 1/5] Remove recovery smart contract --- .gas-snapshot | 35 +++----- script/DeployRecovery.s.sol | 5 +- src/IRecovery.sol | 114 ----------------------- src/IRecoveryModule.sol | 55 ++++++++++++ src/Recovery.sol | 175 ------------------------------------ src/RecoveryModule.sol | 153 +++++++++++++++++++++---------- src/T.sol | 15 ++++ test/Recovery.t.sol | 137 ---------------------------- test/RecoveryModule.t.sol | 109 +++++++++++----------- 9 files changed, 239 insertions(+), 559 deletions(-) delete mode 100644 src/IRecovery.sol delete mode 100644 src/Recovery.sol create mode 100644 src/T.sol delete mode 100644 test/Recovery.t.sol diff --git a/.gas-snapshot b/.gas-snapshot index cfceb74..a5a7cdb 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,23 +1,12 @@ -RecoveryModuleTest:testAddRecovery() (gas: 86490) -RecoveryModuleTest:testAddRecoveryModule() (gas: 80360) -RecoveryModuleTest:testAddRecoveryShouldRevert() (gas: 20492) -RecoveryModuleTest:testAddRecoveryShouldRevertOnInvalidAddress() (gas: 23035) -RecoveryModuleTest:testAddRecoveryWithoutSubscription() (gas: 44064) -RecoveryModuleTest:testClearRecovery() (gas: 38353) -RecoveryModuleTest:testRemoveRecoveryModule() (gas: 63596) -RecoveryModuleTest:testSubscriptionAmount(uint256) (runs: 256, μ: 15631, ~: 15894) -RecoveryModuleTest:testUpdateLastActivity() (gas: 106312) -RecoveryModuleTest:testUpdateLastActivityShouldFail() (gas: 16050) -RecoveryModuleTest:testCancelTransferOwnership() (gas: 161908) -RecoveryModuleTest:testDuplicateAddRecoveryShouldWork() (gas: 146115) -RecoveryModuleTest:testFailFinalizeOwnership() (gas: 14282) -RecoveryModuleTest:testFinalizeTransferOwnership() (gas: 204028) -RecoveryModuleTest:testFinalizeTransferOwnershipShouldRevert() (gas: 17285) -RecoveryModuleTest:testInactiveForShouldWork() (gas: 189607) -RecoveryModuleTest:testInactiveForTooEarly() (gas: 123639) -RecoveryModuleTest:testInitiateFinalizeTooEarlyShouldRevert() (gas: 147919) -RecoveryModuleTest:testInitiateTransferOwnership() (gas: 151898) -RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithAlreadyInitiated() (gas: 153909) -RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithInvalidAddress() (gas: 19488) -RecoveryModuleTest:testInitiateTransferOwnershipTooEarlyShouldRevert() (gas: 122070) -RecoveryModuleTest:testSetUp() (gas: 15617) +RecoveryModuleTest:testCancelTransferOwnership() (gas: 147567) +RecoveryModuleTest:testFailFinalizeOwnership() (gas: 10336) +RecoveryModuleTest:testFinalizeTransferOwnership() (gas: 207041) +RecoveryModuleTest:testFinalizeTransferOwnershipShouldRevert() (gas: 13383) +RecoveryModuleTest:testInactiveForShouldWork() (gas: 189844) +RecoveryModuleTest:testInactiveForTooEarly() (gas: 109414) +RecoveryModuleTest:testInitiateFinalizeTooEarlyShouldRevert() (gas: 133400) +RecoveryModuleTest:testInitiateTransferOwnership() (gas: 138872) +RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithAlreadyInitiated() (gas: 140950) +RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithInvalidAddress() (gas: 15564) +RecoveryModuleTest:testInitiateTransferOwnershipTooEarlyShouldRevert() (gas: 109042) +RecoveryModuleTest:testSetUp() (gas: 15573) diff --git a/script/DeployRecovery.s.sol b/script/DeployRecovery.s.sol index d843aa6..5306a82 100644 --- a/script/DeployRecovery.s.sol +++ b/script/DeployRecovery.s.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.13; import "forge-std/Script.sol"; -import "../src/Recovery.sol"; import "../src/RecoveryModule.sol"; contract DeployRecovery is Script { @@ -10,10 +9,8 @@ contract DeployRecovery is Script { uint256 deployerPrivateKey = vm.envUint("PK"); vm.startBroadcast(deployerPrivateKey); - Recovery recovery = new Recovery(); - RecoveryModule module = new RecoveryModule(address(recovery), 10 minutes); + RecoveryModule module = new RecoveryModule(10 minutes); - console2.log("Recovery deployed:", address(recovery)); console2.log("Safe module deployed:", address(module)); vm.stopBroadcast(); diff --git a/src/IRecovery.sol b/src/IRecovery.sol deleted file mode 100644 index 69d4191..0000000 --- a/src/IRecovery.sol +++ /dev/null @@ -1,114 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity >=0.7.0 <0.9.0; - -/// @title IRecovery interface -/// @author Benjamin H - -interface IRecovery { - enum RecoveryType { - After, - InactiveFor - } - - /// @notice Thrown when amount is not enough for a desired subscription - /// @param amountYouShouldPay amount you should pay for this subscription - /// Selector 0x0e2e0926 - error InvalidPayment(uint256 amountYouShouldPay); - - /// @notice Thrown when the recovery date is not valid - /// Selector 0x0ecc12af - error InvalidRecoveryDate(); - - /// @notice Thrown when the caller is unauthorized - /// Selector 0x82b42900 - error Unauthorized(); - - /// @notice Thrown when the recovery address iz address(0) - /// Or the recovery address is the first owner of safe - /// That is not possible with current implementation - /// Selector 0xa61421a2 - error InvalidRecoveryAddress(); - - /// @notice Emitted when the safe owner adds recovery data - /// @param safe is the address of a safe - /// @param recoveryAddress is the address to which safe ownership will eventually be transfered - /// @param recoveryDate is the recovery date timestamp (in seconds) that marks the start transfer ownership - /// @param recoveryType is the recovery type - event RecoveryAddressAdded( - address indexed safe, address indexed recoveryAddress, uint64 recoveryDate, RecoveryType recoveryType - ); - - /// @notice Emitted when the safe owner adds recovery data with subscription - /// @param safe is the address of a safe - /// @param recoveryAddress is the address to which safe ownership will eventually be transfered - /// @param recoveryDate is the recovery date timestamp (in seconds) that marks the start transfer ownership - /// @param recoveryType is the recovery type - event RecoveryAddressAddedWithSubscription( - address indexed safe, address indexed recoveryAddress, uint64 recoveryDate, RecoveryType recoveryType - ); - - /// @notice Emitted when the new recovery module is added to registry - /// @param module is the address of the new module - event RecoveryModuleAdded(address indexed module); - - /// @notice Emitted when the new recovery module is removed from the registry - /// @param module is the address of the new module - event RecoveryModuleRemoved(address indexed module); - - /// @notice Emitted when the safe owner clears his recovery data - /// @param safe is the address of a safe - event RecoveryDataCleared(address indexed safe); - - /// @notice Emitted when the owner changes subscription amount - /// @param amount is the new yearly subscription amount in wei - event SubscriptionAmountChanged(uint256 amount); - - /// @notice Adds recovery address and a recovery date - /// Safe is expected to be a caller - /// @param recoveryAddress is an address to which safe ownership will be transfered - /// @param recoveryDate is a timestamp (in seconds) in the future when the recovery process will start - function addRecovery(address recoveryAddress, uint40 recoveryDate, RecoveryType recoveryType) external; - - /// @notice Adds recovery address and a recovery date with subscription - /// Safe is expected to be a caller - /// @param recoveryAddress is an address to which safe ownership will be transfered - /// @param recoveryDate is a timestamp (in seconds) in the future when the recovery process will start - function addRecoveryWithSubscription(address recoveryAddress, uint40 recoveryDate, RecoveryType recoveryType) - external - payable; - - /// @notice Clears recovery data - /// @param safe is the address of the safe - function clearRecoveryDataFromModule(address safe) external; - - /// @notice Clears recovery data - /// Safe is expected to be a caller - function clearRecoveryData() external; - - /// @notice Updates the last activity of a safe - /// @param safe is the address of the safe - function updateLastActivity(address safe) external; - - /// @notice Gets the last activity of a safe - /// @param safe is the address of the safe - /// @return last activity timestamp (in seconds) - function getLastActivity(address safe) external view returns (uint256); - - /// @notice Returns recovery address for a `safe` - /// @param safe is the address of the safe - /// @return recovery address - function getRecoveryAddress(address safe) external view returns (address); - - /// @notice Returns recovery date for a `safe` - /// @param safe is the address of the safe - /// @return recovery date timestamp (in seconds) - function getRecoveryDate(address safe) external view returns (uint64); - - /// @notice Returns subscription amount in wei - /// @return amount in wei - function getSubscriptionAmount() external view returns (uint256); - - /// @notice Returns if the recovery module is whitelisted - /// @param module is the address of the module - /// @return bool - function isRecoveryModule(address module) external view returns (bool); -} diff --git a/src/IRecoveryModule.sol b/src/IRecoveryModule.sol index c23936c..2de37b8 100644 --- a/src/IRecoveryModule.sol +++ b/src/IRecoveryModule.sol @@ -6,6 +6,16 @@ import {GnosisSafe} from "safe-contracts/GnosisSafe.sol"; /// @title IRecoveryModule interface /// @author Benjamin H - interface IRecoveryModule { + enum RecoveryType { + After, + InactiveFor + } + + /// @notice Thrown when amount is not enough for a desired subscription + /// @param amountYouShouldPay amount you should pay for this subscription + /// Selector 0x0e2e0926 + error InvalidPayment(uint256 amountYouShouldPay); + /// @notice Thrown when the current time is lower than `timeLockExpiration` /// Selector 0x085de625 error TooEarly(); @@ -22,6 +32,25 @@ interface IRecoveryModule { /// Selector 0x77fcbe52 error TransferOwnershipAlreadyInitiated(); + /// @notice Thrown when the recovery address iz address(0) + /// Or the recovery address is the first owner of safe + /// That is not possible with current implementation + /// Selector 0xa61421a2 + error InvalidRecoveryAddress(); + + /// @notice Emitted when the safe owner adds recovery data + /// @param safe is the address of a safe + /// @param recoveryAddress is the address to which safe ownership will eventually be transfered + /// @param recoveryDate is the recovery date timestamp (in seconds) that marks the start transfer ownership + /// @param recoveryType is the recovery type + event RecoveryAddressAdded( + address indexed safe, address indexed recoveryAddress, uint64 recoveryDate, RecoveryType recoveryType + ); + + /// @notice Emitted when the safe owner clears his recovery data + /// @param safe is the address of a safe + event RecoveryDataCleared(address indexed safe); + /// @notice Emitted when the transfer ownership is initiated /// @param safe is the safe address /// @param timeLockExpiration is the timestamp (seconds) when the timelock expires @@ -50,4 +79,30 @@ interface IRecoveryModule { /// @param safe is the safe address /// @return timestamp in seconds function getTimelockExpiration(address safe) external view returns (uint256); + + /// @notice Gets the last activity of a safe + /// @param safe is the address of the safe + /// @return last activity timestamp (in seconds) + function getLastActivity(address safe) external view returns (uint256); + + /// @notice Gets the recovery type + /// @param safe is the address of the safe + /// @return recovery type + function getRecoveryType(address safe) external view returns (RecoveryType); + + /// @notice Returns recovery address for a `safe` + /// @param safe is the address of the safe + /// @return recovery address + function getRecoveryAddress(address safe) external view returns (address); + + /// @notice Returns recovery date for a `safe` + /// @param safe is the address of the safe + /// @return recovery date timestamp (in seconds) + function getRecoveryDate(address safe) external view returns (uint64); + + /// @notice Adds recovery address and a recovery date + /// Safe is expected to be a caller + /// @param recoveryAddress is an address to which safe ownership will be transfered + /// @param recoveryDate is a timestamp (in seconds) in the future when the recovery process will start + function addRecovery(address recoveryAddress, uint40 recoveryDate, RecoveryType recoveryType) external payable; } diff --git a/src/Recovery.sol b/src/Recovery.sol deleted file mode 100644 index 0fd7f90..0000000 --- a/src/Recovery.sol +++ /dev/null @@ -1,175 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.17; - -import {IRecovery} from "./IRecovery.sol"; -import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol"; -import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {GnosisSafe} from "safe-contracts/GnosisSafe.sol"; - -contract Recovery is IRecovery, AccessControl { - using EnumerableSet for EnumerableSet.AddressSet; - - bytes32 public constant MULTISIG = keccak256("MULTISIG"); - bytes32 public constant CREATOR = keccak256("CREATOR"); - - // Subscription amount used to support web2 and notification infrastructure - uint256 private _subscriptionAmount = 0.1 ether; - - struct RecoveryData { - address recoveryAddress; - uint40 recoveryDate; - RecoveryType recoveryType; - uint40 lastActivity; - } - - // [safe address] -> Recovery data - mapping(address => RecoveryData) private _recovery; - - EnumerableSet.AddressSet private _recoveryModules; - - constructor() { - _setupRole(CREATOR, msg.sender); - _setupRole(MULTISIG, msg.sender); - _setupRole(DEFAULT_ADMIN_ROLE, msg.sender); - } - - modifier onlyRecoveryModule() { - if (!_recoveryModules.contains(msg.sender)) { - revert Unauthorized(); - } - _; - } - - function addRecoveryModule(address module) external onlyRole(MULTISIG) { - _recoveryModules.add(module); - emit RecoveryModuleAdded(module); - } - - function removeRecoveryModule(address module) external onlyRole(MULTISIG) { - _recoveryModules.remove(module); - emit RecoveryModuleRemoved(module); - } - - /// @inheritdoc IRecovery - function addRecovery(address recoveryAddress, uint40 recoveryDate, RecoveryType recoveryType) external { - _validateRecoveryAddress(recoveryAddress); - - _recovery[msg.sender] = RecoveryData({ - recoveryAddress: recoveryAddress, - recoveryDate: recoveryDate, - recoveryType: recoveryType, - lastActivity: uint40(block.timestamp) - }); - - emit RecoveryAddressAdded(msg.sender, recoveryAddress, recoveryDate, recoveryType); - } - - /// @inheritdoc IRecovery - function addRecoveryWithSubscription(address recoveryAddress, uint40 recoveryDate, RecoveryType recoveryType) - external - payable - { - uint256 amount = calculatePaymentAmount(recoveryDate, _subscriptionAmount, recoveryType); - - if (msg.value != amount) { - revert InvalidPayment(amount); - } - - _validateRecoveryAddress(recoveryAddress); - - _recovery[msg.sender] = RecoveryData({ - recoveryAddress: recoveryAddress, - recoveryDate: recoveryDate, - recoveryType: recoveryType, - lastActivity: uint40(block.timestamp) - }); - - emit RecoveryAddressAddedWithSubscription(msg.sender, recoveryAddress, recoveryDate, recoveryType); - } - - /// @inheritdoc IRecovery - function updateLastActivity(address safe) external onlyRecoveryModule { - _recovery[safe].lastActivity = uint40(block.timestamp); - } - - /// @inheritdoc IRecovery - function clearRecoveryDataFromModule(address safe) external onlyRecoveryModule { - _clearRecoveryData(safe); - } - - /// @inheritdoc IRecovery - function clearRecoveryData() external { - _clearRecoveryData(msg.sender); - } - - /// @inheritdoc IRecovery - function getLastActivity(address safe) external view returns (uint256) { - return _recovery[safe].lastActivity; - } - - /// @inheritdoc IRecovery - function getRecoveryAddress(address safe) external view returns (address) { - return _recovery[safe].recoveryAddress; - } - - /// @inheritdoc IRecovery - function getRecoveryDate(address safe) external view returns (uint64) { - return _recovery[safe].recoveryDate; - } - - function getRecoveryType(address safe) external view returns (RecoveryType) { - return _recovery[safe].recoveryType; - } - - /// @inheritdoc IRecovery - function getSubscriptionAmount() external view returns (uint256) { - return _subscriptionAmount; - } - - /// @inheritdoc IRecovery - function isRecoveryModule(address module) external view returns (bool) { - return _recoveryModules.contains(module); - } - - /// @notice Sets subscription amount - /// @param amount in wei - function setSubscription(uint256 amount) external onlyRole(CREATOR) { - _subscriptionAmount = amount; - emit SubscriptionAmountChanged(amount); - } - - /// @notice Withdraws eth from the contract - /// @param amount to withdraw - /// @param to address - function withdrawFunds(uint256 amount, address to) external onlyRole(CREATOR) { - (bool success,) = to.call{value: amount}(""); - require(success); - } - - function _clearRecoveryData(address safe) private { - delete _recovery[safe]; - emit RecoveryDataCleared(safe); - } - - function _validateRecoveryAddress(address recoveryAddress) private pure { - if (recoveryAddress == address(0)) { - revert InvalidRecoveryAddress(); - } - } - - function calculatePaymentAmount(uint256 recoveryDate, uint256 subscriptionAmount, RecoveryType recoveryType) - public - view - returns (uint256) - { - if (recoveryType == RecoveryType.After) { - uint256 yearsOfSubscription = (recoveryDate - block.timestamp) / 365 days; - // +1 is because of solidity's rounding - return (yearsOfSubscription + 1) * subscriptionAmount; - } - - // +1 is because of solidity's rounding - uint256 monthsOfSubscription = recoveryDate / 30 days; - return (monthsOfSubscription + 1) * subscriptionAmount; - } -} diff --git a/src/RecoveryModule.sol b/src/RecoveryModule.sol index 07d2d09..07ee479 100644 --- a/src/RecoveryModule.sol +++ b/src/RecoveryModule.sol @@ -5,33 +5,83 @@ import {Enum} from "safe-contracts/common/Enum.sol"; import {GnosisSafe} from "safe-contracts/GnosisSafe.sol"; import {Guard} from "safe-contracts/base/GuardManager.sol"; import {OwnerManager} from "safe-contracts/base/OwnerManager.sol"; -import {Recovery} from "./Recovery.sol"; -import {IRecovery} from "./IRecovery.sol"; import {IRecoveryModule} from "./IRecoveryModule.sol"; /// @author Benjamin H - contract RecoveryModule is IRecoveryModule, Guard { - // Recovery Registry - Recovery public immutable recoveryRegistry; - // Timelock period for ownership transfer uint256 public immutable timeLock; + struct RecoveryData { + address recoveryAddress; + uint40 recoveryDate; + RecoveryType recoveryType; + uint40 lastActivity; + } + + uint256 private _subscriptionAmount = 0.1 ether; + + // [safe address] -> Recovery data + mapping(address => RecoveryData) private _recoveryData; + // Safe address -> timelockExpiration timestamp mapping(address => uint256) private _recovery; - constructor(address recoveryAddress, uint256 lock) { - recoveryRegistry = Recovery(recoveryAddress); + constructor(uint256 lock) { timeLock = lock; } + /// @inheritdoc IRecoveryModule + function addRecovery(address recoveryAddress, uint40 recoveryDate, RecoveryType recoveryType) external payable { + if (msg.value != _subscriptionAmount) { + revert InvalidPayment(_subscriptionAmount); + } + + if (recoveryAddress == address(0)) { + revert InvalidRecoveryAddress(); + } + + _recoveryData[msg.sender] = RecoveryData({ + recoveryAddress: recoveryAddress, + recoveryDate: recoveryDate, + recoveryType: recoveryType, + lastActivity: uint40(block.timestamp) + }); + + emit RecoveryAddressAdded(msg.sender, recoveryAddress, recoveryDate, recoveryType); + } + + function clearRecovery() external { + _clearRecovery(msg.sender); + } + + /// @inheritdoc IRecoveryModule + function initiateTransferOwnership(address safe) external { + // This is done to prevent somebody from extending timeLockExpiration value + if (_recovery[safe] != 0) { + revert TransferOwnershipAlreadyInitiated(); + } + + if (getRecoveryAddress(safe) == address(0)) { + revert InvalidAddress(); + } + + if (getRecoveryType(safe) == IRecoveryModule.RecoveryType.InactiveFor) { + _ensureSafeIsInactive(safe); + } else { + _ensureRecoveryDateHasPassed(safe); + } + + uint256 timeLockExpiration = block.timestamp + timeLock; + + _recovery[safe] = timeLockExpiration; + emit TransferOwnershipInitiated(safe, timeLockExpiration); + } + /// @inheritdoc IRecoveryModule function finalizeTransferOwnership(address safeAddress) external { - address newOwner = recoveryRegistry.getRecoveryAddress(safeAddress); - // If the new owner is not zero - // that means that the owner did not cancel transfer ownership - // we don't need to validate safe inactivity / or other dates - // just newOwner and that timelock expiration is passed + address newOwner = getRecoveryAddress(safeAddress); + // owner != address zero means that the owner did not cancel transfer ownership if (newOwner == address(0)) { revert InvalidAddress(); } @@ -40,8 +90,9 @@ contract RecoveryModule is IRecoveryModule, Guard { if (block.timestamp < getTimelockExpiration(safeAddress)) { revert TooEarly(); } - - recoveryRegistry.clearRecoveryDataFromModule(safeAddress); + + // Clear recovery data + _clearRecovery(safeAddress); delete _recovery[safeAddress]; GnosisSafe safe = GnosisSafe(payable(safeAddress)); @@ -79,40 +130,12 @@ contract RecoveryModule is IRecoveryModule, Guard { emit TransferOwnershipFinalized(safeAddress); } - /// @inheritdoc IRecoveryModule - function initiateTransferOwnership(address safe) external { - // This is done to prevent somebody from extending timeLockExpiration value - if (_recovery[safe] != 0) { - revert TransferOwnershipAlreadyInitiated(); - } - - if (recoveryRegistry.getRecoveryAddress(safe) == address(0)) { - revert InvalidAddress(); - } - - if (recoveryRegistry.getRecoveryType(safe) == IRecovery.RecoveryType.InactiveFor) { - _ensureSafeIsInactive(safe); - } else { - _ensureRecoveryDateHasPassed(safe); - } - - uint256 timeLockExpiration = block.timestamp + timeLock; - - _recovery[safe] = timeLockExpiration; - emit TransferOwnershipInitiated(safe, timeLockExpiration); - } - /// @inheritdoc IRecoveryModule function cancelTransferOwnership() external { delete _recovery[msg.sender]; emit TransferOwnershipCanceled(msg.sender); } - /// @inheritdoc IRecoveryModule - function getTimelockExpiration(address safe) public view returns (uint256) { - return _recovery[safe]; - } - function checkTransaction( address, uint256, @@ -129,21 +152,57 @@ contract RecoveryModule is IRecoveryModule, Guard { // do nothing, required by `Guard` interface } - /// @notice Required by `Guard` interface - /// Is used to record last activity of a Safe + /// @dev Required by `Guard` interface from Safe + /// Records last safe activity function checkAfterExecution(bytes32, bool) external { - recoveryRegistry.updateLastActivity(msg.sender); + _recoveryData[msg.sender].lastActivity = uint40(block.timestamp); + } + + function _clearRecovery(address safe) internal { + delete _recovery[safe]; + emit RecoveryDataCleared(safe); + } + + // View functions + + /// @inheritdoc IRecoveryModule + function getTimelockExpiration(address safe) public view returns (uint256) { + return _recovery[safe]; + } + + /// @inheritdoc IRecoveryModule + function getRecoveryAddress(address safe) public view returns (address) { + return _recoveryData[safe].recoveryAddress; + } + + /// @inheritdoc IRecoveryModule + function getRecoveryDate(address safe) public view returns (uint64) { + return _recoveryData[safe].recoveryDate; + } + + /// @inheritdoc IRecoveryModule + function getRecoveryType(address safe) public view returns (RecoveryType) { + return _recoveryData[safe].recoveryType; + } + + /// @inheritdoc IRecoveryModule + function getLastActivity(address safe) public view returns (uint256) { + return _recoveryData[safe].lastActivity; + } + + function getSubscriptionAmount() external view returns (uint256) { + return _subscriptionAmount; } function _ensureSafeIsInactive(address safe) private view { // Recovery date represents for how long the safe must be inactive to not revert - if (block.timestamp - recoveryRegistry.getLastActivity(safe) < recoveryRegistry.getRecoveryDate(safe)) { + if (block.timestamp - getLastActivity(safe) < getRecoveryDate(safe)) { revert TooEarly(); } } function _ensureRecoveryDateHasPassed(address safe) private view { - if (block.timestamp < recoveryRegistry.getRecoveryDate(safe)) { + if (block.timestamp < getRecoveryDate(safe)) { revert TooEarly(); } } diff --git a/src/T.sol b/src/T.sol new file mode 100644 index 0000000..b46314a --- /dev/null +++ b/src/T.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.17; + +contract T { + struct RecoveryData { + uint128 whatever; + uint128 lastActivity; + } + + mapping(address => RecoveryData) public recovery; + + function setTimestamp() external { + recovery[msg.sender].lastActivity = uint128(block.timestamp); + } +} diff --git a/test/Recovery.t.sol b/test/Recovery.t.sol deleted file mode 100644 index 5046480..0000000 --- a/test/Recovery.t.sol +++ /dev/null @@ -1,137 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.17; - -import "./helpers/SafeDeployer.sol"; -import {Recovery, IRecovery} from "../src/Recovery.sol"; -import {Users} from "./helpers/Users.sol"; - -contract RecoveryModuleTest is SafeDeployer, Users { - Recovery public recovery; - GnosisSafe public safeContract; - address public safe; - - function setUp() public { - recovery = new Recovery(); - vm.label(address(recovery), "Recovery"); - _deploySafe(); - } - - function _deploySafe() private { - address[] memory owners = new address[](3); - owners[0] = _alice; - owners[1] = _bob; - owners[2] = _charlie; - - // Deploy safe - safeContract = super.deploySafe({owners: owners, threshold: 1}); - safe = address(safe); - vm.label(safe, "Safe"); - - vm.deal(safe, 1 ether); - } - - function testAddRecovery() external { - uint256 amount = 0.2 ether; - - vm.prank(safe); - - recovery.addRecoveryWithSubscription{value: amount}( - _alice, uint40(block.timestamp) + 500 days, IRecovery.RecoveryType.After - ); - - assert(_alice.balance == 0); - - recovery.withdrawFunds(amount, _alice); - - assert(_alice.balance == amount); - } - - function testClearRecovery() external { - vm.startPrank(safe); - - recovery.addRecoveryWithSubscription{value: 0.2 ether}( - _alice, uint40(block.timestamp) + 500 days, IRecovery.RecoveryType.After - ); - - recovery.clearRecoveryData(); - vm.stopPrank(); - - assert(recovery.getRecoveryAddress(safe) == address(0)); - } - - function testAddRecoveryShouldRevertOnInvalidAddress() external { - vm.expectRevert(abi.encodeWithSelector(IRecovery.InvalidRecoveryAddress.selector)); - recovery.addRecoveryWithSubscription{value: 0.1 ether}( - address(0), uint40(block.timestamp) + 25 days, IRecovery.RecoveryType.After - ); - - vm.expectRevert(abi.encodeWithSelector(IRecovery.InvalidRecoveryAddress.selector)); - recovery.addRecovery(address(0), uint40(block.timestamp) + 25 days, IRecovery.RecoveryType.After); - } - - function testAddRecoveryShouldRevert() external { - uint256 amount = 1337 ether; - vm.expectRevert(abi.encodeWithSelector(IRecovery.InvalidPayment.selector, 0.1 ether)); - recovery.addRecoveryWithSubscription{value: amount}( - _alice, uint40(block.timestamp) + 25 days, IRecovery.RecoveryType.After - ); - } - - function testAddRecoveryWithoutSubscription() external { - address recoveryAddress = address(1337); - uint40 recoveryDate = uint40(block.timestamp) + 500 days; - - vm.prank(safe); - recovery.addRecovery(recoveryAddress, recoveryDate, IRecovery.RecoveryType.After); - - assert(recovery.getRecoveryAddress(safe) == recoveryAddress); - assert(recovery.getRecoveryDate(safe) == uint256(recoveryDate)); - assert(recovery.getRecoveryType(safe) == IRecovery.RecoveryType.After); - } - - function testSubscriptionAmount(uint256 amount) external { - recovery.setSubscription(amount); - assert(recovery.getSubscriptionAmount() == amount); - } - - function testAddRecoveryModule() external { - address module = address(555555); - assert(recovery.isRecoveryModule(module) == false); - recovery.addRecoveryModule(module); - assert(recovery.isRecoveryModule(module) == true); - } - - function testRemoveRecoveryModule() external { - address module = address(555555); - recovery.addRecoveryModule(module); - assert(recovery.isRecoveryModule(module) == true); - recovery.removeRecoveryModule(module); - assert(recovery.isRecoveryModule(module) == false); - } - - function testUpdateLastActivity() external { - address module = address(555555); - // whitelist module - recovery.addRecoveryModule(module); - vm.startBroadcast(module); - - vm.warp(1641070800); - address fakeSafe = address(12345); - recovery.updateLastActivity(fakeSafe); - assertEq(recovery.getLastActivity(fakeSafe), block.timestamp, "bad timestamp"); - vm.stopBroadcast(); - } - - function testUpdateLastActivityShouldFail() external { - address module = address(555555); - // module is not whitelisted - vm.startBroadcast(module); - - address fakeSafe = address(12345); - vm.expectRevert(IRecovery.Unauthorized.selector); - recovery.updateLastActivity(fakeSafe); - vm.stopBroadcast(); - - assert(recovery.getLastActivity(fakeSafe) == 0); - } -} diff --git a/test/RecoveryModule.t.sol b/test/RecoveryModule.t.sol index 60cb443..d63f975 100644 --- a/test/RecoveryModule.t.sol +++ b/test/RecoveryModule.t.sol @@ -4,14 +4,12 @@ pragma solidity 0.8.17; import {GnosisSafe, GuardManager, Enum, ModuleManager} from "safe-contracts/GnosisSafe.sol"; import {SafeDeployer} from "./helpers/SafeDeployer.sol"; import {RecoveryModule, IRecoveryModule} from "../src/RecoveryModule.sol"; -import {Recovery, IRecovery} from "../src/Recovery.sol"; import {Users} from "./helpers/Users.sol"; contract RecoveryModuleTest is SafeDeployer, Users { RecoveryModule public module; GnosisSafe public safeContract; address public safe; - Recovery public recovery; uint256 private _timelock = 10 days; @@ -33,11 +31,8 @@ contract RecoveryModuleTest is SafeDeployer, Users { safe = address(safeContract); vm.label(safe, "Safe"); - recovery = new Recovery(); - vm.label(address(recovery), "Recovery"); - // Deploy module - module = new RecoveryModule(address(recovery), _timelock); + module = new RecoveryModule(_timelock); vm.label(address(module), "RecoveryModule"); vm.deal(safe, 1 ether); @@ -71,8 +66,6 @@ contract RecoveryModuleTest is SafeDeployer, Users { refundReceiver: payable(address(0)), signatures: _validSignature }); - - recovery.addRecoveryModule(address(module)); } function testSetUp() external view { @@ -80,14 +73,14 @@ contract RecoveryModuleTest is SafeDeployer, Users { } function _addRecoveryAfter(address recoveryAddress, uint40 recoveryDate) private returns (bool) { - uint256 subscriptionAmount = recovery.getSubscriptionAmount(); + uint256 subscriptionAmount = module.getSubscriptionAmount(); // Add recovery address bool success = safeContract.execTransaction({ - to: address(recovery), + to: address(module), value: subscriptionAmount, data: abi.encodeCall( - IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.After) + IRecoveryModule.addRecovery, (recoveryAddress, recoveryDate, IRecoveryModule.RecoveryType.After) ), operation: Enum.Operation.Call, safeTxGas: 0, @@ -102,14 +95,14 @@ contract RecoveryModuleTest is SafeDeployer, Users { } function _addRecoveryInactiveFor(address recoveryAddress, uint40 recoveryDate) private returns (bool) { - uint256 subscriptionAmount = recovery.getSubscriptionAmount(); + uint256 subscriptionAmount = module.getSubscriptionAmount(); // Add recovery address bool success = safeContract.execTransaction({ - to: address(recovery), + to: address(module), value: subscriptionAmount, data: abi.encodeCall( - IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.InactiveFor) + IRecoveryModule.addRecovery, (recoveryAddress, recoveryDate, IRecoveryModule.RecoveryType.InactiveFor) ), operation: Enum.Operation.Call, safeTxGas: 0, @@ -124,7 +117,7 @@ contract RecoveryModuleTest is SafeDeployer, Users { } function _initiateTransferOwnership() private { - assert(recovery.getRecoveryAddress(safe) == address(0)); + assert(module.getRecoveryAddress(safe) == address(0)); address recoveryAddress = address(1337); uint40 recoveryDate = uint40(block.timestamp) + 25 days; @@ -137,8 +130,8 @@ contract RecoveryModuleTest is SafeDeployer, Users { // Validate that recovery address and recovery date are set address safeAddress = safe; - assert(recovery.getRecoveryAddress(safeAddress) == recoveryAddress); - assert(recovery.getRecoveryDate(safeAddress) == recoveryDate); + assert(module.getRecoveryAddress(safeAddress) == recoveryAddress); + assert(module.getRecoveryDate(safeAddress) == recoveryDate); // fast forward blockchain so that we can call `initiateTransferOwnership` successfully vm.warp(recoveryDate); @@ -239,48 +232,46 @@ contract RecoveryModuleTest is SafeDeployer, Users { module.initiateTransferOwnership(safe); } - function testDuplicateAddRecoveryShouldWork() external { - uint256 subscriptionAmount = recovery.getSubscriptionAmount(); - - address recoveryAddress = address(1337); - uint40 recoveryDate = uint40(block.timestamp) + 25 days; - - // Add recovery address - bool success = safeContract.execTransaction({ - to: address(recovery), - value: subscriptionAmount, - data: abi.encodeCall( - IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.After) - ), - operation: Enum.Operation.Call, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: payable(address(0)), - signatures: _validSignature - }); - - assert(success == true); - - // Add recovery address - success = safeContract.execTransaction({ - to: address(recovery), - value: subscriptionAmount, - data: abi.encodeCall( - IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.After) - ), - operation: Enum.Operation.Call, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: payable(address(0)), - signatures: _validSignature - }); - - assert(success == true); - } + // function testDuplicateAddRecoveryShouldWork() external { + // address recoveryAddress = address(1337); + // uint40 recoveryDate = uint40(block.timestamp) + 25 days; + + // // Add recovery address + // bool success = safeContract.execTransaction({ + // to: address(module), + // value: subscriptionAmount, + // data: abi.encodeCall( + // IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.After) + // ), + // operation: Enum.Operation.Call, + // safeTxGas: 0, + // baseGas: 0, + // gasPrice: 0, + // gasToken: address(0), + // refundReceiver: payable(address(0)), + // signatures: _validSignature + // }); + + // assert(success == true); + + // // Add recovery address + // success = safeContract.execTransaction({ + // to: address(recovery), + // value: subscriptionAmount, + // data: abi.encodeCall( + // IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.After) + // ), + // operation: Enum.Operation.Call, + // safeTxGas: 0, + // baseGas: 0, + // gasPrice: 0, + // gasToken: address(0), + // refundReceiver: payable(address(0)), + // signatures: _validSignature + // }); + + // assert(success == true); + // } function testInactiveForShouldWork() external { address recoveryAddress = address(1337); From bb91b877aa5b051d003bc4048b392ec001c3b171 Mon Sep 17 00:00:00 2001 From: Benjamin Halimic Date: Fri, 20 Jan 2023 11:07:46 +0100 Subject: [PATCH 2/5] forge install: solady v0.0.73 --- .gitmodules | 4 ++++ lib/solady | 1 + 2 files changed, 5 insertions(+) create mode 160000 lib/solady diff --git a/.gitmodules b/.gitmodules index 6e755ef..fbdd021 100644 --- a/.gitmodules +++ b/.gitmodules @@ -10,3 +10,7 @@ path = lib/openzeppelin-contracts url = https://github.com/OpenZeppelin/openzeppelin-contracts branch = v4.8.0 +[submodule "lib/solady"] + path = lib/solady + url = https://github.com/Vectorized/solady + branch = v0.0.73 diff --git a/lib/solady b/lib/solady new file mode 160000 index 0000000..459f665 --- /dev/null +++ b/lib/solady @@ -0,0 +1 @@ +Subproject commit 459f665c760dc5c471b37954430a336df795fb95 From 1948586a635b772253d61e2957e4f211731b4da2 Mon Sep 17 00:00:00 2001 From: Benjamin Halimic Date: Fri, 20 Jan 2023 18:46:42 +0100 Subject: [PATCH 3/5] Variant 2 --- .gas-snapshot | 24 ++-- README.md | 34 +++++- remappings.txt | 3 +- src/IRecoveryModule.sol | 17 ++- src/RecoveryModule.sol | 62 +++++----- test/RecoveryModule.t.sol | 235 ++++++++++++++++++++------------------ test/helpers/Users.sol | 2 + 7 files changed, 219 insertions(+), 158 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index a5a7cdb..76a3f5d 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,12 +1,14 @@ -RecoveryModuleTest:testCancelTransferOwnership() (gas: 147567) +RecoveryModuleTest:testAddRecoveryShouldRevertForBadRecoveryAddress() (gas: 70027) +RecoveryModuleTest:testCancelTransferOwnership() (gas: 170364) +RecoveryModuleTest:testDuplicateAddRecoveryShouldWork() (gas: 158583) RecoveryModuleTest:testFailFinalizeOwnership() (gas: 10336) -RecoveryModuleTest:testFinalizeTransferOwnership() (gas: 207041) -RecoveryModuleTest:testFinalizeTransferOwnershipShouldRevert() (gas: 13383) -RecoveryModuleTest:testInactiveForShouldWork() (gas: 189844) -RecoveryModuleTest:testInactiveForTooEarly() (gas: 109414) -RecoveryModuleTest:testInitiateFinalizeTooEarlyShouldRevert() (gas: 133400) -RecoveryModuleTest:testInitiateTransferOwnership() (gas: 138872) -RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithAlreadyInitiated() (gas: 140950) -RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithInvalidAddress() (gas: 15564) -RecoveryModuleTest:testInitiateTransferOwnershipTooEarlyShouldRevert() (gas: 109042) -RecoveryModuleTest:testSetUp() (gas: 15573) +RecoveryModuleTest:testFinalizeTransferOwnership() (gas: 211308) +RecoveryModuleTest:testFinalizeTransferOwnershipShouldRevert() (gas: 13361) +RecoveryModuleTest:testInactiveForShouldWork(address,uint40,uint256) (runs: 256, μ: 200220, ~: 201587) +RecoveryModuleTest:testInactiveForTooEarlyShouldRevert(uint40) (runs: 256, μ: 129346, ~: 129346) +RecoveryModuleTest:testInitiateFinalizeTooEarlyShouldRevert() (gas: 152601) +RecoveryModuleTest:testInitiateTransferOwnership() (gas: 158115) +RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithAlreadyInitiated() (gas: 160105) +RecoveryModuleTest:testInitiateTransferOwnershipShouldRevertWithInvalidAddress() (gas: 15565) +RecoveryModuleTest:testInitiateTransferOwnershipTooEarlyShouldRevert() (gas: 128243) +RecoveryModuleTest:testSetUp() (gas: 15595) diff --git a/README.md b/README.md index 91cfcc3..f1fe70e 100644 --- a/README.md +++ b/README.md @@ -1,8 +1,40 @@ -# Safe module template repository with Forge +# Safe recovery module More info: https://forum.gnosis-safe.io/t/social-recovery-module/2117/5 +## How to activate recovery: + +1. Go to safe web app +2. Go to `apps` +3. Click Add custom app +4. + + +Now the module is activated. + +5. From safe, create a transaction to that module and call `addRecovery` +`addRecovery` takes 3 parameters (recoveryAddress, recoveryDate, recoveryType) +RecoveryAddress - Address to where we transfer the ownership of the safe +RecoveryType - If value is 0 the selected mode is 'Trigger transfer ownership if safe did not have any new transactions for x seconds' +RecoveryType - If value is 1 the selected mode is 'Trigger after x timestamp' + +RecoveryDate - Based on `RecoveryType` it is either seconds, or timestamp (in seconds) +For timestamp value check https://www.epochconverter.com/ + +6. After executing this transaction safe is now in 'recovery mode' +7. If the transfer ownership condition is met, safe ownership is not transferred right away. It has a timelock period of 10 days. +8. If safe owner doesn't cancel ownership transfer after it has been initiated, ownership is finally transferred after timelock passes. + +## Q&A: +1. Can this module steal tokens/nft's/eth from my safe? +
+No, Module does not have permissions to take money from safe. +2. Can this module steal safe ownership +
+No, safe ownership is only transferred to a predefined recovery address. + + ## Usage ``` forge install diff --git a/remappings.txt b/remappings.txt index 8de2897..f3f7198 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1,4 +1,5 @@ ds-test/=lib/forge-std/lib/ds-test/src/ forge-std/=lib/forge-std/src/ safe-contracts/=lib/safe-contracts/contracts/ -@openzeppelin/=lib/openzeppelin-contracts/ \ No newline at end of file +@openzeppelin/=lib/openzeppelin-contracts/ +@solady/=lib/solady/ \ No newline at end of file diff --git a/src/IRecoveryModule.sol b/src/IRecoveryModule.sol index 2de37b8..ddb2127 100644 --- a/src/IRecoveryModule.sol +++ b/src/IRecoveryModule.sol @@ -51,6 +51,10 @@ interface IRecoveryModule { /// @param safe is the address of a safe event RecoveryDataCleared(address indexed safe); + /// @notice Emitted ether is transfered to an address + /// @param recipient is the address of a recipient + event EtherTransferred(address indexed recipient); + /// @notice Emitted when the transfer ownership is initiated /// @param safe is the safe address /// @param timeLockExpiration is the timestamp (seconds) when the timelock expires @@ -60,12 +64,8 @@ interface IRecoveryModule { /// @param safe is the safe address event TransferOwnershipFinalized(address indexed safe); - /// @notice Emitted when the Safe cancels ownership transfer - /// @param safe is the safe address - event TransferOwnershipCanceled(address indexed safe); - - /// @notice Cancels the ownership transfer when called by Safe - function cancelTransferOwnership() external; + /// @notice Cancels/clears the ownership transfer when called by Safe + function clearRecovery() external; /// @notice Initiates ownership transfer /// @param safe is the safe address @@ -100,6 +100,11 @@ interface IRecoveryModule { /// @return recovery date timestamp (in seconds) function getRecoveryDate(address safe) external view returns (uint64); + /// @notice Returns recovery value for a `safe` + /// @param safe is the address of the safe + /// @return recovery value + function getRecoveryValue(address safe) external view returns (uint256); + /// @notice Adds recovery address and a recovery date /// Safe is expected to be a caller /// @param recoveryAddress is an address to which safe ownership will be transfered diff --git a/src/RecoveryModule.sol b/src/RecoveryModule.sol index 07ee479..9f801e6 100644 --- a/src/RecoveryModule.sol +++ b/src/RecoveryModule.sol @@ -6,26 +6,28 @@ import {GnosisSafe} from "safe-contracts/GnosisSafe.sol"; import {Guard} from "safe-contracts/base/GuardManager.sol"; import {OwnerManager} from "safe-contracts/base/OwnerManager.sol"; import {IRecoveryModule} from "./IRecoveryModule.sol"; +import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol"; /// @author Benjamin H - contract RecoveryModule is IRecoveryModule, Guard { + using SafeTransferLib for address; + // Timelock period for ownership transfer uint256 public immutable timeLock; struct RecoveryData { - address recoveryAddress; - uint40 recoveryDate; - RecoveryType recoveryType; - uint40 lastActivity; + address recoveryAddress; // slot 0 + uint40 recoveryDate; // slot 0 + RecoveryType recoveryType; // slot 0 + uint40 lastActivity; // slot 0 + uint256 recoveryValue; // slot 1 } - uint256 private _subscriptionAmount = 0.1 ether; - - // [safe address] -> Recovery data + // [Safe address] -> Recovery data mapping(address => RecoveryData) private _recoveryData; - // Safe address -> timelockExpiration timestamp - mapping(address => uint256) private _recovery; + // [Safe address] -> timelockExpiration timestamp in seconds + mapping(address => uint256) private _recoveryTimelock; constructor(uint256 lock) { timeLock = lock; @@ -33,10 +35,6 @@ contract RecoveryModule is IRecoveryModule, Guard { /// @inheritdoc IRecoveryModule function addRecovery(address recoveryAddress, uint40 recoveryDate, RecoveryType recoveryType) external payable { - if (msg.value != _subscriptionAmount) { - revert InvalidPayment(_subscriptionAmount); - } - if (recoveryAddress == address(0)) { revert InvalidRecoveryAddress(); } @@ -45,20 +43,25 @@ contract RecoveryModule is IRecoveryModule, Guard { recoveryAddress: recoveryAddress, recoveryDate: recoveryDate, recoveryType: recoveryType, - lastActivity: uint40(block.timestamp) + lastActivity: uint40(block.timestamp), + recoveryValue: getRecoveryValue(msg.sender) + msg.value // new amount is previous + this }); emit RecoveryAddressAdded(msg.sender, recoveryAddress, recoveryDate, recoveryType); } + /// @inheritdoc IRecoveryModule function clearRecovery() external { + uint256 amount = getRecoveryValue(msg.sender); _clearRecovery(msg.sender); + msg.sender.safeTransferETH(amount); + emit EtherTransferred(msg.sender); } /// @inheritdoc IRecoveryModule function initiateTransferOwnership(address safe) external { // This is done to prevent somebody from extending timeLockExpiration value - if (_recovery[safe] != 0) { + if (_recoveryTimelock[safe] != 0) { revert TransferOwnershipAlreadyInitiated(); } @@ -74,7 +77,7 @@ contract RecoveryModule is IRecoveryModule, Guard { uint256 timeLockExpiration = block.timestamp + timeLock; - _recovery[safe] = timeLockExpiration; + _recoveryTimelock[safe] = timeLockExpiration; emit TransferOwnershipInitiated(safe, timeLockExpiration); } @@ -90,10 +93,6 @@ contract RecoveryModule is IRecoveryModule, Guard { if (block.timestamp < getTimelockExpiration(safeAddress)) { revert TooEarly(); } - - // Clear recovery data - _clearRecovery(safeAddress); - delete _recovery[safeAddress]; GnosisSafe safe = GnosisSafe(payable(safeAddress)); address[] memory owners = safe.getOwners(); @@ -127,13 +126,14 @@ contract RecoveryModule is IRecoveryModule, Guard { } } - emit TransferOwnershipFinalized(safeAddress); - } + uint256 amount = getRecoveryValue(safeAddress); - /// @inheritdoc IRecoveryModule - function cancelTransferOwnership() external { - delete _recovery[msg.sender]; - emit TransferOwnershipCanceled(msg.sender); + // Clear recovery data + _clearRecovery(safeAddress); + + msg.sender.safeTransferETH(amount); + + emit TransferOwnershipFinalized(safeAddress); } function checkTransaction( @@ -159,7 +159,8 @@ contract RecoveryModule is IRecoveryModule, Guard { } function _clearRecovery(address safe) internal { - delete _recovery[safe]; + delete _recoveryTimelock[safe]; + delete _recoveryData[safe]; emit RecoveryDataCleared(safe); } @@ -167,7 +168,7 @@ contract RecoveryModule is IRecoveryModule, Guard { /// @inheritdoc IRecoveryModule function getTimelockExpiration(address safe) public view returns (uint256) { - return _recovery[safe]; + return _recoveryTimelock[safe]; } /// @inheritdoc IRecoveryModule @@ -190,8 +191,9 @@ contract RecoveryModule is IRecoveryModule, Guard { return _recoveryData[safe].lastActivity; } - function getSubscriptionAmount() external view returns (uint256) { - return _subscriptionAmount; + /// @inheritdoc IRecoveryModule + function getRecoveryValue(address safe) public view returns (uint256) { + return _recoveryData[safe].recoveryValue; } function _ensureSafeIsInactive(address safe) private view { diff --git a/test/RecoveryModule.t.sol b/test/RecoveryModule.t.sol index d63f975..e6f2f4e 100644 --- a/test/RecoveryModule.t.sol +++ b/test/RecoveryModule.t.sol @@ -35,7 +35,7 @@ contract RecoveryModuleTest is SafeDeployer, Users { module = new RecoveryModule(_timelock); vm.label(address(module), "RecoveryModule"); - vm.deal(safe, 1 ether); + vm.deal(safe, 100 ether); // Enable module on Safe // This assumes that threshold for safe will be 1, and that this contract is one of the safe owners @@ -69,83 +69,44 @@ contract RecoveryModuleTest is SafeDeployer, Users { } function testSetUp() external view { - assert(safeContract.isModuleEnabled(address(module)) == true); - } - - function _addRecoveryAfter(address recoveryAddress, uint40 recoveryDate) private returns (bool) { - uint256 subscriptionAmount = module.getSubscriptionAmount(); - - // Add recovery address - bool success = safeContract.execTransaction({ - to: address(module), - value: subscriptionAmount, - data: abi.encodeCall( - IRecoveryModule.addRecovery, (recoveryAddress, recoveryDate, IRecoveryModule.RecoveryType.After) - ), - operation: Enum.Operation.Call, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: payable(address(0)), - signatures: _validSignature - }); - - return success; - } - - function _addRecoveryInactiveFor(address recoveryAddress, uint40 recoveryDate) private returns (bool) { - uint256 subscriptionAmount = module.getSubscriptionAmount(); - - // Add recovery address - bool success = safeContract.execTransaction({ - to: address(module), - value: subscriptionAmount, - data: abi.encodeCall( - IRecoveryModule.addRecovery, (recoveryAddress, recoveryDate, IRecoveryModule.RecoveryType.InactiveFor) - ), - operation: Enum.Operation.Call, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: payable(address(0)), - signatures: _validSignature - }); - - return success; + require(safeContract.isModuleEnabled(address(module)) == true); } function _initiateTransferOwnership() private { - assert(module.getRecoveryAddress(safe) == address(0)); + require(module.getRecoveryAddress(safe) == address(0)); address recoveryAddress = address(1337); uint40 recoveryDate = uint40(block.timestamp) + 25 days; // subscription for 1 year - bool success = _addRecoveryAfter(recoveryAddress, recoveryDate); + bool success = _addRecoveryAfter(recoveryAddress, recoveryDate, 3 ether); require(success, "tx failed"); // Validate that recovery address and recovery date are set address safeAddress = safe; - assert(module.getRecoveryAddress(safeAddress) == recoveryAddress); - assert(module.getRecoveryDate(safeAddress) == recoveryDate); + require(module.getRecoveryAddress(safeAddress) == recoveryAddress); + require(module.getRecoveryDate(safeAddress) == recoveryDate); // fast forward blockchain so that we can call `initiateTransferOwnership` successfully vm.warp(recoveryDate); module.initiateTransferOwnership(safeAddress); - // Assert that we have 10 day timelock on transfer ownership - assert(module.getTimelockExpiration(safeAddress) == recoveryDate + 10 days); + // require that we have 10 day timelock on transfer ownership + require(module.getTimelockExpiration(safeAddress) == recoveryDate + 10 days); } function testInitiateTransferOwnership() external { _initiateTransferOwnership(); } + function testAddRecoveryShouldRevertForBadRecoveryAddress() external { + vm.expectRevert("GS013"); // GS013 is Safe error + _addRecoveryAfter(address(0), uint40(block.timestamp + 25 days), 1 ether); + } + function testCancelTransferOwnership() external { _initiateTransferOwnership(); @@ -153,7 +114,7 @@ contract RecoveryModuleTest is SafeDeployer, Users { bool success = safeContract.execTransaction({ to: address(module), value: 0, - data: abi.encodeCall(IRecoveryModule.cancelTransferOwnership, ()), + data: abi.encodeCall(IRecoveryModule.clearRecovery, ()), operation: Enum.Operation.Call, safeTxGas: 0, baseGas: 0, @@ -165,7 +126,7 @@ contract RecoveryModuleTest is SafeDeployer, Users { require(success, "tx failed"); - assert(module.getTimelockExpiration(safe) == 0); + require(module.getTimelockExpiration(safe) == 0); } /// Finalize transfer ownership should work @@ -176,12 +137,14 @@ contract RecoveryModuleTest is SafeDeployer, Users { vm.warp(block.timestamp + 11 days); // 3 Owners before - assert(safeContract.getOwners().length == 3); + require(safeContract.getOwners().length == 3); + + vm.broadcast(_charlie); module.finalizeTransferOwnership(safe); // 1 owner after - assert(safeContract.getOwners().length == 1); + require(safeContract.getOwners().length == 1); } function testFailFinalizeOwnership() external { @@ -204,7 +167,7 @@ contract RecoveryModuleTest is SafeDeployer, Users { address recoveryAddress = address(1337); uint40 recoveryDate = uint40(block.timestamp) + 25 days; - _addRecoveryAfter(recoveryAddress, recoveryDate); + _addRecoveryAfter(recoveryAddress, recoveryDate, 1 ether); vm.expectRevert(IRecoveryModule.TooEarly.selector); module.initiateTransferOwnership(safe); @@ -214,7 +177,7 @@ contract RecoveryModuleTest is SafeDeployer, Users { address recoveryAddress = address(1337); uint40 recoveryDate = uint40(block.timestamp) + 25 days; - _addRecoveryAfter(recoveryAddress, recoveryDate); + _addRecoveryAfter(recoveryAddress, recoveryDate, 2 ether); vm.warp(recoveryDate); @@ -232,74 +195,128 @@ contract RecoveryModuleTest is SafeDeployer, Users { module.initiateTransferOwnership(safe); } - // function testDuplicateAddRecoveryShouldWork() external { - // address recoveryAddress = address(1337); - // uint40 recoveryDate = uint40(block.timestamp) + 25 days; - - // // Add recovery address - // bool success = safeContract.execTransaction({ - // to: address(module), - // value: subscriptionAmount, - // data: abi.encodeCall( - // IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.After) - // ), - // operation: Enum.Operation.Call, - // safeTxGas: 0, - // baseGas: 0, - // gasPrice: 0, - // gasToken: address(0), - // refundReceiver: payable(address(0)), - // signatures: _validSignature - // }); - - // assert(success == true); - - // // Add recovery address - // success = safeContract.execTransaction({ - // to: address(recovery), - // value: subscriptionAmount, - // data: abi.encodeCall( - // IRecovery.addRecoveryWithSubscription, (recoveryAddress, recoveryDate, IRecovery.RecoveryType.After) - // ), - // operation: Enum.Operation.Call, - // safeTxGas: 0, - // baseGas: 0, - // gasPrice: 0, - // gasToken: address(0), - // refundReceiver: payable(address(0)), - // signatures: _validSignature - // }); - - // assert(success == true); - // } - - function testInactiveForShouldWork() external { - address recoveryAddress = address(1337); + function testDuplicateAddRecoveryShouldWork() external { + uint40 recoveryDate = uint40(block.timestamp) + 25 days; + + uint256 amount1 = 1 ether; + uint256 amount2 = 5 ether; + + // Add recovery address + bool success = safeContract.execTransaction({ + to: address(module), + value: amount1, + data: abi.encodeCall(IRecoveryModule.addRecovery, (_charlie, recoveryDate, IRecoveryModule.RecoveryType.After)), + operation: Enum.Operation.Call, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: _validSignature + }); + + require(success); + + // Add recovery address + success = safeContract.execTransaction({ + to: address(module), + value: amount2, + data: abi.encodeCall(IRecoveryModule.addRecovery, (_charlie, recoveryDate, IRecoveryModule.RecoveryType.After)), + operation: Enum.Operation.Call, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: _validSignature + }); + + require(success); + require(module.getRecoveryValue(address(safeContract)) == (amount1 + amount2), "amounts don't match"); + } - bool success = _addRecoveryInactiveFor(recoveryAddress, 2 days); + function testInactiveForShouldWork(address recoveryAddress, uint40 inactivityInSeconds, uint256 recoveryValue) + external + { + vm.assume(recoveryAddress != address(0)); + vm.assume(recoveryValue < address(safe).balance); + vm.assume(recoveryAddress != address(1)); // sentinel modules address - assert(success == true); + vm.assume(inactivityInSeconds != 0); - // fast forward 3 days - vm.warp(block.timestamp + 3 days); + bool success = _addRecoveryInactiveFor(recoveryAddress, inactivityInSeconds, recoveryValue); + + require(success); + + vm.warp(block.timestamp + inactivityInSeconds); module.initiateTransferOwnership(safe); - vm.warp(block.timestamp + 11 days); + vm.warp(block.timestamp + _timelock + 1); + + vm.broadcast(_greg); module.finalizeTransferOwnership(safe); } - function testInactiveForTooEarly() external { + function testInactiveForTooEarlyShouldRevert(uint40 timestamp) external { + vm.assume(timestamp > block.timestamp); + address recoveryAddress = address(1337); - bool success = _addRecoveryInactiveFor(recoveryAddress, 2 days); + bool success = _addRecoveryInactiveFor(recoveryAddress, timestamp, 1 ether); - assert(success == true); + require(success); // no fast forward vm.expectRevert(IRecoveryModule.TooEarly.selector); module.initiateTransferOwnership(safe); } + + function _addRecoveryAfter(address recoveryAddress, uint40 recoveryDate, uint256 subscriptionAmount) + private + returns (bool) + { + // Add recovery address + bool success = safeContract.execTransaction({ + to: address(module), + value: subscriptionAmount, + data: abi.encodeCall( + IRecoveryModule.addRecovery, (recoveryAddress, recoveryDate, IRecoveryModule.RecoveryType.After) + ), + operation: Enum.Operation.Call, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: _validSignature + }); + + return success; + } + + function _addRecoveryInactiveFor(address recoveryAddress, uint40 recoveryDate, uint256 recoveryValue) + private + returns (bool) + { + // Add recovery address + bool success = safeContract.execTransaction({ + to: address(module), + value: recoveryValue, + data: abi.encodeCall( + IRecoveryModule.addRecovery, (recoveryAddress, recoveryDate, IRecoveryModule.RecoveryType.InactiveFor) + ), + operation: Enum.Operation.Call, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: _validSignature + }); + + return success; + } } diff --git a/test/helpers/Users.sol b/test/helpers/Users.sol index 1df3ada..9d62129 100644 --- a/test/helpers/Users.sol +++ b/test/helpers/Users.sol @@ -8,11 +8,13 @@ contract Users is Test { address internal constant _alice = address(52345345); address internal constant _bob = address(6435634534); address internal constant _charlie = address(33333333); + address internal constant _greg = address(452345343123); constructor() { vm.label(_owner, "Owner"); vm.label(_alice, "Alice"); vm.label(_bob, "Bob"); vm.label(_charlie, "Charlie"); + vm.label(_greg, "Greg"); } } From c522b4dce92e6125c57f97cd337f504ee9b72620 Mon Sep 17 00:00:00 2001 From: Benjamin Halimic Date: Fri, 20 Jan 2023 18:57:06 +0100 Subject: [PATCH 4/5] increase fuzz --- foundry.toml | 5 ++++- test/RecoveryModule.t.sol | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/foundry.toml b/foundry.toml index 3d6e1c9..3c26780 100644 --- a/foundry.toml +++ b/foundry.toml @@ -13,4 +13,7 @@ extra_output = ["abi"] # goerli = "${GOERLI_RPC_URL}" [etherscan] -goerli = { key = "${ETHERSCAN_API_KEY}" } \ No newline at end of file +goerli = { key = "${ETHERSCAN_API_KEY}" } + +[fuzz] +runs = 50000 \ No newline at end of file diff --git a/test/RecoveryModule.t.sol b/test/RecoveryModule.t.sol index e6f2f4e..341003e 100644 --- a/test/RecoveryModule.t.sol +++ b/test/RecoveryModule.t.sol @@ -239,10 +239,10 @@ contract RecoveryModuleTest is SafeDeployer, Users { external { vm.assume(recoveryAddress != address(0)); - vm.assume(recoveryValue < address(safe).balance); vm.assume(recoveryAddress != address(1)); // sentinel modules address - + vm.assume(recoveryValue < address(safe).balance); vm.assume(inactivityInSeconds != 0); + vm.label(recoveryAddress, "Recovery address"); bool success = _addRecoveryInactiveFor(recoveryAddress, inactivityInSeconds, recoveryValue); From f5e44979fbb45a1b5988998118590fbb804586d3 Mon Sep 17 00:00:00 2001 From: Benjamin Halimic Date: Fri, 20 Jan 2023 19:08:11 +0100 Subject: [PATCH 5/5] Update fuzz test --- test/RecoveryModule.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/test/RecoveryModule.t.sol b/test/RecoveryModule.t.sol index 341003e..34a82e6 100644 --- a/test/RecoveryModule.t.sol +++ b/test/RecoveryModule.t.sol @@ -240,6 +240,7 @@ contract RecoveryModuleTest is SafeDeployer, Users { { vm.assume(recoveryAddress != address(0)); vm.assume(recoveryAddress != address(1)); // sentinel modules address + vm.assume(recoveryAddress != address(safe)); vm.assume(recoveryValue < address(safe).balance); vm.assume(inactivityInSeconds != 0); vm.label(recoveryAddress, "Recovery address");