Biconomy: Nexus
Biconomy
HardhatFoundry
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Any module can stop their own removal

EgisSecurity

Summary

ModuleManager handles all logic with modules, including installing and uninstalling them from the manager. There are 4 types of modules: Validator, Executor, Hook and Fallback. All of them have similar installing and uninstalling logic, so we'll use the Validator module's installation and uninstallation functions for the example.

function _installValidator(address validator, bytes calldata data) internal virtual withRegistry(validator, MODULE_TYPE_VALIDATOR) {
if (!IValidator(validator).isModuleType(MODULE_TYPE_VALIDATOR)) revert MismatchModuleTypeId(MODULE_TYPE_VALIDATOR);
_getAccountStorage().validators.push(validator);
IValidator(validator).onInstall(data);
}

Vulnerability Details

You can see when the validator is installed, onInstall is a callback that is called on the validator at the end of the function. The same happens when _uninstallValidator is called:

function _uninstallValidator(address validator, bytes calldata data) internal virtual {
SentinelListLib.SentinelList storage validators = _getAccountStorage().validators;
​
(address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes));
​
// Check if the account has at least one validator installed before proceeding
// Having at least one validator is a requirement for the account to function properly
require(!(prev == address(0x01) && validators.getNext(validator) == address(0x01)), CannotRemoveLastValidator());
​
validators.pop(prev, validator);
IValidator(validator).onUninstall(disableModuleData);
}


onUninstall is a callback to the validator that is being removed. The issue here is that there is nothing stopping the validator to simply revert when onUninstall is called. This will force the entire tx to revert along with the entirety of _uninstallValidator, which will make the validator unremovable from the Module Manager. Even if the function goes through _tryExecute, the whole tx won't revert, but the removal of the validator will be rolled back, thus he won't be removed. As stated above, this affects both Validator, Executor, Hook and Fallback, since all of them have onUninstall callbacks that are called when their respective uninstall function is called, none of them can be removed.

Impact

DoS

Tools Used

Manual Review

Recommendations

Wrap the `onUninstall` call in a `try/catch`, if the call reverts handle it correctly in the `catch`.

Updates

Lead Judging Commences

0xnevi Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope
Assigned finding tags:

finding-onuninstall-revert

Invalid, - hook logic is OOS - all other `onUninstall()` functions do not revert, so the hawk here is essentially introducing code logic that doesn't exist. - Known issue: > The security of Nexus smart accounts relies heavily on the modules used. Only secure and audited modules should be installed to maintain the overall security of the system.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.