Summary
The withHook modifier is not implemented in uninstallModule or any internal uninstall function. Depending on it's implementation it can cause various problems.
Root Cause
https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/Nexus.sol#L201-L216
The unistallModule function is missing withHook modifier. On the other hand _installModule function has it implemented.
function uninstallModule(uint256 moduleTypeId, address module, bytes calldata deInitData) external payable onlyEntryPointOrSelf {
require(IModule(module).isModuleType(moduleTypeId), MismatchModuleTypeId(moduleTypeId));
require(_isModuleInstalled(moduleTypeId, module, deInitData), ModuleNotInstalled(moduleTypeId, module));
emit ModuleUninstalled(moduleTypeId, module);
if (moduleTypeId == MODULE_TYPE_VALIDATOR) {
_uninstallValidator(module, deInitData);
} else if (moduleTypeId == MODULE_TYPE_EXECUTOR) {
_uninstallExecutor(module, deInitData);
} else if (moduleTypeId == MODULE_TYPE_FALLBACK) {
_uninstallFallbackHandler(module, deInitData);
} else if (moduleTypeId == MODULE_TYPE_HOOK) {
_uninstallHook(module, deInitData);
}
}
Internal function responsible for installing modules. It has withHook modifier.
function _installModule(uint256 moduleTypeId, address module, bytes calldata initData) internal withHook {
if (module == address(0)) revert ModuleAddressCanNotBeZero();
if (moduleTypeId == MODULE_TYPE_VALIDATOR) {
_installValidator(module, initData);
} else if (moduleTypeId == MODULE_TYPE_EXECUTOR) {
_installExecutor(module, initData);
} else if (moduleTypeId == MODULE_TYPE_FALLBACK) {
_installFallbackHandler(module, initData);
} else if (moduleTypeId == MODULE_TYPE_HOOK) {
_installHook(module, initData);
} else if (moduleTypeId == MODULE_TYPE_MULTI) {
_multiTypeInstall(module, initData);
} else {
revert InvalidModuleTypeId(moduleTypeId);
}
}
One of internal uninstall function in ModuleManager.
function _uninstallExecutor(address executor, bytes calldata data) internal virtual {
(address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes));
_getAccountStorage().executors.pop(prev, executor);
IExecutor(executor).onUninstall(disableModuleData);
}
Impact
The impact depends on the hook implementation. For example if the hook is responisble for sending tokens every time a module is installed or uninstalled, it will not send these tokens (when module is uninstalled).
Mitigation
Add withHook modifier to either uninstallModule in Nexus contract or add withHook modifier to every internal uninstall function inside ModuleManager.
Example pseudocode:
- function uninstallModule(uint256 moduleTypeId, address module, bytes calldata deInitData) external payable onlyEntryPointOrSelf
+ function uninstallModule(uint256 moduleTypeId, address module, bytes calldata deInitData) external payable withHook onlyEntryPointOrSelf {
require(IModule(module).isModuleType(moduleTypeId), MismatchModuleTypeId(moduleTypeId));
require(_isModuleInstalled(moduleTypeId, module, deInitData), ModuleNotInstalled(moduleTypeId, module));
emit ModuleUninstalled(moduleTypeId, module);
if (moduleTypeId == MODULE_TYPE_VALIDATOR) {
_uninstallValidator(module, deInitData);
} else if (moduleTypeId == MODULE_TYPE_EXECUTOR) {
_uninstallExecutor(module, deInitData);
} else if (moduleTypeId == MODULE_TYPE_FALLBACK) {
_uninstallFallbackHandler(module, deInitData);
} else if (moduleTypeId == MODULE_TYPE_HOOK) {
_uninstallHook(module, deInitData);
}
}
