spec: ecdsa inputs sanitation

[0xAndoroid]

Summary

• Add spec for moving ECDSA input validation into ecdsa_verify() for both secp256k1 and P-256
• Restrict _unchecked constructors to pub(crate) visibility
• Addresses the security footgun documented in Clarify the security of the ecdsa_verify functions for secp256k1 and P256 #1391

Test plan

• Review spec for completeness
• Run @claude analyze on the PR for second-opinion analysis

[0xAndoroid]

@claude analyze

[github-actions]

Claude finished @0xAndoroid's task in 5m 23s —— View job

---

Spec Analysis: ECDSA Inputs Sanitation

• Read the spec file
• Read issue Clarify the security of the ecdsa_verify functions for secp256k1 and P256 #1391 for context
• Explore relevant ECDSA source code
• Analyze spec completeness and correctness
• Post analysis

---

Strengths

The spec correctly identifies the security footgun and proposes the right high-level fix: move validation into ecdsa_verify() itself. The invariants are well-defined, the alternatives analysis is sound, and all the needed validation functions (is_fq_non_canonical, is_fr_non_canonical/is_non_canonical, is_on_curve) already exist in the codebase. Both error enums (Secp256k1Error, P256Error) already have the needed variants (InvalidFqElement, InvalidFrElement, NotOnCurve, etc.).

---

Issues Found

1. Critical: ECField trait leaks from_u64_arr_unchecked

The spec proposes making from_u64_arr_unchecked pub(crate) on the inherent impls, but ECField is a pub trait that defines from_u64_arr_unchecked as a required method (jolt-inlines/sdk/src/ec.rs:20):

pub trait ECField: Clone + PartialEq + core::fmt::Debug + Sized {
    // ...
    fn from_u64_arr_unchecked(arr: &[u64; 4]) -> Self;
}

Even with pub(crate) inherent methods, external callers can bypass them via the trait:

<Secp256k1Fq as ECField>::from_u64_arr_unchecked(&malicious_limbs)

The spec must address the trait. Options: remove the method from ECField, make ECField itself pub(crate), or introduce a sealed internal-only trait for unchecked construction.

2. Critical: Cross-crate breakage for AffinePoint::from_u64_arr_unchecked

AffinePoint lives in the sdk crate, but from_u64_arr_unchecked is called from separate crates:

• jolt-inlines/secp256k1/src/sdk.rs — lines 683, 697, 711 (generator_times_2_pow_128, etc.)
• jolt-inlines/p256/src/sdk.rs — lines 709, 732 (inside fake_glv_scalar_mul)

Making AffinePoint::from_u64_arr_unchecked pub(crate) would break these cross-crate call sites. The spec needs an alternative strategy — e.g., using AffinePoint::new_unchecked(x, y) with already-constructed field elements, or keeping from_u64_arr_unchecked accessible but renaming it to signal danger.

3. AffinePoint::new_unchecked not addressed

AffinePoint::new_unchecked(x, y) is pub and bypasses on-curve validation. This is another path to construct invalid points that the spec doesn't mention. It's used heavily internally (neg, double, add, double_and_add, generator constructors), so it can't simply be made pub(crate) without the same cross-crate issue. The spec should at least acknowledge it.

---

Minor Issues

4. Invariants 4 & 5 already implemented

Both ecdsa_verify functions already check q.is_infinity() and r.is_zero() || s.is_zero() (secp256k1 L873-878, p256 L833-838). The spec lists these as invariants but could clarify they're existing checks being preserved, not new additions.

5. Grumpkin consistency

GrumpkinFq::from_u64_arr_unchecked and GrumpkinFr::from_u64_arr_unchecked (grumpkin/sdk.rs:97, 312) have the same pub visibility. The Non-Goals section says "Supporting curves beyond secp256k1 and P-256," but if the ECField trait is modified, grumpkin will be affected too. Worth acknowledging.

6. Performance estimate undercount

The spec says "~3 field operations" but the full cost includes the on-curve check (~3 field ops) plus 5 range comparisons (z, r, s each checked against n; q.x, q.y each checked against p — each comparing 4 limbs). Still negligible relative to GLV muls, but the count is imprecise.

---

Recommendations

1. Address the ECField trait — this is the primary escape hatch that undermines the visibility restriction. Consider removing from_u64_arr_unchecked from the trait and adding a separate pub(crate) trait or standalone function.
2. Plan for cross-crate usage — either keep point/field _unchecked constructors accessible across jolt-inlines sub-crates (e.g., #[doc(hidden)] + naming convention) or restructure the internal calls to use checked constructors where values are known-valid.
3. Scope new_unchecked — at minimum, document that it exists and explain why it's excluded or include it in the restriction plan.
4. Mark invariants 4 & 5 as pre-existing — minor clarity improvement.

Overall the spec correctly identifies the problem and the right solution shape. The critical gap is that the ECField pub trait and the cross-crate workspace structure mean pub(crate) on inherent impls alone won't achieve invariant 7. Addressing these two issues would make the spec implementation-ready.

---