safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.89k stars 932 forks source link

Add simulateTransaction Function for Static Safe Transaction Simulation #788

Closed mshakeg closed 2 months ago

mshakeg commented 4 months ago

Context / issue

IMU Safe contract does not currently have the functionality to simulate a safe transaction without requiring the threshold of owner signatures. This limitation makes it difficult to estimate gas usage or verify if a transaction would succeed without actually executing it. As a result, users and developers face challenges in performing pre-execution checks especially on chains not supported by Tenderly(not too sure how tenderly gets around this issue).

Proposed solution

To address this issue, I propose adding a new function, simulateTransaction, to the Safe contract. This function will allow for the simulation of transactions in a secure manner without requiring the threshold of owner signatures. To enable this the Safe will have to delegatecall to a new contract that does Executor.execute but immediately afterwards reverts to undo any state changes. This feature will enable users to eth_call the simulateTransaction function to obtain transaction success status, gas used, and the transaction hash, enhancing the ability to perform pre-execution checks. This external contract should be immutably configured somewhere, perhaps singularly in the Safe implementation contract.

Alternatives

Do what Tenderly does, though not sure what they do and if it's generally achievable across EVM chains, if so, then ideally this simulation should be exposed via a function in the ts sdk.

mmv08 commented 4 months ago

Do what Tenderly does, though not sure what they do and if it's generally achievable across EVM chains

We use state overrides on Tenderly, it should be also available in many ethereum nodes by default. Check out the stateDiff parameter here: https://geth.ethereum.org/docs/interacting-with-geth/rpc/ns-eth#eth-call. We use it to override the threshold to 1 + use a pre-approved signature for simulation

nlordell commented 4 months ago

I also think that there is a SimulateTxAccessor that can be used to estimate executing transactions from a Safe without checking signatures.

I think this already can be used with eth_call quite conveniently, especially with the simulate function provided by the compatibility fallback handler.

const safe = new ethers.Contract(
  SAFE_ADDRESS,
  ["function simulate(address, bytes) view returns (bytes)"],
  provider,
);
const accessor = new ethers.Contract(
  // NOTE: address may vary per network, use `safe-deployments` package to get
  // correct address.
  "0x59AD6735bCd8152B84860Cb256dD9e96b85F69Da",
  ["function simulate(address, uint256, bytes, uint8) returns (uint256, bool, bytes)"],
  provider,
);

const data = await safe.simulate(
  accessor.target,
  accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.bytes, tx.operation]),
);
const [gasEstimate, success, returnData] = accessor.interface.decodeFunctionResult("simulate", data);

console.log({
  gasEstimate,
  success,
  returnData,
});

Is there some additional feature that is missing here?

mmv08 commented 4 months ago

I also think that there is a SimulateTxAccessor that can be used to estimate executing transactions from a Safe without checking signatures.

I think this already can be used with eth_call quite conveniently, especially with the simulate function provided by the compatibility fallback handler.

const safe = new ethers.Contract(
  SAFE_ADDRESS,
  ["function simulate(address, bytes) view returns (bytes)"],
  provider,
);
const accessor = new ethers.Contract(
  // NOTE: address may vary per network, use `safe-deployments` package to get
  // correct address.
  "0x59AD6735bCd8152B84860Cb256dD9e96b85F69Da",
  ["function simulate(address, uint256, bytes, uint8) returns (uint256, bool, bytes)"],
  provider,
);

const data = await safe.simulate(
  accessor.target,
  accessor.interface.encodeFunctionData("simulate", [tx.to, tx.value, tx.bytes, tx.operation]),
);
const [gasEstimate, success, returnData] = accessor.interface.decodeFunctionResult("simulate", data);

console.log({
  gasEstimate,
  success,
  returnData,
});

Is there some additional feature that is missing here?

I think the largest thing missing is that this simulation doesn't take into account the guard.

EDIT: Well, I think it's possible to orchestrate multiple calls to different contracts to get a proper simulation, but yeah, I think the initial contract makes sense for more efficiency

mshakeg commented 4 months ago

@nlordell I've actually already made a change to my PoC to simulate safe transactions using the method you described yesterday(see the simulateTx ts function), however it doesn't seem to work when the safeTx involves a native transfer as stated here

@mmv08 the guard would be a pretty major part of getting a more accurate simulation, but I think it's worth adding a standard periphery contract that returns a gas estimate for the safeTx if y'all don't want to make any changes to core. If there isn't a periphery repo should I added the periphery contract here, say under ./contracts/periphery/Simulator.sol

mmv08 commented 4 months ago

@mmv08 the guard would be a pretty major part of getting a more accurate simulation, but I think it's worth adding a standard periphery contract that returns a gas estimate for the safeTx if y'all don't want to make any changes to core. If there isn't a periphery repo should I added the periphery contract here, say under ./contracts/periphery/Simulator.sol

What gas are you referring to? Safe transaction has two gas parameters: dataGas (everything outside of the internal call: signature verification, calldata gas costs, etc) and safeTxGas (internal call gas). Both can be estimated without any additional contracts. For the safeTxGas you can use the simulateTxAccessor contract. For data gas you can calculate the tx broadcast cost (21k, calldata costs, signature verification costs, memory expansion etc) + add a pre-defined buffer value

mmv08 commented 4 months ago

Safe team ran a relayer service before, you can check its code to see how it was done: https://github.com/5afe/safe-relay-service

nlordell commented 4 months ago

however it doesn't seem to work when the safeTx involves a native transfer

Weird, I just ran my snippet with the following modification and it worked:

const data = await safe.simulate(
  accessor.target,
  accessor.interface.encodeFunctionData("simulate", [
    tx.to,
    ethers.parseEther("0.001"), // native transfer amount
    tx.bytes,
    tx.operation,
  ]),
);

Does your Safe have enough funds for the native transfer? If not, I would expect it to revert.

mshakeg commented 4 months ago

@nlordell I mean the slightly modified test cases that do simulateTx fail when native transfers are involved.

https://github.com/safe-global/safe-smart-account/pull/789/files#diff-291a4add941d879c99b79d09eed2c4206c6f23edd4f5edbc93d2bdd18e1c91de

mshakeg commented 4 months ago

@mmv08 ah yes, I didn't notice that SimulateTxAccessor.simulate(...) also returns the safeTxGas

mshakeg commented 4 months ago

@nlordell so took another look and the issue with the simulation success mismatch is not actually on safeTxs that transfer native, rather there's a mismatch with the simulation returning success = true instead of false since the other validation checks(e.g. gasPrice) done in execTransaction aren't done in simulateAndRevert.

nlordell commented 4 months ago

Ah OK - so you want the simulate function to also simulate the relayer refunding code?

mshakeg commented 4 months ago

@nlordell more or less, simulate should simulate the entire execTransaction except for signature validation, basically want to know if whether a safeTx is successful or not without requiring any owner signature.

nlordell commented 4 months ago

Ok, I see. Yes this isn't directly supported at the moment, but can be "hacked" together with the current contracts (depending on the urgency for getting this to work). You could simulate a multisend which internally sets approved hashes for each owner, then call execTransaction using approved hash signatures. Not pretty but should work.

In general, not against adding an additional accessor to simulate this kind of thing (but it shouldn't change the original contracts IMO).