hashgraph / hedera-json-rpc-relay

Implementation of Ethereum JSON-RPC APIs for Hedera
Apache License 2.0
68 stars 73 forks source link

Transaction reverted with encoded error but string was expected #2821

Open mgarbs opened 3 months ago

mgarbs commented 3 months ago

Description

The Axelar team is encountering an issue where they are expecting a revert message to be a string but it is coming back encoded. Below are the details of the scenario:

Expected Behavior The transaction should revert with NativeCurrencyNotAccepted in the string.

Actual Behavior The transaction reverts with the right error but it is encoded and Axelar is expecting it to be a string in parity with many other evms they've integrated

Environment

mgarbs commented 3 months ago

It appears some events are not emitted for the following tests within Axelargateway:

should allow the operators to burn external tokens should allow the operators to burn external tokens even if the deposit address has ether should allow the operators to burn the external token multiple times from the same address All fail with: AssertionError: Expected event "Transfer" to be emitted, but it wasn't

Sample Transaction Hashes for Tests: should allow the operators to burn the external token multiple times from the same address: https://hashscan.io/testnet/transaction/1723125697.573982315 should allow the operators to burn external tokens: https://hashscan.io/testnet/transaction/1723125656.258025003

One of their partners faced similar issues, which was related to selfdestruct. Comment for reference: https://github.com/onflow/flow-evm-gateway/issues/404#issuecomment-2260946569

NOTE: This is a separate issue. New issue created: https://github.com/hashgraph/hedera-smart-contracts/issues/898

blockchainguyy commented 3 months ago

these 3 fail as well: should allow governance to upgrade to the correct implementation should allow governance to upgrade to the correct implementation with new governance and mint limiter should not allow governance to upgrade to a wrong implementation All have the issue: Error: cannot estimate gas; transaction may fail or may require manual gas limit. I am already using manual gasLimit of: "gasOptions": { "gasLimit": 10000000 }

acuarica commented 3 months ago

Hi @blockchainguyy and @mgarbs, thanks for reporting this. After some investigation, we were able to reproduce the error. However, the error happens on Sepolia as well. Could you to confirm that's the case? (that the error happens in both Hedera Testnet and Sepolia). If yes, it might be better to change the test to wait for the transaction.

Also FYI, I had to modify expectRevert to include the getGasOptions() whenever network.config.skipRevertTests is not set. Without this change, we were not able to reproduce the error.

blockchainguyy commented 3 months ago

The tests for Ethereum Sepolia and Fantom networks appear to be running successfully:

npx hardhat test --network ethereum-sepolia --grep AxelarGateway

  AxelarGateway
    axelar gateway proxy
      ✔ should fail on receiving native value (93427ms)

  1 passing (3m)
npx hardhat test --network fantom --grep AxelarGateway

  AxelarGateway
    axelar gateway proxy
      ✔ should fail on receiving native value (25494ms)

  1 passing (40s)

However, this might be related to this issue for which I have added the below mentioned workaround for Hedera:

const customHttpOptions = {
    url: network.config.rpc,
    timeout: 60000, // Set timeout to 60 seconds
};

The tests that passed on the Fantom and Ethereum Sepolia networks were executed from the main branch in axelar-cgp-solidity repo, which might explain the differences and the failures you’re encountering on these networks. So the issue here could be dependent on the above mentioned issue.

acuarica commented 3 months ago

We have detected the root cause of the issue. Essentially, the eth_estimateGas method call does not properly handle HBAR transfers when the to address is a contract that reverts execution.

How to Reproduce

Create an empty Hardhat project, set up hardhat.config.js to use a Local Node network named local, and use the following contract and test case

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.24;

contract RevertOnReceive {
    error NativeCurrencyNotAccepted();

    receive() external payable {
        revert NativeCurrencyNotAccepted();
    }
}
const { expect } = require("chai");
const { ethers } = require("hardhat");

describe("RevertOnReceive", function () {
  it("should revert with custom error on `receive`", async function () {
    const [signer] = await ethers.getSigners();
    const ContractFactory = await ethers.getContractFactory("RevertOnReceive");
    const contract = await ContractFactory.deploy();

    await expect(signer.sendTransaction({
      to: await contract.getAddress(),
      value: 10_000_000_001,
    })).to.be.revertedWithCustomError(contract, "NativeCurrencyNotAccepted");
  });
});

Then run

$ npx hardhat test --network local
[...]
  1) RevertOnReceive
       should revert with custom error on `receive`:
     AssertionError: Expected transaction to be reverted with custom error 'NativeCurrencyNotAccepted', but it didn't revert
[...]

In the Relay logs, you can notice the line Returning predefined gas for simple transfer, which indicates that the transaction is assumed to be a simple HBAR transfer, when actually is executing contract bytecode (and reverting).

