diff --git a/contracts/smart-account/modules/AccountRecoveryModule.sol b/contracts/smart-account/modules/AccountRecoveryModule.sol index 87fbf8c9..89b10267 100644 --- a/contracts/smart-account/modules/AccountRecoveryModule.sol +++ b/contracts/smart-account/modules/AccountRecoveryModule.sol @@ -8,10 +8,10 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; /** * @title Account Recovery module for Biconomy Smart Accounts. * @dev Compatible with Biconomy Modular Interface v 0.1 - * - It allows to submit and execute recovery requests + * - It allows the submission and execution of recovery requests * - EOA guardians only - * - For security reasons guardian address is not stored, - * instead its signature over CONTROL_HASH is used + * - For security reasons the guardian's address is not stored, + * instead, its signature over CONTROL_HASH is used * - Security delay is always applied to new guardians and recovery requests * - It is highly recommended to not set security delay to 0 * @dev For the validation stage (validateUserOp) can not use custom errors @@ -77,7 +77,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule { TimeFrame timeFrame ); event ThresholdChanged(address indexed smartAccount, uint8 threshold); - event SecurityDelayChanged(address indexed smartAccount, uint48 securityDelay); + event SecurityDelayChanged( + address indexed smartAccount, + uint48 securityDelay + ); error AlreadyInitedForSmartAccount(address smartAccount); error ZeroGuardian(); @@ -152,8 +155,8 @@ contract AccountRecoveryModule is BaseAuthorizationModule { * @dev validates userOps to submut and execute recovery requests * - if securityDelay is 0, it allows to execute the request immediately * - if securityDelay is non 0, the request is submitted and stored on-chain - * - if userOp.callData matches the callData of the request already submitted, - * - and the security delays has passed, it allows to execute the request + * - if userOp.callData matches the callData of the request already submitted, + * - and the security delay has passed, it allows to execute the request * @param userOp User Operation to be validated. * @param userOpHash Hash of the User Operation to be validated. * @return validation data (sig validation result + validUntil + validAfter) @@ -223,7 +226,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { userOp.sender ].validUntil; - // validUntil == 0 means the `currentGuardian` has not been set as guardian + // validUntil == 0 means the `currentGuardian` has not been set as guardian // for the userOp.sender smartAccount // validUntil can never be 0 as it is set to type(uint48).max in initForSmartAccount if (validUntil == 0) { @@ -363,7 +366,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { /** * @dev Removes guardian for a Smart Account (msg.sender) * Should be called by the Smart Account - * @param guardian guardian to remove + * @param guardian guardian to remove */ function removeGuardian(bytes32 guardian) external { if (_guardians[guardian][msg.sender].validUntil == 0) @@ -373,9 +376,9 @@ contract AccountRecoveryModule is BaseAuthorizationModule { /** * @dev Removes the expired guardian for a Smart Account - * Can be called by anyone. Allows clearing expired guardians automatically + * Can be called by anyone. Allows clearing expired guardians automatically * and maintain the list of guardians actual - * @param guardian guardian to remove + * @param guardian guardian to remove */ function removeExpiredGuardian( bytes32 guardian, @@ -388,7 +391,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { _removeGuardianAndChangeTresholdIfNeeded(guardian, smartAccount); } - /** + /** * @dev Internal method to check and adjust validUntil and validAfter * @dev if validUntil is 0, guardian is considered active forever * Thus we put type(uint48).max as value for validUntil in this case, @@ -403,7 +406,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { * thus we do not need to check than validUntil is gte now * @param validUntil guardian validity end timestamp * @param validAfter guardian validity start timestamp - */ + */ function _checkAndAdjustValidUntilValidAfter( uint48 validUntil, uint48 validAfter @@ -420,13 +423,13 @@ contract AccountRecoveryModule is BaseAuthorizationModule { return (validUntil, validAfter); } - /** + /** * @dev Internal method to remove guardian and adjust threshold if needed * It is needed when after removing guardian, the threshold becomes higher than * the number of guardians - * @param guardian guardian to remove + * @param guardian guardian to remove * @param smartAccount smartAccount to remove guardian from - */ + */ function _removeGuardianAndChangeTresholdIfNeeded( bytes32 guardian, address smartAccount @@ -447,27 +450,34 @@ contract AccountRecoveryModule is BaseAuthorizationModule { } } - /** + /** * @dev Changes guardian validity timeframes for the Smart Account (msg.sender) - * @param guardian guardian to change + * @param guardian guardian to change * @param validUntil guardian validity end timestamp * @param validAfter guardian validity start timestamp - */ + */ function changeGuardianParams( bytes32 guardian, uint48 validUntil, uint48 validAfter ) external { - (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(validUntil, validAfter); + (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter( + validUntil, + validAfter + ); _guardians[guardian][msg.sender] = TimeFrame(validUntil, validAfter); - emit GuardianChanged(msg.sender, guardian, TimeFrame(validUntil, validAfter)); + emit GuardianChanged( + msg.sender, + guardian, + TimeFrame(validUntil, validAfter) + ); } /** * @dev Changes recovery threshold for a Smart Account (msg.sender) * Should be called by the Smart Account * @param newThreshold new recovery threshold - */ + */ function setThreshold(uint8 newThreshold) external { if (newThreshold == 0) revert ZeroThreshold(); if (newThreshold > _smartAccountSettings[msg.sender].guardiansCount) @@ -483,18 +493,18 @@ contract AccountRecoveryModule is BaseAuthorizationModule { * @dev Changes security delay for a Smart Account (msg.sender) * Should be called by the Smart Account * @param newSecurityDelay new security delay - */ + */ function setSecurityDelay(uint48 newSecurityDelay) external { _smartAccountSettings[msg.sender].securityDelay = newSecurityDelay; emit SecurityDelayChanged(msg.sender, newSecurityDelay); } - /** - * @dev Returns guardian validity timeframes for the Smart Account - * @param guardian guardian to get params for - * @param smartAccount smartAccount to get params for - * @return TimeFrame struct - */ + /** + * @dev Returns guardian validity timeframes for the Smart Account + * @param guardian guardian to get params for + * @param smartAccount smartAccount to get params for + * @return TimeFrame struct + */ function getGuardianParams( bytes32 guardian, address smartAccount @@ -502,11 +512,11 @@ contract AccountRecoveryModule is BaseAuthorizationModule { return _guardians[guardian][smartAccount]; } - /** - * @dev Returns Smart Account settings - * @param smartAccount smartAccount to get settings for - * @return Smart Account Settings struct - */ + /** + * @dev Returns Smart Account settings + * @param smartAccount smartAccount to get settings for + * @return Smart Account Settings struct + */ function getSmartAccountSettings( address smartAccount ) external view returns (SaSettings memory) { @@ -518,7 +528,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { * Only one request per Smart Account is stored at a time * @param smartAccount smartAccount to get recovery request for * @return RecoveryRequest struct - */ + */ function getRecoveryRequest( address smartAccount ) external view returns (RecoveryRequest memory) { @@ -529,7 +539,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { * @dev Submits recovery request for a Smart Account * Hash of the callData is stored on-chain along with the timestamp of the request submission * @param recoveryCallData callData of the recovery request - */ + */ function submitRecoveryRequest(bytes calldata recoveryCallData) public { if (recoveryCallData.length == 0) revert EmptyRecoveryCallData(); if ( diff --git a/test/bundler-integration/module/AccountRecovery.Module.specs.ts b/test/bundler-integration/module/AccountRecovery.Module.specs.ts index 34f4eb8e..7d1c116c 100644 --- a/test/bundler-integration/module/AccountRecovery.Module.specs.ts +++ b/test/bundler-integration/module/AccountRecovery.Module.specs.ts @@ -1,12 +1,7 @@ -import { expect, use } from "chai"; -import { ethers, deployments, waffle } from "hardhat"; -import { - makeEcdsaModuleUserOp, - makeUnsignedUserOp, -} from "../../utils/userOp"; -import { - makeMultiSignedUserOpWithGuardiansList, -} from "../../utils/accountRecovery"; +import { expect } from "chai"; +import { ethers, deployments } from "hardhat"; +import { makeEcdsaModuleUserOp, makeUnsignedUserOp } from "../../utils/userOp"; +import { makeMultiSignedUserOpWithGuardiansList } from "../../utils/accountRecovery"; import { getEntryPoint, getSmartAccountImplementation, diff --git a/test/module/AccountRecovery.Module.specs.ts b/test/module/AccountRecovery.Module.specs.ts index e165e3c1..41d66f8e 100644 --- a/test/module/AccountRecovery.Module.specs.ts +++ b/test/module/AccountRecovery.Module.specs.ts @@ -20,7 +20,7 @@ import { makeMultisignedSubmitRecoveryRequestUserOp, } from "../utils/accountRecovery"; import { arrayify } from "ethers/lib/utils"; -import {Contract} from "ethers"; +import { Contract } from "ethers"; describe("Account Recovery Module: ", async () => { const [ @@ -1599,7 +1599,7 @@ describe("Account Recovery Module: ", async () => { "changeGuardianParams", [ guardians[1], - 17641936496, + 17641936496, closestValidUntil + 1000, // validAfter for this guardian is after previous one expires ] ), @@ -2595,7 +2595,8 @@ describe("Account Recovery Module: ", async () => { describe("changeGuardianParams", async () => { it("Can change Guardian params and event is emitted", async () => { - const { accountRecoveryModule, guardians, defaultSecurityDelay } = await setupTests(); + const { accountRecoveryModule, guardians, defaultSecurityDelay } = + await setupTests(); const validUntilBefore = 16741936496; const validAfterBefore = 0; @@ -2610,8 +2611,14 @@ describe("Account Recovery Module: ", async () => { const validAfterAfter = validAfterBefore + 100; // by some reason blockchain timestamp for the txn is 1 sec higher - const nowPlusSecurityDelay = defaultSecurityDelay + 1 + (await ethers.provider.getBlock("latest")).timestamp; - const expectedOnchainValidAfter = validAfterAfter > nowPlusSecurityDelay ? validAfterAfter : nowPlusSecurityDelay; + const nowPlusSecurityDelay = + defaultSecurityDelay + + 1 + + (await ethers.provider.getBlock("latest")).timestamp; + const expectedOnchainValidAfter = + validAfterAfter > nowPlusSecurityDelay + ? validAfterAfter + : nowPlusSecurityDelay; const newTimeFrame = { validUntil: validUntilAfter, @@ -2639,7 +2646,6 @@ describe("Account Recovery Module: ", async () => { }); }); - describe("setThreshold", async () => { let accountRecoveryModuleWithGuardiansForDeployer: Contract; before(async () => { @@ -2653,17 +2659,27 @@ describe("Account Recovery Module: ", async () => { it("Can set a new threshold", async () => { const currentThreshold = ( - await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(deployer.address) + await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings( + deployer.address + ) ).recoveryThreshold; const newThreshold = currentThreshold - 1; - const setThresholdTxn = await accountRecoveryModuleWithGuardiansForDeployer.setThreshold(newThreshold); + const setThresholdTxn = + await accountRecoveryModuleWithGuardiansForDeployer.setThreshold( + newThreshold + ); expect(setThresholdTxn) - .to.emit(accountRecoveryModuleWithGuardiansForDeployer, "ThresholdChanged") + .to.emit( + accountRecoveryModuleWithGuardiansForDeployer, + "ThresholdChanged" + ) .withArgs(deployer.address, newThreshold); const newThresholdAfter = ( - await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(deployer.address) + await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings( + deployer.address + ) ).recoveryThreshold; - expect(newThresholdAfter).to.equal(newThreshold); + expect(newThresholdAfter).to.equal(newThreshold); }); it("Reverts for zero threshold", async () => { @@ -2674,21 +2690,27 @@ describe("Account Recovery Module: ", async () => { it("Reverts if threshold is more than number of guardians", async () => { const currentNumberOfGuardians = ( - await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(deployer.address) + await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings( + deployer.address + ) ).guardiansCount; await expect( - accountRecoveryModuleWithGuardiansForDeployer.setThreshold(currentNumberOfGuardians + 1) + accountRecoveryModuleWithGuardiansForDeployer.setThreshold( + currentNumberOfGuardians + 1 + ) ).to.be.revertedWith("ThresholdTooHigh"); }); }); - describe("setSecurityDelay", async () => { it("Can set security delay and event is emitted", async () => { - const { accountRecoveryModule, defaultSecurityDelay } = await setupTests(); + const { accountRecoveryModule, defaultSecurityDelay } = + await setupTests(); const newSecurityDelay = defaultSecurityDelay + 1000; - const setSecurityDelayTxn = await accountRecoveryModule.setSecurityDelay(newSecurityDelay); + const setSecurityDelayTxn = await accountRecoveryModule.setSecurityDelay( + newSecurityDelay + ); expect(setSecurityDelayTxn) .to.emit(accountRecoveryModule, "SecurityDelayChanged") .withArgs(deployer.address, newSecurityDelay); @@ -2701,6 +2723,4 @@ describe("Account Recovery Module: ", async () => { // for the execution stage (all methods not related to validateUserOp) custom errors can be used // make the according explanation in the smart contract header - - });