From f72a4a974a9c8dec148f5446503e2d3c9fbb6e63 Mon Sep 17 00:00:00 2001 From: highskore Date: Mon, 14 Oct 2024 20:57:32 +0200 Subject: [PATCH 1/9] feat: add banned opcode and out of gas validation --- foundry.toml | 2 +- package.json | 6 +- pnpm-lock.yaml | 12 +-- src/Simulator.sol | 13 +++- src/SpecsParser.sol | 184 +++++++++++++++++++++++++++++++++++++++----- src/lib/Vm.sol | 8 ++ 6 files changed, 190 insertions(+), 35 deletions(-) diff --git a/foundry.toml b/foundry.toml index b8c02d9..7c312aa 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,7 +1,7 @@ [profile.default] src = "src" out = "out" -libs = ["node_modules"] +libs = ["node_modules", "lib"] [fmt] bracket_spacing = true diff --git a/package.json b/package.json index 0409aa6..ef28c4f 100644 --- a/package.json +++ b/package.json @@ -29,12 +29,12 @@ }, "dependencies": { "@openzeppelin/contracts": "5.0.1", - "solady": "github:vectorized/solady", "account-abstraction": "github:kopy-kat/account-abstraction#develop", "account-abstraction-v0.6": "github:eth-infinitism/account-abstraction#v0.6.0", "ds-test": "github:dapphub/ds-test", - "forge-std": "github:foundry-rs/forge-std", - "prettier": "^2.8.8" + "forge-std": "github:foundry-rs/forge-std#8a225d81aa8e2e013580564588c79abb65eacc9e", + "prettier": "^2.8.8", + "solady": "github:vectorized/solady" }, "devDependencies": { "@changesets/cli": "^2.27.2", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5e7c13f..3be1179 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -21,8 +21,8 @@ importers: specifier: github:dapphub/ds-test version: https://codeload.github.com/dapphub/ds-test/tar.gz/e282159d5170298eb2455a6c05280ab5a73a4ef0 forge-std: - specifier: github:foundry-rs/forge-std - version: https://codeload.github.com/foundry-rs/forge-std/tar.gz/52715a217dc51d0de15877878ab8213f6cbbbab5 + specifier: github:foundry-rs/forge-std#8a225d81aa8e2e013580564588c79abb65eacc9e + version: https://codeload.github.com/foundry-rs/forge-std/tar.gz/8a225d81aa8e2e013580564588c79abb65eacc9e prettier: specifier: ^2.8.8 version: 2.8.8 @@ -1053,9 +1053,9 @@ packages: debug: optional: true - forge-std@https://codeload.github.com/foundry-rs/forge-std/tar.gz/52715a217dc51d0de15877878ab8213f6cbbbab5: - resolution: {tarball: https://codeload.github.com/foundry-rs/forge-std/tar.gz/52715a217dc51d0de15877878ab8213f6cbbbab5} - version: 1.8.2 + forge-std@https://codeload.github.com/foundry-rs/forge-std/tar.gz/8a225d81aa8e2e013580564588c79abb65eacc9e: + resolution: {tarball: https://codeload.github.com/foundry-rs/forge-std/tar.gz/8a225d81aa8e2e013580564588c79abb65eacc9e} + version: 1.9.3 form-data-encoder@2.1.4: resolution: {integrity: sha512-yDYSgNMraqvnxiEXO4hi88+YZxaHC6QKzb5N84iRCTDeRO7ZALpir/lVmf/uXUhnwUr2O4HU8s/n6x+yNjQkHw==} @@ -3623,7 +3623,7 @@ snapshots: optionalDependencies: debug: 4.3.4(supports-color@8.1.1) - forge-std@https://codeload.github.com/foundry-rs/forge-std/tar.gz/52715a217dc51d0de15877878ab8213f6cbbbab5: {} + forge-std@https://codeload.github.com/foundry-rs/forge-std/tar.gz/8a225d81aa8e2e013580564588c79abb65eacc9e: {} form-data-encoder@2.1.4: {} diff --git a/src/Simulator.sol b/src/Simulator.sol index 47101cd..c64606c 100644 --- a/src/Simulator.sol +++ b/src/Simulator.sol @@ -13,9 +13,11 @@ import { VmSafe } from "forge-std/Vm.sol"; import { snapshot, startMappingRecording, + startDebugTraceRecording, startStateDiffRecording, stopAndReturnStateDiff, stopMappingRecording, + stopAndReturnDebugTraceRecording, revertTo, expectRevert } from "./lib/Vm.sol"; @@ -73,7 +75,7 @@ library Simulator { } // Create a UserOperationDetails struct - // This is to make it easier to maintain compatibility of the differnt UserOperation + // This is to make it easier to maintain compatibility of the different UserOperation // versions UserOperationDetails memory userOpDetails = UserOperationDetails({ entryPoint: onEntryPoint, @@ -124,7 +126,7 @@ library Simulator { } // Create a UserOperationDetails struct - // This is to make it easier to maintain compatibility of the differnt UserOperation + // This is to make it easier to maintain compatibility of the different UserOperation // versions UserOperationDetails memory userOpDetails = UserOperationDetails({ entryPoint: onEntryPoint, @@ -150,9 +152,10 @@ library Simulator { sstore(snapShotSlot, snapShotId) } - // Start recording mapping accesses and state diffs + // Start recording mapping accesses, state diffs and debug trace startMappingRecording(); startStateDiffRecording(); + startDebugTraceRecording(); } /** @@ -163,9 +166,11 @@ library Simulator { function _postSimulation(UserOperationDetails memory userOpDetails) internal { // Get the state diffs VmSafe.AccountAccess[] memory accesses = stopAndReturnStateDiff(); + // Get the recorded opcodes + VmSafe.DebugStep[] memory debugTrace = stopAndReturnDebugTraceRecording(); // Validate the ERC-4337 rules - ERC4337SpecsParser.parseValidation(accesses, userOpDetails); + ERC4337SpecsParser.parseValidation(accesses, userOpDetails, debugTrace); // Stop (and remove) recording mapping accesses stopMappingRecording(); diff --git a/src/SpecsParser.sol b/src/SpecsParser.sol index 431e237..fb71cb9 100644 --- a/src/SpecsParser.sol +++ b/src/SpecsParser.sol @@ -7,14 +7,14 @@ import { IStakeManager, UserOperationDetails } from "./lib/ERC4337.sol"; -import { VmSafe } from "forge-std/Vm.sol"; +import { VmSafe, Vm } from "forge-std/Vm.sol"; import { getLabel, getMappingKeyAndParentOf } from "./lib/Vm.sol"; /** * @title ERC4337SpecsParser * @author kopy-kat * @dev Parses and validates the ERC-4337 rules - * @custom:credits Credits to Dror (drortirosh) for the approach and an intial prototype + * @custom:credits Credits to Dror (drortirosh) for the approach and an initial prototype */ library ERC4337SpecsParser { // Minimum stake delay @@ -22,7 +22,7 @@ library ERC4337SpecsParser { // Minimum stake value uint256 constant MIN_STAKE_VALUE = 0.5 ether; - // Emmitted if an invalid storage location is accessed + // Emitted if an invalid storage location is accessed error InvalidStorageLocation( address contractAddress, string contractLabel, @@ -32,8 +32,8 @@ library ERC4337SpecsParser { bool isWrite ); - // Emmitted if a banned opcode is used - error InvalidOpcode(address contractAddress, string opcode); + // Emitted if a banned opcode is used + error InvalidOpcode(address contractAddress, uint8 opcode); /** * Entity struct @@ -59,18 +59,31 @@ library ERC4337SpecsParser { * * @param accesses The state diffs to validate * @param userOpDetails The UserOperationDetails to validate + * @param debugTrace A trace of used opcodes, stack and memory to validate */ function parseValidation( VmSafe.AccountAccess[] memory accesses, - UserOperationDetails memory userOpDetails + UserOperationDetails memory userOpDetails, + VmSafe.DebugStep[] memory debugTrace ) internal { // Get entities for the userOp Entities memory entities = getEntities(userOpDetails); - // Validate banned opcodes - validateBannedOpcodes(); + // Filter debug trace to get the debug steps for the userOp and paymasterUserOp + ( + VmSafe.DebugStep[] memory filteredUserOpSteps, + VmSafe.DebugStep[] memory filteredPaymasterUserOpSteps + ) = filterDebugTrace(debugTrace, entities, userOpDetails.entryPoint); + + // Validate banned opcodes for the userOp and paymasterUserOp + validateBannedOpcodes(filteredUserOpSteps); + validateBannedOpcodes(filteredPaymasterUserOpSteps); + + // Validate there are not out of gas errors for the userOp and paymasterUserOp + validateOutOfGas(filteredUserOpSteps); + validateOutOfGas(filteredPaymasterUserOpSteps); // Loop over the state diffs for (uint256 i; i < accesses.length; i++) { @@ -95,20 +108,127 @@ library ERC4337SpecsParser { /** * Validates that no banned opcodes are used - * @notice This function is not implemented yet, it depends on - * https://github.com/foundry-rs/foundry/issues/6704 + * @param debugTrace The debug trace to validate */ - function validateBannedOpcodes() internal pure { - // todo + function validateBannedOpcodes(VmSafe.DebugStep[] memory debugTrace) internal pure { // forbidden opcodes are GASPRICE, GASLIMIT, DIFFICULTY, TIMESTAMP, BASEFEE, BLOCKHASH, // NUMBER, SELFBALANCE, BALANCE, ORIGIN, GAS, CREATE, COINBASE, SELFDESTRUCT - // Exception: GAS is allowed if followed immediately by one of { CALL, DELEGATECALL, + // Exception: GAS is allowed if followed immediately by5 one of { CALL, DELEGATECALL, // CALLCODE, STATICCALL }] - // revert InvalidOpcode(currentAccess.account, opcode); + + // Loop over the debug steps to validate the opcodes + for (uint256 i; i < debugTrace.length; i++) { + // Check if the current opcode is a forbidden opcode + if (isForbiddenOpcode(debugTrace[i].opcode)) { + // Check if the current opcode is GAS, if so, check if it is followed by a CALL, + // DELEGATECALL, CALLCODE or STATICCALL + if ( + debugTrace[i].opcode == 0x5A // GAS + ) { + if ( + debugTrace[i + 1].opcode != 0xF1 // CALL + && debugTrace[i + 1].opcode != 0xF4 // DELEGATECALL + && debugTrace[i + 1].opcode != 0xF5 // CALLCODE + && debugTrace[i + 1].opcode != 0xFA // STATICCALL + ) { + revert InvalidOpcode(debugTrace[i].contractAddr, 0x5A); + } + } else { + revert InvalidOpcode(debugTrace[i].contractAddr, debugTrace[i].opcode); + } + } + } + } + + /** + * Filter debug trace, we are interested in the following debug traces: + * - Entrypoint -> validateUserOp + * - Entrypoint - validatePaymasterUserOp + * @param debugTrace The debug trace to filter + * @param entities The entities of the userOp + * @param entryPoint The entryPoint address + * @return filteredUserOpSteps The filtered debug steps + * @return filteredPaymasterUserOpSteps The filtered debug steps + */ + function filterDebugTrace( + VmSafe.DebugStep[] memory debugTrace, + Entities memory entities, + address entryPoint + ) + private + pure + returns ( + VmSafe.DebugStep[] memory filteredUserOpSteps, + VmSafe.DebugStep[] memory filteredPaymasterUserOpSteps + ) + { + // Init filtered debug steps lengths + uint256 filteredUserOpStepsLength; + uint256 filteredPaymasterUserOpStepsLength; + + // Init start depth (first time we enter the entryPoint) + uint256 startDepth = 0; + + // Loop over the debug steps to find the start depth + for (uint256 i; i < debugTrace.length; i++) { + // Check if the current debug step contract address is the entryPoint + if (debugTrace[i].contractAddr == entryPoint) { + // Set the start depth + startDepth = debugTrace[i].depth; + break; + } + } + + // Init currentContractAddr + address currentContractAddr; + + // Loop over the debug steps to filter the debug steps + for (uint256 i = 0; i < debugTrace.length; i++) { + // Ignore calls on base depth where the contract address is the entryPoint + if (debugTrace[i].depth == startDepth && debugTrace[i].contractAddr == entryPoint) { + // If the current opcode is call or static call, update the currentContractAddr with + // the value from the stack + if ( + debugTrace[i].opcode == 0xF1 // CALL + || debugTrace[i].opcode == 0xFA // STATICCALL + ) { + currentContractAddr = address(uint160(uint256(debugTrace[i].stack[1]))); + } + continue; + } + + // If the depth is grander than the start depth, we are interested in the debug steps, + // add to the filteredUserOpSteps + // or filteredPaymasterUserOpSteps based on the currentContractAddr + if (debugTrace[i].depth > startDepth) { + if (currentContractAddr == entities.account) { + filteredUserOpSteps[filteredUserOpStepsLength++] = debugTrace[i]; + } else if (currentContractAddr == entities.paymaster) { + filteredPaymasterUserOpSteps[filteredPaymasterUserOpStepsLength++] = + debugTrace[i]; + } + } + } + + // Update the filtered debug steps lengths + assembly { + mstore(filteredUserOpSteps, filteredUserOpStepsLength) + mstore(filteredPaymasterUserOpSteps, filteredPaymasterUserOpStepsLength) + } } - // todo: [OP-020] Revert on "out of gas" is forbidden as it can "leak" the gas limit or the - // current call stack depth. + /** + * Validate that the simulation does not revert with Out of Gas + * @param debugTrace The debug trace to validate + */ + function validateOutOfGas(VmSafe.DebugStep[] memory debugTrace) internal pure { + // Loop over the debug steps to validate out of gas errors + for (uint256 i; i < debugTrace.length; i++) { + if (debugTrace[i].isOutOfGas) { + revert("[OP-020] Simulation reverts with Out of Gas"); + } + } + } /** * Validates that no banned storage locations are accessed @@ -256,7 +376,7 @@ library ERC4337SpecsParser { { if (currentAccess.kind == VmSafe.AccountAccessKind.Create) { // Check that the initCode is not empty and that only the sender is created - // Note: If the initCode is emptpy, the factory address is address(0) + // Note: If the initCode is empty, the factory address is address(0) // Revert otherwise if (entities.factory == address(0) || currentAccess.account != entities.account) { revert( @@ -373,9 +493,7 @@ library ERC4337SpecsParser { * * @return entities The entities of the UserOperation */ - function getEntities( - UserOperationDetails memory userOpDetails - ) + function getEntities(UserOperationDetails memory userOpDetails) internal view returns (Entities memory entities) @@ -470,7 +588,31 @@ library ERC4337SpecsParser { * * @return isPrecompile Whether the address is a precompile */ - function isPrecompile(address target) internal view returns (bool isPrecompile) { + function isPrecompile(address target) internal pure returns (bool) { return uint256(uint160(target)) <= 0x09 || uint256(uint160(target)) == 0x100; } + + /** + * Checks if the opcode is a forbidden opcode + * + * @param opcode The opcode to check + * + * @return isForbidden Whether the opcode is forbidden + */ + function isForbiddenOpcode(uint8 opcode) private pure returns (bool isForbidden) { + return opcode == 0x3A // GASPRICE + || opcode == 0x45 // GASLIMIT + || opcode == 0x44 // DIFFICULTY + || opcode == 0x42 // TIMESTAMP + || opcode == 0x48 // BASEFEE + || opcode == 0x40 // BLOCKHASH + || opcode == 0x43 // NUMBER + || opcode == 0x47 // SELFBALANCE + || opcode == 0x31 // BALANCE + || opcode == 0x32 // ORIGIN + || opcode == 0x5A // GAS + || opcode == 0xF0 // CREATE + || opcode == 0x41 // COINBASE + || opcode == 0xFF; // SELFDESTRUCT + } } diff --git a/src/lib/Vm.sol b/src/lib/Vm.sol index 1f26c04..0bb6a2e 100644 --- a/src/lib/Vm.sol +++ b/src/lib/Vm.sol @@ -44,3 +44,11 @@ function getMappingKeyAndParentOf(address target, bytes32 slot) returns (bool, b function expectRevert() { Vm(VM_ADDR).expectRevert(); } + +function startDebugTraceRecording() { + Vm(VM_ADDR).startDebugTraceRecording(); +} + +function stopAndReturnDebugTraceRecording() returns (VmSafe.DebugStep[] memory steps) { + return Vm(VM_ADDR).stopAndReturnDebugTraceRecording(); +} From fadce3d82a9f248d305b7813e6cdb0b441a442a8 Mon Sep 17 00:00:00 2001 From: highskore Date: Tue, 15 Oct 2024 11:26:07 +0200 Subject: [PATCH 2/9] test: add opcode validation and OOG revert tests --- .github/workflows/ci.yaml | 17 ++- foundry.toml | 3 + src/SpecsParser.sol | 22 ++- test/specs-parser/Opcodes.t.sol | 117 +++++++++++++++ test/specs-parser/mocks/OpcodeValidator.sol | 154 ++++++++++++++++++++ 5 files changed, 298 insertions(+), 15 deletions(-) create mode 100644 test/specs-parser/Opcodes.t.sol create mode 100644 test/specs-parser/mocks/OpcodeValidator.sol diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 61e4a1a..1af35e4 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -2,20 +2,23 @@ on: workflow_dispatch: push: branches: - - "main" + - 'main' pull_request: jobs: lint: - uses: "rhinestonewtf/reusable-workflows/.github/workflows/forge-lint.yaml@main" + uses: 'rhinestonewtf/reusable-workflows/.github/workflows/forge-lint.yaml@main' build: - uses: "rhinestonewtf/reusable-workflows/.github/workflows/forge-build.yaml@main" + uses: 'rhinestonewtf/reusable-workflows/.github/workflows/forge-build.yaml@main' test: - needs: ["lint", "build"] - uses: "rhinestonewtf/reusable-workflows/.github/workflows/forge-test.yaml@main" + needs: ['lint', 'build'] + uses: 'rhinestonewtf/reusable-workflows/.github/workflows/forge-test.yaml@main' with: foundry-fuzz-runs: 5000 - foundry-profile: "test" - match-path: "test/**/*.sol" + foundry-profile: 'test' + match-path: 'test/**/*.sol' + verbosity: 3 + gas_limit: '18446744073709551615' + memory_limit: 2147483648 diff --git a/foundry.toml b/foundry.toml index 7c312aa..70ffd26 100644 --- a/foundry.toml +++ b/foundry.toml @@ -2,6 +2,9 @@ src = "src" out = "out" libs = ["node_modules", "lib"] +gas_limit = "18446744073709551615" +memory_limit = 2147483648 +verbosity = 3 [fmt] bracket_spacing = true diff --git a/src/SpecsParser.sol b/src/SpecsParser.sol index fb71cb9..f96c1b7 100644 --- a/src/SpecsParser.sol +++ b/src/SpecsParser.sol @@ -126,10 +126,11 @@ library ERC4337SpecsParser { debugTrace[i].opcode == 0x5A // GAS ) { if ( - debugTrace[i + 1].opcode != 0xF1 // CALL - && debugTrace[i + 1].opcode != 0xF4 // DELEGATECALL - && debugTrace[i + 1].opcode != 0xF5 // CALLCODE - && debugTrace[i + 1].opcode != 0xFA // STATICCALL + i + 1 >= debugTrace.length + || debugTrace[i + 1].opcode != 0xF1 // CALL + && debugTrace[i + 1].opcode != 0xF4 // DELEGATECALL + && debugTrace[i + 1].opcode != 0xF2 // CALLCODE + && debugTrace[i + 1].opcode != 0xFA // STATICCALL ) { revert InvalidOpcode(debugTrace[i].contractAddr, 0x5A); } @@ -157,11 +158,13 @@ library ERC4337SpecsParser { ) private pure - returns ( - VmSafe.DebugStep[] memory filteredUserOpSteps, - VmSafe.DebugStep[] memory filteredPaymasterUserOpSteps - ) + returns (VmSafe.DebugStep[] memory, VmSafe.DebugStep[] memory) { + // Init filtered debug steps + VmSafe.DebugStep[] memory filteredUserOpSteps = new VmSafe.DebugStep[](debugTrace.length); + VmSafe.DebugStep[] memory filteredPaymasterUserOpSteps = + new VmSafe.DebugStep[](debugTrace.length); + // Init filtered debug steps lengths uint256 filteredUserOpStepsLength; uint256 filteredPaymasterUserOpStepsLength; @@ -215,6 +218,9 @@ library ERC4337SpecsParser { mstore(filteredUserOpSteps, filteredUserOpStepsLength) mstore(filteredPaymasterUserOpSteps, filteredPaymasterUserOpStepsLength) } + + // Return the filtered debug steps + return (filteredUserOpSteps, filteredPaymasterUserOpSteps); } /** diff --git a/test/specs-parser/Opcodes.t.sol b/test/specs-parser/Opcodes.t.sol new file mode 100644 index 0000000..fdaa2b1 --- /dev/null +++ b/test/specs-parser/Opcodes.t.sol @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.23; + +import { TestBaseUtil } from "test/utils/TestBaseUtil.sol"; +import { ERC4337SpecsParser } from "src/SpecsParser.sol"; +import { OpcodeValidator } from "test/specs-parser/mocks/OpcodeValidator.sol"; + +contract OpcodeParserTest is TestBaseUtil { + OpcodeValidator internal validator; + + function setUp() public override { + super.setUp(); + validator = new OpcodeValidator(); + vm.label(address(validator), "OpcodeValidator"); + } + + /*////////////////////////////////////////////////////////////// + TESTS + //////////////////////////////////////////////////////////////*/ + + function testValidOpcodes() public { + _runOpcodeTest(0x00, false); // Valid opcodes (using 0x00 as a placeholder for valid + // opcodes) + } + + function testValidGAS__WhenFollowedByCALL() public { + _runOpcodeTest(0xF1, false); // CALL + } + + function testValidGAS__WhenFollowedByDELEGATECALL() public { + _runOpcodeTest(0xF4, false); // DELEGATECALL + } + + function testValidGAS__WhenFollowedByCALLCODE() public { + _runOpcodeTest(0xF2, false); // CALLCODE + } + + function testValidGAS__WhenFollowedBySTATICCALL() public { + _runOpcodeTest(0xFA, false); // STATICCALL + } + + function testBannedOpcode__RevertWhen__UsingGASPRICE() public { + _runOpcodeTest(0x3A, true); // GASPRICE + } + + function testBannedOpcode__RevertWhen__UsingGASLIMIT() public { + _runOpcodeTest(0x45, true); // GASLIMIT + } + + function testBannedOpcode__RevertWhen__UsingDIFFICULTY() public { + _runOpcodeTest(0x44, true); // DIFFICULTY + } + + function testBannedOpcode__RevertWhen__UsingTIMESTAMP() public { + _runOpcodeTest(0x42, true); // TIMESTAMP + } + + function testBannedOpcode__RevertWhen__UsingBASEFEE() public { + _runOpcodeTest(0x48, true); // BASEFEE + } + + function testBannedOpcode__RevertWhen__UsingBLOCKHASH() public { + _runOpcodeTest(0x40, true); // BLOCKHASH + } + + function testBannedOpcode__RevertWhen__UsingNUMBER() public { + _runOpcodeTest(0x43, true); // NUMBER + } + + function testBannedOpcode__RevertWhen__UsingSELFBALANCE() public { + _runOpcodeTest(0x47, true); // SELFBALANCE + } + + function testBannedOpcode__RevertWhen__UsingBALANCE() public { + _runOpcodeTest(0x31, true); // BALANCE + } + + function testBannedOpcode__RevertWhen__UsingORIGIN() public { + _runOpcodeTest(0x32, true); // ORIGIN + } + + function testBannedOpcode__RevertWhen__UsingGAS() public { + _runOpcodeTest(0x5A, true); // GAS + } + + function testBannedOpcode__RevertWhen__UsingCREATE() public { + _runOpcodeTest(0xF0, true); // CREATE + } + + function testBannedOpcode__RevertWhen__UsingCOINBASE() public { + _runOpcodeTest(0x41, true); // COINBASE + } + + function testOutOfGas__RevertWhen__RunningOutOfGas() public { + _runOpcodeTest(0xFF, true); // SELFDESTRUCT (used for out of gas scenario) + } + + /*////////////////////////////////////////////////////////////// + HELPERS + //////////////////////////////////////////////////////////////*/ + + function _runOpcodeTest(uint256 opcode, bool shouldRevert) internal { + if (shouldRevert) { + (bool success,) = + address(this).call(abi.encodeWithSelector(this.runSimulation.selector, opcode)); + assertFalse(success); + } else { + (bool success,) = + address(this).call(abi.encodeWithSelector(this.runSimulation.selector, opcode)); + assertTrue(success); + } + } + + function runSimulation(uint256 opcode) public { + simulateUserOp(address(validator), abi.encode(opcode)); + } +} diff --git a/test/specs-parser/mocks/OpcodeValidator.sol b/test/specs-parser/mocks/OpcodeValidator.sol new file mode 100644 index 0000000..999c8c3 --- /dev/null +++ b/test/specs-parser/mocks/OpcodeValidator.sol @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.23; + +import { PackedUserOperation, UserOperation } from "src/lib/ERC4337.sol"; +import { IValidator } from "test/utils/IValidator.sol"; + +contract OpcodeValidator is IValidator { + // To make the optimizer actually use our opcodes + event Log(uint256 value); + + function mockFunction() public pure returns (uint256) { + return 0; + } + + function _validateUserOp(bytes calldata signature) internal { + uint256 opcode = abi.decode(signature, (uint256)); + if (opcode == 0x00) { + // Valid opcodes, do nothing + assembly { + pop(0) + } + } else if (opcode == 0x3A) { + // Gasprice + assembly { + let _gasPrice := gasprice() + log0(0, _gasPrice) + } + } else if (opcode == 0x45) { + // Gaslimit + uint256 limit; + assembly { + limit := gaslimit() + } + emit Log(limit); + } else if (opcode == 0x44) { + // Difficulty + assembly { + let _difficulty := prevrandao() + log0(0, _difficulty) + } + } else if (opcode == 0x42) { + // Timestamp + assembly { + let _timestamp := timestamp() + log0(0, _timestamp) + } + } else if (opcode == 0x48) { + // Basefee + assembly { + let _basefee := basefee() + log0(0, _basefee) + } + } else if (opcode == 0x40) { + uint256 _blockhash; + // Blockhash + assembly { + _blockhash := blockhash(0) + } + emit Log(_blockhash); + } else if (opcode == 0x43) { + // Number + assembly { + let _number := number() + log0(0, _number) + } + } else if (opcode == 0x47) { + // Selfbalance + assembly { + let _balance := selfbalance() + log0(0, _balance) + } + } else if (opcode == 0x31) { + // Balance + assembly { + let _balance := balance(0xdead) + log0(0, _balance) + } + } else if (opcode == 0x32) { + // Origin + address origin = tx.origin; + emit Log(uint256(uint160(origin))); + } else if (opcode == 0x5A) { + // Gas + uint256 _gasleft; + assembly { + _gasleft := gas() + } + emit Log(_gasleft); + } else if (opcode == 0xF0) { + // Create + assembly { + pop(create(0, 0, 0)) + } + } else if (opcode == 0x41) { + // Coinbase + assembly { + let _coinbase := coinbase() + log0(0, _coinbase) + } + } else if (opcode == 0xF1) { + // CALL + assembly { + let g := gas() + pop(call(g, 0x0000000000000000000000000000000000000000, 0, 0, 0, 0, 0)) + } + } else if (opcode == 0xF4) { + // DELEGATECALL + assembly { + let g := gas() + pop(delegatecall(g, 0x0000000000000000000000000000000000000000, 0, 0, 0, 0)) + } + } else if (opcode == 0xF2) { + // CALLCODE + assembly { + let g := gas() + pop(callcode(g, 0x0000000000000000000000000000000000000000, 0, 0, 0, 0, 0)) + } + } else if (opcode == 0xFA) { + // STATICCALL + assembly { + let g := gas() + pop(staticcall(gas(), 0x0000000000000000000000000000000000000000, 0, 0, 0, 0)) + } + } else if (opcode == 0xFF) { + // Out of gas + uint256 gasLimit = 10; + address(this).call{ gas: gasLimit }(abi.encodeWithSignature("mockFunction()")); + } + } + + function validateUserOp( + PackedUserOperation calldata userOp, + bytes32 userOpHash + ) + external + override + returns (uint256) + { + _validateUserOp(userOp.signature); + return 0; + } + + function validateUserOp( + UserOperation calldata userOp, + bytes32 userOpHash + ) + external + override + returns (uint256) + { + _validateUserOp(userOp.signature); + return 0; + } +} From 15e4588edccbc3ca59b683a97961602727e6b735 Mon Sep 17 00:00:00 2001 From: highskore <80688826+highskore@users.noreply.github.com> Date: Tue, 15 Oct 2024 11:29:48 +0200 Subject: [PATCH 3/9] chore(ci): revert "/i lint --- .github/workflows/ci.yaml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1af35e4..45ed103 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -2,23 +2,23 @@ on: workflow_dispatch: push: branches: - - 'main' + - "main" pull_request: jobs: lint: - uses: 'rhinestonewtf/reusable-workflows/.github/workflows/forge-lint.yaml@main' + uses: "rhinestonewtf/reusable-workflows/.github/workflows/forge-lint.yaml@main" build: - uses: 'rhinestonewtf/reusable-workflows/.github/workflows/forge-build.yaml@main' + uses: "rhinestonewtf/reusable-workflows/.github/workflows/forge-build.yaml@main" test: - needs: ['lint', 'build'] - uses: 'rhinestonewtf/reusable-workflows/.github/workflows/forge-test.yaml@main' + needs: ["lint", "build"] + uses: "rhinestonewtf/reusable-workflows/.github/workflows/forge-test.yaml@main" with: foundry-fuzz-runs: 5000 - foundry-profile: 'test' - match-path: 'test/**/*.sol' + foundry-profile: "test" + match-path: "test/**/*.sol" verbosity: 3 - gas_limit: '18446744073709551615' + gas_limit: "18446744073709551615" memory_limit: 2147483648 From bc4c09ec3812c8590b48c7a1bc08d29ecc131e2f Mon Sep 17 00:00:00 2001 From: highskore <80688826+highskore@users.noreply.github.com> Date: Tue, 15 Oct 2024 11:31:04 +0200 Subject: [PATCH 4/9] chore: fix typo --- src/SpecsParser.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SpecsParser.sol b/src/SpecsParser.sol index f96c1b7..be606e1 100644 --- a/src/SpecsParser.sol +++ b/src/SpecsParser.sol @@ -113,7 +113,7 @@ library ERC4337SpecsParser { function validateBannedOpcodes(VmSafe.DebugStep[] memory debugTrace) internal pure { // forbidden opcodes are GASPRICE, GASLIMIT, DIFFICULTY, TIMESTAMP, BASEFEE, BLOCKHASH, // NUMBER, SELFBALANCE, BALANCE, ORIGIN, GAS, CREATE, COINBASE, SELFDESTRUCT - // Exception: GAS is allowed if followed immediately by5 one of { CALL, DELEGATECALL, + // Exception: GAS is allowed if followed immediately by one of { CALL, DELEGATECALL, // CALLCODE, STATICCALL }] // Loop over the debug steps to validate the opcodes From 4ded08da0b35959aed2b84d0c5b22271875c5705 Mon Sep 17 00:00:00 2001 From: highskore Date: Tue, 15 Oct 2024 12:03:04 +0200 Subject: [PATCH 5/9] fix(ci): remove properties from ci --- .github/workflows/ci.yaml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 45ed103..61e4a1a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -19,6 +19,3 @@ jobs: foundry-fuzz-runs: 5000 foundry-profile: "test" match-path: "test/**/*.sol" - verbosity: 3 - gas_limit: "18446744073709551615" - memory_limit: 2147483648 From e184f950d4e3c454c8aeab53478f2738b19e9dfc Mon Sep 17 00:00:00 2001 From: highskore Date: Tue, 15 Oct 2024 12:05:23 +0200 Subject: [PATCH 6/9] chore: lint --- test/utils/TestBaseUtil.sol | 4 +--- test/utils/TestBaseUtilV060.sol | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/test/utils/TestBaseUtil.sol b/test/utils/TestBaseUtil.sol index 007894d..90b0289 100644 --- a/test/utils/TestBaseUtil.sol +++ b/test/utils/TestBaseUtil.sol @@ -24,9 +24,7 @@ contract TestBaseUtil is Test { factory = new MockFactory(address(implementation)); } - function getAccountAndInitCode( - bytes32 salt - ) + function getAccountAndInitCode(bytes32 salt) internal returns (address account, bytes memory initCode) { diff --git a/test/utils/TestBaseUtilV060.sol b/test/utils/TestBaseUtilV060.sol index 97a77ae..5868c8e 100644 --- a/test/utils/TestBaseUtilV060.sol +++ b/test/utils/TestBaseUtilV060.sol @@ -27,9 +27,7 @@ contract TestBaseUtil is Test { factory = new MockFactory(address(implementation)); } - function getAccountAndInitCode( - bytes32 salt - ) + function getAccountAndInitCode(bytes32 salt) internal returns (address account, bytes memory initCode) { From 557fa2fb9b12585d8fe592db5c544bab402dc0db Mon Sep 17 00:00:00 2001 From: highskore Date: Tue, 15 Oct 2024 12:23:56 +0200 Subject: [PATCH 7/9] feat(ci): re-introduce test ci params --- .github/workflows/ci.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 61e4a1a..46a19b6 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -19,3 +19,6 @@ jobs: foundry-fuzz-runs: 5000 foundry-profile: "test" match-path: "test/**/*.sol" + foundry-verbosity: 3 + foundry-gas-limit: "18446744073709551615" + foundry-memory-limit: 2147483648 From 3e49aa77ad9b3ee2d2e5104208fc7aee2a9a1a4e Mon Sep 17 00:00:00 2001 From: highskore <80688826+highskore@users.noreply.github.com> Date: Tue, 15 Oct 2024 15:06:49 +0200 Subject: [PATCH 8/9] chore: remove unused import --- src/SpecsParser.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SpecsParser.sol b/src/SpecsParser.sol index be606e1..bb6ad99 100644 --- a/src/SpecsParser.sol +++ b/src/SpecsParser.sol @@ -7,7 +7,7 @@ import { IStakeManager, UserOperationDetails } from "./lib/ERC4337.sol"; -import { VmSafe, Vm } from "forge-std/Vm.sol"; +import { VmSafe } from "forge-std/Vm.sol"; import { getLabel, getMappingKeyAndParentOf } from "./lib/Vm.sol"; /** From 8f5c23cc5c1080326a012cdb6511a18d60659d6a Mon Sep 17 00:00:00 2001 From: highskore Date: Tue, 15 Oct 2024 15:28:24 +0200 Subject: [PATCH 9/9] feat: add invalid opcode check --- src/SpecsParser.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/SpecsParser.sol b/src/SpecsParser.sol index be606e1..4976826 100644 --- a/src/SpecsParser.sol +++ b/src/SpecsParser.sol @@ -608,7 +608,7 @@ library ERC4337SpecsParser { function isForbiddenOpcode(uint8 opcode) private pure returns (bool isForbidden) { return opcode == 0x3A // GASPRICE || opcode == 0x45 // GASLIMIT - || opcode == 0x44 // DIFFICULTY + || opcode == 0x44 // DIFFICULTY (PREVRANDAO) || opcode == 0x42 // TIMESTAMP || opcode == 0x48 // BASEFEE || opcode == 0x40 // BLOCKHASH @@ -619,6 +619,7 @@ library ERC4337SpecsParser { || opcode == 0x5A // GAS || opcode == 0xF0 // CREATE || opcode == 0x41 // COINBASE + || opcode == 0xFE // INVALID || opcode == 0xFF; // SELFDESTRUCT } }