Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Remove gasleft in setupModules, add erc4337 compatibility test #572

Merged
merged 1 commit into from
May 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .env.sample
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,8 @@ INFURA_KEY=""
# Used for custom network
NODE_URL=""
ETHERSCAN_API_KEY=""
# (Optional) Used to run ERC-4337 compatibility test. MNEMONIC is also required.
ERC4337_TEST_BUNDLER_URL=
ERC4337_TEST_NODE_URL=
ERC4337_TEST_SINGLETON_ADDRESS=
ERC4337_TEST_SAFE_FACTORY_ADDRESS=
18 changes: 17 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,29 @@ Usage
yarn
```

### Run all tests:
### Testing

To run the tests:

```bash
yarn build
yarn test
```

Optionally, if you want to run the ERC-4337 compatibility test, it uses a live bundler and node, so it contains some pre-requisites:

1. Define the environment variables:

```
ERC4337_TEST_BUNDLER_URL=
ERC4337_TEST_NODE_URL=
ERC4337_TEST_SINGLETON_ADDRESS=
ERC4337_TEST_SAFE_FACTORY_ADDRESS=
MNEMONIC=
```

2. Pre-fund the executor account derived from the mnemonic with some Native Token to cover the deployment of an ERC4337 module and the pre-fund of the Safe for the test operation.

### Deployments

A collection of the different Safe contract deployments and their addresses can be found in the [Safe deployments](https://github.com/safe-global/safe-deployments) repository.
Expand Down
2 changes: 1 addition & 1 deletion contracts/base/ModuleManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor {
if (to != address(0)) {
require(isContract(to), "GS002");
// Setup has to complete successfully or transaction fails.
require(execute(to, 0, data, Enum.Operation.DelegateCall, gasleft()), "GS000");
require(execute(to, 0, data, Enum.Operation.DelegateCall, type(uint256).max), "GS000");
}
}

Expand Down
65 changes: 65 additions & 0 deletions contracts/test/4337/Test4337ModuleAndHandler.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;
pragma abicoder v2;

import "../../libraries/SafeStorage.sol";

struct UserOperation {
address sender;
uint256 nonce;
bytes initCode;
bytes callData;
uint256 callGasLimit;
uint256 verificationGasLimit;
uint256 preVerificationGas;
uint256 maxFeePerGas;
uint256 maxPriorityFeePerGas;
bytes paymasterAndData;
bytes signature;
}

interface ISafe {
function execTransactionFromModule(address to, uint256 value, bytes memory data, uint8 operation) external returns (bool success);
}

/// @dev A Dummy 4337 Module/Handler for testing purposes
/// ⚠️ ⚠️ ⚠️ DO NOT USE IN PRODUCTION ⚠️ ⚠️ ⚠️
/// The module does not perform ANY validation, it just executes validateUserOp and execTransaction
/// to perform the opcode level compliance by the bundler.
contract Test4337ModuleAndHandler is SafeStorage {
address public immutable myAddress;
address public immutable entryPoint;

address internal constant SENTINEL_MODULES = address(0x1);

constructor(address entryPointAddress) {
entryPoint = entryPointAddress;
myAddress = address(this);
}

function validateUserOp(UserOperation calldata userOp, bytes32, uint256 missingAccountFunds) external returns (uint256 validationData) {
address payable safeAddress = payable(userOp.sender);
ISafe senderSafe = ISafe(safeAddress);

if (missingAccountFunds != 0) {
senderSafe.execTransactionFromModule(entryPoint, missingAccountFunds, "", 0);
}

return 0;
}

function execTransaction(address to, uint256 value, bytes calldata data) external payable {
address payable safeAddress = payable(msg.sender);
ISafe safe = ISafe(safeAddress);
require(safe.execTransactionFromModule(to, value, data, 0), "tx failed");
}

function enableMyself() public {
require(myAddress != address(this), "You need to DELEGATECALL, sir");

// Module cannot be added twice.
require(modules[myAddress] == address(0), "GS102");
modules[myAddress] = modules[SENTINEL_MODULES];
modules[SENTINEL_MODULES] = myAddress;
}
}
5 changes: 3 additions & 2 deletions hardhat.config.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import "@nomiclabs/hardhat-ethers";
import type { HardhatUserConfig, HttpNetworkUserConfig } from "hardhat/types";
import "@nomiclabs/hardhat-etherscan";
import "@nomiclabs/hardhat-waffle";
Expand Down Expand Up @@ -41,7 +42,7 @@ import { BigNumber } from "@ethersproject/bignumber";
import { DeterministicDeploymentInfo } from "hardhat-deploy/dist/types";

const primarySolidityVersion = SOLIDITY_VERSION || "0.7.6";
const soliditySettings = !!SOLIDITY_SETTINGS ? JSON.parse(SOLIDITY_SETTINGS) : undefined;
const soliditySettings = SOLIDITY_SETTINGS ? JSON.parse(SOLIDITY_SETTINGS) : undefined;

const deterministicDeployment = (network: string): DeterministicDeploymentInfo => {
const info = getSingletonFactoryInfo(parseInt(network));
Expand Down Expand Up @@ -132,7 +133,7 @@ const userConfig: HardhatUserConfig = {
},
};
if (NODE_URL) {
userConfig.networks!!.custom = {
userConfig.networks!.custom = {
...sharedNetworkConfig,
url: NODE_URL,
};
Expand Down
133 changes: 133 additions & 0 deletions test/integration/Safe.ERC4337.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import hre from "hardhat";
import { expect } from "chai";
import { AddressZero } from "@ethersproject/constants";
import { hexConcat } from "ethers/lib/utils";
import { getFactoryContract, getSafeSingletonContract } from "../utils/setup";
import { calculateProxyAddress } from "../../src/utils/proxies";

const ERC4337_TEST_ENV_VARIABLES_DEFINED =
typeof process.env.ERC4337_TEST_BUNDLER_URL !== "undefined" &&
typeof process.env.ERC4337_TEST_NODE_URL !== "undefined" &&
typeof process.env.ERC4337_TEST_SAFE_FACTORY_ADDRESS !== "undefined" &&
typeof process.env.ERC4337_TEST_SINGLETON_ADDRESS !== "undefined" &&
typeof process.env.MNEMONIC !== "undefined";

const itif = ERC4337_TEST_ENV_VARIABLES_DEFINED ? it : it.skip;
const SAFE_FACTORY_ADDRESS = process.env.ERC4337_TEST_SAFE_FACTORY_ADDRESS;
const SINGLETON_ADDRESS = process.env.ERC4337_TEST_SINGLETON_ADDRESS;
const BUNDLER_URL = process.env.ERC4337_TEST_BUNDLER_URL;
const NODE_URL = process.env.ERC4337_TEST_NODE_URL;
const MNEMONIC = process.env.MNEMONIC;

type UserOperation = {
sender: string;
nonce: string;
initCode: string;
callData: string;
callGasLimit: string;
verificationGasLimit: string;
preVerificationGas: string;
maxFeePerGas: string;
maxPriorityFeePerGas: string;
paymasterAndData: string;
signature: string;
};

const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));

describe("Safe.ERC4337", () => {
const setupTests = async () => {
const factory = await getFactoryContract();
const singleton = await getSafeSingletonContract();
const bundlerProvider = new hre.ethers.providers.JsonRpcProvider(BUNDLER_URL);
const provider = new hre.ethers.providers.JsonRpcProvider(NODE_URL);
const userWallet = hre.ethers.Wallet.fromMnemonic(MNEMONIC as string).connect(provider);

const entryPoints = await bundlerProvider.send("eth_supportedEntryPoints", []);
if (entryPoints.length === 0) {
throw new Error("No entry points found");
}

return {
factory: factory.attach(SAFE_FACTORY_ADDRESS).connect(userWallet),
singleton: singleton.attach(SINGLETON_ADDRESS).connect(provider),
bundlerProvider,
provider,
userWallet,
entryPoints,
};
};

/**
* This test verifies the ERC4337 based on gas estimation for a user operation
* The user operation deploys a Safe with the ERC4337 module and a handler
* and executes a transaction, thus verifying two things:
* 1. Deployment of the Safe with the ERC4337 module and handler is possible
* 2. Executing a transaction is possible
*/
itif("should pass the ERC4337 validation", async () => {
const { singleton, factory, provider, bundlerProvider, userWallet, entryPoints } = await setupTests();
const ENTRYPOINT_ADDRESS = entryPoints[0];

const erc4337ModuleAndHandlerFactory = (await hre.ethers.getContractFactory("Test4337ModuleAndHandler")).connect(userWallet);
const erc4337ModuleAndHandler = await erc4337ModuleAndHandlerFactory.deploy(ENTRYPOINT_ADDRESS);
// The bundler uses a different node, so we need to allow it sometime to sync
await sleep(10000);

const feeData = await provider.getFeeData();
const maxFeePerGas = feeData.maxFeePerGas.toHexString();

const maxPriorityFeePerGas = feeData.maxPriorityFeePerGas.toHexString();

const moduleInitializer = erc4337ModuleAndHandler.interface.encodeFunctionData("enableMyself", []);
const encodedInitializer = singleton.interface.encodeFunctionData("setup", [
[userWallet.address],
1,
erc4337ModuleAndHandler.address,
moduleInitializer,
erc4337ModuleAndHandler.address,
AddressZero,
0,
AddressZero,
]);
const deployedAddress = await calculateProxyAddress(factory, singleton.address, encodedInitializer, 73);

// The initCode contains 20 bytes of the factory address and the rest is the calldata to be forwarded
const initCode = hexConcat([
factory.address,
factory.interface.encodeFunctionData("createProxyWithNonce", [singleton.address, encodedInitializer, 73]),
]);
const userOpCallData = erc4337ModuleAndHandler.interface.encodeFunctionData("execTransaction", [userWallet.address, 0, 0]);

// Native tokens for the pre-fund 💸
await userWallet.sendTransaction({ to: deployedAddress, value: hre.ethers.utils.parseEther("0.001") });
// The bundler uses a different node, so we need to allow it sometime to sync
await sleep(10000);

const userOperation: UserOperation = {
sender: deployedAddress,
nonce: "0x0",
initCode,
callData: userOpCallData,
callGasLimit: "0x7A120",
verificationGasLimit: "0x7A120",
preVerificationGas: "0x186A0",
maxFeePerGas,
maxPriorityFeePerGas,
paymasterAndData: "0x",
signature: "0x",
};

const DEBUG_MESSAGE = `
Using entry point: ${ENTRYPOINT_ADDRESS}
Deployed Safe address: ${deployedAddress}
Module/Handler address: ${erc4337ModuleAndHandler.address}
User operation:
${JSON.stringify(userOperation, null, 2)}
`;
console.log(DEBUG_MESSAGE);

const estimatedGas = await bundlerProvider.send("eth_estimateUserOperationGas", [userOperation, ENTRYPOINT_ADDRESS]);
expect(estimatedGas).to.not.be.undefined;
});
});
12 changes: 12 additions & 0 deletions test/utils/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,18 @@ export const getSafeSingleton = async () => {
return Safe.attach(SafeDeployment.address);
};

export const getSafeSingletonContract = async () => {
const safeSingleton = await hre.ethers.getContractFactory(safeContractUnderTest());

return safeSingleton;
};

export const getFactoryContract = async () => {
const factory = await hre.ethers.getContractFactory("SafeProxyFactory");

return factory;
};

export const getFactory = async () => {
const FactoryDeployment = await deployments.get("SafeProxyFactory");
const Factory = await hre.ethers.getContractFactory("SafeProxyFactory");
Expand Down
3 changes: 2 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@
"resolveJsonModule": true
},
"exclude": ["dist", "node_modules"],
"include": ["./src/index.ts", "./types"]
"include": ["./src/index.ts", "./types"],
"files": ["./hardhat.config.ts"]
Copy link
Member Author

@mmv08 mmv08 May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was required for the ts compiler to pick up imports in the hardhat config that extend the hardhat runtime to update types accordingly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't enough, so I created #573

}