estimateGas(transaction={"value":"0x2540be401","from":"0x67d8d32e9bf1a9968a5ff53b87d777aa8ebbee69","to":"0x23f5e49569a835d7bf9aefd30e4f60cdd570f225"}, _blockParam=pending)
[Request ID: 1eb66cce-de49-48da-b0a4-3d01c473f4f1] [POST] contracts/call Contract Revert: ( StatusCode: '400', StatusText: '', Detail: 'undefined',Data: '{"_status":{"messages":[{"message":"CONTRACT_REVERT_EXECUTED","detail":"","data":"0x858d70bd"}]}}')
Error raised while fetching estimateGas from mirror-node: {"detail":"","data":"0x858d70bd","statusCode":400}
[GET] accounts/0x23f5e49569a835d7bf9aefd30e4f60cdd570f225?transactions=false 200 4 ms
caching account_0x23f5e49569a835d7bf9aefd30e4f60cdd570f225: [...]
Returning predefined gas for simple transfer: 0x5208
[POST]: eth_estimateGas 200 35 ms

This is the eth_estimateGas request that should have failed. eth_estimateGas assumes this is an HBAR transfer, when in reality is executing the bytecode set in the to address.

{
  method: 'eth_estimateGas',
  params: [
    {
      value: '0x2540be401',
      from: '0x67d8d32e9bf1a9968a5ff53b87d777aa8ebbee69',
      to: '0x23f5e49569a835d7bf9aefd30e4f60cdd570f225'
    },
    'pending'
  ]
}
victor-yanev commented 3 months ago

Thank you for bringing this issue to our attention. After further investigation, I have identified the following:

As a result of this investigation, I have created the following issues:

We will prioritize these issues as soon as possible. In the meantime, if you have any additional concerns or questions, please feel free to add them in the comments.

acuarica commented 3 months ago

Great findings, thanks @victor-yanev.

mgarbs commented 3 months ago

Still getting reports of failures on reverts and timeouts as before- @blockchainguyy can you confirm?

