📄 update Natspec security message #69

Aboudjem · 2024-05-15T15:01:48Z

Pull Request for Smart Contract Improvement

Describe your changes:

Link any related issues:

Testing:

Note: Please ensure all tests and lint checks pass before requesting a review. If there are any errors, fix them prior to submission.

Checklist:

I have performed a self-review of my own code.
I have added tests that prove my fix is effective or that my feature works.
I have made corresponding changes to the documentation, if applicable.
My changes generate no new warnings or errors.

github-actions · 2024-05-15T15:03:18Z

🤖 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#L404-L484

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#L404-L484

unused-return

🟡 Impact: Medium
🟡 Confidence: Medium

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

Nexus.sol#L260-L265

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

Nexus.sol#L110-L149

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

Nexus.sol#L94-L103

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

base/ModuleManager.sol#L303-L309

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#L406

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#L77

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.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#L94-L103

ID-10
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#L170-L188

ID-11
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-12
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#L199-L211

ID-13
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#L110-L149

assembly

ℹ️ Impact: Informational
🔴 Confidence: High

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

base/ExecutionHelper.sol#L58-L69

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

base/ExecutionHelper.sol#L126-L138

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

base/Storage.sol#L35-L39

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

Nexus.sol#L240-L244

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

lib/ModeLib.sol#L91-L100

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

Nexus.sol#L404-L484

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

lib/ExecLib.sol#L13-L28

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

Nexus.sol#L527-L550

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

factory/AccountFactory.sol#L75-L92

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

base/BaseAccount.sol#L71-L79

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

Nexus.sol#L71-L87

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

base/ExecutionHelper.sol#L34-L49

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

lib/ModeLib.sol#L122-L126

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

base/BaseAccount.sol#L107-L122

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

factory/AccountFactory.sol#L48-L67

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

base/ModuleManager.sol#L65-L103

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

base/BaseAccount.sol#L84-L96

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

base/ExecutionHelper.sol#L103-L119

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#L154-L158

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#L283-L285

redundant-statements

ℹ️ Impact: Informational
🔴 Confidence: High

ID-39
Redundant expression "additionalContext" inNexus

Nexus.sol#L518

ID-40
Redundant expression "newImplementation" inNexus

Nexus.sol#L320

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#L404-L484

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.

github-actions · 2024-05-15T15:04:44Z

Changes to gas cost

Generated at commit: e4015add0d14f9cbc922d69e8338aefb9435c244, compared to commit: 3833865ae2b83b9a9ce562ff3a2fe5399f617446

🧾 Summary (5% most significant diffs)

Contract	Method	Avg (+/-)	%
Nexus	upgradeToAndCall	-2,557 ✅	-60.92%
MockExecutor	executeBatchViaAccount	-125,595 ✅	-49.22%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
Nexus	4,366,150 (0)	addDeposit execute executeFromExecutor getExecutorsPaginated getValidatorsPaginated initialize installModule isModuleInstalled uninstallModule upgradeToAndCall validateUserOp withdrawDepositTo	2,332 (0) 586 (0) 699 (-2,195) 2,499 (0) 3,019 (0) 2,831 (0) 641 (0) 853 (0) 685 (0) 617 (0) 485 (0) 410 (0)	0.00% 0.00% -75.85% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00%	22,357 (-432) 78,442 (-2,173) 17,732 (-295) 3,012 (+5) 3,858 (-21) 94,253 (-131) 26,002 (-105) 1,275 (+3) 10,549 (-74) 1,640 (-2,557) 13,610 (-6) 33,713 (-347)	-1.90% -2.70% -1.64% +0.17% -0.54% -0.14% -0.40% +0.24% -0.70% -60.92% -0.04% -1.02%	27,832 (0) 11,703 (0) 14,542 (0) 3,062 (0) 3,019 (0) 94,917 (0) 30,205 (0) 1,145 (0) 8,005 (0) 617 (-3,580) 13,912 (0) 37,555 (0)	0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% -85.30% 0.00% 0.00%	27,832 (0) 2,949,253 (-47) 66,094 (0) 3,935 (0) 6,456 (0) 94,923 (0) 39,826 (0) 3,187 (0) 20,532 (0) 7,778 (0) 42,612 (0) 37,555 (0)	0.00% -0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% 0.00%	1,900 (-160) 13,537 (+302) 5,926 (+398) 288 (-7) 294 (-1) 1,045 (+1) 2,987 (+2) 4,285 (-10) 1,042 (+1) 7 (+5) 15,097 (+316) 291 (+3)
MockExecutor	684,527 (0)	executeBatchViaAccount executeViaAccount	36,720 (0) 27,102 (+336)	0.00% +1.26%	129,563 (-125,595) 50,777 (-188)	-49.22% -0.37%	38,374 (0) 47,870 (0)	0.00% 0.00%	952,966 (-69,827) 74,436 (0)	-6.83% 0.00%	24 (-9) 5,889 (+379)
Counter	152,533 (0)	decrementNumber getNumber incrementNumber	2,345 (-1,852) 312 (0) 5,253 (0)	-44.13% 0.00% 0.00%	4,434 (-4) 751 (+52) 17,937 (-792)	-0.09% +7.44% -4.23%	4,197 (0) 312 (0) 22,353 (0)	0.00% 0.00% 0.00%	5,246 (0) 2,312 (0) 22,353 (0)	0.00% 0.00% 0.00%	824 (+78) 1,306 (0) 3,106 (-84)
AccountFactory	1,176,131 (0)	createAccount withdrawTo	0 (0) 2,629 (0)	+∞% 0.00%	192,256 (-41) 23,898 (+1,209)	-0.02% +5.33%	193,564 (-36) 28,262 (0)	-0.02% 0.00%	201,207 (+36) 28,262 (0)	+0.02% 0.00%	270 (-1) 47 (+24)
MockValidator	514,041 (0)	onInstall	2,869 (0)	0.00%	15,770 (+439)	+2.86%	22,769 (0)	0.00%	22,807 (0)	0.00%	4,969 (-135)
MockHandler	304,551 (0)	onGenericFallback	2,644 (0)	0.00%	3,072 (-3)	-0.10%	2,906 (0)	0.00%	25,042 (0)	0.00%	3,081 (-243)
ExecLibTest	1,164,623 (0)	decode	1,352 (0)	0.00%	1,362 (+1)	+0.07%	1,358 (0)	0.00%	1,376 (0)	0.00%	256 (0)

openzeppelin-code · 2024-05-15T15:05:39Z

📄 update Natspec security message

Generated at commit: 1fe328f04593135d21df6981ea6d1acb8be208b9

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 9 27 38
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

📄 update Natspec security message

1fe328f

Aboudjem requested a review from livingrockrises May 15, 2024 15:25

Aboudjem self-assigned this May 15, 2024

livingrockrises merged commit 46548b5 into feat/SMA-828-fuzz-invariant-tests May 15, 2024
8 checks passed

livingrockrises deleted the feat/fix-natspec-security branch May 15, 2024 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

📄 update Natspec security message #69

📄 update Natspec security message #69

Aboudjem 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

📄 update Natspec security message #69

📄 update Natspec security message #69

Conversation

Aboudjem commented May 15, 2024

Pull Request for Smart Contract Improvement

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

github-actions bot commented May 15, 2024

Changes to gas cost

🧾 Summary (5% most significant diffs)

openzeppelin-code bot commented May 15, 2024

📄 update Natspec security message

🚨 Report Summary