Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(protocol): use 35000 as gas limit for sending Ether in Brdge #16666

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Apr 6, 2024

This is based on OZ's feedback:

Screenshot 2024-04-06 at 11 46 33

BTW, according to ChatGPT:

When you send Ether to an EOA with a zero balance, you are changing the state from zero to a non-zero balance, which has a higher gas cost due to the additional work required to update the state. The gas cost for sending Ether to an account with a zero balance is 21,000 gas for the transaction itself, plus an additional 20,000 gas for changing the state from zero to non-zero, totaling 41,000 gas.

@Brechtpd do you think 60000 is a good number?

@dantaik dantaik requested review from Brechtpd and adaki2004 April 6, 2024 03:47
@dantaik dantaik marked this pull request as ready for review April 6, 2024 03:47
Copy link

openzeppelin-code bot commented Apr 6, 2024

feat(protocol): use 35000 as gas limit for sending Ether in Brdge

Generated at commit: 096237e416eb48e19ab5fb093ae0e58be0477ce9

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
3
39
46
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@Brechtpd
Copy link
Contributor

Brechtpd commented Apr 6, 2024

image

As far as I understand, the cost for most ETH transfer related costs is charged on the call itself, not inside the call. So the gas passed into I think could be set much lower. There's also already an automatic stipend of 2300 gas.

My feeling is that 60k gas is quite high, on the other hand it's also quite critical that people can count on the refund to work even if the refund address is a smart contract that may contain some fancy logic. Taking that into account I think 60k is reasonable.

I think we shouldn't make it a general gas limit though for all ETH transfers everywhere, if a limit makes sense for the refund I think we should just specify a gas limit there.

@dantaik
Copy link
Contributor Author

dantaik commented Apr 7, 2024

I think we shouldn't make it a general gas limit though for all ETH transfers everywhere, if a limit makes sense for the refund I think we should just specify a gas limit there.

I read it as: we don't need to adopt this change. I'm ok without this PR merged.

@Brechtpd
Copy link
Contributor

Brechtpd commented Apr 7, 2024

Adding this limit to the refund ETH transfer I think makes sense because something the relayer does have to pay the cost for so a risk (depending on how sophisticated the relayer is). I'm just not sure about adding this limit to all ETH transfers because those transfers mostly do not have the same concern. But I'm not against it to keep things simple and uniform.

@dantaik
Copy link
Contributor Author

dantaik commented Apr 8, 2024

Adding this limit to the refund ETH transfer I think makes sense because something the relayer does have to pay the cost for so a risk (depending on how sophisticated the relayer is). I'm just not sure about adding this limit to all ETH transfers because those transfers mostly do not have the same concern. But I'm not against it to keep things simple and uniform.

I changed to use the 35K gas limit only in Bridge.

@dantaik dantaik changed the title feat(protocol): use 60000 as gas limit for sending Ether feat(protocol): use 3500 as gas limit for sending Ether in Brdge Apr 8, 2024
@dantaik dantaik changed the title feat(protocol): use 3500 as gas limit for sending Ether in Brdge feat(protocol): use 35000 as gas limit for sending Ether in Brdge Apr 8, 2024
@dantaik dantaik enabled auto-merge April 8, 2024 02:58
@dantaik dantaik requested a review from davidtaikocha April 8, 2024 06:44
@dantaik dantaik added this pull request to the merge queue Apr 8, 2024
Merged via the queue into main with commit 4909782 Apr 8, 2024
6 checks passed
@dantaik dantaik deleted the sendEtherAndVerify_gaslimit branch April 8, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants