Support Aave v3 aToken balance overrides#4345
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a specialized balance override strategy for Aave v3 aTokens, addressing an issue where the standard storage-slot-based overrides failed due to Aave's internal scaling and packed storage layout. It adds a new aave module to handle the necessary math and RPC probes, integrates this into the Detector for automatic discovery, and updates the BalanceOverrides component to support asynchronous state overrides. A performance improvement was suggested to parallelize independent RPC calls during the aToken probing process to reduce latency.
|
@claude review |
|
Claude finished @squadgazzz's task in 2m 45s —— View job Code Review
This is a well-structured fix for a real production bug. The three-call identity check for auto-detection is the right approach — it prevents false positives without requiring a hardcoded registry. The math is correct and is tested with a round-trip reproduction. A few issues to address: 1. Performance: Parallelize independent eth_calls in
|
jmg-duarte
left a comment
There was a problem hiding this comment.
logic wise, everything looks sound to me
| /// holder and amount. `web3` is only consulted by strategies that need | ||
| /// on-chain reads at override time (currently `AaveV3AToken`); other | ||
| /// variants ignore it and complete synchronously. |
There was a problem hiding this comment.
If web3 is only used by strategies could we not put the Web3 struct into the Strategy itself so that it has everything it needs?
There was a problem hiding this comment.
Considered this and the tension is that Strategy lives in configs::balance_overrides as a serde-deserialized config type (loaded from TOML). A Web3 handle cannot sit inside a serde type, so carrying it on the enum would require splitting into a config variant and a runtime variant with an attachment pass on BalanceOverrides construction.
That's a fair chunk of plumbing for a one-off Option<&Web3> parameter on the trait. Leaving it as the extra arg for now.
There was a problem hiding this comment.
I see. Given that we don't use the hardcoded configurations in practice and that our detection capabilities should now automatically find anything the hardcoded list can cover what do you think about dropping the hardcoded configuration altogether?
That way we can drop the Deserialize on the strategy and have the cleaner implementation with the provider inside the Strategy.
Given that this change would be not super small it would probably make the most sense to merge this PR as is and follow up with another clean up PR.
MartinquaXD
left a comment
There was a problem hiding this comment.
LGTM besides the 2 stray comments
Description
Quote verification was silently failing for any Aave v3 aToken as the sell token (e.g. aEthWETH → WETH) because the balance-override mechanism assumes the value at the balance storage slot is what
balanceOfreturns. Aave v3 aTokens break both assumptions:balanceOfapplies arayMul(scaled, liquidityIndex)scaling, and storage is packedUserState { uint128 balance; uint128 additionalData }. The auto-detector's verify step therefore never matched, returnedNotFound, and the trade verifier silently skipped the override, producing the production revertexecution reverted: trader does not have enough sell tokenvisible in barn logs for aEthWETH quotes today (reproduced on Tenderly: https://dashboard.tenderly.co/cow-protocol/barn/simulator/1e69fa91-9496-44d1-9aec-a0e34166f9df).Changes
Strategy::AaveV3AToken { target_contract, pool, underlying, map_slot }variant inconfigs::balance_overrides::Strategyand a corresponding async resolver onBalanceOverridesthat fetches the currentgetReserveNormalizedIncomefrom the Aave v3 Pool, rayDivs the requested amount (round-half-up, bit-for-bit compatible withWadRayMath.rayDiv), and writes it into the low 128 bits of the packed_userStateslot.UNDERLYING_ASSET_ADDRESS()+POOL()and then callspool.getReserveData(underlying)and verifies the returnedaTokenAddressequals the probed token — an identity check that only passes for tokens the pool itself has registered as an aToken for their underlying, preventing rogue contracts that merely implement the aToken selectors from being accepted. When the probe succeeds, mapping-style slots are also offered asAaveV3ATokencandidates and verified with a 1-wei round-trip tolerance instead of strict equality. No hardcoded per-token list is needed. aEthWETH and every other Aave v3 aToken on every chain (mainnet, Arbitrum, Base, Gnosis, Polygon, BNB, Linea, Plasma, Ink, Avalanche, etc.) are picked up automatically the first time they're quoted.balance-overrides::aavemodule so the production override builder and the detector probe/verify use identical arithmetic.trade_verifier::prepare_state_overridesnow emitstracing::warn!when the spardose balance override for the sell token can't be resolved — mirroring the existing warn on the buy-side path so future missing overrides surface in logs instead of only showing up as downstream reverts.Aave v4 note
Aave v4's user-facing deposit receipt is
TokenizationSpoke, an ERC-4626 vault built on OpenZeppelin'sERC20Upgradeable— not a scaled aToken.balanceOfis the plain OZ_balances[user]mapping (norayMulscaling), and the contract doesn't exposeUNDERLYING_ASSET_ADDRESS()orPOOL(). Our probe therefore correctly rejects v4 Spokes and the detector falls through to the existingSolidityMapping/DirectSlotheuristics, which handle them as standard ERC-20s. No v4-specific work is needed here.Source: https://github.com/aave/aave-v4/blob/main/src/spoke/TokenizationSpoke.sol
How to test
Unit tests:
balance-overrides::tests::a_token_balance_override_bug_reproduction— pins the arithmetic: writing the raw amount (what the old strategies do) makesbalanceOfreturnrayMul(amount, index)(off by ~6e16 wei at aEthWETH's current index); writingrayDiv(amount, index)round-trips within 1 wei.balance-overrides::aave::tests— mock-provider tests for the probe: accepts a valid aToken, rejects when either selector reverts, rejects when the claimed pool doesn't look like Aave v3, rejects when the pool registers a different aToken for the declared underlying.balance-overrides::tests::aave_v3_a_token_override_scales_amount_and_writes_low_128— mock-provider integration test for the override builder.e2e local + forked node tests:
balance-overrides::detector::tests::detects_aave_v3_a_token_mainnet— asserts the detector returnsAaveV3AToken { pool=0x87870bca…4fa4e2, underlying=WETH, map_slot=52 }for aEthWETH.balance-overrides::tests::aave_v3_a_token_override_mainnet_roundtrip— applies the override viaeth_callagainst real aEthWETH and assertsbalanceOf(holder) ≈ amount.e2e::quote_verification::forked_node_mainnet_aave_atoken_quote— same round-trip against an anvil mainnet fork, exercising the fullBalanceOverrides::state_overridepath.Follow-up items
eth_calls per cold-cache token. An alternative is to enumerate all reserves once per chain from Aave'sAaveProtocolDataProvider(or equivalent), cache the full list of aTokens, and do a pureHashMaplookup on each quote. That would make detection amortised O(1) and provide the strongest identity guarantee (taken from Aave's own registry), at the cost of introducing a per-chain constant for the DataProvider address, a refresh cadence for new markets, and handling multiple Aave v3 markets per chain (e.g. mainnet has Ethereum + Prime). Probably worth doing once aToken quote volume warrants the optimisation.getReserveNormalizedIncome. The index drifts slowly (fractions of a wei per block), so caching for 6–12 s would drop the per-quote cost to zero without meaningful accuracy loss.LendingPoolinterface. v2 is deprecated but still has markets; worth revisiting if any start to attract volume.