feat(balance_overrides): use debug_traceCall to find slots #3937

kaze-cow · 2025-12-01T04:06:39Z

Description

Implement a more reliable, and likely (usually) faster approach for detecting solidity balance override storage addresses.

Rather than bulk storage override/scanning many storage slots for a match, a single debug_traceCall is made on the ERC20 balanceOf function, and reading the resolved SLOAD slots. If only one SLOAD is detected, then we are done. If more than one slot is found, each slot can be tested through eth_call with a single slot override. We test from last accessed to first accessed, as the last accessed storage slot is theoretically the most likely to be the balance slot (as that would be the return value). Since storage slots could be ccessed from anywhere (ex. not just in a solidity mapping), a new DirectSlot strategy has been added.

Probably the only major drawback of the DirectSlot strategy is that it can only be used for a single account. If a separate account balance from the check needs to be overridden, either the slot has to be recalculated, or the balance has to be sent from a separate address after allocation (ex. Spardose). Due to this limitation, any detected slots are additionally tested to match up with SolidityMapping, and the SolidityMapping strategy is returned instead of DirectSlot if able.

This approach is very similar to one followed by Foundry for their deal cheatcode. This is replacing the existing pattern of overriding 50 storage slots and seeing which test balance gets applied.

This method has advantages:

Assuming that there is only one SLOAD in a balanceOf call (most cases), only a single debug_traceCall is required for a single token/address pair. In the case of more than one SLOAD, an additional call is required for each slot to validate the correct storage, which is still inexpensive.
Works even if the token calls a separate contract to get the balance (rather than reading from its internal storage)
Basically any token that stores a user's balance in a single uint256 will be supported by this method.
Since this strategy can be reliably used anywhere, we can potentially. Performance allowing.

Potential drawbacks:

debug_traceCall isnt supported everywhere. This could be overcome by using eth_createAccessList as a fallback, but the createAccessList API does not return accessed storage slots in order, so the check could be considerably more expensive if the balance function accesses many slots.
The DirectSlot strategy can only update the balance of a single token holder whereas SolidityMapping can update any holder.

Other considerations:

Since we now support just about every reasonable token out there with this system, we should consider automatically banning any tokens for which we cannot override the balance. this will make simulations much more simple and reliable.

Since slot detection using this method is only able to find the storage slot for a specific account in question, the interface needed to be updated in a couple places to reflect this. This also means that there is a potential performance disadvantage with this method since balance overrides for different addresses cannot be detected due to not computing via Solidity mapping. This could be mitigated by only supporting balance overrides on the spardose contract (followed by a transfer call) to the needed account as required.

This method requires a node that supports the debug_traceCall API. I believe this is the case for our infra, but please double check 🙏.

When possible this storage detection method will use SolidityMapping to assist in finding cases where it is possible to use. So there should be no need to do heuristic scanning anymore, so I have removed this code segment (and any supporting code).

I added a E2E test and contract to validate that the storage slot detection is working as expected.

Changes

add DirectSlot detector
update caching interface to cache by the pair (token address, overridden balance address)
add E2E test to verify the detector works in practice

How to test

Run just test-e2e local_node_trace_based_balance_detection

