refactor: remove unneeded logic #71

VGabriel45 · 2024-05-15T19:31:35Z

No description provided.

github-actions · 2024-05-15T19:32:33Z

Changes to gas cost

Generated at commit: 1ee7dc2e7cc5eded5f6512dec0dd48c7bda2d4e1, compared to commit: 73771e38dd31bd420a3658eb9eee9b0e50e10850

🧾 Summary (5% most significant diffs)

Contract	Method	Avg (+/-)	%
Nexus	initialize installModule	-698 ✅ -421 ✅	-0.74% -1.50%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
Nexus	4,225,093 (-141,057)	execute initialize installModule	4,865 (0) 2,831 (0) 818 (0)	0.00% 0.00% 0.00%	29,661 (-207) 93,939 (-698) 27,601 (-421)	-0.69% -0.74% -1.50%	32,076 (-706) 94,222 (-695) 30,205 (0)	-2.15% -0.73% 0.00%	97,226 (0) 94,222 (-695) 39,090 (-736)	0.00% -0.73% -1.85%	108 (0) 496 (0) 53 (0)
AccountFactory	1,176,131 (0)	createAccount	0 (0)	+∞%	141,108 (-769)	-0.54%	192,392 (-695)	-0.36%	192,404 (-695)	-0.36%	8 (0)

github-actions · 2024-05-15T19:33:39Z

🤖 Slither Analysis Report 🔎

Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary
🔴 - incorrect-shift (1 results) (High)
🟡 - divide-before-multiply (1 results) (Medium)
🟡 - unused-return (4 results) (Medium)
🟡 - write-after-write (1 results) (Medium)
🔵 - shadowing-local (1 results) (Low)
🔵 - missing-zero-check (1 results) (Low)
🔵 - reentrancy-events (5 results) (Low)
ℹ️ - assembly (18 results) (Informational)
ℹ️ - dead-code (3 results) (Informational)
ℹ️ - low-level-calls (1 results) (Informational)
ℹ️ - missing-inheritance (1 results) (Informational)
ℹ️ - naming-convention (2 results) (Informational)
ℹ️ - redundant-statements (2 results) (Informational)
ℹ️ - too-many-digits (2 results) (Informational)

incorrect-shift

🔴 Impact: High
🔴 Confidence: High

ID-0
Nexus._erc1271HashForIsValidSignatureViaNestedEIP712(bytes32,bytes) contains an incorrect shift operation: d__erc1271HashForIsValidSignatureViaNestedEIP712_asm_1 = d__erc1271HashForIsValidSignatureViaNestedEIP712_asm_1 | 0x120100000001 >> b__erc1271HashForIsValidSignatureViaNestedEIP712_asm_1

Nexus.sol#L395-L467

divide-before-multiply

🟡 Impact: Medium
🟡 Confidence: Medium

ID-1
Nexus._erc1271HashForIsValidSignatureViaNestedEIP712(bytes32,bytes) performs a multiplication on the result of a division:
- calldataload(uint256)(signature + signature - 0x20) == 0x6492 * ~ mload(uint256)(0x60) / 0xffff

Nexus.sol#L395-L467

unused-return

🟡 Impact: Medium
🟡 Confidence: Medium

ID-2
Nexus.execute(ExecutionMode,bytes) ignores return value by (callType,execType) = mode.decode()

Nexus.sol#L95-L104

ID-3
Nexus.supportsExecutionMode(ExecutionMode) ignores return value by (callType,execType) = mode.decode()

Nexus.sol#L261-L267

ID-4
Nexus.executeFromExecutor(ExecutionMode,bytes) ignores return value by (callType,execType) = mode.decode()

Nexus.sol#L111-L150

ID-5
ModuleManager._paginate(SentinelListLib.SentinelList,address,uint256) ignores return value by list.getEntriesPaginated(cursor,size)

base/ModuleManager.sol#L292-L298

write-after-write

🟡 Impact: Medium
🔴 Confidence: High

ID-6
Nexus._erc1271HashForIsValidSignatureViaNestedEIP712(bytes32,bytes).signature is written in both
signature = calldataload(uint256)(o__erc1271HashForIsValidSignatureViaNestedEIP712_asm_0)
signature = o__erc1271HashForIsValidSignatureViaNestedEIP712_asm_0 + 0x20

Nexus.sol#L395

shadowing-local

🔵 Impact: Low
🔴 Confidence: High

ID-7
Nexus.validateUserOp(PackedUserOperation,bytes32,uint256).nonce shadows:
- BaseAccount.nonce(uint192) (function)
- IBaseAccount.nonce(uint192) (function)

Nexus.sol#L78

missing-zero-check

🔵 Impact: Low
🟡 Confidence: Medium

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

factory/AccountFactory.sol#L38

