Skip to content

[BUG] Validator uninstallation can be blocked by a reverting isInitialized() call in a remaining validator #300

Description

@Odhiambo526

Summary

uninstallModule() does call _uninstallValidator() and then immediately runs _checkInitializedValidators() for validator removals. That helper performs raw external calls to IValidator(...).isInitialized(address(this)) with no try/catch, so any revert in a remaining validator aborts the entire uninstall transaction.

Steps to Reproduce

In Nexus.uninstallModule(), the validator removal branch first calls _uninstallValidator(module, deInitData) and then immediately calls _checkInitializedValidators(). That helper only runs when the account is not an ERC-7702 account and the default validator is not initialized, and it iterates the remaining validators with direct external calls to IValidator(next).isInitialized(address(this)) without any try/catch or fallback handling. If any remaining validator reverts in isInitialized(), runs out of gas, or otherwise fails, the entire uninstallModule() transaction reverts, which rolls back the earlier storage changes as well. The result is that validator removal cannot complete while the failing validator remains installed in the set being checked.

Impact

validator-management denial of service on accounts where the default validator is not initialized. any installed validator whose isInitialized() call reverts can prevent successful removal of validators, because the uninstall transaction cannot pass the post-removal liveness check. This does not partially remove state and then continue, the whole transaction reverts, so the uninstall does not finalize. A practical case is an account that has multiple validators installed and one remaining validator implements isInitialized() in a way that reverts; attempts to uninstall any validator while that failing validator is still among the remaining validators will revert at the _checkInitializedValidators() step.

    function test_UninstallValidatorWithRevertingValidator() public {
        MockRevertingValidator revertingValidator = new MockRevertingValidator();
        MockValidator normalValidator = new MockValidator();

        bytes memory installReverting = abi.encodeWithSelector(
            IModuleManager.installModule.selector,
            MODULE_TYPE_VALIDATOR,
            address(revertingValidator),
            ""
        );
        installModule(installReverting, MODULE_TYPE_VALIDATOR, address(revertingValidator), EXECTYPE_DEFAULT);

        bytes memory installNormal = abi.encodeWithSelector(
            IModuleManager.installModule.selector,
            MODULE_TYPE_VALIDATOR,
            address(normalValidator),
            abi.encodePacked(BOB.addr)
        );
        installModule(installNormal, MODULE_TYPE_VALIDATOR, address(normalValidator), EXECTYPE_DEFAULT);

        assertTrue(BOB_ACCOUNT.isModuleInstalled(MODULE_TYPE_VALIDATOR, address(revertingValidator), ""));
        assertTrue(BOB_ACCOUNT.isModuleInstalled(MODULE_TYPE_VALIDATOR, address(normalValidator), ""));

        assertFalse(DEFAULT_VALIDATOR_MODULE.isInitialized(address(BOB_ACCOUNT)));

        (address[] memory validators, ) = BOB_ACCOUNT.getValidatorsPaginated(address(0x1), 100);
        address remove = address(normalValidator);
        address prev = SentinelListHelper.findPrevious(validators, remove);
        if (prev == address(0)) prev = address(0x01);

        bytes memory uninstallData = abi.encodeWithSelector(
            IModuleManager.uninstallModule.selector,
            MODULE_TYPE_VALIDATOR,
            address(normalValidator),
            abi.encode(prev, "")
        );

        Execution[] memory execution = new Execution[](1);
        execution[0] = Execution(address(BOB_ACCOUNT), 0, uninstallData);
        PackedUserOperation[] memory userOps = buildPackedUserOperation(BOB, BOB_ACCOUNT, EXECTYPE_DEFAULT, execution, address(VALIDATOR_MODULE), 0);

        ENTRYPOINT.handleOps(userOps, payable(BOB.addr));

        assertTrue(BOB_ACCOUNT.isModuleInstalled(MODULE_TYPE_VALIDATOR, address(normalValidator), ""));
        assertTrue(BOB_ACCOUNT.isModuleInstalled(MODULE_TYPE_VALIDATOR, address(revertingValidator), ""));
    }
forge test --match-test test_UninstallValidatorWithRevertingValidator -vvv
[⠊] 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)

Expected vs. Actual Behavior

Expected Behaviour

Uninstalling the normal validator should succeed, even if another installed validator has a reverting isInitialized() call. After uninstall:
-> The normal validator should be uninstalled.
->The reverting validator should still be installed.

Actual Behavior

When uninstalling the normal validator, the whole transaction reverts entirely because of the reverting isInitialized() call on the remaining validator. After uninstall attempt both validators are still installed. The normal validator was never removed.

Environment

foundry

Code of Conduct

  • I agree to follow this project's Code of Conduct.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinghelp wantedExtra attention is needed

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions