fix: STAA-12 remediation commit iv

Author: livingrockrises

src/StartaleSmartAccount.sol

@@ -2,21 +2,14 @@
 pragma solidity ^0.8.29;
 
 import {BaseAccount} from './core/BaseAccount.sol';
-
 import {ERC7779Adapter} from './core/ERC7779Adapter.sol';
 import {ExecutionHelper} from './core/ExecutionHelper.sol';
 import {ModuleManager} from './core/ModuleManager.sol';
-
 import {IERC7579Account} from './interfaces/IERC7579Account.sol';
 import {IValidator} from './interfaces/IERC7579Module.sol';
 import {IStartaleSmartAccount} from './interfaces/IStartaleSmartAccount.sol';
 import {IAccountConfig} from './interfaces/core/IAccountConfig.sol';
 import {ExecutionLib} from './lib/ExecutionLib.sol';
-import {ACCOUNT_STORAGE_LOCATION} from './types/Constants.sol';
-import {IERC1271} from '@openzeppelin/contracts/interfaces/IERC1271.sol';
-import {IERC1155Receiver} from '@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol';
-import {IERC721Receiver} from '@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol';
-import {IERC165} from '@openzeppelin/contracts/utils/introspection/IERC165.sol';
 
 import {Initializable} from './lib/Initializable.sol';
 import {
@@ -31,6 +24,7 @@ import {
   ModeLib
 } from './lib/ModeLib.sol';
 import {NonceLib} from './lib/NonceLib.sol';
+import {ACCOUNT_STORAGE_LOCATION} from './types/Constants.sol';
 import {
   MODULE_TYPE_EXECUTOR,
   MODULE_TYPE_FALLBACK,
@@ -43,11 +37,13 @@ import {
   VALIDATION_FAILED,
   VALIDATION_SUCCESS
 } from './types/Constants.sol';
-
 import {EmergencyUninstall} from './types/Structs.sol';
-
 import {_packValidationData} from '@account-abstraction/core/Helpers.sol';
 import {PackedUserOperation} from '@account-abstraction/interfaces/PackedUserOperation.sol';
+import {IERC1271} from '@openzeppelin/contracts/interfaces/IERC1271.sol';
+import {IERC1155Receiver} from '@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol';
+import {IERC721Receiver} from '@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol';
+import {IERC165} from '@openzeppelin/contracts/utils/introspection/IERC165.sol';
 import {SENTINEL, SentinelListLib, ZERO_ADDRESS} from 'sentinellist/SentinelList.sol';
 import {UUPSUpgradeable} from 'solady/utils/UUPSUpgradeable.sol';
 
@@ -338,11 +334,10 @@ contract StartaleSmartAccount is
   /// @dev Uninstalls all validators, executors, hooks, and pre-validation hooks.
   /// @notice It is worth noting that, onRedelegation() does not obligate the account to completely wipe out it’s storage.
   /// @notice It is an optional action for the account where it could uninitialize the storage variables as much as it can to provide clean storage for new wallet.
-  /// Review: _onRedelegation. If ERC authors agrees with the change then we could have bytes calldata context as param.
   function _onRedelegation() internal override {
-    _tryUninstallValidators();
-    _tryUninstallExecutors();
-    _tryUninstallHook(_getHook());
+    _uninstallAllValidators();
+    _uninstallAllExecutors();
+    _uninstallHook(_getHook());
     _tryUninstallPreValidationHook(
       _getPreValidationHook(MODULE_TYPE_PREVALIDATION_HOOK_ERC1271), MODULE_TYPE_PREVALIDATION_HOOK_ERC1271
     );

src/core/ModuleManager.sol

@@ -245,20 +245,10 @@ abstract contract ModuleManager is AllStorage, EIP712, IModuleManager {
     }
   }
 
-  /// Review: _tryUninstallValidators Might as well not call onUninstall() at all
-  /// Review: separate internal method can be added that accepts an array of data.
   /// @dev Uninstalls all validators from the smart account.
   /// @dev This function is called in the _onRedelegation function in StartaleSmartAccount.sol
-  function _tryUninstallValidators() internal {
+  function _uninstallAllValidators() internal {
     SentinelListLib.SentinelList storage $valdiators = _getAccountStorage().validators;
-    address validator = $valdiators.getNext(SENTINEL);
-    while (validator != SENTINEL) {
-      try IValidator(validator).onUninstall('') {}
-      catch {
-        emit ValidatorUninstallFailed(validator, '');
-      }
-      validator = $valdiators.getNext(validator);
-    }
     $valdiators.popAll();
   }
 
@@ -289,20 +279,10 @@ abstract contract ModuleManager is AllStorage, EIP712, IModuleManager {
     }
   }
 
-  /// Review: _tryUninstallExecutors Might as well not call onUninstall() at all
-  /// Review: separate internal method can be added that accepts an array of data.
   /// @dev Uninstalls all executors from the smart account.
   /// @dev This function is called in the _onRedelegation function in StartaleSmartAccount.sol
-  function _tryUninstallExecutors() internal {
+  function _uninstallAllExecutors() internal {
     SentinelListLib.SentinelList storage $executors = _getAccountStorage().executors;
-    address executor = $executors.getNext(SENTINEL);
-    while (executor != SENTINEL) {
-      try IExecutor(executor).onUninstall('') {}
-      catch {
-        emit ExecutorUninstallFailed(executor, '');
-      }
-      executor = $executors.getNext(executor);
-    }
     $executors.popAll();
   }
 
@@ -337,20 +317,14 @@ abstract contract ModuleManager is AllStorage, EIP712, IModuleManager {
     }
   }
 
-  /// Review: _tryUninstallHook
   /// @dev Uninstalls a hook module.
   /// @param hook The address of the hook to be uninstalled.
-  function _tryUninstallHook(address hook) internal virtual {
+  function _uninstallHook(address hook) internal virtual {
     if (hook != address(0)) {
-      try IHook(hook).onUninstall('') {}
-      catch {
-        emit HookUninstallFailed(hook, '');
-      }
       _setHook(address(0));
     }
   }
 
-  // Review: if uninstalling selectors also need some data.
   function _tryUninstallFallbacks() internal {
     AccountStorage storage ds = _getAccountStorage();
     uint256 len = ds.fallbackSelectors.length;
@@ -556,14 +530,13 @@ abstract contract ModuleManager is AllStorage, EIP712, IModuleManager {
     uint256 hookType,
     bytes calldata data
   ) internal virtual {
-    // Review
-    // Check if the hook is installed according to supplied type
-    // Check if the hook is one of the allowed types
-    // IModule(preValidationHook).onUninstall(data);
     _setPreValidationHook(hookType, address(0));
+    try IModule(preValidationHook).onUninstall(data) {}
+    catch {
+      emit PreValidationHookUninstallFailed(preValidationHook, data);
+    }
   }
 
-  // Review: _tryUninstallPreValidationHook
   function _tryUninstallPreValidationHook(address hook, uint256 hookType) internal virtual {
     if (hook == address(0)) return;
     if (hookType == MODULE_TYPE_PREVALIDATION_HOOK_ERC1271) {

src/modules/validators/ECDSAValidator.sol

@@ -139,7 +139,6 @@ contract ECDSAValidator is IValidator, ERC7739Validator {
    * @param _newOwner The address of the new owner
    */
   function transferOwnership(address _newOwner) external {
-    // Review: other checks on newOwner address
     if (_newOwner == address(0)) {
       revert OwnerCannotBeZeroAddress();
     }

src/types/Constants.sol

@@ -13,11 +13,6 @@ uint256 constant VALIDATION_SUCCESS = 0;
 // Value indicating failed validation
 uint256 constant VALIDATION_FAILED = 1;
 
-// Review: Alternate naming
-// Todo: Maybe arrange constants in category of their context
-// uint256 constant SIG_VALIDATION_FAILED_UINT = 1;
-// uint256 constant SIG_VALIDATION_SUCCESS_UINT = 0;
-
 // Module type identifier for Multitype install
 uint256 constant MODULE_TYPE_MULTI = 0;
 

test/foundry/unit/concrete/execution/TestExecution_ExecuteFromExecutor.t.sol

@@ -47,7 +47,6 @@ contract TestExecution_ExecuteFromExecutor is TestExecutionBase {
   }
 
   /// @notice Tests delegate call execution via MockExecutor
-  // Review
   function test_ExecuteDelegateCallFromExecutor_Success() public {
     (bool res,) = payable(address(BOB_ACCOUNT)).call{value: 2 ether}(''); // Fund BOB_ACCOUNT
     assertEq(res, true, 'Funding BOB_ACCOUNT should succeed');