reentrancy-events

🔵 Impact: Low
🟡 Confidence: Medium

ID-9
Reentrancy in Nexus.installModule(uint256,address,bytes):
External calls:
- _installValidator(module,initData)
  - IValidator(validator).onInstall(data)
- _installExecutor(module,initData)
  - IExecutor(executor).onInstall(data)
- _installFallbackHandler(module,initData)
  - IFallback(handler).onInstall(initData)
- _installHook(module,initData)
  - IHook(hook).onInstall(data)
- withHook()
  - hookData = IHook(hook).preCheck(msg.sender,msg.value,msg.data)
  - IHook(hook).postCheck(hookData)
    Event emitted after the call(s):
- ModuleInstalled(moduleTypeId,module)

Nexus.sol#L171-L189

ID-10
Reentrancy in Nexus.execute(ExecutionMode,bytes):
External calls:
- withHook()
  - hookData = IHook(hook).preCheck(msg.sender,msg.value,msg.data)
  - IHook(hook).postCheck(hookData)
    Event emitted after the call(s):
- TryExecuteUnsuccessful(i,result[i])
  - _handleBatchExecution(executionCalldata,execType)

Nexus.sol#L95-L104

ID-11
Reentrancy in Nexus.executeFromExecutor(ExecutionMode,bytes):
External calls:
- withHook()
  - hookData = IHook(hook).preCheck(msg.sender,msg.value,msg.data)
  - IHook(hook).postCheck(hookData)
    Event emitted after the call(s):
- TryExecuteUnsuccessful(i,result[i])
  - returnData = _tryExecuteBatch(executions)
- TryExecuteUnsuccessful(0,returnData[0])

Nexus.sol#L111-L150

ID-12
Reentrancy in AccountFactory.createAccount(address,bytes,uint256):
External calls:
- INexus(account).initialize(validationModule,moduleInstallData)
  Event emitted after the call(s):
- AccountCreated(account,validationModule,moduleInstallData)

factory/AccountFactory.sol#L48-L67

ID-13
Reentrancy in Nexus.uninstallModule(uint256,address,bytes):
External calls:
- _uninstallValidator(module,deInitData)
  - IValidator(validator).onUninstall(disableModuleData)
- _uninstallExecutor(module,deInitData)
  - IExecutor(executor).onUninstall(disableModuleData)
- _uninstallFallbackHandler(module,deInitData)
  - IFallback(fallbackHandler).onUninstall(deInitData)
- _uninstallHook(module,deInitData)
  - IHook(hook).onUninstall(data)
    Event emitted after the call(s):
- ModuleUninstalled(moduleTypeId,module)

Nexus.sol#L200-L212

assembly

ℹ️ Impact: Informational
🔴 Confidence: High

ID-14
Nexus.getImplementation() uses assembly
- INLINE ASM

Nexus.sol#L241-L245

ID-15
ExecutionHelper._tryExecute(address,uint256,bytes) uses assembly
- INLINE ASM

base/ExecutionHelper.sol#L58-L69

ID-16
ExecutionHelper._tryExecuteDelegatecall(address,bytes) uses assembly
- INLINE ASM

base/ExecutionHelper.sol#L126-L138

ID-17
ModuleManager.fallback() uses assembly
- INLINE ASM
- INLINE ASM

base/ModuleManager.sol#L64-L102

ID-18
Storage._getAccountStorage() uses assembly
- INLINE ASM

base/Storage.sol#L35-L39

ID-19
ModeLib.decode(ExecutionMode) uses assembly
- INLINE ASM

lib/ModeLib.sol#L91-L100

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

lib/ExecLib.sol#L13-L28

ID-21
AccountFactory.getCounterFactualAddress(address,bytes,uint256) uses assembly
- INLINE ASM

factory/AccountFactory.sol#L75-L92

ID-22
BaseAccount.addDeposit() uses assembly
- INLINE ASM

base/BaseAccount.sol#L71-L79

ID-23
Nexus._typedDataSignFields() uses assembly
- INLINE ASM

Nexus.sol#L520-L543

ID-24
ExecutionHelper._execute(address,uint256,bytes) uses assembly
- INLINE ASM

base/ExecutionHelper.sol#L34-L49

ID-25
ModeLib.getCallType(ExecutionMode) uses assembly
- INLINE ASM

lib/ModeLib.sol#L122-L126

ID-26
BaseAccount.getDeposit() uses assembly
- INLINE ASM

base/BaseAccount.sol#L107-L122

ID-27
Nexus.validateUserOp(PackedUserOperation,bytes32,uint256) uses assembly
- INLINE ASM

Nexus.sol#L72-L88

ID-28
AccountFactory.createAccount(address,bytes,uint256) uses assembly
- INLINE ASM

