Summary
Handling execution may revert despite being in TRY mode (return on failure).
Vulnerability Details
Nexus::execute(...) is designed to execute transactions in various modes:
function execute(ExecutionMode mode, bytes calldata executionCalldata) external payable onlyEntryPointOrSelf withHook {
(CallType callType, ExecType execType) = mode.decodeBasic();
if (callType == CALLTYPE_SINGLE) {
_handleSingleExecution(executionCalldata, execType);
} else if (callType == CALLTYPE_BATCH) {
_handleBatchExecution(executionCalldata, execType);
} else if (callType == CALLTYPE_DELEGATECALL) {
_handleDelegateCallExecution(executionCalldata, execType);
} else {
revert UnsupportedCallType(callType);
}
}
What if the call type is CALLTYPE_SINGLE? well, ExecutionHelper::_handleSingleExecution(...) is triggered:
...
function _handleSingleExecution(bytes calldata executionCalldata, ExecType execType) internal {
(address target, uint256 value, bytes calldata callData) = executionCalldata.decodeSingle();
if (execType == EXECTYPE_DEFAULT) _execute(target, value, callData);
else if (execType == EXECTYPE_TRY) _tryExecute(target, value, callData);
else revert UnsupportedExecType(execType);
}
To generalize, if callType == CALLTYPE_X, ExecutionHelper::_handleXExecution(...) is triggered. It either calls ExecutionHelper::_execute(...) fore revert on failure, or ExecutionHelper::_tryExecute(...) for return on failure.
Now, withHook modifier enforces hooks. A hook does pre and post checks on an execution logic:
modifier withHook() {
address hook = _getHook();
if (hook == address(0)) {
_;
} else {
bytes memory hookData = IHook(hook).preCheck(msg.sender, msg.value, msg.data);
_;
IHook(hook).postCheck(hookData);
}
}
If one of the checks reverts for any reason,withHook reverts the entire execution, even when operating in TRY mode (which is intended to return on failure rather than revert).
Proof of Concept
...
contract MockHook is IModule {
...
function postCheck(bytes calldata hookData) external {
hookData;
emit PostCheckCalled();
+ revert("REVERT_FROM_HOOK");
}
...
}
pragma solidity ^0.8.26;
import "../shared/TestModuleManagement_Base.t.sol";
import "../shared/TestAccountExecution_Base.t.sol";
contract POC is TestModuleManagement_Base, TestAccountExecution_Base {
function setUp() public {
setUpModuleManagement_Base();
setUpTestAccountExecution_Base();
}
function test_poc() public {
bytes memory callData = abi.encodeWithSelector(IModuleManager.installModule.selector, MODULE_TYPE_HOOK, address(mockHook), "");
installModule(callData, MODULE_TYPE_HOOK, address(mockHook), EXECTYPE_DEFAULT);
assertTrue(BOB_ACCOUNT.isModuleInstalled(MODULE_TYPE_HOOK, address(mockHook), ""), "Hook module should be installed");
Execution[] memory execution = new Execution[](1);
execution[0] = Execution(address(counter), 0, abi.encodeWithSelector(Counter.incrementNumber.selector));
ExecType catchExecType = ExecType.wrap(bytes1(0x01));
CallType callType = CALLTYPE_SINGLE;
ExecutionMode mode = ModeLib.encodeCustom(callType, catchExecType);
vm.expectRevert("REVERT_FROM_HOOK");
prank(address(BOB_ACCOUNT));
BOB_ACCOUNT.execute(mode, abi.encode(execution));
}
}
➜ 2024-07-biconomy git:(main) ✗ forge test --mt test_poc
[PASS] test_poc() (gas: 190406)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 15.39ms (1.05ms CPU time)
Ran 1 test suite in 122.82ms (15.39ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Tools Used
Recommendation
Restrict the hook execution to revert mode only. Update Nexus::execute(...):
+ function execute(...) external payable onlyEntryPointOrSelf {
- function execute(...) external payable onlyEntryPointOrSelf withHook {
Then, restrict withHook modifier to ExecutionHelper::_execute(...) only:
+ function _execute(...) internal virtual returns (bytes memory result) withHook {
- function _execute(...) internal virtual returns (bytes memory result) {
