Calls returning data with a length multiple of 32 do not throw on failure

[mmv08]

I'm trying to update https://github.com/safe-global/safe-contracts to the latest hardhat version, but I can not get tests for read-only method reverts pass.

Here is an example of the tested method:

    function simulateAndRevert(address targetContract, bytes memory calldataPayload) external {
        // solhint-disable-next-line no-inline-assembly
        assembly {
            let success := delegatecall(gas(), targetContract, add(calldataPayload, 0x20), mload(calldataPayload), 0, 0)

            mstore(0x00, success)
            mstore(0x20, returndatasize())
            returndatacopy(0x40, 0, returndatasize())
            revert(0, add(returndatasize(), 0x40))
        }
    }

Any call to this method should revert and reverts with hardhat 2.2.1 but not 2.12.4. In 2.12.4 it returns an empty array.

I did some casual debugging and it seems like the node returns an error but it disappears on the way to the provider. I added some console logs to packages/hardhat-core/src/internal/hardhat-network/provider/modules/eth.ts:

    async _callAction(rpcCall, blockTag) {
        this._validateTransactionAndCallRequest(rpcCall);
        const blockNumberOrPending = await this._resolveNewBlockTag(blockTag);
        const callParams = await this._rpcCallRequestToNodeCallParams(rpcCall);
        const { result: returnData, trace, error, consoleLogMessages, } = await this._node.runCall(callParams, blockNumberOrPending);
        console.log({returnData, error, throwOnCallFailures: this._throwOnCallFailures})
        const code = await this._node.getCodeFromTrace(trace, blockNumberOrPending);
        this._logger.logCallTrace(callParams, code, trace, consoleLogMessages, error);
        await this._runHardhatNetworkMessageTraceHooks(trace, true);
        if (error !== undefined && this._throwOnCallFailures) {
            const callReturnData = trace?.returnData.toString("hex") ?? "";
            error.data = `0x${callReturnData}`;
            console.log("gonna throw")
            throw error;
        }
        return (0, base_types_1.bufferToRpcData)(returnData.value);
    }

and here's the output:

{
  returnData: ReturnData {
    value: <Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 46 more bytes>,
    _selector: '00000000'
  },
  error: Error: VM Exception while processing transaction: reverted with an unrecognized custom error with selector 00000000
      at GnosisSafe.simulateAndRevert (contracts/common/StorageAccessible.sol:44)
      at GnosisSafeProxy.<fallback> (contracts/proxies/GnosisSafeProxy.sol:36)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at runNextTicks (node:internal/process/task_queues:65:3)
      at listOnTimeout (node:internal/timers:528:9)
      at processTimers (node:internal/timers:502:7)
      at async HardhatNode.runCall (/Users/mmv/projects/safe/safe-contracts/node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:652:20)
      at async EthModule._callAction (/Users/mmv/projects/safe/safe-contracts/node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:354:9)
      at async HardhatNetworkProvider.request (/Users/mmv/projects/safe/safe-contracts/node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18),
  throwOnCallFailures: true
}
gonna throw

To reproduce:

1. clone https://github.com/safe-global/safe-contracts/tree/feature/js-tooling-overhaul (notice the branch)
2. run yarn && yarn test

[github-actions]

This issue is also being tracked on Linear.

We use Linear to manage our development process, but we keep the conversations on Github.

LINEAR-ID: eb766a2a-19a2-49f4-b9fc-f2ea93df841d

[fvictorio]

Thank you, I'll look into this as soon as I can

[fvictorio]

@mikhailxyz can I close this in favor of #3446? I mean, does this only happen for eth_calls that revert without return data (for example, because of a require(false) without a reason string, or an assert pre-solidity 0.8.0)?

[mmv08]

@fvictorio in our case it's not empty, it's a method that reverts with the return data and the particular failing test has non-empty data

[mmv08]

If I add this log statement for return data in node_modules/hardhat/internal/hardhat-network/provider/modules/eth.js:

        if (error !== undefined && this._throwOnCallFailures) {
            const callReturnData = trace?.returnData.toString("hex") ?? "";
            console.log({callReturnData})
            error.data = `0x${callReturnData}`;
            throw error;
        }

I get this:

{
  callReturnData: '000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000945'
}

Which doesn't seem empty?

[fvictorio]

Thank you @mikhailxyz. I looked into this and I believe it's a bug in ethers. I opened an issue about it.

A (super ugly) temporary workaround would be something like this:

        it.only("should return estimate in revert", async () => {
            const { safe, killLib } = await setupTests()
            const tx = await safe.populateTransaction.simulateAndRevert(
                killLib.address,
                killLib.interface.encodeFunctionData("estimate", [safe.address, "0x"]),
            )

            let hasThrown = false
            try {
                await network.provider.send("eth_call", [
                    {
                        from: tx.from,
                        to: tx.to,
                        data: tx.data,
                        gas: tx.gasLimit.toHexString().replace("0x0", "0x"),
                    },
                    "latest",
                ])
            } catch {
                hasThrown = true
            }

            expect(hasThrown, "Expected call to revert").to.be.true
        })

Hopefully this gets fixed in ethers soon and you don't have to do that.

[mmv08]

@fvictorio Interesting. I tried something similar, but I used call instead of send and it didn't throw. I can confirm that with .send it throws as expected:

            const encodedData = safe.interface.encodeFunctionData("simulateAndRevert", [
                killLib.address,
                killLib.interface.encodeFunctionData("estimate", [safe.address, "0x"]),
            ])

            console.log(
                "call result",
                await user1.provider.call({
                    to: safe.address,
                    data: encodedData,
                }),
            ) // returns 0x000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000945
            console.log(
                "send result",
                await user1.provider.send("eth_call", [
                    {
                        from: user1.address,
                        to: safe.address,
                        data: encodedData,
                    },
                    "latest",
                ]),
            ) // throws

[mmv08]

Thank you for checking this! 🙏

[fvictorio]

I tried something similar, but I used call instead of send and it didn't throw.

It's because provider.send is a lower-level method that just makes a raw JSON-RPC call, while provider.call is a higher-level abstraction that probably has the same bug.