factory/AccountFactory.sol#L48-L67

ID-29
BaseAccount.withdrawDepositTo(address,uint256) uses assembly
- INLINE ASM

base/BaseAccount.sol#L84-L96

ID-30
ExecutionHelper._executeDelegatecall(address,bytes) uses assembly
- INLINE ASM

base/ExecutionHelper.sol#L103-L119

ID-31
Nexus._erc1271HashForIsValidSignatureViaNestedEIP712(bytes32,bytes) uses assembly
- INLINE ASM
- INLINE ASM

Nexus.sol#L395-L467

dead-code

ℹ️ Impact: Informational
🟡 Confidence: Medium

ID-32
ExecutionHelper._executeDelegatecall(address,bytes) is never used and should be removed

base/ExecutionHelper.sol#L103-L119

ID-33
ExecutionHelper._tryExecuteDelegatecall(address,bytes) is never used and should be removed

base/ExecutionHelper.sol#L126-L138

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

lib/ModeLib.sol#L141-L143

low-level-calls

ℹ️ Impact: Informational
🔴 Confidence: High

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

Nexus.sol#L155-L159

missing-inheritance

ℹ️ Impact: Informational
🔴 Confidence: High

ID-36
K1Validator should inherit from IValidator

modules/validators/K1Validator.sol#L32-L88

naming-convention

ℹ️ Impact: Informational
🔴 Confidence: High

ID-37
Variable AccountFactory.ACCOUNT_IMPLEMENTATION is not in mixedCase

factory/AccountFactory.sol#L34

ID-38
Function Nexus.DOMAIN_SEPARATOR() is not in mixedCase

Nexus.sol#L481-L483

redundant-statements

ℹ️ Impact: Informational
🔴 Confidence: High

ID-39
Redundant expression "newImplementation" inNexus

Nexus.sol#L311

ID-40
Redundant expression "additionalContext" inNexus

Nexus.sol#L511

too-many-digits

ℹ️ Impact: Informational
🟡 Confidence: Medium

ID-41
Nexus._erc1271HashForIsValidSignatureViaNestedEIP712(bytes32,bytes) uses literals with too many digits:
- d__erc1271HashForIsValidSignatureViaNestedEIP712_asm_1 = d__erc1271HashForIsValidSignatureViaNestedEIP712_asm_1 | 0x120100000001 >> b__erc1271HashForIsValidSignatureViaNestedEIP712_asm_1

Nexus.sol#L395-L467

ID-42
BaseAccount.withdrawDepositTo(address,uint256) uses literals with too many digits:
- mstore(uint256,uint256)(0x00,0x205c2878000000000000000000000000)

base/BaseAccount.sol#L84-L96

This comment was automatically generated by the GitHub Actions workflow.

openzeppelin-code · 2024-05-15T19:35:34Z

refactor: remove unneeded logic

Generated at commit: 98c67907dab7329ca7df146c1eb444cb8132b1fc

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 9 28 39
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

Aboudjem · 2024-05-15T19:57:31Z

Thanks Gabi good catch 🚀 it's all all good but to merge we need branch name to start by feat|fix|release|chore, it's fine I'll do it tomorrow

removed unneeded module checks and custom errors

98c6790

VGabriel45 requested review from Aboudjem and livingrockrises May 15, 2024 19:33

Aboudjem approved these changes May 15, 2024

View reviewed changes

livingrockrises approved these changes May 15, 2024

View reviewed changes

livingrockrises merged commit 592b025 into dev May 16, 2024
7 of 8 checks passed

livingrockrises deleted the refactor/remove_unneeded_logic branch May 16, 2024 12:41

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove unneeded logic #71

refactor: remove unneeded logic #71

VGabriel45 commented May 15, 2024

github-actions bot commented May 15, 2024

github-actions bot commented May 15, 2024

openzeppelin-code bot commented May 15, 2024

Aboudjem commented May 15, 2024 •

edited

Loading

refactor: remove unneeded logic #71

refactor: remove unneeded logic #71

Conversation

VGabriel45 commented May 15, 2024

github-actions bot commented May 15, 2024

Changes to gas cost

🧾 Summary (5% most significant diffs)

github-actions bot commented May 15, 2024

🤖 Slither Analysis Report 🔎

Slither report

Slither report

incorrect-shift

divide-before-multiply

unused-return

write-after-write

shadowing-local

missing-zero-check

reentrancy-events

assembly

dead-code

low-level-calls

missing-inheritance

naming-convention

redundant-statements

too-many-digits

openzeppelin-code bot commented May 15, 2024

refactor: remove unneeded logic

🚨 Report Summary

Aboudjem commented May 15, 2024 • edited Loading

Aboudjem commented May 15, 2024 •

edited

Loading