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

Add A Balance Override Detector #3

Closed

Conversation

nlordell
Copy link
Owner

@nlordell nlordell commented Dec 4, 2024

Description

This PR adds a balance override detector for quote verification. This allows tokens to automatically support balance overrides without automatic configurations.

Changes

A new (but as of yet) unused component is added that can detect balance override strategies for tokens. The current implementation strategy is quite simple - just try a bunch of different slots and see which one works. For now, the number of slots to try is hard-coded at 25, but this can trivially be changed to be configurable in the future.

The component is marked with #[allow(dead_code)] as it isn't being used anywhere, In the interest of keeping the PR size down, I will add all of the glue code to make it used in a follow up (which will also test whether or not the detector works as expected).

How to Test

In the meantime, I did some local testing to check that it works with tokens for which I know the mapping slot on Ethereum Mainnet (test not included in the PR):

async fn temporary_test() {
    let web3 = Arc::new(ethrpc::Web3::new(ethrpc::create_test_transport(
        "https://eth.llamarpc.com",
    )));
    let detector = Detector::new(web3);
    let result = detector
        .detect(addr!("A0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48"))
        .await;
    println!("{result:?}");
    assert!(matches!(result, Ok(Strategy::Mapping { slot }) if slot == U256::from(9_u8)));
}

This test successfully passes!

@nlordell nlordell force-pushed the feat/token-state-override-4 branch from d7805e8 to fc868c8 Compare December 4, 2024 12:41
@MartinquaXD
Copy link

Let me summarise the strategy in my own words to see if I get it fully.
In ethereum the member variables of a contract get assigned storage slots in increasing order. Multiple smaller types (e.g. bool) may be packed into one to save space but mapping()s (the way to commonly associate a balance with a given address) are guaranteed to have their own storage slot.
Additionally the balance of a given address does not actually live in the storage slot of the mapping() but rather in the slot keccak(holder || slot_of_mapping).

This strategy generates a new unknown address as a test token holder which is guaranteed to not conflict with any address that could be used for special purposes in a SC implementation.
Ultimately this strategy calls balanceOf(holder) with 25 state overrides. The state overrides are for the slots keccak(holder || 0), keccak(holder || 1), ... keccak(holder || 24) and will contain unique marker values.
So finally when the balanceOf() with all the state overrides returns one of the marker values we know which storage override resulted in the balance change. And that is the storage slot which contains the SC member variable akin to mapping(address => uint256) balances.

In that case this strategy should be sufficient for faking balances for tokens with:

  • balances being stored in a simple mapping(address => uint256)
  • balances being among first 24 member variables (not taking space savings for small variables into account)
  • balanceOf() not applying any logic to the value stored in the mapping (e.g. yield bearing based on timestamp)

However, all of this still leaves me with a question about your example test. USDC is proxy contract that is verified on etherscan and your test suggests that the balances mapping is stored in the 10th storage slot. Wouldn't that mean the balances are stored in the wrapper proxy contract and not the implementation? Given that the proxy is verified on etherscan shouldn't we see the relevant variables there?

@nlordell
Copy link
Owner Author

nlordell commented Dec 4, 2024

Let me summarise the strategy in my own words to see if I get it fully.

Summary is correct.

Wouldn't that mean the balances are stored in the wrapper proxy contract and not the implementation? Given that the proxy is verified on etherscan shouldn't we see the relevant variables there?

The proxy contract does a DELEGATECALL to the implementation. This executes the contract code "in the context of the caller". This means that the implementation is executed with the storage of the calling contract (here, the proxy).

This is important for proxy upgrades - if it were not a DELEGATECALL, then every time a new implementation of USDC were to be deployed, it would start with "fresh" and 0-ed out storage; meaning everyone's balance would be reset to 0.

You essentially need to look at the implementation code (which AFAIU is also verified on Etherscan) to manually figure out the storage slot.

nlordell added a commit that referenced this pull request Dec 4, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
nlordell added a commit that referenced this pull request Dec 4, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
@nlordell nlordell force-pushed the feat/token-state-override-4 branch from fc868c8 to 6cf73ec Compare December 4, 2024 16:46
nlordell added a commit that referenced this pull request Dec 4, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
@nlordell nlordell force-pushed the feat/token-state-override-3 branch from 723758d to 83ae3bb Compare December 4, 2024 16:58
@nlordell nlordell force-pushed the feat/token-state-override-4 branch from 6cf73ec to 0e32024 Compare December 4, 2024 16:58
nlordell added a commit that referenced this pull request Dec 4, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
This PR adds a balance override detector for quote verification. This
allows

The current implementation strategy is quite simple - just try a bunch
of different slots and see which one works. For now, the number of slots
to try is hardcoded at 25, but this can be configurable in the future.

The component is marked with `#[allow(dead_code)]` as it isn't being
used anywhere, In the interest of keeping the PR size down, I will add
all of the glue code to make it used in a follow up (which will also
test whether or not the detector works as expected).

In the meantime, I did some local testing to check that it works with
tokens for which I know the mapping slot on Ethereum Mainnet (test not
included in the PR).
@nlordell nlordell force-pushed the feat/token-state-override-3 branch from 83ae3bb to f7fcfff Compare December 4, 2024 18:26
@nlordell nlordell force-pushed the feat/token-state-override-4 branch from 0e32024 to df8f9ba Compare December 4, 2024 18:27
nlordell added a commit that referenced this pull request Dec 4, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
nlordell added a commit that referenced this pull request Dec 5, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
@nlordell
Copy link
Owner Author

nlordell commented Dec 5, 2024

Superseded by cowprotocol#3148

@nlordell nlordell closed this Dec 5, 2024
nlordell added a commit that referenced this pull request Dec 5, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
nlordell added a commit that referenced this pull request Dec 6, 2024
This PR is a follow up to #3 in that it adds configuration and glue code
for enabling the automatic token balance override detection introduced
in the aforementioned PR.

An E2E test was modified to include balance override tests for both
configured and auto-detected tokens.
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.

2 participants