From 6753638186dec7e253a23331989176959cc30069 Mon Sep 17 00:00:00 2001 From: Filipp Makarov Date: Fri, 13 Oct 2023 10:06:59 +0300 Subject: [PATCH] :white_checkmark: addGuardian tests --- .../modules/AccountRecoveryModule.sol | 38 ++++--- test/module/AccountRecovery.Module.specs.ts | 99 +++++++++++++++++-- 2 files changed, 112 insertions(+), 25 deletions(-) diff --git a/contracts/smart-account/modules/AccountRecoveryModule.sol b/contracts/smart-account/modules/AccountRecoveryModule.sol index 0f2a49af..5de6b7b3 100644 --- a/contracts/smart-account/modules/AccountRecoveryModule.sol +++ b/contracts/smart-account/modules/AccountRecoveryModule.sol @@ -177,7 +177,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { uint256 requiredSignatures = _smartAccountSettings[userOp.sender] .recoveryThreshold; if (requiredSignatures == 0) revert("AccRecovery: Threshold not set"); - + bytes calldata moduleSignature = userOp.signature[96:]; require( @@ -196,13 +196,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule { for (uint256 i; i < requiredSignatures; ) { address currentUserOpSignerAddress = userOpHash .toEthSignedMessageHash() - .recover( - moduleSignature[2 * i * 65:(2 * i + 1) * 65] - ); + .recover(moduleSignature[2 * i * 65:(2 * i + 1) * 65]); - currentGuardianSig = moduleSignature[ - (2 * i + 1) * 65 : (2 * i + 2) * 65 - ]; + currentGuardianSig = moduleSignature[(2 * i + 1) * 65:(2 * i + 2) * + 65]; currentGuardianAddress = CONTROL_HASH .toEthSignedMessageHash() @@ -292,7 +289,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule { if (_guardians[guardian][msg.sender].validUntil != 0) revert GuardianAlreadySet(guardian, msg.sender); - (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(validUntil, validAfter); + (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter( + validUntil, + validAfter + ); // TODO: // make a test case that it fails if validAfter + securityDelay together overflow uint48 @@ -319,7 +319,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule { if (guardian == bytes32(0)) revert ZeroGuardian(); if (newGuardian == bytes32(0)) revert ZeroGuardian(); - (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(validUntil, validAfter); + (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter( + validUntil, + validAfter + ); // make the new one valid _guardians[newGuardian][msg.sender] = TimeFrame( @@ -358,11 +361,13 @@ contract AccountRecoveryModule is BaseAuthorizationModule { // REMOVE EXPIRED GUARDIAN // not permissioned - anyone can call it but the check if the guardian is expired is on-chain // it will allow us clearing expired guardians from the backend and maintain the list of guardians actual - function removeExpiredGuardian(bytes32 guardian, address smartAccount) external { + function removeExpiredGuardian( + bytes32 guardian, + address smartAccount + ) external { uint48 validUntil = _guardians[guardian][smartAccount].validUntil; - if (validUntil == 0) - revert GuardianNotSet(guardian, smartAccount); - if(validUntil>=block.timestamp) + if (validUntil == 0) revert GuardianNotSet(guardian, smartAccount); + if (validUntil >= block.timestamp) revert GuardianNotExpired(guardian, smartAccount); _removeGuardianAndChangeTresholdIfNeeded(guardian, smartAccount); } @@ -376,7 +381,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { // @note if validAfter is less then now + securityDelay, it is set to now + securityDelay // as for security reasons new guardian is only active after securityDelay function _checkAndAdjustValidUntilValidAfter( - uint48 validUntil, + uint48 validUntil, uint48 validAfter ) internal view returns (uint48, uint48) { if (validUntil == 0) validUntil = type(uint48).max; @@ -392,7 +397,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule { return (validUntil, validAfter); } - function _removeGuardianAndChangeTresholdIfNeeded(bytes32 guardian, address smartAccount) internal { + function _removeGuardianAndChangeTresholdIfNeeded( + bytes32 guardian, + address smartAccount + ) internal { delete _guardians[guardian][smartAccount]; --_smartAccountSettings[smartAccount].guardiansCount; emit GuardianRemoved(smartAccount, guardian); diff --git a/test/module/AccountRecovery.Module.specs.ts b/test/module/AccountRecovery.Module.specs.ts index 69e1dc23..6ba10bae 100644 --- a/test/module/AccountRecovery.Module.specs.ts +++ b/test/module/AccountRecovery.Module.specs.ts @@ -2054,22 +2054,31 @@ describe("Account Recovery Module: ", async () => { }); describe("addGuardian", async () => { - it("Can add a guardian and proper event is emitted", async () => { + let newGuardian: String; + + before (async () => { const { - entryPoint, - userSA, - accountRecoveryModule, - ecdsaModule, controlMessage, } = await setupTests(); const messageHash = ethers.utils.id(controlMessage); const messageHashBytes = ethers.utils.arrayify(messageHash); - const newGuardian = ethers.utils.keccak256( + newGuardian = ethers.utils.keccak256( await eve.signMessage(messageHashBytes) ); + }); + + it("Can add a guardian and proper event is emitted", async () => { + const { + entryPoint, + userSA, + accountRecoveryModule, + ecdsaModule, + controlMessage, + } = await setupTests(); + const guardiansBefore = ( await accountRecoveryModule.getSmartAccountSettings(userSA.address) ).guardiansCount; @@ -2125,13 +2134,83 @@ describe("Account Recovery Module: ", async () => { expect(guardiansAfter).to.equal(guardiansBefore + 1); }); - /* - it("Should revert if zero guardian is provided", async () => {}); + + it("Should revert if zero guardian is provided", async () => { + const { + userSA, + accountRecoveryModule, + } = await setupTests(); - it("Should revert if such a guardian has already been set", async () => {}); + //assign empty bytes32 + const zeroGuardian = ethers.utils.hexZeroPad("0x", 32); + const guardiansCountBefore = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount; + + await expect(accountRecoveryModule.addGuardian( + zeroGuardian, + 16741936496, + 0, + )).to.be.revertedWith("ZeroGuardian"); + + const guardiansCountAfter = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount; + expect(guardiansCountAfter).to.equal(guardiansCountBefore); + }); - it("Should set validUntil to uint48.max if validUntil = 0 is provided", async () => {}); + it("Should revert if such a guardian has already been set", async () => { + const { + accountRecoveryModule, + guardians + } = await setupTests(); + + const guardian = guardians[1]; + + //add guardian first + await accountRecoveryModule.addGuardian( + guardian, + 15555934444, + 0, + ); + + const guardiansCountBefore = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount; + + await expect(accountRecoveryModule.addGuardian( + guardian, + 15555934444, + 0, + )).to.be.revertedWith("GuardianAlreadySet") + .withArgs( + guardian, + deployer.address + ); + + const guardiansCountAfter = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount; + expect(guardiansCountAfter).to.equal(guardiansCountBefore); + }); + + it("Should set validUntil to uint48.max if validUntil = 0 is provided", async () => { + const { + accountRecoveryModule, + } = await setupTests(); + + const validUntil = 0; + const validAfter = 0; + + //add guardian first + await accountRecoveryModule.addGuardian( + newGuardian, + validUntil, + validAfter, + ); + + const newGuardianTimeFrame = await accountRecoveryModule.getGuardianParams( + newGuardian, + deployer.address + ); + expect(newGuardianTimeFrame.validUntil).to.equal(2**48-1); + }); + + + /* it("Should set validAfter as guardian.timeframe.validAfter if it is bigger than now+securityDelay", async () => {}); it("Should set now+securityDelay if validAfter is less than it", async () => {});