-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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): improve Bridge gasleft()
validation
#17422
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feat(protocol): improve Bridge
|
Severity Level | Results | |
---|---|---|
Contracts | Critical High Medium Low Note Total |
2 2 0 8 42 54 |
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
commented
May 30, 2024
Brechtpd
reviewed
May 30, 2024
Brechtpd
approved these changes
Jun 1, 2024
adaki2004
approved these changes
Jun 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolving L-23 Diff-Audit: Inaccurate Gas Limit on Message Invocation
Users can send cross-chain messages by calling sendMessage on the Bridge contract. An optional gas limit can be defined for this message. In that case, if a user wants their message to be executed with at least A gas, message.gasLimit has to be at least A + minGas, where minGas depends on the message length plus a flat gas amount.
When a message is processed by a third party, the gas remaining before invoking the user's message is validated to be greater than 64 / 63 * A. This is because of EIP-150, which silently caps the amount of gas sent in external calls to 63 / 64 * gasleft(). If such a check was not present, it would be possible for the gas to be capped by EIP-150, executing the call with less than A gas.
However, while the intention was right, a gas limit of A is not guaranteed. This is due to the gas expenses that accumulate between the EIP-150 check and the actual message execution on the target:
costs of memory expansion
account access
transfers
opcode costs
Because 1 / 64 * gasleft() has to be enough to execute the rest of the processMessage function without running out of gas, a gas limit below A on the target is only possible for high gas limits. A proof of concept shows the issue in practice.
Consider addressing the above to build a more predictable bridge for users and projects sending cross-chain messages. What follows is an example of how this could be achieved.
The goal of the computation is to ensure that EIP-150 does not silently cap the amount of gas sent with the external call to be less than A. We note:
memory_cost: the amount of gas needed to expand the memory when storing the inputs and outputs of the external call.
access_gas_cost: the gas cost of accessing the message.to account. This currently corresponds to 2600 gas if the account is cold, and 100 otherwise.
transfer_gas_cost: the cost of transferring a nonzero msg.value. This cost is currently 9000, but provides a 2300 gas stipend to the called contract.
create_gas_cost: the cost of creating a new account, currently 25000. This only applies if message.value != 0, message.to.nounce == 0, message.to.code == b"" and message.to.balance == 0. Because message.to is checked to have code, this cost can be ignored here.
We thus want to check that the following:
63 / 64 * (gasleft() - memory_cost - access_gas_cost - transfer_gas_cost)
is at least as much as A (reference implementation). The sum of access_gas_cost and transfer_gas_cost can be upper bounded with 2_600 + 9_000 - 2_300 = 9_300. The memory_cost is cumbersome to compute in practice, but by estimating the costs through tests, we can upper bound the cost of memory expansion for up to 10_000 bytes of message.data in the context of the call with the formula 1_200 + 3 * message.data.length / 32.
Thus, it would be possible to validate that the call will have enough gas by checking the following condition right before the external call is made:
63 * gasleft() >= 64 * A + 63 * (9_300 + 1_200 + 3 * message.data.length / 32) + small_buffer
This would be accurate for messages with up to 10_000 bytes of message.data. Any message above this limit could be required to have a gas limit of zero. This would force it to be processed by its destOwner. A similar approach, without a constraint on the data size, has been adopted by Optimism and can be used for inspiration.
Note that I didn't put a hard limit as suggested to
message.data.length
as we don't know yet in practice how largemessage.data
can be.