W3: Unnecessary computation of calldataHash value in validateRecovery…

…Subject function
zkemail · Jul 23, 2024 · f80edd7 · f80edd7
1 parent c9028df
commit f80edd7
Show file tree

Hide file tree

Showing 8 changed files with 167 additions and 19 deletions.
diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol
@@ -354,8 +354,9 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
         internal
         override
     {
-        (address account, bytes32 calldataHash) = IEmailRecoverySubjectHandler(subjectHandler)
-            .validateRecoverySubject(templateIdx, subjectParams, address(this));
+        address account = IEmailRecoverySubjectHandler(subjectHandler).validateRecoverySubject(
+            templateIdx, subjectParams, address(this)
+        );
 
         if (!IEmailRecoveryModule(emailRecoveryModule).isAuthorizedToRecover(account)) {
             revert RecoveryModuleNotAuthorized();
@@ -378,6 +379,9 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco
         recoveryRequest.currentWeight += guardianStorage.weight;
 
         if (recoveryRequest.currentWeight >= guardianConfig.threshold) {
+            bytes32 calldataHash = IEmailRecoverySubjectHandler(subjectHandler)
+                .parseRecoveryCalldataHash(templateIdx, subjectParams);
+
             uint256 executeAfter = block.timestamp + recoveryConfigs[account].delay;
             uint256 executeBefore = block.timestamp + recoveryConfigs[account].expiry;
 

diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol
@@ -121,8 +121,6 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
      * @param subjectParams The subject parameters of the recovery email
      * @param recoveryManager The recovery manager address. Used to help with validation
      * @return accountInEmail The account address in the acceptance email
-     * @return calldataHash The keccak256 hash of the recovery calldata. Verified against later when
-     * recovery is executed
      */
     function validateRecoverySubject(
         uint256 templateIdx,
@@ -131,7 +129,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
     )
         public
         view
-        returns (address, bytes32)
+        returns (address)
     {
         if (templateIdx != 0) {
             revert InvalidTemplateIndex();
@@ -159,6 +157,28 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {
             revert InvalidRecoveryModule();
         }
 
-        return (accountInEmail, calldataHash);
+        return accountInEmail;
+    }
+
+    /**
+     * @notice parses the recovery calldata hash from the subject params. The calldata hash is
+     * verified against later when recovery is executed
+     * @param templateIdx The index of the template used for the recovery request
+     * @param subjectParams The subject parameters of the recovery email
+     * @return calldataHash The keccak256 hash of the recovery calldata
+     */
+    function parseRecoveryCalldataHash(
+        uint256 templateIdx,
+        bytes[] calldata subjectParams
+    )
+        external
+        returns (bytes32)
+    {
+        if (templateIdx != 0) {
+            revert InvalidTemplateIndex();
+        }
+
+        string memory calldataHashInEmail = abi.decode(subjectParams[2], (string));
+        return StringUtils.hexToBytes32(calldataHashInEmail);
     }
 }
diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol
@@ -129,8 +129,6 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
      * @param subjectParams The subject parameters of the recovery email
      * @param recoveryManager The recovery manager address. Used to help with validation
      * @return accountInEmail The account address in the acceptance email
-     * @return calldataHash The keccak256 hash of the recovery calldata. Verified against later when
-     * recovery is executed
      */
     function validateRecoverySubject(
         uint256 templateIdx,
@@ -139,7 +137,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
     )
         public
         view
-        returns (address, bytes32)
+        returns (address)
     {
         if (templateIdx != 0) {
             revert InvalidTemplateIndex();
@@ -172,15 +170,38 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {
             revert InvalidRecoveryModule();
         }
 
+        return accountInEmail;
+    }
+
+    /**
+     * @notice parses the recovery calldata hash from the subject params. The calldata hash is
+     * verified against later when recovery is executed
+     * @param templateIdx The index of the template used for the recovery request
+     * @param subjectParams The subject parameters of the recovery email
+     * @return calldataHash The keccak256 hash of the recovery calldata
+     */
+    function parseRecoveryCalldataHash(
+        uint256 templateIdx,
+        bytes[] calldata subjectParams
+    )
+        external
+        returns (bytes32)
+    {
+        if (templateIdx != 0) {
+            revert InvalidTemplateIndex();
+        }
+
+        address accountInEmail = abi.decode(subjectParams[0], (address));
+        address oldOwnerInEmail = abi.decode(subjectParams[1], (address));
+        address newOwnerInEmail = abi.decode(subjectParams[2], (address));
+
         address previousOwnerInLinkedList =
             getPreviousOwnerInLinkedList(accountInEmail, oldOwnerInEmail);
         string memory functionSignature = "swapOwner(address,address,address)";
         bytes memory recoveryCallData = abi.encodeWithSignature(
             functionSignature, previousOwnerInLinkedList, oldOwnerInEmail, newOwnerInEmail
         );
-        bytes32 calldataHash = keccak256(recoveryCallData);
-
-        return (accountInEmail, calldataHash);
+        return keccak256(recoveryCallData);
     }
 
     /**

diff --git a/src/interfaces/IEmailRecoverySubjectHandler.sol b/src/interfaces/IEmailRecoverySubjectHandler.sol
@@ -36,5 +36,12 @@ interface IEmailRecoverySubjectHandler {
     )
         external
         view
-        returns (address, bytes32);
+        returns (address);
+
+    function parseRecoveryCalldataHash(
+        uint256 templateIdx,
+        bytes[] calldata subjectParams
+    )
+        external
+        returns (bytes32);
 }
diff --git a/test/unit/handlers/EmailRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol b/test/unit/handlers/EmailRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.25;
+
+import { console2 } from "forge-std/console2.sol";
+import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
+import { UnitBase } from "../../UnitBase.t.sol";
+import { EmailRecoverySubjectHandler } from "src/handlers/EmailRecoverySubjectHandler.sol";
+
+contract EmailRecoverySubjectHandler_parseRecoveryCalldataHash_Test is UnitBase {
+    using Strings for uint256;
+
+    string calldataHashString;
+    bytes[] subjectParams;
+
+    function setUp() public override {
+        super.setUp();
+
+        calldataHashString = uint256(calldataHash).toHexString(32);
+
+        subjectParams = new bytes[](3);
+        subjectParams[0] = abi.encode(accountAddress);
+        subjectParams[1] = abi.encode(recoveryModuleAddress);
+        subjectParams[2] = abi.encode(calldataHashString);
+    }
+
+    function test_ParseRecoveryCalldataHash_RevertWhen_InvalidTemplateIndex() public {
+        uint256 invalidTemplateIdx = 1;
+        vm.expectRevert(EmailRecoverySubjectHandler.InvalidTemplateIndex.selector);
+        emailRecoveryHandler.parseRecoveryCalldataHash(invalidTemplateIdx, subjectParams);
+    }
+
+    function test_ParseRecoveryCalldataHash_Succeeds() public {
+        bytes32 expectedCalldataHash = keccak256(recoveryCalldata);
+
+        bytes32 calldataHash =
+            emailRecoveryHandler.parseRecoveryCalldataHash(templateIdx, subjectParams);
+
+        assertEq(calldataHash, expectedCalldataHash);
+    }
+}
diff --git a/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol b/test/unit/handlers/EmailRecoverySubjectHandler/validateRecoverySubject.t.sol
@@ -83,10 +83,28 @@ contract EmailRecoverySubjectHandler_validateRecoverySubject_Test is UnitBase {
         );
     }
 
+    function test_ValidateRecoverySubject_RevertWhen_ZeroCalldataHash() public {
+        subjectParams[2] = abi.encode(bytes32(0));
+
+        vm.expectRevert("invalid hex prefix");
+        emailRecoveryHandler.validateRecoverySubject(
+            templateIdx, subjectParams, emailRecoveryManagerAddress
+        );
+    }
+
+    function test_ValidateRecoverySubject_RevertWhen_InvalidHashLength() public {
+        subjectParams[2] = abi.encode(uint256(calldataHash).toHexString(33));
+
+        vm.expectRevert("invalid hex string length");
+        emailRecoveryHandler.validateRecoverySubject(
+            templateIdx, subjectParams, emailRecoveryManagerAddress
+        );
+    }
+
     function test_ValidateRecoverySubject_Succeeds() public view {
-        (address accountFromEmail, bytes32 calldataHashFromEmail) = emailRecoveryHandler
-            .validateRecoverySubject(templateIdx, subjectParams, emailRecoveryManagerAddress);
+        address accountFromEmail = emailRecoveryHandler.validateRecoverySubject(
+            templateIdx, subjectParams, emailRecoveryManagerAddress
+        );
         assertEq(accountFromEmail, accountAddress);
-        assertEq(calldataHashFromEmail, calldataHash);
     }
 }
diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/parseRecoveryCalldataHash.t.sol
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.25;
+
+import { console2 } from "forge-std/console2.sol";
+import { SafeUnitBase } from "../../SafeUnitBase.t.sol";
+import { SafeRecoverySubjectHandler } from "src/handlers/SafeRecoverySubjectHandler.sol";
+
+contract SafeRecoverySubjectHandler_parseRecoveryCalldataHash_Test is SafeUnitBase {
+    bytes[] subjectParams;
+
+    function setUp() public override {
+        super.setUp();
+
+        subjectParams = new bytes[](4);
+        subjectParams[0] = abi.encode(accountAddress1);
+        subjectParams[1] = abi.encode(owner1);
+        subjectParams[2] = abi.encode(newOwner1);
+        subjectParams[3] = abi.encode(recoveryModuleAddress);
+    }
+
+    function test_ParseRecoveryCalldataHash_RevertWhen_InvalidTemplateIndex() public {
+        skipIfNotSafeAccountType();
+
+        uint256 invalidTemplateIdx = 1;
+        vm.expectRevert(SafeRecoverySubjectHandler.InvalidTemplateIndex.selector);
+        safeRecoverySubjectHandler.parseRecoveryCalldataHash(invalidTemplateIdx, subjectParams);
+    }
+
+    function test_ParseRecoveryCalldataHash_Succeeds() public {
+        skipIfNotSafeAccountType();
+        bytes32 expectedCalldataHash = keccak256(recoveryCalldata);
+
+        bytes32 calldataHash =
+            safeRecoverySubjectHandler.parseRecoveryCalldataHash(templateIdx, subjectParams);
+
+        assertEq(calldataHash, expectedCalldataHash);
+    }
+}
diff --git a/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol b/test/unit/handlers/SafeRecoverySubjectHandler/validateRecoverySubject.t.sol
@@ -100,9 +100,9 @@ contract SafeRecoverySubjectHandler_validateRecoverySubject_Test is SafeUnitBase
 
     function test_ValidateRecoverySubject_Succeeds() public {
         skipIfNotSafeAccountType();
-        (address accountFromEmail, bytes32 calldataHashFromEmail) = safeRecoverySubjectHandler
-            .validateRecoverySubject(templateIdx, subjectParams, emailRecoveryManagerAddress);
+        address accountFromEmail = safeRecoverySubjectHandler.validateRecoverySubject(
+            templateIdx, subjectParams, emailRecoveryManagerAddress
+        );
         assertEq(accountFromEmail, accountAddress1);
-        assertEq(calldataHashFromEmail, calldataHash);
     }
 }