Sending ETH to (contract address) will give an out of gas error (Native Wallet / Keepkey)

tshifty commented 2 years ago

Overview

When sending ETH directly to a contract address from the send modal you will get an out of gas error and the TX will fail when using Native Wallet (ShapeShift Wallet or Keepkey). Failed TX: https://etherscan.io/tx/0x670880a8f8c28b6b2f25aecec184b04797f3362bb8fc973e8dea1b7b5122efbb

Recreation Steps:

Go to app.shapeshift.com using a native wallet or keepkey
Send ETH directly to a contract using the send modal (not trade) Example Address https://etherscan.io/address/0x8f5df2d1045543d2e1a4be1879cabde24b81d2b9
Once sent notice that the out of gas error will populate once the TX is accepted on chain

Reference

This probably requires looking into how we estimate gas in the ethereum chain adapter, or in unchained itself

Acceptance Criteria

Users should be to send directly to a contract address from the send modal with a higher gas limit and not experience an out of gas error

Screenshots/Mockups

n/a

Ownership

[X] If my bounty needs engineering or needs product I have added the respective labels on the right
[X] As the sponsor of this bounty I will review the changes in a preview environment (ops/product) or review the PR (engineering)

gomesalexandre commented 2 years ago

Quoting @willyogo on this one:

The app is recognizing it's a contract and increasing the gas limit beyond the 21k that a transfer to an EOA would cost, just not increasing enough.
It appears the app is successfully recognizing this is a contract, but failing to properly estimate the gas necessary to process the tx.

0xdef1cafe commented 2 years ago

@tshifty what is this contract and why are you sending ETH directly to it?

willyogo commented 2 years ago

@0xdef1cafe this is coincenter's donation address, I was trying to send eth for the fundraising dinner. I think it's a multisig contract

0xdef1cafe commented 2 years ago

@willyogo thanks for the color, this is valid, we'll look into it, but not a high priority.

gitcoinbot commented 2 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 800.0 FOX (256.24 USD @ $0.32/FOX) attached to it as part of the ShapeShift fund.

If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
- Want to chip in? Add your own contribution here.
- Questions? Checkout Gitcoin Help or the Gitcoin's Discord
- $4,703,942.33 more funded OSS Work available on the Gitcoin Issue Explorer

gitcoinbot commented 2 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 264 years, 8 months from now. Please review their action plans below:

1) ervindimitri has been approved to start work.

I understand the current issue and will work on it to make it possible to send ETH to a contract address 2) g0bl1nsl4y3r has applied to start work _(Funders only: approve worker | reject worker)_.

Implement EIP1559 transactions into shapshift API when sending to a contract address. Transactions are failing because they are using legacy Tx type 3) turtus has applied to start work _(Funders only: approve worker | reject worker)_.

actually its contract code need to be fixed - https://etherscan.io/address/0x8f5df2d1045543d2e1a4be1879cabde24b81d2b9#code

if you are the owner / developer of the contract - please add code here and i will tell more

out of gas error not always mean contract needs more gas - sometimes its bad "wrap" of some require in contract code that are not set

Learn more on the Gitcoin Issue Details page.

naftalimurgor commented 2 years ago

Hey @0xean I'm currently looking into this issue and will submit a PR that best addresses this issue immediately it's ready. I was wondering is there a way to reproduce this behavior on a Testnet deployment as I currently do not have real ETH to test this out. Also, it would be uneconomical to use real ETH.

Best, Naftali

0xean commented 2 years ago

You shouldn't need to create any transactions on testnet or mainnet.

https://ethereum.stackexchange.com/questions/82210/gas-cost-for-ether-transfer-to-smart-contract-from-eoa

Basically, we need to get an estimate when sending ETH to smart contract address vs using 21000 for sending ETH to an EOA

naftalimurgor commented 2 years ago

Wow, great insight, Thanks. That clears the doubt. I will submit a PR once I get something working

janus commented 2 years ago

@0xean , @naftalimurgor , @gomesalexandre

While checking your code I found this[0],

    const { data: responseData } = await axios.get<chainAdapters.ZrxGasApiResponse>(
      'https://gas.api.0x.org/'
    )
    const fees = responseData.result.find((result) => result.source === 'MEDIAN')

Why using responseData which is a type instead of data which is a named variable in subsequent line?

I would like to run the code locally, how do I go about it?

Go to [app.shapeshift.com](http://app.shapeshift.com/) using a native wallet or keepkey I used MetaMask but I didn't see send modal button.

Is Solidity version 0.8.10?

[0] https://github.com/shapeshift/lib/blob/main/packages/chain-adapters/src/ethereum/EthereumChainAdapter.ts#L247

0xean commented 2 years ago

@naftalimurgor - can you please let us know a timeline for a PR on this one? TY!

naftalimurgor commented 2 years ago

@0xean I will submit a PR immediately after I'm done with this. TY

gitcoinbot commented 2 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 800.0 FOX (216.34 USD @ $0.27/FOX) has been submitted by:

@ervindimitri

@0xean please take a look at the submitted work:

PR by @ervindimitri

Learn more on the Gitcoin Issue Details page
Want to chip in? Add your own contribution here.
Questions? Checkout Gitcoin Help or the Gitcoin's Discord
$4,450,908.08 more funded OSS Work available on the Gitcoin Issue Explorer

ghost commented 2 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 800.0 FOX (216.34 USD @ $0.27/FOX) has been submitted by:

1. [@ervindimitri](https://gitcoin.co/ervindimitri)

@0xean please take a look at the submitted work:

* [PR](https://github.com/shapeshift/web/pull/1165) by @ErvinDimitri

* Learn more [on the Gitcoin Issue Details page](https://gitcoin.co/issue/shapeshift/web/1025/100027849)

* Want to chip in? Add your own contribution [here](https://gitcoin.co/issue/shapeshift/web/1025/100027849).

* Questions? Checkout [Gitcoin Help](https://gitcoin.co/help) or the [Gitcoin's Discord](https://discord.gg/gitcoin/)

* $4,450,908.08 more funded OSS Work available on the [Gitcoin Issue Explorer](https://gitcoin.co/explorer)

still sending legacy tx

ErvinDimitri commented 2 years ago

I had been analyzing the code and made some changes to not send legacy tx. And now I was able to send a Tx following the EIP-1559 successfully. So for that I made the following changes: https://github.com/shapeshift/lib/pull/439 : Added the fields that was missing https://github.com/shapeshift/web/pull/1165 : Made some changes on params https://github.com/shapeshift/hdwallet/pull/477 : added some itens to ETHSignTx's interface

0xean commented 2 years ago

https://github.com/shapeshift/web/pull/1215

gitcoinbot commented 2 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 800.0 FOX (316.27 USD @ $0.4/FOX) attached to this issue has been approved & issued to @ervindimitri.

Learn more on the Gitcoin Issue Details page
Questions? Checkout Gitcoin Help or the Gitcoin's Discord
$4,753,594.97 more funded OSS Work available on the Gitcoin Issue Explorer

theoboldfrazier commented 2 years ago

@purelycrickets Can you take a look into reassigning this bounty?

0xean commented 2 years ago

@purelycrickets - close and reopen for new bounty on clean issue.

Lychbot commented 2 years ago

hey @0xean currently moving bounties to Dework. After closing this ticket, should I just reopen this or write up a fresh new ticket?

Lychbot commented 2 years ago

Closing this one. Opened new one #1780

shapeshift / web