attemptTransaction error has overwritten the original error message

[leckylao]

Description

Function attemptTransaction on https://github.com/gnosis/contract-proxy-kit/blob/v1.1.1/src/index.js#L468 has new Error message 'transaction execution expected to fail' which will overwrite the original error message on the CALL.

attemptTransaction(
            this.contract, this.viewContract,
            'execTransaction',
            [
              to, value, data, operation,
              0, 0, 0, zeroAddress, zeroAddress,
              signatureForAddress(ownerAccount),
            ],
            new Error('transaction execution expected to fail'),
          );

For example when running setup for a module it will run setManager https://github.com/gnosis/safe-contracts/blob/v1.1.1/contracts/base/Module.sol#L23 which will return "Manager has already been set" but it will get overwritten with "transaction execution expected to fail".

require(address(manager) == address(0), "Manager has already been set");

So is there a way to pass the original error message if there's one?

[cag]

Hey Lecky!

Yeah, this is really tricky. Part of the challenge is that the error is lost during the call to execTransaction: https://github.com/gnosis/safe-contracts/blob/892448e93f6203b530630f20de45d8a55fde7463/contracts/base/Executor.sol#L27

I try to address this a bit with CALLs by doing a preflight eth_call in the special case of a single CALL transaction (see checkSingleCall in the code for details; basically I do the same transaction, but not through execTransaction, but rather directly an eth_call from the proxy instance, and let any errors bubble up), but it's not a perfect solution, and would only work for Ganache right now I think, as with Geth, IIRC the call would just execute and return the revert data (an ABI-encoded Error(string)) as the result, and with OpenEthereum, there would be a revert but the revert data wouldn't be decoded.

I dunno if that answers your question, but we're working on sanitizing this as part of the next major version.

[leckylao]

I try to address this a bit with CALLs by doing a preflight eth_call in the special case of a single CALL transaction (see checkSingleCall in the code for details; basically I do the same transaction, but not through execTransaction, but rather directly an eth_call from the proxy instance, and let any errors bubble up), but it's not a perfect solution, and would only work for Ganache right now I think, as with Geth, IIRC the call would just execute and return the revert data (an ABI-encoded Error(string)) as the result, and with OpenEthereum, there would be a revert but the revert data wouldn't be decoded.

Maybe fire eth_call as postflight on errors only, that way we can get the correct error message.

How about using the new try/catch functionality in Solidity 0.6?

https://forum.openzeppelin.com/t/a-brief-analysis-of-the-new-try-catch-functionality-in-solidity-0-6/2564

So maybe use try on the executeCall and catch error then fire eth_call?

try executeCall(to, value, data, txGas){
  emit SuccessEvent();
} catch {
  this.web3.eth.call({
      from,
      to,
      value,
      data,
    });
}

[cag]

Hey Lecky! Unfortunately, that change is not in the purview of this repo. Contract changes which allow this to happen should probably be argued for in the original issue thread here: safe-global/safe-smart-account#182

However, we are aware of the new try/catch mechanism introduced in 0.6.0! We just tend to be more conservative about changes, since our contracts have been audited and formally verified, and introducing security regressions is a big concern for us when we do contract updates. That said, there might be a chance this could make it into the next release.

As far as doing this with the current contracts at this layer, the preflight eth_call approach was just a first try at this issue. I think doing an eth_call postflight to collect the revert reason is a good idea, and I think I'll try to fold this approach into a current vein of work to ensure compatibility with major clients (#55 #58).

[rmeissner]

If you want to debug this on a test network, just deploy your own multisend contract (only the interface is required to be the same) and adjust it in a way that it returns the error message. The chances are very low that we will adjust our MultiSend this year.

Edit: the address of your multisend can be passed into the configuration of the CPK (https://github.com/gnosis/contract-proxy-kit#networks-configuration)

[cag]

@rmeissner I don't think that will allow the error message to be recovered though, because MultiSend has to be used through a delegatecall. To get the revert message, one would have to do a call to execTransactionFromModule as a call from some module address, which would not be possible on current vanilla Safes.

Also, emitting an event instead of reverting would work only if the transaction gets processed/mined. Currently the semantics are to reject with an error if transactions are going to fail.

Basically, to get an error from a batch transaction, we would have to:

• Modify MultiSend to pass reverted data up the stack, as mentioned
• Register a module address intended to be the from for an eth_call. It could be some dummy address like 0x2 or something
• Add a call to execTransactionFromModule to get the revert data.

Also a feature like this would also necessitate a factory change, to facilitate getting an error that would happen when a contract that doesn't exist yet does a batch of transactions.

@leckylao Long story short, I think the only time the original error message from the contract will get displayed will be for a single call (no batches, and no delegatecalls). The other scenarios will involve changes that probably won't happen. I will close this issue when this behavior is guaranteed for all major clients, though I must note that this should already work for Ganache.

[rmeissner]

ahhh yes what you write is correct. You can also make use of a geth feature were you can overwrite the contract source atr a specific address to expose the error. It is not planned that execTransaction will return the error in the future, but different read methods might allow this.