Introduce logic to compute approval overrides#4401
Conversation
![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 implements an ERC-20 approval override detection system, enabling the simulator to identify and override allowance storage slots using Solidity and Solady heuristics. A high-severity security issue was identified in the Detector caching logic, which is vulnerable to cache stampedes because the mutex is released before performing expensive uncached detections.
| let mut cache = self.cache.lock().unwrap(); | ||
| if let Some(strategy) = cache.cache_get(&(token, None)) { | ||
| tracing::trace!(?token, "cache hit (strategy valid for all pairs)"); | ||
| return strategy.clone(); | ||
| } | ||
| if let Some(strategy) = cache.cache_get(&(token, Some((owner, spender)))) { | ||
| tracing::trace!( | ||
| ?token, | ||
| ?owner, | ||
| ?spender, | ||
| "cache hit (pair-specific strategy)" | ||
| ); | ||
| return strategy.clone(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current caching logic is susceptible to a cache stampede (thundering herd) because the lock is released before performing the potentially expensive detect_uncached call. If multiple concurrent requests arrive for the same uncached token, they will all trigger redundant simulations and RPC calls. Consider using a synchronization primitive like tokio::sync::OnceCell or a dashmap with entry API to ensure only one detection task runs per token. Additionally, ensure the cache is size-bounded to prevent potential Denial of Service attacks via memory exhaustion from user-provided data.
References
- Caches that store user-provided data must be size-bounded to prevent potential Denial of Service attacks via memory exhaustion.
MartinquaXD
force-pushed
the
refactor-balance-override-logic
branch
from
f86a64c to
c66dbfd
Compare
MartinquaXD
force-pushed
the
approval-overrides
branch
2 times, most recently
from
377a941 to
2d1f649
Compare
Adds ERC-20 allowance override detection and application: a new `approval/` submodule with `ApprovalStrategy`, `ApprovalOverrideRequest`, and `Detector` (mirrors the balance side). Extends the `StateOverriding` trait with `approval_override()` and wires it into the simulator's `AccountOverrideRequest::Approval` variant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
MartinquaXD
force-pushed
the
approval-overrides
branch
from
14721db to
826863d
Compare
2 participants
Description
Uses the same ideas as the balance override logic to compute approval overrides. This will be used in a new implementation of the trade verification.
Changes
simulatorcrate to generate approval overridesHow to test
added a few tests