From 71fe6c7565c8e87d44c1b8086a2c4abeecffed68 Mon Sep 17 00:00:00 2001 From: zeroknots Date: Thu, 13 Jun 2024 07:53:31 +0700 Subject: [PATCH 1/4] feat: use calldata slicing in module should allow the removal of BytesLib. less code to be audited foo --- src/libraries/BytesLib.sol | 71 ----------------------------- src/modules/EmailRecoveryModule.sol | 12 ++--- 2 files changed, 3 insertions(+), 80 deletions(-) delete mode 100644 src/libraries/BytesLib.sol diff --git a/src/libraries/BytesLib.sol b/src/libraries/BytesLib.sol deleted file mode 100644 index 1cd66b10..00000000 --- a/src/libraries/BytesLib.sol +++ /dev/null @@ -1,71 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.25; - -library BytesLib { - function slice( - bytes memory _bytes, - uint256 _start, - uint256 _length - ) - internal - pure - returns (bytes memory) - { - require(_length + 31 >= _length, "slice_overflow"); - require(_bytes.length >= _start + _length, "slice_outOfBounds"); - - bytes memory tempBytes; - - assembly { - switch iszero(_length) - case 0 { - // Get a location of some free memory and store it in tempBytes as - // Solidity does for memory variables. - tempBytes := mload(0x40) - - // The first word of the slice result is potentially a partial - // word read from the original array. To read it, we calculate - // the length of that partial word and start copying that many - // bytes into the array. The first word we copy will start with - // data we don't care about, but the last `lengthmod` bytes will - // land at the beginning of the contents of the new array. When - // we're done copying, we overwrite the full first word with - // the actual length of the slice. - let lengthmod := and(_length, 31) - - // The multiplication in the next line is necessary - // because when slicing multiples of 32 bytes (lengthmod == 0) - // the following copy loop was copying the origin's length - // and then ending prematurely not copying everything it should. - let mc := add(add(tempBytes, lengthmod), mul(0x20, iszero(lengthmod))) - let end := add(mc, _length) - - for { - // The multiplication in the next line has the same exact purpose - // as the one above. - let cc := add(add(add(_bytes, lengthmod), mul(0x20, iszero(lengthmod))), _start) - } lt(mc, end) { - mc := add(mc, 0x20) - cc := add(cc, 0x20) - } { mstore(mc, mload(cc)) } - - mstore(tempBytes, _length) - - //update free-memory pointer - //allocating the array padded to 32 bytes like the compiler does now - mstore(0x40, and(add(mc, 31), not(31))) - } - //if we want a zero-length slice let's just return a zero-length array - default { - tempBytes := mload(0x40) - //zero out the 32 bytes slice we are about to return - //we need to do it because Solidity does not garbage collect - mstore(tempBytes, 0) - - mstore(0x40, add(tempBytes, 0x20)) - } - } - - return tempBytes; - } -} diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 905ce423..3a01c351 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -4,16 +4,10 @@ pragma solidity ^0.8.25; import { ERC7579ExecutorBase } from "@rhinestone/modulekit/src/Modules.sol"; import { IERC7579Account } from "erc7579/interfaces/IERC7579Account.sol"; import { IModule } from "erc7579/interfaces/IERC7579Module.sol"; -import { ExecutionLib } from "erc7579/lib/ExecutionLib.sol"; -import { ModeLib } from "erc7579/lib/ModeLib.sol"; - import { IRecoveryModule } from "../interfaces/IRecoveryModule.sol"; import { IEmailRecoveryManager } from "../interfaces/IEmailRecoveryManager.sol"; -import { ISafe } from "../interfaces/ISafe.sol"; -import { BytesLib } from "../libraries/BytesLib.sol"; contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { - using BytesLib for bytes; /*////////////////////////////////////////////////////////////////////////// CONSTANTS //////////////////////////////////////////////////////////////////////////*/ @@ -124,12 +118,12 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { MODULE LOGIC //////////////////////////////////////////////////////////////////////////*/ - function recover(address account, bytes memory recoveryCalldata) external { + function recover(address account, bytes calldata recoveryCalldata) external { if (msg.sender != emailRecoveryManager) { revert NotTrustedRecoveryManager(); } - bytes4 selector = bytes4(recoveryCalldata.slice({ _start: 0, _length: 4 })); + bytes4 selector = bytes4(recoveryCalldata[:4]); address validator = validators[account]; bytes4 allowedSelector = allowedSelectors[validator][account]; @@ -137,7 +131,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { revert InvalidSelector(selector); } - _execute({ account: account, to: validators[account], value: 0, data: recoveryCalldata }); + _execute({ account: account, to: validator, value: 0, data: recoveryCalldata }); } function getTrustedRecoveryManager() external view returns (address) { From 9c7badea33d4d4d4f217a3d1400100c6ff748f9b Mon Sep 17 00:00:00 2001 From: zeroknots Date: Thu, 13 Jun 2024 07:55:49 +0700 Subject: [PATCH 2/4] feat: readability wip --- src/modules/EmailRecoveryModule.sol | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 3a01c351..f3f986ea 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -12,7 +12,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { CONSTANTS //////////////////////////////////////////////////////////////////////////*/ - address public immutable emailRecoveryManager; + address public immutable EMAIL_RECOVERY_MANAGER; event NewValidatorRecovery(address indexed validatorModule, bytes4 recoverySelector); @@ -20,13 +20,14 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { error InvalidSubjectParams(); error InvalidValidator(address validator); error InvalidSelector(bytes4 selector); + error InvalidOnInstallData(); mapping(address validatorModule => mapping(address account => bytes4 allowedSelector)) internal allowedSelectors; mapping(address account => address validator) internal validators; constructor(address _zkEmailRecovery) { - emailRecoveryManager = _zkEmailRecovery; + EMAIL_RECOVERY_MANAGER = _zkEmailRecovery; } modifier withoutUnsafeSelector(bytes4 recoverySelector) { @@ -49,6 +50,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { * @param data The data to initialize the module with */ function onInstall(bytes calldata data) external { + if (data.length == 0) revert InvalidOnInstallData(); ( address validator, bytes4 selector, @@ -59,11 +61,11 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { uint256 expiry ) = abi.decode(data, (address, bytes4, address[], uint256[], uint256, uint256, uint256)); - allowValidatorRecovery(validator, bytes("0"), selector); + _allowValidatorRecovery(validator, bytes("0"), selector); validators[msg.sender] = validator; _execute({ - to: emailRecoveryManager, + to: EMAIL_RECOVERY_MANAGER, value: 0, data: abi.encodeCall( IEmailRecoveryManager.configureRecovery, @@ -72,7 +74,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { }); } - function allowValidatorRecovery( + function _allowValidatorRecovery( address validator, bytes memory isInstalledContext, bytes4 recoverySelector @@ -101,7 +103,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { address validator = validators[msg.sender]; delete allowedSelectors[validator][msg.sender]; delete validators[msg.sender]; - IEmailRecoveryManager(emailRecoveryManager).deInitRecoveryFromModule(msg.sender); + IEmailRecoveryManager(EMAIL_RECOVERY_MANAGER).deInitRecoveryFromModule(msg.sender); } /** @@ -110,8 +112,8 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { * @return true if the module is initialized, false otherwise */ function isInitialized(address smartAccount) external view returns (bool) { - return IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount).threshold - != 0; + return IEmailRecoveryManager(EMAIL_RECOVERY_MANAGER).getGuardianConfig(smartAccount) + .threshold != 0; } /*////////////////////////////////////////////////////////////////////////// @@ -119,7 +121,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { //////////////////////////////////////////////////////////////////////////*/ function recover(address account, bytes calldata recoveryCalldata) external { - if (msg.sender != emailRecoveryManager) { + if (msg.sender != EMAIL_RECOVERY_MANAGER) { revert NotTrustedRecoveryManager(); } @@ -135,7 +137,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { } function getTrustedRecoveryManager() external view returns (address) { - return emailRecoveryManager; + return EMAIL_RECOVERY_MANAGER; } /*////////////////////////////////////////////////////////////////////////// @@ -147,7 +149,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { * @return name The name of the module */ function name() external pure returns (string memory) { - return "EmailRecoveryModule"; + return "ZKEmail.EmailRecoveryModule"; } /** From 8cc744b2f370e8890ef710d6796899a9253a2191 Mon Sep 17 00:00:00 2001 From: zeroknots Date: Thu, 13 Jun 2024 08:00:03 +0700 Subject: [PATCH 3/4] fix: make email_recovery internal since there is an external getter functions --- src/modules/EmailRecoveryModule.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index f3f986ea..64df5f0e 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -12,7 +12,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { CONSTANTS //////////////////////////////////////////////////////////////////////////*/ - address public immutable EMAIL_RECOVERY_MANAGER; + address private immutable EMAIL_RECOVERY_MANAGER; event NewValidatorRecovery(address indexed validatorModule, bytes4 recoverySelector); From 72057f1555d3b29f56f0c6895fbc88db92496424 Mon Sep 17 00:00:00 2001 From: zeroknots Date: Thu, 13 Jun 2024 08:15:37 +0700 Subject: [PATCH 4/4] feat: minor coding best practice improvement wip --- src/EmailRecoveryManager.sol | 7 +++---- src/experimental/EmailAccountRecoveryNew.sol | 12 +++++++++--- src/handlers/EmailRecoverySubjectHandler.sol | 6 +++--- src/handlers/SafeRecoverySubjectHandler.sol | 6 +++--- src/modules/EmailRecoveryModule.sol | 2 +- 5 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/EmailRecoveryManager.sol b/src/EmailRecoveryManager.sol index 1e6e31bc..ebd93e23 100644 --- a/src/EmailRecoveryManager.sol +++ b/src/EmailRecoveryManager.sol @@ -80,10 +80,9 @@ contract EmailRecoveryManager is EmailAccountRecoveryNew, IEmailRecoveryManager address _dkimRegistry, address _emailAuthImpl, address _subjectHandler - ) { - verifierAddr = _verifier; - dkimAddr = _dkimRegistry; - emailAuthImplementationAddr = _emailAuthImpl; + ) + EmailAccountRecoveryNew(_verifier, _dkimRegistry, _emailAuthImpl) + { subjectHandler = _subjectHandler; } diff --git a/src/experimental/EmailAccountRecoveryNew.sol b/src/experimental/EmailAccountRecoveryNew.sol index ed85031e..55260d5e 100644 --- a/src/experimental/EmailAccountRecoveryNew.sol +++ b/src/experimental/EmailAccountRecoveryNew.sol @@ -12,9 +12,15 @@ import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy /// new guardian and recovering a wallet. abstract contract EmailAccountRecoveryNew { uint8 constant EMAIL_ACCOUNT_RECOVERY_VERSION_ID = 1; - address public verifierAddr; - address public dkimAddr; - address public emailAuthImplementationAddr; + address internal immutable verifierAddr; + address internal immutable dkimAddr; + address internal immutable emailAuthImplementationAddr; + + constructor(address _verifierAddr, address _dkimAddr, address _emailAuthImplementationAddr) { + verifierAddr = _verifierAddr; + dkimAddr = _dkimAddr; + emailAuthImplementationAddr = _emailAuthImplementationAddr; + } /// @notice Returns the address of the verifier contract. /// @dev This function is virtual and can be overridden by inheriting contracts. diff --git a/src/handlers/EmailRecoverySubjectHandler.sol b/src/handlers/EmailRecoverySubjectHandler.sol index 34f0303e..237cc20d 100644 --- a/src/handlers/EmailRecoverySubjectHandler.sol +++ b/src/handlers/EmailRecoverySubjectHandler.sol @@ -49,10 +49,10 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { function validateAcceptanceSubject( uint256 templateIdx, - bytes[] memory subjectParams + bytes[] calldata subjectParams ) external - view + pure returns (address) { if (templateIdx != 0) { @@ -70,7 +70,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler { function validateRecoverySubject( uint256 templateIdx, - bytes[] memory subjectParams, + bytes[] calldata subjectParams, address recoveryManager ) public diff --git a/src/handlers/SafeRecoverySubjectHandler.sol b/src/handlers/SafeRecoverySubjectHandler.sol index 4a06fcb2..6906cd3a 100644 --- a/src/handlers/SafeRecoverySubjectHandler.sol +++ b/src/handlers/SafeRecoverySubjectHandler.sol @@ -52,10 +52,10 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { function validateAcceptanceSubject( uint256 templateIdx, - bytes[] memory subjectParams + bytes[] calldata subjectParams ) external - view + pure returns (address) { if (templateIdx != 0) { @@ -73,7 +73,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler { function validateRecoverySubject( uint256 templateIdx, - bytes[] memory subjectParams, + bytes[] calldata subjectParams, address recoveryManager ) public diff --git a/src/modules/EmailRecoveryModule.sol b/src/modules/EmailRecoveryModule.sol index 64df5f0e..f3f986ea 100644 --- a/src/modules/EmailRecoveryModule.sol +++ b/src/modules/EmailRecoveryModule.sol @@ -12,7 +12,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule { CONSTANTS //////////////////////////////////////////////////////////////////////////*/ - address private immutable EMAIL_RECOVERY_MANAGER; + address public immutable EMAIL_RECOVERY_MANAGER; event NewValidatorRecovery(address indexed validatorModule, bytes4 recoverySelector);