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 #3148

Merged

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Dec 5, 2024

Description

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

Changes

A new (but as of yet) unused component is added that can detect balance override strategies for tokens. The current implementation 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!

 # Description

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

 # Changes

A new (but as of yet) unused component is added that can detect balance
override strategies for tokens. The current implementation 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):

```rust
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 requested a review from a team as a code owner December 5, 2024 15:56
@nlordell
Copy link
Contributor Author

nlordell commented Dec 5, 2024

Reposting @MartinquaXD's comment for additional context for others:

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?

And my answer:

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.

@MartinquaXD MartinquaXD merged commit a88bfbd into cowprotocol:main Dec 6, 2024
10 of 11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants