From 86371b3f7cd1181cad1083ee27592a20a9e13f39 Mon Sep 17 00:00:00 2001 From: Michael Standen Date: Mon, 11 Sep 2023 14:45:27 +1200 Subject: [PATCH] Correct error handling --- contracts/hooks/EIP4337Hook.sol | 14 ++- foundry_test/hooks/EIP4337Hook.t.sol | 157 +++++++++++++++++++++++++-- 2 files changed, 155 insertions(+), 16 deletions(-) diff --git a/contracts/hooks/EIP4337Hook.sol b/contracts/hooks/EIP4337Hook.sol index b308c0bd..e6320861 100644 --- a/contracts/hooks/EIP4337Hook.sol +++ b/contracts/hooks/EIP4337Hook.sol @@ -29,7 +29,6 @@ contract EIP4337Hook is IEIP4337Hook, ModuleNonce { /** * Allow the EIP-4337 entrypoint to execute a transaction on the wallet. * @dev This function does not validate as the entrypoint is trusted to have called validateUserOp. - * @dev Failure handling done by ModuleCalls. * @notice This functions is only callable by the Entrypoint. */ function eip4337SelfExecute(IModuleCalls.Transaction[] calldata txs) external payable onlyEntrypoint { @@ -37,7 +36,13 @@ contract EIP4337Hook is IEIP4337Hook, ModuleNonce { (bool success, ) = payable(address(this)).call{value: msg.value}( abi.encodeWithSelector(IModuleCalls.selfExecute.selector, txs) ); - (success); + if (!success) { + // Bubble up revert reason + bytes memory reason = LibOptim.returnData(); + assembly { + revert(add(reason, 0x20), mload(reason)) + } + } } /** @@ -55,7 +60,8 @@ contract EIP4337Hook is IEIP4337Hook, ModuleNonce { _validateNonce(userOp.nonce); // Check signature - bytes32 ethHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", userOpHash)); + bytes32 ethHash = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', userOpHash)); + // solhint-disable-next-line avoid-low-level-calls (bool sigSuccess, bytes memory data) = address(this).call{gas: type(uint256).max}( abi.encodeWithSelector(ERC1271_SELECTOR, ethHash, userOp.signature) ); @@ -66,7 +72,7 @@ contract EIP4337Hook is IEIP4337Hook, ModuleNonce { // Pay entrypoint if (missingAccountFunds != 0) { - (bool success,) = payable(msg.sender).call{value: missingAccountFunds, gas: type(uint256).max}(''); + (bool success, ) = payable(msg.sender).call{value: missingAccountFunds, gas: type(uint256).max}(''); (success); } diff --git a/foundry_test/hooks/EIP4337Hook.t.sol b/foundry_test/hooks/EIP4337Hook.t.sol index b52e6eb1..3cb7280c 100644 --- a/foundry_test/hooks/EIP4337Hook.t.sol +++ b/foundry_test/hooks/EIP4337Hook.t.sol @@ -3,6 +3,8 @@ pragma solidity 0.8.18; import 'contracts/hooks/EIP4337Hook.sol'; import 'contracts/hooks/interfaces/IEIP4337Hook.sol'; +import 'contracts/modules/commons/ModuleAuth.sol'; +import 'contracts/modules/commons/ModuleCalls.sol'; import 'contracts/modules/commons/ModuleHooks.sol'; import 'contracts/modules/MainModule.sol'; import 'contracts/modules/MainModuleUpgradable.sol'; @@ -10,23 +12,61 @@ import 'contracts/Factory.sol'; import 'foundry_test/base/AdvTest.sol'; +contract MockModules is ModuleAuth, ModuleCalls, ModuleHooks { + function validateNonce(uint256 _rawNonce) external { + _validateNonce(_rawNonce); + } + + function writeNonce(uint256 _space, uint256 _nonce) external { + _writeNonce(_space, _nonce); + } + + // Module Auth imp + mapping(bytes32 => mapping(bytes => bytes32)) public sigToSubdigest; + mapping(bytes32 => mapping(bytes => bool)) public sigToIsValid; + + function _signatureValidation( + bytes32 _digest, + bytes calldata _signature + ) internal view override(IModuleAuth, ModuleAuth) returns (bool isValid, bytes32 subdigest) { + subdigest = sigToSubdigest[_digest][_signature]; + isValid = sigToIsValid[_digest][_signature]; + } + + function mockSignature(bytes32 _digest, bytes calldata _signature, bytes32 _subdigest, bool _isValid) external { + sigToSubdigest[_digest][_signature] = _subdigest; + sigToIsValid[_digest][_signature] = _isValid; + } + + // solhint-disable no-empty-blocks + function _isValidImage(bytes32) internal view override returns (bool) {} + + function _updateImageHash(bytes32) internal override {} + + // solhint-enable no-empty-blocks + + function supportsInterface( + bytes4 _interfaceID + ) public pure virtual override(ModuleAuth, ModuleCalls, ModuleHooks) returns (bool) { + return + ModuleAuth.supportsInterface(_interfaceID) || + ModuleCalls.supportsInterface(_interfaceID) || + ModuleHooks.supportsInterface(_interfaceID); + } +} + contract EIP4337HookTest is AdvTest, IEIP4337HookErrors { - MainModule private template; + MockModules private walletMod; EIP4337Hook private wallet; address private constant ENTRYPOINT = address(uint160(uint256(keccak256('entrypoint')))); function setUp() external { Factory factory = new Factory(); - address upgradeable = address(new MainModuleUpgradable()); - template = new MainModule(address(factory), upgradeable); - ModuleHooks walletMod = ModuleHooks(payable(factory.deploy(address(template), bytes32(0)))); // Add hook below - vm.label(address(walletMod), "wallet"); + ModuleHooks template = new MockModules(); + walletMod = MockModules(payable(factory.deploy(address(template), bytes32(0)))); EIP4337Hook hook = new EIP4337Hook(ENTRYPOINT); - // Fund wallet - vm.deal(address(walletMod), 10 ether); - // Add hooks vm.startPrank(address(walletMod)); walletMod.addHook(IAccount.validateUserOp.selector, address(hook)); @@ -34,6 +74,7 @@ contract EIP4337HookTest is AdvTest, IEIP4337HookErrors { vm.stopPrank(); wallet = EIP4337Hook(address(walletMod)); + vm.label(address(wallet), 'wallet'); } struct ToVal { @@ -41,6 +82,10 @@ contract EIP4337HookTest is AdvTest, IEIP4337HookErrors { uint256 value; } + // + // Execute + // + function test_4337execute_invalidCaller(ToVal memory sendTx, address sender) external { vm.assume(sender != ENTRYPOINT); IModuleCalls.Transaction[] memory txs = new IModuleCalls.Transaction[](1); @@ -60,8 +105,9 @@ contract EIP4337HookTest is AdvTest, IEIP4337HookErrors { function test_4337execute_sendEth(ToVal memory sendTx) external { vm.assume(sendTx.target.code.length == 0); // Non contract - uint256 walletBal = address(wallet).balance; - vm.assume(sendTx.value <= walletBal); + + // Give wallet exact funds + vm.deal(address(wallet), sendTx.value); IModuleCalls.Transaction[] memory txs = new IModuleCalls.Transaction[](1); txs[0] = IModuleCalls.Transaction({ @@ -76,9 +122,96 @@ contract EIP4337HookTest is AdvTest, IEIP4337HookErrors { vm.prank(ENTRYPOINT); wallet.eip4337SelfExecute(txs); - assertEq(address(wallet).balance, walletBal - sendTx.value); + assertEq(address(wallet).balance, 0); assertEq(sendTx.target.balance, sendTx.value); } - function test_validateUserOp(ToVal[] memory sendTx) external {} + function test_4337execute_insufficientEth(ToVal memory sendTx) external { + vm.assume(sendTx.target.code.length == 0); // Non contract + vm.assume(sendTx.value > 0); + + IModuleCalls.Transaction[] memory txs = new IModuleCalls.Transaction[](1); + txs[0] = IModuleCalls.Transaction({ + delegateCall: false, + revertOnError: true, + gasLimit: 0, + target: sendTx.target, + value: sendTx.value, + data: '' + }); + + vm.expectRevert(); + vm.prank(ENTRYPOINT); + wallet.eip4337SelfExecute(txs); + } + + // + // Validate + // + + function test_4337validateUserOp_invalidCaller( + IAccount.UserOperation calldata userOp, + bytes32 userOpHash, + uint256 missingAccountFunds + ) external { + vm.expectRevert(InvalidCaller.selector); + wallet.validateUserOp(userOp, userOpHash, missingAccountFunds); + } + + function test_4337validateUserOp( + IAccount.UserOperation calldata userOp, + bytes32 userOpHash, + uint256 missingAccountFunds + ) external { + vm.assume(userOp.nonce == 0); + + // Give wallet enough funds + vm.deal(address(wallet), missingAccountFunds); + + // Accept the hash + bytes32 encodedHash = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', userOpHash)); + walletMod.mockSignature(encodedHash, userOp.signature, bytes32(0), true); + + // Validate + vm.prank(ENTRYPOINT); + uint256 validationData = wallet.validateUserOp(userOp, userOpHash, missingAccountFunds); + + assertEq(validationData, 0); + } + + function test_4337validateUserOp_failedSignature( + IAccount.UserOperation calldata userOp, + bytes32 userOpHash, + uint256 missingAccountFunds + ) external { + vm.assume(userOp.nonce == 0); + + // Give wallet enough funds + vm.deal(address(wallet), missingAccountFunds); + + // Validate + vm.prank(ENTRYPOINT); + uint256 validationData = wallet.validateUserOp(userOp, userOpHash, missingAccountFunds); + + assertEq(validationData, 1); + } + + function test_4337validateUserOp_invalidFunds( + IAccount.UserOperation calldata userOp, + bytes32 userOpHash, + uint256 missingAccountFunds + ) external { + vm.assume(userOp.nonce == 0); + + // Accept the hash + bytes32 encodedHash = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', userOpHash)); + walletMod.mockSignature(encodedHash, userOp.signature, bytes32(0), true); + + // Validate + vm.prank(ENTRYPOINT); + uint256 validationData = wallet.validateUserOp(userOp, userOpHash, missingAccountFunds); + + // Passes. Account doesn't validate + assertEq(validationData, 0); + } }