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

fix(protocol): fix bridge bugs in getMessageMinGasLimit #17284

Merged
merged 8 commits into from
May 21, 2024

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented May 21, 2024

Fix two bugs in Bridge.getMessageMinGasLimit reported by OZ:

The abi encoding of A = (Message calldata msg) is actually 10 * 32 bytes (not 7 * 32 as elements are not packed) + 32 bytes (A is a dynamic tuple, offset to first elements) + 32 bytes (offset to last bytes element of Message ) + 32 bytes (padded encoding of length of Message.data + dataLength (padded to 32 bytes) = 13 * 32 + (dataLength // 32 * 32) + 32.

For example, if dataLength = 4, the actual abi encoding is 448 bytes long, whereas dataLength + 256 which is the formula used in the code gives only 260 bytes long (code here)

I don't understand the division by 16: I assume the 16 comes from the gas cost of a nonzero calldata byte, but then why divide the number of bytes by 16 instead of multiplying ?

@dantaik dantaik marked this pull request as ready for review May 21, 2024 08:09
@adaki2004
Copy link
Contributor

adaki2004 commented May 21, 2024

Seems OK, but we shall immediately try it out on testnet to see in action - as currently (even tho seems inaccurate, it works :) )

And 2 tests (related to gas) are failing.

@dantaik dantaik enabled auto-merge May 21, 2024 08:35
@dantaik
Copy link
Contributor Author

dantaik commented May 21, 2024

Seems OK, but we shall immediately try it out on testnet to see in action - as currently (even tho seems inaccurate, it works :) )

And 2 tests (related to gas) are failing.

Fixed

@cyberhorsey
Copy link
Contributor

Seems OK, but we shall immediately try it out on testnet to see in action - as currently (even tho seems inaccurate, it works :) )

And 2 tests (related to gas) are failing.

it works in testnet because we are multiplying the return value offchain

Copy link

openzeppelin-code bot commented May 21, 2024

fix(protocol): fix bridge bugs in getMessageMinGasLimit

Generated at commit: 5daf562ed7d7fbeee08f04c9f2bd682e1012e7c0

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
8
41
53
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@dantaik dantaik added this pull request to the merge queue May 21, 2024
@dantaik dantaik disabled auto-merge May 21, 2024 16:22
Merged via the queue into main with commit 859f854 May 21, 2024
4 checks passed
@dantaik dantaik deleted the address_oz_question branch May 21, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants