Skip to content

Fix inflated gas estimates#9984

Merged
Gudahtt merged 1 commit intodevelopfrom
fix-inflated-gas-estimates
Dec 3, 2020
Merged

Fix inflated gas estimates#9984
Gudahtt merged 1 commit intodevelopfrom
fix-inflated-gas-estimates

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Dec 3, 2020

If a gasPrice was specified in a transaction sent via a dapp, we would include it in our eth_estimateGas call, causing it to fail if the user had insufficient balance (for either the transaction amount or the gas fee). This resulted in the fallback gas estimate being used; the block gas limit. The block gas limit is quite a bit larger than most transactions need, so this resulted in wildly inflated gas costs being shown on our confirmation screen.

The gasPrice has been removed from the txParams object we pass to eth_estimateGas, so now it won't perform any balance checks anymore. This ensures that we'll get a valid gas estimate, as long as geth is able to simulate the contract execution properly.

Fixes #9967

Manual testing steps:
Follow the reproduction steps listed in #9967, and observe that the initial value set for gas limit is not absurdly high (it was 79500 when I tested it).

@Gudahtt Gudahtt requested a review from a team as a code owner December 3, 2020 15:37
@Gudahtt Gudahtt requested a review from rekmarks December 3, 2020 15:37
@Gudahtt Gudahtt changed the title Fix inflated gas estimtes Fix inflated gas estimates Dec 3, 2020
If a `gasPrice` was specified in a transaction sent via a dapp, we
would include it in our `eth_estimateGas` call, causing it to fail if
the user had insufficient balance (for either the transaction amount or
the gas fee). This resulted in the fallback gas estimate being used;
the block gas limit. The block gas limit is quite a bit larger than
most transactions need, so this resulted in wildly inflated gas costs
being shown on our confirmation screen.

The `gasPrice` has been removed from the `txParams` object we pass to
`eth_estimateGas`, so now it won't perform any balance checks anymore.
This ensures that we'll get a valid gas estimate, as long as geth is
able to simulate the contract execution properly.

Fixes #9967
@Gudahtt Gudahtt force-pushed the fix-inflated-gas-estimates branch from 160ac44 to 505441c Compare December 3, 2020 15:37
@danjm
Copy link
Copy Markdown
Contributor

danjm commented Dec 3, 2020

Looks good. Should we also do this in the estimateGasForSend method in send.utils.js? Users can send to contracts via our send flow, so they could hit the same issue there and see the inflated estimate in the advanced gas modal on the send form screen.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [505441c]
Page Load Metrics (381 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint317256168
domContentLoaded2935663799445
load2955673819345
domInteractive2935653799445

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 3, 2020

This function is the one used in estimateGasForSend. I looked for other cases, and couldn't find any.

@rekmarks
Copy link
Copy Markdown
Member

rekmarks commented Dec 3, 2020

Do the reproduction steps in #9967 produce a transaction that is in fact valid? I see strange errors in the background console when I try it. I can't reproduce while specifying a recipient.

@rekmarks
Copy link
Copy Markdown
Member

rekmarks commented Dec 3, 2020

Specifically, I think I'm getting this warning, which just says (Error#NUM) where NUM is the number of times I've hit that warning. https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/controllers/transactions/index.js/#L838-L843

Copy link
Copy Markdown
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still get a gas limit of 79500 with this call, which to my mind cannot produce a valid transaction:

await ethereum.request({ method: 'eth_sendTransaction', params: [{ from: window.ethereum.selectedAddress, gasPrice: '0x1000' }] })

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 3, 2020

That call was intended to reproduce the inflated gas limit; I didn't test that it produced a valid transaction. I don't think it matters as far as this bug is concerned. It sounds like you may have found a separate bug (i.e. maybe we're missing validation, and a to parameter should be required?).

I'm not sure what those (Error#NUM) statements are about - I'm seeing them too. But I can confirm that before this PR, estimateGas was failing, but after this PR it is not. Something is still calling console.warn but it's a different thing.

Copy link
Copy Markdown
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This does appear to fix the problem it's intended to fix, but we have some tightening up to do of our transaction validation logic, I think.

@Gudahtt Gudahtt merged commit 52d25f0 into develop Dec 3, 2020
@Gudahtt Gudahtt deleted the fix-inflated-gas-estimates branch December 3, 2020 17:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gas limit set too high when dapp specifies a gasPrice on a transaction, and user has insufficient funds

4 participants