diff --git a/contracts/smart-account/modules/AccountRecoveryModule.sol b/contracts/smart-account/modules/AccountRecoveryModule.sol index 5de6b7b3..4128b1ea 100644 --- a/contracts/smart-account/modules/AccountRecoveryModule.sol +++ b/contracts/smart-account/modules/AccountRecoveryModule.sol @@ -288,14 +288,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule { if (guardian == bytes32(0)) revert ZeroGuardian(); if (_guardians[guardian][msg.sender].validUntil != 0) revert GuardianAlreadySet(guardian, msg.sender); - (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter( validUntil, validAfter ); - - // TODO: - // make a test case that it fails if validAfter + securityDelay together overflow uint48 _guardians[guardian][msg.sender] = TimeFrame(validUntil, validAfter); ++_smartAccountSettings[msg.sender].guardiansCount; emit GuardianAdded( @@ -306,7 +302,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule { } // natspec - // same as adding guardian, but also makes the old one active only until the new one is active + // deletes guardian and adds the new one, however it will still be valid not earlier than now+securityDelay function replaceGuardian( bytes32 guardian, bytes32 newGuardian, @@ -319,6 +315,8 @@ contract AccountRecoveryModule is BaseAuthorizationModule { if (guardian == bytes32(0)) revert ZeroGuardian(); if (newGuardian == bytes32(0)) revert ZeroGuardian(); + // remove guardian + (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter( validUntil, validAfter @@ -329,7 +327,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule { validUntil == 0 ? type(uint48).max : validUntil, validAfter ); - ++_smartAccountSettings[msg.sender].guardiansCount; + + // don't increment guardiansCount as we haven't decremented it when deleting previous one + // ++_smartAccountSettings[msg.sender].guardiansCount; + emit GuardianAdded( msg.sender, newGuardian, @@ -338,16 +339,6 @@ contract AccountRecoveryModule is BaseAuthorizationModule { validAfter ) ); - - // make the previous stay valid until the new one becomes valid - // if the new one becomes valid earlier, than old one validUntil, change the validUntil for the old one - // to the validAfter of the new one. So two are never valid at the same time - uint48 oldGuardianValidUntil = _guardians[guardian][msg.sender] - .validUntil; - _guardians[guardian][msg.sender].validUntil = (oldGuardianValidUntil < - validAfter) - ? oldGuardianValidUntil - : validAfter; } // natspec @@ -380,6 +371,9 @@ contract AccountRecoveryModule is BaseAuthorizationModule { // validUntil's and validAfter's for several guardians works correctly // @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 + // validAfter is always gte now+securityDelay + // and validUntil is always gte validAfter + // thus we do not need to check than validUntil is gte now function _checkAndAdjustValidUntilValidAfter( uint48 validUntil, uint48 validAfter @@ -393,7 +387,6 @@ contract AccountRecoveryModule is BaseAuthorizationModule { : validAfter; if (validUntil < validAfter) revert InvalidTimeFrame(validUntil, validAfter); - if (validUntil < block.timestamp) revert ExpiredValidUntil(validUntil); return (validUntil, validAfter); } diff --git a/test/module/AccountRecovery.Module.specs.ts b/test/module/AccountRecovery.Module.specs.ts index 6ba10bae..fd7c3ae3 100644 --- a/test/module/AccountRecovery.Module.specs.ts +++ b/test/module/AccountRecovery.Module.specs.ts @@ -2054,12 +2054,10 @@ describe("Account Recovery Module: ", async () => { }); describe("addGuardian", async () => { - let newGuardian: String; + let newGuardian: string; - before (async () => { - const { - controlMessage, - } = await setupTests(); + before(async () => { + const { controlMessage } = await setupTests(); const messageHash = ethers.utils.id(controlMessage); const messageHashBytes = ethers.utils.arrayify(messageHash); @@ -2067,7 +2065,6 @@ describe("Account Recovery Module: ", async () => { newGuardian = ethers.utils.keccak256( await eve.signMessage(messageHashBytes) ); - }); it("Can add a guardian and proper event is emitted", async () => { @@ -2134,98 +2131,137 @@ describe("Account Recovery Module: ", async () => { expect(guardiansAfter).to.equal(guardiansBefore + 1); }); - it("Should revert if zero guardian is provided", async () => { - const { - userSA, - accountRecoveryModule, - } = await setupTests(); + const { accountRecoveryModule } = await setupTests(); - //assign empty bytes32 + // 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; + 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 revert if such a guardian has already been set", async () => { - const { - accountRecoveryModule, - guardians - } = await setupTests(); + const { accountRecoveryModule, guardians } = await setupTests(); const guardian = guardians[1]; - //add guardian first - await accountRecoveryModule.addGuardian( - guardian, - 15555934444, - 0, - ); + // add guardian first + await accountRecoveryModule.addGuardian(guardian, 15555934444, 0); - const guardiansCountBefore = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount; + 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; + 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 { accountRecoveryModule } = await setupTests(); const validUntil = 0; const validAfter = 0; - //add guardian first + // add guardian first await accountRecoveryModule.addGuardian( newGuardian, validUntil, - validAfter, + validAfter ); - const newGuardianTimeFrame = await accountRecoveryModule.getGuardianParams( + 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 () => { + const { accountRecoveryModule, defaultSecurityDelay } = await setupTests(); + await accountRecoveryModule.setSecurityDelay(defaultSecurityDelay); + + const nowPlusSecurityDelay = ( + await ethers.provider.getBlock("latest") + ).timestamp + defaultSecurityDelay; + + const validAfter = nowPlusSecurityDelay + 1000; + + await accountRecoveryModule.addGuardian(newGuardian, 15555934444, validAfter); + + const guardianDetails = await accountRecoveryModule.getGuardianParams( newGuardian, deployer.address ); - expect(newGuardianTimeFrame.validUntil).to.equal(2**48-1); + expect(guardianDetails.validAfter).to.equal(validAfter); }); - - /* - 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 () => { + const { accountRecoveryModule, defaultSecurityDelay } = await setupTests(); + await accountRecoveryModule.setSecurityDelay(defaultSecurityDelay); + + const nowPlusSecurityDelay = ( + await ethers.provider.getBlock("latest") + ).timestamp + defaultSecurityDelay; + + const validAfter = nowPlusSecurityDelay - 1000; + + await accountRecoveryModule.addGuardian(newGuardian, 15555934444, validAfter); - it("Should set now+securityDelay if validAfter is less than it", async () => {}); + const guardianDetails = await accountRecoveryModule.getGuardianParams( + newGuardian, + deployer.address + ); - it("Should revert if validUntil is less than resulting validAfter", async () => {}); + expect(guardianDetails.validAfter).to.be.gte(nowPlusSecurityDelay); + }); - it("Should revert if validUntil < now", async () => {}); - */ + it("Should revert if validUntil is less than resulting validAfter", async () => { + const { accountRecoveryModule, defaultSecurityDelay } = await setupTests(); - // it("_________", async () => {}); + const nowPlusSecurityDelay = ( + await ethers.provider.getBlock("latest") + ).timestamp + defaultSecurityDelay; + + const validAfter = nowPlusSecurityDelay + 1000; + const validUntil = nowPlusSecurityDelay - 1000; + + await expect(accountRecoveryModule.addGuardian(newGuardian, validUntil, validAfter)).to.be.revertedWith("InvalidTimeFrame") + .withArgs(validUntil, validAfter); + }); }); /* describe("replaceGuardian", async () => { - it("_________", async () => {}); + + it("Can replace guardian, old is deleted, new is active, guardians count has not changed and events are emitted", async () => {}); + + it("reverts if guardian has not been set", async () => {}); + + it("reverts if guardian to replace and new guardian are identical", async () => {}); + + it("reverts if guardian to replace is zero", async () => {}); + + it("reverts if new guardian is zero", async () => {}); + }); describe("removeGuardian", async () => {