Implement a better approach for detecting. Rather than scanning solidity storage slots for a mapping, a new detector strategy `DirectSlot` is added to detect the storage slot through a single `debug_traceCall` on the ERC20 `balanceOf` function, and reading the resolved `SLOAD` slots. This approach is very similar to [one followed by Foundry for their `deal` cheatcode](https://github.com/foundry-rs/foundry/blob/9b13b811849e73654fae046986b8730df8a0d64d/crates/anvil/src/eth/api.rs#L2259). This method has a few advantages: * Assuming that there is only one `SLOAD` in a `balanceOf` call (most cases), only a single `debug_traceCall` is required for a single token/address pair. In the case of more than one SLOAD, an additional call is required for each slot to validate the correct storage, which is still inexpensive. * Basically any token that stores a user's balance in a single `uint256` will be supported by this method. Since slot detection using this method is only able to find the storage slot for a specific account in question, the interface needed to be updated in a couple places to reflect this. This also means that there is a potential performance disadvantage with this method since balance overrides for different addresses cannot be detected due to not computing via Solidity mapping. This could be mitigated by only supporting balance overrides on the spardose contract (followed by a `transfer` call) to the needed account as required. This method requires a node that supports the `debug_traceCall` API. I believe this is the case for our infra, but please double check 🙏. For now I left the original `SolidityMapping` detector strategy as a backup in case `DirectSlot` fails. However, there should theoretically be no cases where `SolidityMapping` would detect when `DirectSlot` does not, so it would be worth removing and relying solely on `DirectSlot`. I added a E2E test and contract to validate that the storage slot detection is working as expected.

crates/ethrpc/src/extensions.rs

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs

MartinquaXD · 2025-12-01T09:57:04Z

crates/shared/src/price_estimation/trade_verifier/balance_overrides/mod.rs

+    /// debug_traceCall. This is similar to Foundry's `deal` approach where
+    /// we trace a balanceOf call to find which storage slot is accessed for
+    /// a given account.
+    DirectSlot { slot: H256 },


I think this needs to also store a target_contract. IIRC tokens like EURe on gnosis chain store the balances in a completely different contract.

yea i probably should have mentioned this. for right now in order to keep the implementation simpler I decided not to update storage slots in other contracts since its somewhat rare and I thought it would amke things a lot more complicated with the trace early on. But its not like it would be hard to add looking at it now.

so I added target contract as you suggest, but in order to properly encode the overrides object in the case that its DirectSlot, both SolidityMapping and SoladyMapping also need to have the target_contract as well. This turned out to be a somewhat big refactor after all. LMK if we should keep it or revert. a5f70bc

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs

…wprotocol/services into feat/tracing-balance-override

crates/contracts/solidity/Makefile

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs

…wprotocol/services into feat/tracing-balance-override

for now not intermingling this with the SolidityMapper strategy because we are likely going to refactor this out in the future according to discussions.

…wprotocol/services into feat/tracing-balance-override

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs

…rides/detector.rs Co-authored-by: Jan [Yann] <4518474+fafk@users.noreply.github.com>

crates/contracts/artifacts/NonStandardERC20Balances.json

crates/contracts/artifacts/RemoteERC20Balances.json

jmg-duarte · 2025-12-16T12:48:09Z

crates/contracts/solidity/tests/RemoteERC20Balances.sol

+import "./NonStandardERC20Balances.sol";
+
+contract RemoteERC20Balances is NonStandardERC20Balances {
+    bool internal immutable balanceFromHere;


Nit

Suggested change

bool internal immutable balanceFromHere;

bool internal immutable isBalanceFromHere;

And possible what does it mean to be "here", the users[user].balance seems to show up from nowhere and it's kind of confusing

the users[user].balance seems to show up from nowhere and it's kind of confusing

yea, sorry about that. as you probably saw it inherits from the original ERC20 contract. The alternative was code duplication, or making a base contract that we dont already have it appears

renamed to isLocalBalance for now. I'm guessing ew are ok on the users[user].balance showing up out of nowhere?

crates/e2e/tests/e2e/quote_verification.rs

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs

crates/shared/src/price_estimation/trade_verifier/balance_overrides/mod.rs

…override

…rides/detector.rs Co-authored-by: José Duarte <duarte.gmj@gmail.com>

…wprotocol/services into feat/tracing-balance-override

crates/shared/src/price_estimation/trade_verifier/balance_overrides/mod.rs

squadgazzz

Great job 🚀
Left a few nit comments.

Apologies for the delayed reviewing process and leaving so many comments.

…rces (#3991) # Description  On my other PR #3937 , I was having difficulty generating the solidity JSONs, and when I did generate them, the format was not correct. I realized that part of the problem is that the CI should be natively checking for this, but it currently does not, so I added a job. Additionally, my computer is ARM64, but the ethereum solidity image is only compatible with `x86_64`, so I had to add a flag to fix this. Finally, the format followed by the other files in the artifacts JSON directory uses a pretty-printed JSON format, but the existing Makefile uses compact format with `jq -c`. So I removed the `-c` flag to ensure this consistent format. It turns out taht, likely due to a newer solidity version, many of the artifacts have changed slightly since they were originally built. So that is included as part of this PR. One other issue that can occur is if the artifact file already exists on your computer, it may fail to write the new version since `make` only builds files that don't exist (or something like that). To resolve this, the `-B` option is used to force rebuilding of all targets. # Changes  - [x] modify the Makefile as described above - [x] add CI job - [x] Ensure the CI job can run properly on ARM64 - [x] run `cd crates/contracts/solidity && make -B artifacts` ## How to test Check the new CI job `solidity-artifacts` Run `make -B artifacts` from `crates/contracts/solidity` to see how it behaves on your own machine.

…rides/mod.rs Co-authored-by: ilya <ilya@cow.fi>

kaze-cow · 2025-12-18T15:55:50Z

Apologies for the delayed reviewing process and leaving so many comments.

No problem at all! This PR rightly deserves scrutiny considering the breadth of impact it can have. Reviews have been great all around 👍

kaze-cow requested a review from a team as a code owner December 1, 2025 04:06

kaze-cow requested a review from MartinquaXD December 1, 2025 04:06

kaze-cow commented Dec 1, 2025

View reviewed changes

crates/ethrpc/src/extensions.rs Outdated Show resolved Hide resolved

kaze-cow commented Dec 1, 2025

View reviewed changes

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs Outdated Show resolved Hide resolved

remove unnecessary code and warnings

aec31dc

MartinquaXD reviewed Dec 1, 2025

View reviewed changes

kaze-cow added 5 commits December 2, 2025 14:05

changes corresponding to comments

a5f70bc

Merge branch 'main' into feat/tracing-balance-override

ef22a7d

fix log, comment

56d7410

just fmt

9896321

Merge branch 'feat/tracing-balance-override' of https://github.com/co…

04d07a5

…wprotocol/services into feat/tracing-balance-override

kaze-cow requested a review from MartinquaXD December 3, 2025 06:16

Merge branch 'main' into feat/tracing-balance-override

a2421ba

kaze-cow requested a review from a team December 4, 2025 09:52

Merge branch 'main' into feat/tracing-balance-override

a4919a4

MartinquaXD reviewed Dec 8, 2025

View reviewed changes

crates/contracts/solidity/Makefile Show resolved Hide resolved

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs Outdated Show resolved Hide resolved

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs Outdated Show resolved Hide resolved

kaze-cow added 8 commits December 9, 2025 00:20

undo unintended makefile change

4979f0b

Merge branch 'feat/tracing-balance-override' of https://github.com/co…

eded10d

…wprotocol/services into feat/tracing-balance-override

add even smarter heuristic detection

2f83e73

for now not intermingling this with the SolidityMapper strategy because we are likely going to refactor this out in the future according to discussions.

Merge branch 'main' into feat/tracing-balance-override

6da8fd3

fix lint

02c314b

Merge branch 'feat/tracing-balance-override' of https://github.com/co…

e9ad7c3

…wprotocol/services into feat/tracing-balance-override

fix clippy

0118199

just fmt

91031ea

kaze-cow requested a review from MartinquaXD December 9, 2025 08:09

kaze-cow enabled auto-merge December 9, 2025 08:09

fafk reviewed Dec 9, 2025

View reviewed changes

crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs Outdated Show resolved Hide resolved

kaze-cow disabled auto-merge December 9, 2025 11:00

Update crates/shared/src/price_estimation/trade_verifier/balance_over…

c5afa35

…rides/detector.rs Co-authored-by: Jan [Yann] <4518474+fafk@users.noreply.github.com>

jmg-duarte reviewed Dec 16, 2025

View reviewed changes

kaze-cow and others added 9 commits December 18, 2025 12:40

Merge remote-tracking branch 'origin/main' into feat/tracing-balance-…

409183e

…override

Update crates/shared/src/price_estimation/trade_verifier/balance_over…

775c5c2

…rides/detector.rs Co-authored-by: José Duarte <duarte.gmj@gmail.com>

Update crates/shared/src/price_estimation/trade_verifier/balance_over…

83f0e2b

…rides/detector.rs Co-authored-by: José Duarte <duarte.gmj@gmail.com>

implement feedbacks

5ec1a59

Merge branch 'feat/tracing-balance-override' of https://github.com/co…

1d707fa

…wprotocol/services into feat/tracing-balance-override

add missing log

e280f5b

changes from feedback, add test for jsons consistency

54b5c79

remove interactive flag

dae2eed

undo playground changes

0a0408b

kaze-cow force-pushed the feat/tracing-balance-override branch from 53c8378 to 0a0408b Compare December 18, 2025 05:44

for right now will move into a separate PR

d83002d

kaze-cow requested review from jmg-duarte and squadgazzz December 18, 2025 06:15

kaze-cow mentioned this pull request Dec 18, 2025

feat(ci): add check to ensure artifacts are in sync with contract sources #3991

Merged

4 tasks

squadgazzz reviewed Dec 18, 2025

View reviewed changes

crates/shared/src/price_estimation/trade_verifier/balance_overrides/mod.rs Outdated Show resolved Hide resolved

squadgazzz reviewed Dec 18, 2025

View reviewed changes

crates/shared/src/price_estimation/trade_verifier/balance_overrides/mod.rs Outdated Show resolved Hide resolved

squadgazzz approved these changes Dec 18, 2025

View reviewed changes

kaze-cow and others added 4 commits December 19, 2025 00:35

Update crates/shared/src/price_estimation/trade_verifier/balance_over…

ffd5e1b

…rides/mod.rs Co-authored-by: ilya <ilya@cow.fi>

Merge branch 'main' into feat/tracing-balance-override

beea0c4

Update crates/shared/src/price_estimation/trade_verifier/balance_over…

74eabb6

…rides/mod.rs Co-authored-by: ilya <ilya@cow.fi>

final fixes for clippy/fmt

9c40d90

kaze-cow enabled auto-merge December 18, 2025 15:54

kaze-cow added this pull request to the merge queue Dec 18, 2025

Merged via the queue into main with commit 3480ee7 Dec 18, 2025
19 checks passed

kaze-cow deleted the feat/tracing-balance-override branch December 18, 2025 16:18

github-actions bot locked and limited conversation to collaborators Dec 18, 2025

	bool internal immutable balanceFromHere;
	bool internal immutable isBalanceFromHere;

feat(balance_overrides): use debug_traceCall to find slots #3937

feat(balance_overrides): use debug_traceCall to find slots #3937

Uh oh!

Conversation

kaze-cow commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Uh oh!

Uh oh!

Uh oh!

MartinquaXD Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

kaze-cow Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

kaze-cow Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmg-duarte Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

kaze-cow Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaze-cow Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

squadgazzz left a comment

Choose a reason for hiding this comment

Uh oh!

kaze-cow commented Dec 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kaze-cow commented Dec 1, 2025 •

edited

Loading

kaze-cow Dec 18, 2025 •

edited

Loading