Conversation
Prices vault tokens by calling `asset()` to find the underlying token, `convertToAssets(1e18)` to get the conversion rate, then delegating to an inner estimator for the underlying token's native price. Configurable as `Eip4626|<inner>`, e.g. `Eip4626|CoinGecko`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a native price estimator for EIP-4626 vault tokens. However, a potential Denial of Service (DoS) vulnerability was identified in the estimator factory. The use of the unreachable! macro when handling nested EIP-4626 estimators can lead to an application panic if a deeply nested configuration is provided from external strings, which could be exploited. A suggestion has been provided to replace the panic with a proper error return. Additionally, a critical logic error was found in the price calculation when vault tokens and underlying assets have different decimal counts, leading to incorrect pricing. The implementation's robustness could be enhanced by parallelizing contract calls and respecting timeouts, and a non-functional unit test was identified, which provides a false sense of coverage.
| let asset: Address = vault.asset().call().await.map_err(|e| { | ||
| PriceEstimationError::EstimatorInternal(anyhow::anyhow!( | ||
| "failed to call asset() on {token}: {e}" | ||
| )) | ||
| })?; | ||
|
|
||
| // Use 1e18 shares as the reference amount. This works correctly for | ||
| // vaults with 18 decimals. For other decimals the rate is still a | ||
| // reasonable approximation since convertToAssets is linear. | ||
| let shares = uint!(1_000_000_000_000_000_000_U256); | ||
| let assets: U256 = vault.convertToAssets(shares).call().await.map_err(|e| { | ||
| PriceEstimationError::EstimatorInternal(anyhow::anyhow!( | ||
| "failed to call convertToAssets() on {token}: {e}" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
The two contract calls (asset() and convertToAssets()) are executed sequentially and do not respect the timeout parameter. This can lead to the estimation taking longer than expected or hanging indefinitely. You can improve this by running the calls in parallel and wrapping them in a tokio::time::timeout.
Note: you'll need to add use futures::future::join; at the top of the file.
use futures::future::join;
let shares = uint!(1_000_000_000_000_000_000_U256);
let (asset_result, assets_result) = tokio::time::timeout(
timeout,
join(
vault.asset().call(),
vault.convertToAssets(shares).call(),
),
)
.await
.map_err(|_| {
PriceEstimationError::EstimatorInternal(anyhow::anyhow!(
"EIP-4626 contract calls timed out"
))
})?;
let asset: Address = asset_result.map_err(|e| {
PriceEstimationError::EstimatorInternal(anyhow::anyhow!(
"failed to call asset() on {token}: {e}"
))
})?;
let assets: U256 = assets_result.map_err(|e| {
PriceEstimationError::EstimatorInternal(anyhow::anyhow!(
"failed to call convertToAssets() on {token}: {e}"
))
})?;| async fn delegates_to_inner_on_error() { | ||
| let mut inner = MockNativePriceEstimating::new(); | ||
| inner | ||
| .expect_estimate_native_price() | ||
| .returning(|_, _| async { Err(PriceEstimationError::NoLiquidity) }.boxed()); | ||
| let _ = inner; |
There was a problem hiding this comment.
This test is ineffective. It sets up a mock with an expectation but never uses it, so the test doesn't actually verify any behavior and will always pass. A correct test should instantiate an Eip4626 estimator with the mock and call estimate_native_price on it to verify that the error from the inner estimator is propagated. This will likely require mocking the provider calls as well.
| ( | ||
| name.clone(), | ||
| Arc::new(InstrumentedPriceEstimator::new( | ||
| native::Eip4626::new(inner, self.network.web3.provider.clone()), |
There was a problem hiding this comment.
To address the decimal correction issue mentioned in eip4626.rs, you'll need to pass the TokenInfoFetching component here when creating the Eip4626 estimator.
native::Eip4626::new(inner, self.network.web3.provider.clone(), self.components.tokens.clone()),| NativePriceEstimatorSource::Eip4626(_) => { | ||
| unreachable!("Eip4626 is handled before calling create_native_estimator") | ||
| } |
There was a problem hiding this comment.
The create_native_estimator function contains an unreachable! macro for the NativePriceEstimatorSource::Eip4626 variant. However, this variant is reachable if a nested EIP-4626 estimator is configured (e.g., Eip4626|Eip4626|CoinGecko). The NativePriceEstimator enum is recursive, and its FromStr implementation allows for arbitrary nesting. When native_price_estimator encounters an Eip4626 variant, it calls create_native_estimator with the inner estimator. If that inner estimator is also an Eip4626 variant, the application will panic, leading to a Denial of Service. It is recommended to return an error instead of panicking.
NativePriceEstimatorSource::Eip4626(_) => anyhow::bail!(
"Nested Eip4626 estimators are not supported; use Eip4626|<inner> \
is not Eip4626"
),
The generated artifacts are useful to avoid paying the compilation time and so we can Cmd+Click into their implementation |
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
What exactly is "their implementation"? The definition right there is technically the definition of the interface... inline. The implnementation of an |
- Change `Eip4626(Box<NativePriceEstimator>)` to `Eip4626` unit variant, eliminating recursive type that caused serde to hit the monomorphization recursion limit with `#[serde(tag = "type")]`. - Eip4626 now wraps the next estimator in the config stage list at construction time instead of nesting inside the enum. - Move Eip4626 handling into `create_native_estimator` by passing the stage iterator, removing the special-case in the caller. - Add deserialization validation rejecting Eip4626 as last in a stage. - Use vendored IERC4626 contract binding and query vault decimals for accurate conversion rate. - Fund sDAI whale with ETH in e2e test to fix gas error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
|
@kaze-cow did you already verify that the token quote coverage will be an issue for those EIP 4626 tokens or is this currently an assumption that this will be needed? The change seems reasonable overall but would still be good to understand what prio this should get. |
Just to give an idea right now on staging we need to add a manual override for the token in order for it to be supported, despite having 3 solvers quoting/solving euler orders. So while I am not well aware of the details, this type of change does appear to be needed, and in any case, is likely to come in handy in improving our native token pricing coverage. |
Introduces a new native price estimator that prices EIP-4626 vault tokens by querying the vault's underlying asset and conversion rate, then delegating to an inner estimator for the underlying token's price. Includes IERC4626 contract bindings and resolves merge conflicts with the contracts crate refactor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove duplicate type definitions from price-estimation/src/lib.rs that conflicted with the canonical definitions in the configs crate. Add the Eip4626 validation (must not be last in a stage) to the configs crate deserializer. Fix e2e test imports to use configs crate paths and correct start_protocol_with_args signature. Change recursive vault test to query native price directly instead of submitting a quote, since the freshly deployed mock wrapper has no DEX liquidity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e-price # Conflicts: # crates/configs/src/native_price_estimators.rs # crates/e2e/tests/e2e/eip4626.rs
- Add negative cache (Mutex<HashSet>) for non-vault tokens to avoid
wasted RPC calls on every estimation cycle
- Enforce timeout on vault RPC calls via tokio::time::timeout so a stuck
node cannot block the pipeline indefinitely
- Forward remaining time budget to the inner estimator for correct
deadline propagation through recursive chains
- Change Eip4626 config from unit variant to Eip4626 { depth: NonZeroU8 }
so recursive depth is declared once instead of repeating the variant
- Extract conversion_rate() for readability
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ding - Add `MintableToken::at` constructor to wrap ABI-compatible contracts (e.g. MockERC4626Wrapper) as MintableToken for use with `seed_uni_v2_pool` - Split native price estimator config into two stages with results_required=1 so the EIP-4626 chain has priority over the standalone driver fallback - Seed Uniswap V2 pools for recursive wrapper tokens so the solver can find routes, enabling full quote submission - Verify native price ratios between wrappers match their vault conversion rates Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Description
Adds a native price estimator for EIP-4626 vault tokens (e.g. sDAI, wrapped yield vaults).
Many vault tokens lack direct DEX liquidity, causing native price estimation to fail. This estimator unwraps the vault by querying the on-chain
asset()andconvertToAssets()functions, then delegates pricing of the underlying token to the next estimator in the stage.Changes
Eip4626native price estimator (crates/price-estimation/src/native/eip4626.rs):calls
asset()+decimals()in parallel, thenconvertToAssets(10^decimals)to compute theshares-to-assets conversion rate, and multiplies by the inner estimator's price for the
underlying token.
asset()call reverts are remembered ina
Mutex<HashSet<Address>>so subsequent requests skip the RPC entirely. Cleared on processrestart.
tokio::time::timeout, and whatever time remains is forwarded to the inner estimator. Thiskeeps the total wall-clock time within the caller's original timeout, which matters for
recursive vault chains.
Eip4626config variant accepts adepthparameter(default: 1) controlling how many nested vault layers to unwrap. In the factory,
depthlayersof
Eip4626wrap the next estimator in the stage.NativePriceEstimatorsdeserialization rejects stages whereEip4626isthe last entry (it must be followed by another estimator to price the underlying asset).
IERC4626interface andMockERC4626Wrappertest contract fore2e tests.
crates/price-estimation/src/factory.rs):create_native_estimatornowconsumes the next estimator from the stage iterator when it encounters
Eip4626, wrapping itin
depthlayers of instrumentedEip4626estimators.crates/configs/src/native_price_estimators.rsthat were lost during the extraction from
price-estimationtoconfigs.How to test
cargo nextest run -p price-estimation eip4626andcargo nextest run -p configs native_price_estimatorsNODE_URL):NODE_URL=... cargo nextest run -p price-estimation -- eip4626 --run-ignored ignored-onlyFORK_URL_MAINNET):cargo nextest run -p e2e forked_node_mainnet_eip4626_native_price --test-threads 1 --run-ignored ignored-onlycargo nextest run -p e2e forked_node_mainnet_eip4626_recursive_native_price --test-threads 1 --run-ignored ignored-only