feat(target_chains/ethereum/pyth): strict minimal updateData parsing #2637
Conversation
This file contains hidden or 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tejasbadadare
changed the title
ali-behjati
reviewed
Apr 30, 2025
| @@ -299,6 +299,24 @@ abstract contract Pyth is | |||
| if (msg.value < requiredFee) revert PythErrors.InsufficientFee(); | |||
| } | |||
|
|
|||
| // In minimal update data mode, revert if we have more or less updates than price IDs | |||
There was a problem hiding this comment.
Can you try out doing it on the fly instead of parsing again to save gas (and hopefully contract size)
There was a problem hiding this comment.
We calculate on the fly now and inlined ParseConfig to chop a struct to save space (see commits after your review,) but we are still 372 bytes short 😭
Any idea if there's something unused that we can remove? Otherwise i'll abandon this approach and just use a heuristic check in Pulse.updatePriceFeeds to check that the updatedata isn't too large
There was a problem hiding this comment.
i recommend you reduce the optimizer runs a bit to match it. the optimizer inlines things to reduce jump costs.
…b/pulse/strict-updatedata-parsing
…b/pulse/strict-updatedata-parsing
1 task
ali-behjati
approved these changes
May 13, 2025
…b/pulse/strict-updatedata-parsing
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.
Summary
Introduces a strict parse mode for
parsePriceUpdatesInternalnamedcheckUpdateDataIsMinimalthat reverts if the number of passed in updates inupdateDatais different thanpriceIds. If the update data is minimal and sufficient to satisfy the requested priceIds, parsing succeeds.Rationale
There is a potential attack vector for Pulse where a bad keeper could provide huge updatedata that contains many more price IDs than requested. This doesn't affect correctness, but it could burn more gas than intended during parsing, which the manager pays for. This could be used to drain manager wallets. Using this new strict parsing mode in Pulse prevents that.
Gas benchmark delta looks good, <0.5% difference: https://app.warp.dev/block/XCs2XtRCKZ1FhQ7StYoYb7
Unfortunately this change causes the contract size to exceed the limit by 666 bytes 👿: https://app.warp.dev/block/x6K9sJRtbLRVMdb4aM212C
How has this been tested?