Post-audit fixes (#48)

* H1: fix validAfter/validUntil

* H3: include preVerificationGas

* H2: fix unlimited type time range

* M3: strip validator header from signature

* L11: add ERC165 to EOAKeyValidator

* L8: make time ranges non-overlapping

* L1: prevent short calldata on fallbacks

* L4: domain length restriction fix

* L7: ban calls to registry from session

* L9: include storage slot gap in session key validator

* L3: prevent returnbombing from malicious modules

* L5: save only recovery data hash to storage

* M2: add address ownership check to session creation to prevent DoS

* M4+L2: refine registry adapter behavior

* R1: check calltype early for fallbacks

* R9: change _addOwner to be internal

* R11: only emit event when state changed

* R12: tighten session calldata validation

* R14: revert on empty time range

* R6: remove redundant checks

* formatting and linting

* M5: clarification

* H4: hardcode offset pointer value

* M1: add msg.sender to the address proof signed data

Author: ly0va

src/ModularSmartAccount.sol

@@ -124,8 +124,11 @@ contract ModularSmartAccount is IMSA, ExecutionHelper, ERC1271Handler, Initializ
         if (!_isValidatorInstalled(validator)) {
             return ERC7579.VALIDATION_FAILED;
         } else {
+            // Remove the validator header from the signature, to be compatible with 3rd-party validators
+            PackedUserOperation memory userOpStripped = userOp;
+            userOpStripped.signature = userOp.signature[20:];
             // bubble up the return value of the validator module
-            validSignature = ERC7579.IValidator(validator).validateUserOp(userOp, userOpHash);
+            validSignature = ERC7579.IValidator(validator).validateUserOp(userOpStripped, userOpHash);
         }
     }
 

src/core/ModuleManager.sol

@@ -2,6 +2,7 @@
 pragma solidity ^0.8.21;
 
 import { EnumerableSet } from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
+import { LibERC7579 } from "solady/accounts/LibERC7579.sol";
 
 import { RegistryAdapter } from "./RegistryAdapter.sol";
 import "../interfaces/IERC7579Module.sol" as ERC7579;
@@ -16,8 +17,10 @@ abstract contract ModuleManager is RegistryAdapter {
     using EnumerableSet for EnumerableSet.AddressSet;
 
     error InvalidModule(address module);
+    error NotEnoughData();
     error NoFallbackHandler(bytes4 selector);
     error CannotRemoveLastValidator();
+    error InvalidCallType(bytes1 calltype);
     error SelectorAlreadyUsed(bytes4 selector);
     error AlreadyInstalled(address module);
     error NotInstalled(address module);
@@ -61,19 +64,34 @@ abstract contract ModuleManager is RegistryAdapter {
         _;
     }
 
-    /// @dev Handles uninstall failures by bubbling up or emitting `ModuleUnlinked` when forced.
-    /// @param reason Revert data returned by the module.
-    /// @param force Whether the uninstall should proceed even if the module reverts.
-    /// @param moduleTypeId Identifier of the module type.
-    /// @param module Address of the module being removed.
-    function _onUninstallFail(bytes memory reason, bool force, uint256 moduleTypeId, address module) internal {
-        if (!force) {
+    function _uninstallModule(bytes memory deinitData, address module, uint256 moduleTypeId, bool force) internal {
+        bytes memory callData = abi.encodeCall(ERC7579.IModule.onUninstall, deinitData);
+        uint256 gasLimit = force ? gasleft() / 2 : gasleft();
+        bool success;
+        assembly {
+            success := call(gasLimit, module, 0, add(callData, 0x20), mload(callData), 0, 0)
+        }
+        if (success) {
+            return;
+        }
+        if (force) {
+            uint256 copySize;
             assembly {
-                // forward revert data
-                revert(add(reason, 32), mload(reason))
+                copySize := returndatasize()
             }
+            // cap return data size at 256 bytes
+            copySize = copySize > 256 ? 256 : copySize;
+            bytes memory returnData = new bytes(copySize);
+            assembly {
+                returndatacopy(add(returnData, 0x20), 0, copySize)
+            }
+            emit ModuleUnlinked(moduleTypeId, module, returnData);
         } else {
-            emit ModuleUnlinked(moduleTypeId, module, reason);
+            assembly {
+                let size := returndatasize()
+                returndatacopy(0, 0, size)
+                revert(0, size)
+            }
         }
     }
 
@@ -96,10 +114,7 @@ abstract contract ModuleManager is RegistryAdapter {
     function _uninstallValidator(address validator, bytes calldata data, bool force) internal {
         require($moduleManager().$validators.remove(validator), NotInstalled(validator));
         require($moduleManager().$validators.length() > 0, CannotRemoveLastValidator());
-        try ERC7579.IValidator(validator).onUninstall(data) { }
-        catch (bytes memory reason) {
-            _onUninstallFail(reason, force, ERC7579.MODULE_TYPE_VALIDATOR, validator);
-        }
+        _uninstallModule(data, validator, ERC7579.MODULE_TYPE_VALIDATOR, force);
     }
 
     /// @dev Checks whether a validator module is currently installed.
@@ -127,10 +142,7 @@ abstract contract ModuleManager is RegistryAdapter {
     /// @param force Whether failures should be swallowed and logged instead of bubbled.
     function _uninstallExecutor(address executor, bytes calldata data, bool force) internal {
         require($moduleManager().$executors.remove(executor), NotInstalled(executor));
-        try ERC7579.IExecutor(executor).onUninstall(data) { }
-        catch (bytes memory reason) {
-            _onUninstallFail(reason, force, ERC7579.MODULE_TYPE_EXECUTOR, executor);
-        }
+        _uninstallModule(data, executor, ERC7579.MODULE_TYPE_EXECUTOR, force);
     }
 
     /// @dev Checks whether an executor module is currently installed.
@@ -150,6 +162,10 @@ abstract contract ModuleManager is RegistryAdapter {
     function _installFallbackHandler(address handler, bytes calldata params) internal virtual {
         bytes4 selector = bytes4(params[0:4]);
         bytes1 calltype = params[4];
+        require(
+            calltype == LibERC7579.CALLTYPE_SINGLE || calltype == LibERC7579.CALLTYPE_STATICCALL,
+            InvalidCallType(calltype)
+        );
         bytes calldata initData = params[5:];
         require(!_isFallbackHandlerInstalled(selector), SelectorAlreadyUsed(selector));
         $moduleManager().$fallbacks[selector] = FallbackHandler(handler, calltype);
@@ -167,10 +183,7 @@ abstract contract ModuleManager is RegistryAdapter {
         FallbackHandler memory activeFallback = $moduleManager().$fallbacks[selector];
         require(activeFallback.handler == handler, NotInstalled(handler));
         $moduleManager().$fallbacks[selector] = FallbackHandler(address(0), 0);
-        try ERC7579.IFallback(handler).onUninstall(_deInitData) { }
-        catch (bytes memory reason) {
-            _onUninstallFail(reason, force, ERC7579.MODULE_TYPE_FALLBACK, handler);
-        }
+        _uninstallModule(_deInitData, handler, ERC7579.MODULE_TYPE_FALLBACK, force);
     }
 
     /// @dev Checks whether any fallback handler is set for a selector.
@@ -202,6 +215,10 @@ abstract contract ModuleManager is RegistryAdapter {
 
     /// @dev Delegates calls to the registered fallback handler or handles ERC token callbacks.
     fallback() external payable {
+        if (msg.data.length > 0 && msg.data.length < 4) {
+            revert NotEnoughData();
+        }
+
         FallbackHandler storage $fallbackHandler = $moduleManager().$fallbacks[msg.sig];
         address handler = $fallbackHandler.handler;
         bytes1 calltype = $fallbackHandler.calltype;

src/core/RegistryAdapter.sol

@@ -40,10 +40,19 @@ abstract contract RegistryAdapter is AccountBase {
         external
         onlyEntryPointOrSelf
     {
+        IERC7484Registry oldRegistry = $registry;
         $registry = registry;
-        if (attesters.length > 0) {
+        if (address(oldRegistry) != address(0)) {
+            // Clean data from old registry
+            oldRegistry.trustAttesters(0, new address[](0));
+        }
+        if (address(registry) != address(0)) {
             registry.trustAttesters(threshold, attesters);
         }
         emit ERC7484RegistryConfigured(address(registry));
     }
+
+    function getRegistry() external view returns (IERC7484Registry) {
+        return $registry;
+    }
 }

src/interfaces/IMSA.sol

@@ -10,7 +10,8 @@ interface IMSA is IERC7579Account, IERC4337Account {
     // Error thrown when account installs/unistalls module with mismatched input `moduleTypeId`
     error MismatchModuleTypeId(uint256 moduleTypeId);
 
-    /// @dev Initializes the account. Function might be called directly, or by a Factory
+    /// @notice Initializes the account. Function might be called directly, or by a Factory.
+    /// @dev All passed in modules have to be unique, and of exactly one module type.
     /// @param modules Array of module addresses to be installed in the account
     /// @param data Array of initialization data corresponding to each module
     function initializeAccount(address[] calldata modules, bytes[] calldata data) external payable;

src/libraries/SessionLib.sol

@@ -21,14 +21,17 @@ library SessionLib {
     error ZeroSigner();
     error InvalidSigner(address recovered, address expected);
     error InvalidCallType(bytes1 callType, bytes1 expected);
+    error InvalidExecType(bytes1 execType);
     error InvalidTopLevelSelector(bytes4 selector, bytes4 expected);
+    error InvalidData();
     error SessionAlreadyExists(bytes32 sessionHash);
     error UnlimitedFees();
     error SessionExpiresTooSoon(uint256 expiresAt);
     error SessionNotActive();
+    error EmptyTimeRange();
     error LifetimeUsageExceeded(uint256 lifetimeUsage, uint256 maxUsage);
     error AllowanceExceeded(uint256 allowance, uint256 maxAllowance, uint64 period);
-    error InvalidDataLength(uint256 actualLength, uint256 expectedMinimumLength);
+    error InvalidDataLength(uint256 actualLength, uint256 expectedLength);
     error ConditionViolated(bytes32 param, bytes32 refValue, uint8 condition);
     error CallPolicyViolated(address target, bytes4 selector);
     error TransferPolicyViolated(address target);
@@ -159,24 +162,24 @@ library SessionLib {
     /// @dev Reverts if the limit is exceeded or the period is invalid.
     function checkAndUpdate(UsageLimit memory limit, UsageTracker storage tracker, uint256 value, uint48 period)
         internal
-        returns (uint48 validAfter, uint48 validUntil)
+        returns (uint48[2] memory validTimeRange)
     {
+        validTimeRange = [0, type(uint48).max];
         if (limit.limitType == LimitType.Lifetime) {
-            validAfter = 0;
-            validUntil = type(uint48).max;
             require(
                 tracker.lifetimeUsage[msg.sender] + value <= limit.limit,
                 LifetimeUsageExceeded(tracker.lifetimeUsage[msg.sender], limit.limit)
             );
             tracker.lifetimeUsage[msg.sender] += value;
         } else if (limit.limitType == LimitType.Allowance) {
-            validAfter = period * limit.period;
-            validUntil = (period + 1) * limit.period;
             require(
                 tracker.allowanceUsage[period][msg.sender] + value <= limit.limit,
                 AllowanceExceeded(tracker.allowanceUsage[period][msg.sender], limit.limit, period)
             );
             tracker.allowanceUsage[period][msg.sender] += value;
+            uint48 validAfter = period * limit.period;
+            uint48 validUntil = (period + 1) * limit.period - 1;
+            validTimeRange = [validAfter, validUntil];
         }
     }
 
@@ -192,7 +195,7 @@ library SessionLib {
         UsageTracker storage tracker,
         bytes memory data,
         uint48 period
-    ) internal returns (uint48, uint48) {
+    ) internal returns (uint48[2] memory validTimeRange) {
         uint256 expectedLength = 4 + constraint.index * 32 + 32;
         if (data.length < expectedLength) {
             revert InvalidDataLength(data.length, expectedLength);
@@ -237,22 +240,34 @@ library SessionLib {
         PackedUserOperation calldata userOp,
         SessionSpec memory spec,
         uint48 periodId
-    ) internal returns (uint48 validAfter, uint48 validUntil) {
+    ) internal returns (uint48[2] memory validTimeRange) {
         // If a paymaster is paying the fee, we don't need to check the fee limit
         if (userOp.paymasterAndData.length == 0) {
             uint256 gasPrice = userOp.gasPrice();
-            uint256 gasLimit = userOp.unpackVerificationGasLimit() + userOp.unpackCallGasLimit();
+            uint256 gasLimit =
+                userOp.preVerificationGas + userOp.unpackVerificationGasLimit() + userOp.unpackCallGasLimit();
             uint256 fee = gasPrice * gasLimit;
             // slither-disable-next-line unused-return
             return spec.feeLimit.checkAndUpdate(state.fee, fee, periodId);
         } else {
-            return (0, type(uint48).max);
+            return [0, type(uint48).max];
         }
     }
 
-    function shrinkRange(uint48[2] memory range, uint48 newAfter, uint48 newUntil) internal pure {
-        range[0] = newAfter > range[0] ? newAfter : range[0];
-        range[1] = newUntil < range[1] ? newUntil : range[1];
+    /// @notice Shrinks the time range to the intersection of itself and the new range.
+    /// @param range The original time range to shrink: validAfter, validUntil
+    /// @param otherRange The new time range to intersect with: newValidAfter, newValidUntil
+    /// @return newRange The shrunk time range: validAfter, validUntil
+    function shrinkRange(uint48[2] memory range, uint48[2] memory otherRange)
+        internal
+        pure
+        returns (uint48[2] memory newRange)
+    {
+        newRange = [
+            otherRange[0] > range[0] ? otherRange[0] : range[0], // max(validAfter, newValidAfter)
+            otherRange[1] < range[1] ? otherRange[1] : range[1] // min(validUntil, newValidUntil)
+        ];
+        require(newRange[0] <= newRange[1], EmptyTimeRange());
     }
 
     /// @notice Validates the transaction against the session spec and updates the usage trackers.
@@ -272,13 +287,17 @@ library SessionLib {
         PackedUserOperation calldata userOp,
         SessionSpec memory spec,
         uint48[] memory periodIds
-    ) internal returns (uint48, uint48) {
+    ) internal returns (uint48[2] memory validTimeRange) {
         require(state.status[msg.sender] == Status.Active, SessionNotActive());
 
         bytes4 topLevelSelector = bytes4(userOp.callData[:4]);
         bytes1 callType = userOp.callData[4];
+        bytes1 execType = userOp.callData[5];
 
         require(callType == LibERC7579.CALLTYPE_SINGLE, InvalidCallType(callType, LibERC7579.CALLTYPE_SINGLE));
+        require(
+            execType == LibERC7579.EXECTYPE_DEFAULT || execType == LibERC7579.EXECTYPE_TRY, InvalidExecType(execType)
+        );
         require(
             topLevelSelector == IERC7579Account.execute.selector,
             InvalidTopLevelSelector(topLevelSelector, IERC7579Account.execute.selector)
@@ -288,15 +307,20 @@ library SessionLib {
         // - first 4 bytes: selector
         // - next 32 bytes: mode
         // - next 32 bytes: data offset
-        // - at offset: data length
+        // - next 32 bytes: data length
         // - next 32 bytes: data
-        uint256 offset = uint256(bytes32(userOp.callData[36:68])) + 4; // offset does not include the selector
-        uint256 length = uint256(bytes32(userOp.callData[offset:offset + 32]));
+        uint256 offsetOffset = 4 + 32;
+        uint256 lengthOffset = 4 + 32 + 32;
+        uint256 dataOffset = 4 + 32 + 32 + 32;
+        // Offset does not include the selector, hence +4
+        uint256 offset = uint256(bytes32(userOp.callData[offsetOffset:offsetOffset + 32])) + 4;
+        require(offset == lengthOffset, InvalidData());
+        uint256 length = uint256(bytes32(userOp.callData[lengthOffset:lengthOffset + 32]));
         (address target, uint256 value, bytes calldata callData) =
-            LibERC7579.decodeSingle(userOp.callData[offset + 32:offset + 32 + length]);
+            LibERC7579.decodeSingle(userOp.callData[dataOffset:dataOffset + length]);
 
         // Time range within which the transaction is valid.
-        uint48[2] memory timeRange = [0, spec.expiresAt];
+        validTimeRange = [0, spec.expiresAt];
 
         if (callData.length >= 4) {
             bytes4 selector = bytes4(callData[:4]);
@@ -313,17 +337,21 @@ library SessionLib {
 
             require(found, CallPolicyViolated(target, selector));
             require(value <= callPolicy.maxValuePerUse, MaxValueExceeded(value, callPolicy.maxValuePerUse));
-            (uint48 newValidAfter, uint48 newValidUntil) =
-                callPolicy.valueLimit.checkAndUpdate(state.callValue[target][selector], value, periodIds[1]);
-            shrinkRange(timeRange, newValidAfter, newValidUntil);
+            validTimeRange = shrinkRange(
+                validTimeRange,
+                callPolicy.valueLimit.checkAndUpdate(state.callValue[target][selector], value, periodIds[1])
+            );
 
             for (uint256 i = 0; i < callPolicy.constraints.length; ++i) {
-                (newValidAfter, newValidUntil) = callPolicy.constraints[i].checkAndUpdate(
-                    state.params[target][selector][i], callData, periodIds[2 + i]
+                validTimeRange = shrinkRange(
+                    validTimeRange,
+                    callPolicy.constraints[i].checkAndUpdate(
+                        state.params[target][selector][i], callData, periodIds[2 + i]
+                    )
                 );
-                shrinkRange(timeRange, newValidAfter, newValidUntil);
             }
         } else {
+            require(callData.length == 0, InvalidDataLength(callData.length, 0));
             TransferSpec memory transferPolicy;
             bool found = false;
 
@@ -337,12 +365,11 @@ library SessionLib {
 
             require(found, TransferPolicyViolated(target));
             require(value <= transferPolicy.maxValuePerUse, MaxValueExceeded(value, transferPolicy.maxValuePerUse));
-            (uint48 newValidAfter, uint48 newValidUntil) =
-                transferPolicy.valueLimit.checkAndUpdate(state.transferValue[target], value, periodIds[1]);
-            shrinkRange(timeRange, newValidAfter, newValidUntil);
+            validTimeRange = shrinkRange(
+                validTimeRange,
+                transferPolicy.valueLimit.checkAndUpdate(state.transferValue[target], value, periodIds[1])
+            );
         }
-
-        return (timeRange[0], timeRange[1]);
     }
 
     /// @notice Getter for the remainder of a usage limit.