Add risk checker endpoint

[dewabisma]

Summary

• Added risk checker endpoint to support risk checker tool, we might not using the q.day again for risk checking.
• Most of the code are taken from q.day
• Added many unit and integration tests

[n13]

I've read through the entire diff carefully. Here's my review of PR #66:

---

Security & Correctness Review: PR #66 — Add Risk Checker Endpoint

Critical: No Authentication

The most significant security issue is that the endpoint is completely unauthenticated. Every other mutating or data-serving route in the codebase uses jwt_auth or jwt_admin_auth middleware, but this one is wide open:

pub fn risk_checker_routes() -> Router<AppState> {
    Router::new().route("/risk-checker/:address_or_ens", get(handle_get_risk_report))
}

This means anyone on the internet can:

• Exhaust the Etherscan/Infura API quotas — each request makes 3-4 upstream API calls against rate-limited free-tier keys.
• Use the server as a proxy to Etherscan, hiding their identity behind your server's IP.
• Denial-of-wallet — flood the endpoint to burn through the etherscan_calls_per_sec budget and block legitimate users.

At minimum, it should be behind jwt_auth. If it's intended for internal tooling, jwt_admin_auth would be better.

No Server-Side Rate Limiting

The inter-call delay (etherscan_call_delay) only throttles sequential calls within a single request. There's nothing stopping an attacker from sending 100 concurrent requests, each of which will fire 3-4 Etherscan calls in parallel across those requests. The existing codebase doesn't have a global rate limiter either, so this endpoint specifically needs protection since it amplifies each inbound request into multiple expensive outbound calls.

Rate Limit Delay Comment is Wrong

The comment says "Add a 20% safety margin" but the actual calculation has no margin at all:

        // Add a 20% safety margin on top of the minimum inter-call interval.
        let etherscan_call_delay = Duration::from_millis(1000 / calls_per_sec);

With calls_per_sec = 3, this gives 333ms — exactly 3 calls/sec with zero margin. A 20% margin would be Duration::from_millis(1200 / calls_per_sec) = 400ms.

is_smart_contract Silently Swallows Errors

    pub async fn is_smart_contract(&self, address: &str) -> bool {
        match self
            .fetch_etherscan(&[
                ("module", "proxy"),
                ("action", "eth_getCode"),
                ("address", address),
                ("tag", "latest"),
            ])
            .await
        {
            Ok(data) => data.result.as_str().map(|code| code.len() > 100).unwrap_or(false),
            Err(_) => false,
        }
    }

If Etherscan is rate-limited or down, this silently returns false (treating the address as a normal EOA). The risk report will then say is_smart_contract: false with no indication that the check failed. This should return Result<bool, RiskCheckerError> like the other methods, so the report can either indicate uncertainty or propagate the error.

Magic Number for Contract Detection

code.len() > 100 is an arbitrary threshold. The standard way to check for a contract is code != "0x" (or code.len() > 2). A minimal proxy contract could have very short bytecode that falls under 100 characters and would be misclassified as an EOA.

wei_to_eth Precision Loss for Large Balances

    pub fn wei_to_eth(wei: &str) -> f64 {
        wei.parse::<u128>().map(|w| w as f64 / 1e18).unwrap_or(0.0)
    }

f64 only has 53 bits of mantissa. For balances above ~9,007 ETH (2^53 wei), precision degrades. Not a showstopper for a risk report, but worth noting. Also, parse failures silently return 0.0 — a genuinely wealthy address with a corrupted balance response would appear as having zero balance.

No HTTP Client Timeout

            client: Client::new(),

The reqwest client is created with default settings (no timeout). If Etherscan or Infura hangs, the request will hang indefinitely, tying up a server thread. Should set a reasonable timeout (e.g., 10-15 seconds) via Client::builder().timeout(Duration::from_secs(15)).build().

ENS Validation is Too Restrictive

    pub fn is_ens_name(input: &str) -> bool {
        let trimmed = input.trim().to_lowercase();
        if !trimmed.ends_with(".eth") || trimmed.starts_with("0x") {
            return false;
        }
        let label = &trimmed[..trimmed.len() - 4];
        !label.is_empty() && label.chars().all(|c| c.is_alphanumeric() || c == '-')
    }

This rejects valid ENS subdomains like pay.vitalik.eth, names with underscores, and internationalized names. Might be intentional as a v1 limitation, but should be documented.

fetch_etherscan Maps All Status "0" to AddressNotFound

        if data.status == "0" {
            let result_str = data.result.as_str().unwrap_or("").to_lowercase();
            if result_str.contains("rate limit") || result_str.contains("max calls") {
                return Err(RiskCheckerError::RateLimit);
            }
            return Err(RiskCheckerError::AddressNotFound);
        }

Etherscan returns status "0" for various reasons (no transactions, invalid API key, module errors). Lumping all of them into AddressNotFound could mask real issues like an expired or invalid API key. The get_first_transaction_timestamp method works around this, but it's fragile.

Minor Issues

• Inconsistent reqwest version — Cargo.toml adds reqwest = "0.11" but Cargo.lock pulls in reqwest 0.13.2 via alloy. Two versions of reqwest in the same binary is wasteful. The service uses reqwest::Client — which version gets resolved could be surprising.
• No input length limit — the Path(address_or_ens) extractor has no length constraint. While Axum has defaults, explicitly limiting to a sane max (e.g., 256 chars) would be defensive.
• The second commit message "fix: delay fetch" is vague — should describe what was fixed (rate limit interval was 3x too short).

What Looks Good

