fix: prevent validator uninstallation from being blocked by reverting validators (#300)

[MDPN23]

I Want To Contribute

Description

This PR addresses and resolves the Denial of Service (DoS) vulnerability in validator management reported in issue #300. Previously, the uninstallModule() function could be completely blocked if a remaining installed validator had a reverting isInitialized() call, preventing the removal of any other validator.

Changes

• Nexus.sol:
  • Introduced the _safeIsInitialized(address validator) internal helper which wraps the isInitialized() external call in a try/catch block.
  • Updated _checkInitializedValidators() to utilize this safe helper, ensuring that one reverting validator can no longer block the uninstallation transaction of others.
  • Added try/catch handling in _get7739Version() to prevent similar reversion issues during ERC-7739 support detection.
• Mocks (MockRevertingValidator.sol):
  • Created a dedicated mock to simulate a validator that intentionally reverts on isInitialized().
• Tests:
  • Added a regression test test_UninstallValidatorWithRevertingValidator in TestModuleManager_UninstallModule.t.sol to strictly enforce that uninstallation succeeds even in the presence of a reverting validator.

Impact

• Security & Robustness: Eliminates a griefing/DoS vector. Account owners can now successfully manage and uninstall validators even if another installed validator is broken or acts maliciously during liveness checks.

Verification Results

The fix has been thoroughly verified using Foundry. Below is the snapshot of the successful dry run of the new regression test:

forge test --match-test test_UninstallValidatorWithRevertingValidator -vvv

Output Snapshot:

[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol:TestModuleManager_UninstallModule
[PASS] test_UninstallValidatorWithRevertingValidator() (gas: 1282528)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 19.70ms (14.06ms CPU time)

Ran 1 test suite in 21.85ms (19.70ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Checklist

• Code follows the project's style guide.
• New and existing tests pass locally with my changes.
• I have added tests that prove my fix is effective.
• Documentation has been updated (if necessary).

---

PR-Codex overview

This PR primarily focuses on code formatting improvements across various Solidity contracts and test files, enhancing readability and consistency without altering functionality.

Detailed summary

• Adjusted formatting for function declarations and parameters across multiple contracts.
• Added spaces and line breaks for improved readability in function signatures.
• Updated comments for clarity in several contracts.
• Ensured consistent use of backticks for identifiers in comments and documentation.

The following files were skipped due to too many changes: test/foundry/shared/interfaces/IUniswapV2Router01.t.sol, test/foundry/unit/concrete/accountexecution/TestAccountExecution_ExecuteBatch.t.sol, test/foundry/fork/base/TestNexusSwapWETH_Integration.t.sol, test/foundry/utils/TestHelper.t.sol, test/foundry/unit/concrete/accountexecution/TestAccountExecution_DelegateCall.t.sol, test/foundry/integration/TestNexusERC721NFT_Integration_ColdAccess.t.sol, test/foundry/integration/TestNexusERC721NFT_Integration_WarmAccess.t.sol, test/foundry/integration/TestNexusERC20Token_Integration_ColdAccess.t.sol, test/foundry/integration/TestNexusERC20Token_Integration_WarmAccess.t.sol, test/foundry/unit/concrete/modules/TestK1Validator.t.sol, test/foundry/unit/concrete/gas/TestGas_ModuleManager.t.sol, test/foundry/unit/concrete/erc4337account/TestERC4337Account_WithdrawDepositTo.t.sol, contracts/base/ModuleManager.sol, test/foundry/unit/concrete/eip7702/TestPREP.t.sol, test/foundry/unit/concrete/accountexecution/TestComposableExecution.t.sol, test/foundry/unit/concrete/accountexecution/TestAccountExecution_ExecuteFromExecutor.t.sol, test/foundry/unit/concrete/modulemanager/TestModuleManager_InstallModule.t.sol, contracts/utils/NexusBootstrap.sol, test/foundry/unit/fuzz/TestFuzz_ModuleManager.t.sol, test/foundry/unit/concrete/modulemanager/TestModuleManager_UninstallModule.t.sol, test/foundry/unit/concrete/modulemanager/TestModuleManager_FallbackHandler.t.sol, test/foundry/unit/concrete/modulemanager/TestModuleManager_EnableMode.t.sol

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

[openzeppelin-code]

fix: prevent validator uninstallation from being blocked by reverting validators (#300)

Generated at commit: 4122dd35e3a1fb4d52c6997206d4865b1883ecfb

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 7 30 39

---

For more details view the full report in OpenZeppelin Code Inspector