`canhaxelar@Canhs-MBP axelar-cgp-solidity % npx hardhat test --network hedera --grep 'AxelarGateway'

AxelarGateway axelar gateway proxy ✔ should revert on invalid gateway implementation address (200ms) 1) should revert if gateway setup fails 2) should fail on receiving native value constructor checks ✔ should revert if auth module is not a contract (731ms) ✔ should revert if token deployer is not a contract (265ms) ✔ check internal constants (22111ms) deployment params ✔ should get the correct governance address (181ms) ✔ should get the correct mint limiter address (182ms) ✔ should get the correct auth module (172ms) ✔ auth module should have the correct owner (168ms) ✔ should get the correct token deployer (176ms) check external methods that should only be called by self 3) should fail on external call to deployToken 4) should fail on external call to mintToken 5) should fail on external call to burnToken 6) should fail on external call to approveContractCall 7) should fail on external call to approveContractCallWithMint 8) should fail on external call to transferOperatorship should preserve the bytecode [ @skip-on-coverage ] ✔ should preserve the same proxy bytecode for each EVM ✔ should preserve the implementation bytecode for each EVM ✔ should have the same deposit handler bytecode preserved for each EVM ✔ should have the same token bytecode preserved for each EVM setTokenMintLimits 9) "before all" hook for "should allow governance to set a token's daily limit" gateway operators 10) "before each" hook for "should allow transferring governance" upgrade 11) should allow governance to upgrade to the correct implementation 12) "before each" hook for "should allow governance to upgrade to the correct implementation with new governance and mint limiter" chain id 13) should fail if chain id mismatches command deployToken deployToken gas: 12000000 ✔ should allow operators to deploy a new token (7027ms) ✔ should not deploy a duplicate token (12096ms) command mintToken ✔ should not allow the operators to mint tokens exceeding the daily limit (18312ms) ✔ should allow the operators to mint tokens (6471ms) 14) "before each" hook for "should not mint wrong symbols" command burnToken burn token positive tests 15) "before all" hook for "should allow the operators to burn internal tokens" burn token negative tests 16) "before all" hook for "should fail if symbol does not correspond to internal token" command transferOperatorship 17) "before each" hook for "should allow operators to transfer operatorship" sendToken send token negative tests 18) should fail if token deployment fails send token positive tests 19) "before all" hook for "should burn internal token and emit an event" external tokens 20) "before all" hook for "should fail if external ERC20 token address is invalid" batch commands 21) "before all" hook for "should revert on mismatch between commandID and command/params length" callContract 22) "before all" hook for "should emit an event" callContractWithToken 23) "before all" hook for "should revert if token does not exist" external contract approval and execution 24) "before all" hook for "should approve and validate contract call" deprecated functions 25) "before all" hook for "should return correct value for allTokensFrozen"

AxelarGatewayUpgrade 26) "before all" hook for "should deploy gateway with the correct modules"

17 passing (18m) 26 failing

1) AxelarGateway axelar gateway proxy should revert if gateway setup fails: HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

2) AxelarGateway axelar gateway proxy should fail on receiving native value: HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

3) AxelarGateway check external methods that should only be called by self should fail on external call to deployToken: AssertionError: Expected transaction to be reverted with custom error 'NotSelf', but it didn't revert at processTicksAndRejections (node:internal/process/task_queues:95:5) at expectRevert (test/utils.js:95:9) at Context. (test/AxelarGateway.js:216:13)

4) AxelarGateway check external methods that should only be called by self should fail on external call to mintToken: AssertionError: Expected transaction to be reverted with custom error 'NotSelf', but it didn't revert at processTicksAndRejections (node:internal/process/task_queues:95:5) at expectRevert (test/utils.js:95:9) at Context. (test/AxelarGateway.js:220:13)

5) AxelarGateway check external methods that should only be called by self should fail on external call to burnToken: AssertionError: Expected transaction to be reverted with custom error 'NotSelf', but it didn't revert at processTicksAndRejections (node:internal/process/task_queues:95:5) at expectRevert (test/utils.js:95:9) at Context. (test/AxelarGateway.js:224:13)

6) AxelarGateway check external methods that should only be called by self should fail on external call to approveContractCall: AssertionError: Expected transaction to be reverted with custom error 'NotSelf', but it didn't revert at processTicksAndRejections (node:internal/process/task_queues:95:5) at expectRevert (test/utils.js:95:9) at Context. (test/AxelarGateway.js:228:13)

7) AxelarGateway check external methods that should only be called by self should fail on external call to approveContractCallWithMint: AssertionError: Expected transaction to be reverted with custom error 'NotSelf', but it didn't revert at processTicksAndRejections (node:internal/process/task_queues:95:5) at expectRevert (test/utils.js:95:9) at Context. (test/AxelarGateway.js:232:13)

8) AxelarGateway check external methods that should only be called by self should fail on external call to transferOperatorship: AssertionError: Expected transaction to be reverted with custom error 'NotSelf', but it didn't revert at processTicksAndRejections (node:internal/process/task_queues:95:5) at expectRevert (test/utils.js:95:9) at Context. (test/AxelarGateway.js:236:13)

9) AxelarGateway setTokenMintLimits "before all" hook for "should allow governance to set a token's daily limit": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

10) AxelarGateway gateway operators "before each" hook for "should allow transferring governance": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

11) AxelarGateway upgrade should allow governance to upgrade to the correct implementation: HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

12) AxelarGateway upgrade "before each" hook for "should allow governance to upgrade to the correct implementation with new governance and mint limiter": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

13) AxelarGateway chain id should fail if chain id mismatches: AssertionError: Expected transaction to be reverted with custom error 'InvalidChainId', but it didn't revert at processTicksAndRejections (node:internal/process/task_queues:95:5) at expectRevert (test/utils.js:95:9) at Context. (test/AxelarGateway.js:672:13)

14) AxelarGateway command mintToken "before each" hook for "should not mint wrong symbols": Error: insufficient funds for intrinsic transaction cost [ See: https://links.ethers.org/v5-errors-INSUFFICIENT_FUNDS ] (error={"name":"ProviderError","_stack":"ProviderError: [Request ID: e008f06c-7b44-4f24-8383-5121124ba309] Insufficient funds for transfer\n at HttpProvider.request (/Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/hardhat/src/internal/core/providers/http.ts:90:21)\n at processTicksAndRejections (node:internal/process/task_queues:95:5)\n at EthersProviderWrapper.send (/Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)","code":-32000,"_isProviderError":true}, method="sendTransaction", transaction=undefined, code=INSUFFICIENT_FUNDS, version=providers/5.7.2) at Logger.makeError (node_modules/@ethersproject/logger/src.ts/index.ts:269:28) at Logger.throwError (node_modules/@ethersproject/logger/src.ts/index.ts:281:20) at checkError (node_modules/@ethersproject/providers/src.ts/json-rpc-provider.ts:98:16) at /Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/@ethersproject/providers/src.ts/json-rpc-provider.ts:265:24 at processTicksAndRejections (node:internal/process/task_queues:95:5)

15) AxelarGateway command burnToken burn token positive tests "before all" hook for "should allow the operators to burn internal tokens": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

16) AxelarGateway command burnToken burn token negative tests "before all" hook for "should fail if symbol does not correspond to internal token": Error: insufficient funds for intrinsic transaction cost [ See: https://links.ethers.org/v5-errors-INSUFFICIENT_FUNDS ] (error={"name":"ProviderError","_stack":"ProviderError: [Request ID: 0c4aad82-0d2a-4388-be05-abc0f3f58a7a] Insufficient funds for transfer\n at HttpProvider.request (/Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/hardhat/src/internal/core/providers/http.ts:90:21)\n at processTicksAndRejections (node:internal/process/task_queues:95:5)\n at EthersProviderWrapper.send (/Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)","code":-32000,"_isProviderError":true}, method="sendTransaction", transaction=undefined, code=INSUFFICIENT_FUNDS, version=providers/5.7.2) at Logger.makeError (node_modules/@ethersproject/logger/src.ts/index.ts:269:28) at Logger.throwError (node_modules/@ethersproject/logger/src.ts/index.ts:281:20) at checkError (node_modules/@ethersproject/providers/src.ts/json-rpc-provider.ts:98:16) at /Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/@ethersproject/providers/src.ts/json-rpc-provider.ts:265:24 at processTicksAndRejections (node:internal/process/task_queues:95:5)

17) AxelarGateway command transferOperatorship "before each" hook for "should allow operators to transfer operatorship": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

18) AxelarGateway sendToken send token negative tests should fail if token deployment fails: Error: insufficient funds for intrinsic transaction cost [ See: https://links.ethers.org/v5-errors-INSUFFICIENT_FUNDS ] (error={"name":"ProviderError","_stack":"ProviderError: [Request ID: 55199612-1c7c-48de-86b6-c5730f1c89a1] Insufficient funds for transfer\n at HttpProvider.request (/Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/hardhat/src/internal/core/providers/http.ts:90:21)\n at processTicksAndRejections (node:internal/process/task_queues:95:5)\n at EthersProviderWrapper.send (/Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)","code":-32000,"_isProviderError":true}, method="sendTransaction", transaction=undefined, code=INSUFFICIENT_FUNDS, version=providers/5.7.2) at Logger.makeError (node_modules/@ethersproject/logger/src.ts/index.ts:269:28) at Logger.throwError (node_modules/@ethersproject/logger/src.ts/index.ts:281:20) at checkError (node_modules/@ethersproject/providers/src.ts/json-rpc-provider.ts:98:16) at /Users/canhaxelar/Documents/dev/axelar-cgp-solidity/node_modules/@ethersproject/providers/src.ts/json-rpc-provider.ts:265:24 at processTicksAndRejections (node:internal/process/task_queues:95:5)

19) AxelarGateway sendToken send token positive tests "before all" hook for "should burn internal token and emit an event": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

20) AxelarGateway external tokens "before all" hook for "should fail if external ERC20 token address is invalid": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

21) AxelarGateway batch commands "before all" hook for "should revert on mismatch between commandID and command/params length": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

22) AxelarGateway callContract "before all" hook for "should emit an event": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

23) AxelarGateway callContractWithToken "before all" hook for "should revert if token does not exist": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

24) AxelarGateway external contract approval and execution "before all" hook for "should approve and validate contract call": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

25) AxelarGateway deprecated functions "before all" hook for "should return correct value for allTokensFrozen": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

26) AxelarGatewayUpgrade "before all" hook for "should deploy gateway with the correct modules": HeadersTimeoutError: Headers Timeout Error at Timeout.onParserTimeout [as callback] (node_modules/undici/lib/client.js:1048:28) at Timeout.onTimeout [as _onTimeout] (node_modules/undici/lib/timers.js:20:13) at listOnTimeout (node:internal/timers:569:17) at processTimers (node:internal/timers:512:7)

canhaxelar@Canhs-MBP axelar-cgp-solidity `%