• Error mapping in map_risk_checker_error properly sanitizes internal details for Other errors (logs the real error, returns generic message).
• Test coverage is solid with unit tests for validation and wiremock-based integration tests for all Etherscan interactions.
• The AddressResolution enum is a clean pattern for handling the ETH address vs ENS duality.
• Sequential calls with rate-limit delays show awareness of the Etherscan free-tier constraints.

Summary of Recommended Fixes

Priority	Issue
P0	Add authentication middleware (jwt_auth or jwt_admin_auth)
P0	Add server-side rate limiting or concurrency cap for this endpoint
P1	Set HTTP client timeout
P1	Fix is_smart_contract to return Result and use code != "0x" instead of len() > 100
P2	Fix rate-limit delay calculation to actually include the 20% margin
P2	Unify reqwest versions or use the alloy-pulled version
P3	Improve status "0" error discrimination in fetch_etherscan
P3	Document ENS subdomain limitation

[dewabisma]

I have addressed the issues. There is intentionally no auth protection because it's for public usage through website. I have added cors and rate limit for the endpoint.

[n13]

Good, I now have the full picture. Here's the follow-up review.

---

Follow-up Review: PR #66 (after 6 additional commits)

What Got Fixed (nice work!)

Original Issue	Status
HTTP client timeout	Fixed - Client::builder().timeout(Duration::from_secs(30))
is_smart_contract error swallowing	Fixed - now returns Result<bool, RiskCheckerError> and propagates errors
Concurrency limiting	Fixed - Semaphore with try_acquire() rejects excess requests immediately
No auth (intentional)	Acknowledged - public endpoint, mitigated by CORS + concurrency limit
Stale "20% safety margin" comment	Fixed - removed

New Issue: CORS Change is Global and Breaking

This is the biggest concern with the updated PR. The CORS change replaces CorsLayer::permissive() on the entire router — affecting every existing endpoint, not just the risk checker.

On main today:

        .layer(
            ServiceBuilder::new()
                .layer(TraceLayer::new_for_http())
                .layer(CorsLayer::permissive()),
        )

The PR changes this to:

CorsLayer::new()
    .allow_origin(state.config.get_cors_allowed_origins())
    .allow_methods([Method::GET, Method::POST, Method::PUT, Method::DELETE, Method::OPTIONS])
    .allow_headers(AllowHeaders::mirror_request())
    .allow_credentials(true),

With the default config being cors_allowed_origins = ["http://localhost:4321"].

This means any existing client not at localhost:4321 — staging frontends, mobile apps, other services, browser extensions, Postman-like tools relying on CORS — will get blocked on ALL endpoints, not just the new risk checker route. This scope creep should either:

• Be a separate PR so it's reviewed/tested independently, or
• At minimum, apply the restrictive CORS only to the risk checker route (via a route-specific layer), leaving the existing behavior for other routes untouched.

AllowHeaders::mirror_request() + allow_credentials(true)

AllowHeaders::mirror_request() echoes back whatever the client asks for in Access-Control-Request-Headers. While technically spec-compliant (it's not the literal * wildcard), it's functionally identical to allowing any header. Combined with allow_credentials(true), this is more permissive than necessary. Better to explicitly list the headers you need (e.g., Authorization, Content-Type).

max_concurrent_requests = 1 is Very Aggressive

With the default of 1, only a single request is processed at a time across all users. Everyone else gets an immediate 429. For a public-facing endpoint, this means:

• If one request takes the full 30s timeout, the endpoint is unavailable for 30 seconds.
• Under any real traffic, most users will be rejected.

Consider raising the default to something like 3-5, or adding a per-IP rate limit instead of (or in addition to) a global concurrency cap.

Remaining Items from First Review (not addressed)

Issue	Impact
Magic number code.len() > 100	A minimal proxy contract with < 100 chars of bytecode will be misclassified as an EOA. Should be code != "0x" or code.len() > 2.
fetch_etherscan maps all status "0" to AddressNotFound	An expired API key, invalid module, etc. will surface as "address not found" rather than a proper error. Fragile.
Inconsistent reqwest versions	Cargo.toml has reqwest = "0.11" while alloy pulls in 0.13.2. Two versions of reqwest in the binary.
ENS subdomain support	is_ens_name rejects pay.vitalik.eth — documented as v1 limitation, acceptable.

Minor Nit

is_smart_contract can be simplified — the match arms are just a passthrough now:

// Current:
match self.fetch_etherscan(&[...]).await {
    Ok(data) => Ok(data.result.as_str().map(|code| code.len() > 100).unwrap_or(false)),
    Err(e) => Err(e),
}

// Simpler:
let data = self.fetch_etherscan(&[...]).await?;
Ok(data.result.as_str().map(|code| code.len() > 100).unwrap_or(false))

Summary

Priority	Issue
P0	CORS change is global — breaks all existing endpoints for non-whitelisted origins. Split into separate PR or scope to risk-checker route only.
P1	AllowHeaders::mirror_request() + credentials — explicitly list allowed headers instead.
P1	max_concurrent_requests = 1 is too aggressive for a public endpoint — raise default or add per-IP limiting.
P2	Fix code.len() > 100 to code != "0x" for contract detection.
P2	Unify reqwest versions (0.11 vs 0.13).
P3	Improve status "0" error discrimination in fetch_etherscan.

The concurrency limiter and timeout additions are solid improvements. The main blocker is the global CORS change — it shouldn't ship as part of this PR without careful coordination.

[n13]

GTG!

[n13]

this should not swallow errors, it should just propagate them

also != 0x might be a better check than code length

[n13]

reason is on error this literally returns a wrong result...