leave execution contract abstract + use interface in msa

[livingrockrises]

Abstract contracts methods are left virtual empty
Abstract contracts implement tiny interfaces
these tiny interfaces make up huge interface for IModularSmartAccount which is reused in implementation (in order to do this methods are overridden by specifying abstract class and interface both)

[github-actions]

🤖 Slither Analysis Report 🔎

Slither report

Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary
🔴 - name-reused (1 results) (High)
🟡 - unused-return (3 results) (Medium)
🔵 - shadowing-local (1 results) (Low)
🔵 - missing-zero-check (1 results) (Low)
🔵 - reentrancy-events (2 results) (Low)
ℹ️ - assembly (9 results) (Informational)
ℹ️ - dead-code (6 results) (Informational)
ℹ️ - low-level-calls (1 results) (Informational)
ℹ️ - naming-convention (1 results) (Informational)
ℹ️ - redundant-statements (1 results) (Informational)

name-reused

🔴 Impact: High
🔴 Confidence: High

• ID-0
  IModuleManager is re-used:
  • IModuleManager
  • IModuleManager

interfaces/base/IHookManager.sol#L8-L11

unused-return

🟡 Impact: Medium
🟡 Confidence: Medium

• ID-1
  SmartAccount.execute(ModeCode,bytes) ignores return value by (callType,execType) = mode.decode()

SmartAccount.sol#L44-L75

• ID-2
  SmartAccount.executeFromExecutor(ModeCode,bytes) ignores return value by (callType,execType) = mode.decode()

SmartAccount.sol#L77-L127

• ID-3
  SmartAccount.supportsExecutionMode(ModeCode) ignores return value by (callType,execType) = mode.decode()

SmartAccount.sol#L175-L189

shadowing-local

🔵 Impact: Low
🔴 Confidence: High

• ID-4
  SmartAccount.validateUserOp(PackedUserOperation,bytes32,uint256).nonce shadows:
  • ERC4337Account.nonce(uint192) (function)

SmartAccount.sol#L31

missing-zero-check

🔵 Impact: Low
🟡 Confidence: Medium

• ID-5
  AccountFactory.constructor(address).implementation lacks a zero-check on :
  - ACCOUNT_IMPLEMENTATION = implementation

factory/AccountFactory.sol#L11

reentrancy-events

🔵 Impact: Low
🟡 Confidence: Medium

• ID-6
  Reentrancy in SmartAccount.installModule(uint256,address,bytes):
  External calls:
  • _installValidator(module,initData)
    • IValidator(validator).onInstall(data)
  • _installExecutor(module,initData)
    • IExecutor(executor).onInstall(data)
      Event emitted after the call(s):
  • ModuleInstalled(moduleTypeId,module)

SmartAccount.sol#L140-L152

• ID-7
  Reentrancy in SmartAccount.uninstallModule(uint256,address,bytes):
  External calls:
  • _uninstallValidator(module,deInitData)
    • IValidator(validator).onUninstall(disableModuleData)
  • _uninstallExecutor(module,deInitData)
    • IExecutor(executor).onUninstall(disableModuleData)
      Event emitted after the call(s):
  • ModuleUninstalled(moduleTypeId,module)

SmartAccount.sol#L154-L165

assembly

ℹ️ Impact: Informational
🔴 Confidence: High

• ID-8
  AccountExecution._executeDelegatecall(address,bytes) uses assembly
  • INLINE ASM

base/AccountExecution.sol#L87-L103

• ID-9
  Storage._getAccountStorage() uses assembly
  • INLINE ASM

base/Storage.sol#L18-L22

• ID-10
  SmartAccount.validateUserOp(PackedUserOperation,bytes32,uint256) uses assembly
  • INLINE ASM

SmartAccount.sol#L25-L42

• ID-11
  AccountExecution._tryExecute(address,uint256,bytes) uses assembly
  • INLINE ASM

base/AccountExecution.sol#L69-L84

• ID-12
  AccountExecution._execute(address,uint256,bytes) uses assembly
  • INLINE ASM

base/AccountExecution.sol#L48-L67

• ID-13
  ModeLib.getCallType(ModeCode) uses assembly
  • INLINE ASM

lib/ModeLib.sol#L120-L124

• ID-14
  AccountExecution._tryExecuteDelegatecall(address,bytes) uses assembly
  • INLINE ASM

base/AccountExecution.sol#L106-L121

• ID-15
  ModeLib.decode(ModeCode) uses assembly
  • INLINE ASM

lib/ModeLib.sol#L87-L100

• ID-16
  ExecLib.decodeBatch(bytes) uses assembly
  • INLINE ASM

lib/ExecLib.sol#L11-L27

dead-code

ℹ️ Impact: Informational
🟡 Confidence: Medium

• ID-17
  ModuleManager._isAlreadyInitialized() is never used and should be removed

base/ModuleManager.sol#L92-L96

• ID-18
  _eqModeSelector(ModeSelector,ModeSelector) is never used and should be removed

lib/ModeLib.sol#L139-L141

• ID-19
  AccountExecution._executeDelegatecall(address,bytes) is never used and should be removed

base/AccountExecution.sol#L87-L103

• ID-20
  AccountExecution._tryExecute(Execution[]) is never used and should be removed

base/AccountExecution.sol#L36-L46

• ID-21
  AccountExecution._tryExecute(address,uint256,bytes) is never used and should be removed

base/AccountExecution.sol#L69-L84

• ID-22
  AccountExecution._tryExecuteDelegatecall(address,bytes) is never used and should be removed

base/AccountExecution.sol#L106-L121

low-level-calls

ℹ️ Impact: Informational
🔴 Confidence: High

• ID-23
  🔵 Low level call in SmartAccount.executeUserOp(PackedUserOperation,bytes32):
  • (success) = address(this).delegatecall(callData)

SmartAccount.sol#L131-L138

naming-convention

ℹ️ Impact: Informational
🔴 Confidence: High

• ID-24
  Variable AccountFactory.ACCOUNT_IMPLEMENTATION is not in mixedCase

factory/AccountFactory.sol#L9

redundant-statements

ℹ️ Impact: Informational
🔴 Confidence: High

• ID-25
  Redundant expression "additionalContext" inSmartAccount

SmartAccount.sol#L196

This comment was automatically generated by the GitHub Actions workflow.

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 26 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (feat/phase-1-implementations@9a8e952). Click here to learn what that means.

Files	Patch %	Lines
contracts/SmartAccount.sol	38.23%	21 Missing ⚠️
contracts/base/ERC4337Account.sol	0.00%	4 Missing ⚠️
contracts/lib/ExecLib.sol	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                       @@
##             feat/phase-1-implementations      #24   +/-   ##
===============================================================
  Coverage                                ?   34.84%           
===============================================================
  Files                                   ?       11           
  Lines                                   ?      221           
  Branches                                ?       28           
===============================================================
  Hits                                    ?       77           
  Misses                                  ?      144           
  Partials                                ?        0           

Flag	Coverage Δ
foundry	34.84% <33.33%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[livingrockrises]

Abstract contracts methods are left virtual empty

We could try and implement most of the methods and leave some and still keep it abstract
If all can be implemented, still leave virtual and make contract non-abstract (discussed with Adam)

some methods require modifiers from other abstract classes or defined in child class hence they could not be fully implemented.

what else can be done is implement methods with inner internal methods that can be left virtual / revert("not implemented") if modifier is not a problem.