acuarica commented 3 months ago

Hi @mgarbs, @blockchainguyy. Timeout errors are still expected. We are working on a definitive fix to improve this. See https://github.com/hashgraph/hedera-json-rpc-relay/issues/2808#issuecomment-2298437586 for a workaround.

The fix for expected event errors (related to selfdestruct, issue https://github.com/hashgraph/hedera-smart-contracts/issues/898) was released in previewnet, so the test wouldn't work on testnet yet.

The revert tests also work in testnet as expected. For example

$ npx hardhat test --network hedera-testnet --grep 'should fail on receiving native value'

  AxelarGateway
    axelar gateway proxy
      ✔ should fail on receiving native value (68975ms)
$ npx hardhat test --network hedera-testnet --grep 'check external methods that should only be called by self'

  AxelarGateway
    check external methods that should only be called by self
      ✔ should fail on external call to deployToken (620ms)
      ✔ should fail on external call to mintToken (447ms)
      ✔ should fail on external call to burnToken (354ms)
      ✔ should fail on external call to approveContractCall (524ms)
      ✔ should fail on external call to approveContractCallWithMint (297ms)
      ✔ should fail on external call to transferOperatorship (384ms)
$ npx hardhat test --network hedera-testnet --grep 'should fail if chain id mismatches'

  AxelarGateway
    chain id
      ✔ should fail if chain id mismatches (1686ms)