-
Notifications
You must be signed in to change notification settings - Fork 89
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
Support faking balances for more tokens #3238
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
mstrug
approved these changes
Jan 16, 2025
crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs
Outdated
Show resolved
Hide resolved
squadgazzz
approved these changes
Jan 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, should a new unit test be added into the balance_overrides.rs?
sunce86
approved these changes
Jan 16, 2025
mstrug
approved these changes
Jan 16, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
To verify more quotes we introduced some logic to figure out which storage slots need to be overwritten to fake a token balance for a given owner. That way our quote verification simulation will not revert when a user currently doesn't hold the necessary amounts of token which improves UX a lot.
While looking into some quote we couldn't verify (simulation) it became apparent that our current strategy to detect which storage slot needs to get overwritten doesn't work for some tokens.
The current logic assumes a "normal" contract where member variables are laid out using the default solidity logic.
This is not sufficient to detect proxy contracts which store their state at a hardcoded address since we can't predict where the author decided to put their member variables in their custom memory layout.
However, most of the time people use libraries to handle that logic for them. These libraries hardcode the necessary information to find the member variable storing the balances in the token so we can just add support by probing for these known hardcoded values too.
Kudos to @fedgiac for helping me debug the problem with the original proxy contract.
Changes
This PR adds support for 2 libraries: OpenZeppelin and Solady.
The OpenZeppeline implementation simply puts the
balances
member variable at a hardcoded position so we can use the existing encoding logic with the hardcoded address we found the in their source code.Solady is a bit different since it doesn't hash 64 bytes but instead only hashes 48 bytes to compute the storage slot of an addresses balance. For this I introduced another enum variant.
This PR also fixes a bug with the original implementation. The original implementation works by calling
token.balanceOf(address)
with 25 storage slots. Each slot gets overwritten with a specific value. WhenbalanceOf()
returns that value we recover which storage slot we originally overwrote.This idea makes a lot of sense but had a problem with the very first override. The amount we override was computed with
U256::from(u64::from_be_bytes([i; 8]))
wherei
is an index variable. That means the first override always tried to set the balance to0
. If you calltoken.balanceOf(random_address)
it will return 0 because the address doesn't have any balances. The original logic interpreted this0
as the sentinel0
value it originally overwrote and therefore returned that thebalances
mapping lives in the0th
storage slot.To fix that we now compute the sentinel values with
U256::from(u64::from_be_bytes([i + 1; 8]))
to ensure that we always search for non-zero values to detect the correct storage override.Note that this error only surfaced when none of the storage slots we tried to override was the correct one. If we were able to override the storage slot
balanceOf()
would no longer return0
to cause the false positive.How to test
added an ignored unit test detecting 1 storage slot of each variant:
Also there was already 1 e2e test that already tested the original logic.