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

PoC for Safe simulateTransaction function #789

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
92 changes: 92 additions & 0 deletions contracts/Safe.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;

import {ExecutorExternal} from "./base/ExecutorExternal.sol";
import {ITransactionGuard, GuardManager} from "./base/GuardManager.sol";
import {ModuleManager} from "./base/ModuleManager.sol";
import {OwnerManager} from "./base/OwnerManager.sol";
Expand Down Expand Up @@ -191,6 +192,97 @@ contract Safe is
}
}

struct SimulateTransactionParams {
address to;
uint256 value;
Enum.Operation operation;
uint256 safeTxGas;
uint256 baseGas;
uint256 gasPrice;
address gasToken;
address payable refundReceiver;
}

struct SimulateTransactionResult {
bool success;
uint256 gasUsed;
bytes32 txHash;
}

function simulateTransaction(
address executorExternal, // NB: this is just for this PoC and should obviously be set immutably somewhere for security reasons
SimulateTransactionParams memory params,
bytes calldata data
) external payable returns (SimulateTransactionResult memory result) {
result.txHash = getTransactionHash(
params.to,
params.value,
data,
params.operation,
params.safeTxGas,
params.baseGas,
params.gasPrice,
params.gasToken,
params.refundReceiver,
nonce // We use the current nonce value without incrementing it.
);

address guard = getGuard();
if (guard != address(0)) {
ITransactionGuard(guard).checkTransaction(
params.to,
params.value,
data,
params.operation,
params.safeTxGas,
params.baseGas,
params.gasPrice,
params.gasToken,
params.refundReceiver,
"",
msg.sender
);
}

if (gasleft() < ((params.safeTxGas * 64) / 63).max(params.safeTxGas + 2500) + 500) revertWithError("GS010");

uint256 initialGas = gasleft();

bytes memory delegatecallData = abi.encodeWithSignature(
"externalExecute(address,uint256,bytes,uint8,uint256)",
params.to,
params.value,
data,
params.operation,
params.gasPrice == 0 ? (gasleft() - 2500) : params.safeTxGas
);

(bool success, bytes memory returnData) = executorExternal.delegatecall(delegatecallData);

// Since externalExecute always reverts, we handle the revert reason to get the actual success status
if (!success) {
result.success = parseRevertReason(returnData);
} else {
result.success = false; // This should never happen as externalExecute always reverts
}
result.gasUsed = initialGas - gasleft() + params.baseGas;

if (guard != address(0)) {
ITransactionGuard(guard).checkAfterExecution(result.txHash, true);
}
}

function parseRevertReason(bytes memory reason) private pure returns (bool success) {
if (reason.length != 32) {
if (reason.length < 68) revert("Unexpected error");
assembly {
reason := add(reason, 0x04)
}
revert(abi.decode(reason, (string)));
}
return abi.decode(reason, (bool));
}

/**
* @notice Handles the payment for a Safe transaction.
* @param gasUsed Gas used by the Safe transaction.
Expand Down
14 changes: 14 additions & 0 deletions contracts/base/ExecutorExternal.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;
import "./Executor.sol";

contract ExecutorExternal is Executor {
function externalExecute(address to, uint256 value, bytes memory data, Enum.Operation operation, uint256 txGas) external {
bool success = execute(to, value, data, operation, txGas);
assembly {
let ptr := mload(0x40)
mstore(ptr, success)
revert(ptr, 0x20)
}
}
}
37 changes: 35 additions & 2 deletions src/utils/execution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ export const buildSignatureBytes = (signatures: SafeSignature[]): string => {
let dynamicBytes = "";
for (const sig of signatures) {
if (sig.dynamic) {
/*
A contract signature has a static part of 65 bytes and the dynamic part that needs to be appended
/*
A contract signature has a static part of 65 bytes and the dynamic part that needs to be appended
at the end of signature bytes.
The signature format is
Signature type == 0
Expand Down Expand Up @@ -200,6 +200,39 @@ export const executeTx = async (safe: Safe, safeTx: SafeTransaction, signatures:
);
};

export const simulateTx = async (
safe: Safe,
executorExternal: string,
safeTx: SafeTransaction,
overrides?: any,
): Promise<{
success: boolean;
gasUsed: bigint;
txHash: string;
}> => {
const result = (await safe.simulateTransaction.staticCall(
executorExternal,
{
to: safeTx.to,
value: safeTx.value,
operation: safeTx.operation,
safeTxGas: safeTx.safeTxGas,
baseGas: safeTx.baseGas,
gasPrice: safeTx.gasPrice,
gasToken: safeTx.gasToken,
refundReceiver: safeTx.refundReceiver,
},
safeTx.data,
overrides || {},
)) as [boolean, bigint, string];

return {
success: result[0],
gasUsed: result[1],
txHash: result[2],
};
};

export const buildContractCall = async (
contract: BaseContract,
method: string,
Expand Down
65 changes: 52 additions & 13 deletions test/core/Safe.Execution.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
executeTx,
calculateSafeTransactionHash,
buildContractCall,
simulateTx,
} from "../../src/utils/execution";

import { chainId } from "../utils/encoding";
Expand Down Expand Up @@ -41,12 +42,17 @@ describe("Safe", () => {
}
}`;
const reverter = await deployContract(user1, reverterSource);

const ExecutorExternal = await hre.ethers.getContractFactory("ExecutorExternal");
const executorExternal = await ExecutorExternal.deploy();

return {
safe: await getSafeWithOwners([user1.address]),
reverter,
storageSetter,
nativeTokenReceiver,
signers,
executorExternal,
};
});

Expand Down Expand Up @@ -181,15 +187,19 @@ describe("Safe", () => {
});

it("should revert on unknown operation", async () => {
const { safe, signers } = await setupTests();
const { safe, signers, executorExternal } = await setupTests();
const [user1] = signers;
const safeAddress = await safe.getAddress();

const executorExternalAddress = await executorExternal.getAddress();

const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce(), operation: 2 });
await expect(simulateTx(safe, executorExternalAddress, tx)).to.be.reverted;
await expect(executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)])).to.be.reverted;
});

it("should emit payment in success event", async () => {
const { safe, signers } = await setupTests();
const { safe, signers, executorExternal } = await setupTests();
const [user1, user2] = signers;
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({
Expand All @@ -206,6 +216,11 @@ describe("Safe", () => {
expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.eq(ethers.parseEther("1"));

let executedTx: any;

const executorExternalAddress = await executorExternal.getAddress();
const simulationResult = await simulateTx(safe, executorExternalAddress, tx);
expect(simulationResult.success).to.be.eq(true);

await expect(
executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]).then((tx) => {
executedTx = tx;
Expand All @@ -223,14 +238,15 @@ describe("Safe", () => {
receiptLogs[logIndex].data,
receiptLogs[logIndex].topics,
);
expect(successEvent.txHash).to.be.eq(simulationResult.txHash);
expect(successEvent.txHash).to.be.eq(calculateSafeTransactionHash(safeAddress, tx, await chainId()));
// Gas costs are around 3000, so even if we specified a safeTxGas from 100000 we should not use more
expect(successEvent.payment).to.be.lte(5000n);
expect(await hre.ethers.provider.getBalance(user2.address)).to.eq(userBalance + successEvent.payment);
});

it("should emit payment in failure event", async () => {
const { safe, storageSetter, signers } = await setupTests();
const { safe, storageSetter, signers, executorExternal } = await setupTests();
const [user1, user2] = signers;
const safeAddress = await safe.getAddress();
const storageSetterAddress = await storageSetter.getAddress();
Expand All @@ -249,6 +265,10 @@ describe("Safe", () => {
const userBalance = await hre.ethers.provider.getBalance(user2.address);
await expect(await hre.ethers.provider.getBalance(safeAddress)).to.eq(ethers.parseEther("1"));

const executorExternalAddress = await executorExternal.getAddress();
const simulationResult = await simulateTx(safe, executorExternalAddress, tx);

expect(simulationResult.success).to.be.eq(false);
let executedTx: any;
await expect(
executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)]).then((tx) => {
Expand All @@ -264,23 +284,24 @@ describe("Safe", () => {
receiptLogs[logIndex].data,
receiptLogs[logIndex].topics,
);
expect(successEvent.txHash).to.be.eq(simulationResult.txHash);
expect(successEvent.txHash).to.be.eq(calculateSafeTransactionHash(safeAddress, tx, await chainId()));
// FIXME: When running out of gas the gas used is slightly higher than the safeTxGas and the user has to overpay
expect(successEvent.payment).to.be.lte(10000n);
await expect(await hre.ethers.provider.getBalance(user2.address)).to.eq(userBalance + successEvent.payment);
});

it("should be possible to manually increase gas", async () => {
const { safe, signers } = await setupTests();
const { safe, signers, executorExternal } = await setupTests();
const [user1] = signers;
const safeAddress = await safe.getAddress();
const gasUserSource = `
contract GasUser {

uint256[] public data;

constructor() payable {}

function nested(uint256 level, uint256 count) external {
if (level == 0) {
for (uint256 i = 0; i < count; i++) {
Expand All @@ -290,7 +311,7 @@ describe("Safe", () => {
}
this.nested(level - 1, count);
}

function useGas(uint256 count) public {
this.nested(6, count);
this.nested(8, count);
Expand All @@ -301,18 +322,32 @@ describe("Safe", () => {
const data = gasUser.interface.encodeFunctionData("useGas", [80]);
const safeTxGas = 10000;
const tx = buildSafeTransaction({ to, data, safeTxGas, nonce: await safe.nonce() });

const executorExternalAddress = await executorExternal.getAddress();

let simulationResult = await simulateTx(safe, executorExternalAddress, tx, { gasLimit: 170000 });
expect(simulationResult.success).to.be.eq(false);
await expect(
executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)], { gasLimit: 170000 }),
"Safe transaction should fail with low gasLimit",
).to.emit(safe, "ExecutionFailure");

await expect(
executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)], { gasLimit: 6000000 }),
"Safe transaction should succeed with high gasLimit",
).to.emit(safe, "ExecutionSuccess");
simulationResult = await simulateTx(safe, executorExternalAddress, tx, { gasLimit: 6000000 });
expect(simulationResult.success).to.be.eq(true);
const executedTxPromise = executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)], { gasLimit: 6000000 });
const executedTxResponse = await executedTxPromise;
const receipt = await hre.ethers.provider.getTransactionReceipt(executedTxResponse.hash);
// everything else(i.e. other than the safe tx gas which is being estimated by simulateTransaction)
// includes intrinsic/base gas cost and signature validation, etc
const maxGasForEverythingElse = 50_000n; // it's reasonable that this will take the most
expect(receipt?.gasUsed).to.be.gt(simulationResult.gasUsed);
expect(receipt?.gasUsed).to.be.lt(simulationResult.gasUsed + maxGasForEverythingElse);
await expect(executedTxPromise, "Safe transaction should succeed with high gasLimit").to.emit(safe, "ExecutionSuccess");

// This should only work if the gasPrice is 0
tx.gasPrice = 1;
simulationResult = await simulateTx(safe, executorExternalAddress, tx, { gasLimit: 6000000 });
expect(simulationResult.success).to.be.eq(false);
await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") });
await expect(
executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)], { gasLimit: 6000000 }),
Expand All @@ -321,7 +356,7 @@ describe("Safe", () => {
});

it("should forward all the gas to the native token refund receiver", async () => {
const { safe, nativeTokenReceiver, signers } = await setupTests();
const { safe, nativeTokenReceiver, signers, executorExternal } = await setupTests();
const [user1] = signers;
const safeAddress = await safe.getAddress();
const nativeTokenReceiverAddress = await nativeTokenReceiver.getAddress();
Expand All @@ -338,6 +373,10 @@ describe("Safe", () => {
await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") });
await expect(await hre.ethers.provider.getBalance(safeAddress)).to.eq(ethers.parseEther("1"));

const executorExternalAddress = await executorExternal.getAddress();

const simulationResult = await simulateTx(safe, executorExternalAddress, tx);
expect(simulationResult.success).to.be.eq(true);
const executedTx = await executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)], { gasLimit: 500000 });
const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx.hash);
const receiptLogs = receipt?.logs ?? [];
Expand Down
Loading