feat(sdk): add verification support of foreign transaction signatures

[DSharifi]

closes #2130

[claude]

Code Review: feat(sdk): add verification support of foreign transaction signatures

Issues Found

1. reduce_scalar silently normalizes non-canonical s values (signature malleability concern)

verification.rs:34 — reduce_scalar is applied to the raw s bytes from K256Signature, which is just a [u8; 32] newtype with no validation. If s >= n (the curve order), reduce_bytes silently reduces it modulo n. This means a signature with s_bytes = s_valid + n would be silently normalized to s_valid and pass verification, even though the byte-level input is non-canonical.

The contract's check_ec_signature in kdf.rs:97-118 avoids this because its s parameter is already a k256::Scalar (validated at construction). Here, raw bytes flow in and get silently reduced.

In practice, for the r value this is standard ECDSA behavior (x-coordinate reduction), but for s it introduces signature malleability: an attacker can take a valid signature and produce a different byte-level representation that also passes.

The malleability_flag: true passed to ecrecover rejects s > n/2, but that only catches the n - s malleability vector, not the s + n vector (since after reduction, s is back in [0, n/2]).

Suggested fix — Validate that the raw s bytes represent a value in [1, n-1] before using them, rather than silently reducing. The k256 crate provides NonZeroScalar::try_from or you can use Option::<k256::Scalar>::from(k256::Scalar::from_repr(bytes)) which returns None for non-canonical inputs:

let s = Option::<k256::Scalar>::from(k256::Scalar::from_repr(signature.s.scalar.into()))
    .ok_or(VerificationError::InvalidSignature)?;
let r = reduce_scalar(r_bytes.into()); // reduction on r is fine (standard ECDSA)

Severity: Medium. In this specific context (SDK for verifying MPC-produced signatures), the practical exploit surface is low — the MPC nodes produce canonical signatures. However, as a public SDK verification function, it should enforce canonical inputs. Per CONTRIBUTING.md: "Security concerns trump everything else."

2. Missing test for non-canonical s value

The ec_tampered_s_scalar test sets s = [0xFF; 32] which just happens to not recover the same key. There's no test for the specific case of s_valid + n wrapping around — which would demonstrate whether non-canonical inputs are properly rejected. Adding such a test would exercise the exact boundary this verification function should enforce.

---

Everything else looks solid:

• The check_ed_signature function is clean and delegates correctly to near_sdk::env::ed25519_verify
• Payload hash comparison is done correctly before signature verification (good defense-in-depth)
• The builder pattern + verifier separation is well-designed
• Deref derive additions on Ed25519Signature and Hash256 are appropriate for the ed25519_verify call
• Test coverage is thorough for the happy path and common error paths
• The ForeignChainSignatureVerifier fields are private, only constructible via the builder — good encapsulation

⚠️ (issue found — non-canonical s acceptance in EC signature verification)

[netrome]

Nice stuff, just some nits

[netrome]

Why not?

[DSharifi]

Check the match statement below. A PublicKey can also be a bls public key, which means a possible error case due to invalid input from the caller.

[DSharifi]

Now we've had quite some time pressure, so it's good enough. But Ideally I wonder if we can remove all the public re-exports for the contract interface types and have all types that are part of the SDK interface defined here instead.

Otherwise any change we do in the interface crate will also have to adhere to semver compatiblity to not break users of the SDK crate.

[gilcu3]

Otherwise any change we do in the interface crate will also have to adhere to semver compatiblity to not break users of the SDK crate.

wasnt that one of the purposes of the interface crate (it should avoid as much as possible breaking changes)? Or we are treating it now as an internal interface crate, and we will need a more stable external one?

[netrome]

Yeah I've always pictured the contract-interface to be an externally facing crate and re-exporting types from there should not be an issue.

This also becomes an interesting design question. Would it be wrong from the perspective of this crate if we got a BLS signature? Should we be aware at this point that that is not a supported signing scheme? What happens if we were to add BLS signatures for foreign tx verification down the line?

Perhaps we don't need to bikeshed this too much but when thinking about the interface and separation of concerns this also becomes a consideration imo.

[DSharifi]

From my pov the interface crate has to be stable in terms of how types are serialized/deserialized. Any other change which are breaking semver, such as renaming, moving definitions are all currently fair game and happens frequently.

The latter changes are fine because it's all in the monorepo, but for outside dependants those change can break semver

[netrome]

Hmm yeah I pictured that we'd be cautious about these types of changes in both the contract-interface crate and the SDK, and actually adhere to semver. Although we could start to version the interface/SDK separately from the rest of the MPC repo for this reason.

Until we publish a crate though everything should be considered unstable.

[gilcu3]

I believe the current implementation of check_ec_signature might accept "invalid" signatures. I think we should once and for all unify these crypto type conversions and implementations in a single place/crate, so that we don't need to implement them each time we use them, which is quite error prone. These seems seem fit for our contract-interface crate IMO, maybe behind appropriate feature gates.

If for the moment this is temporarily implemented here, then we should strictly follow the existing implementation in the contract. Let me know if that was the actual aim here.

[frolvanya]

A little bit off topic, but is it possible to make this (and maybe other) structures borsh serializable?

For example, right now I'm doing this (I'm still writing PoC, so maybe I'm missing something out and it can be done easier):

#[near(serializers=[borsh])]
#[derive(Debug, Clone)]
pub struct MpcVerifyProofArgs {
    pub proof_kind: ProofKind,
    pub sign_payload: Vec<u8>,
    pub mpc_response_json: String,
}

// ...

#[allow(clippy::needless_pass_by_value)]
#[handle_result]
#[result_serializer(borsh)]
pub fn verify_proof(
    &self,
    #[serializer(borsh)] input: Vec<u8>,
) -> Result<ProverResult, String> {
    let args = MpcVerifyProofArgs::try_from_slice(&input).near_expect(ProverError::ParseArgs);

    let sign_payload = ForeignTxSignPayload::try_from_slice(&args.sign_payload)
        .near_expect(ProverError::ParseArgs);

    let mpc_response: VerifyForeignTransactionResponse =
        serde_json::from_str(&args.mpc_response_json).near_expect(ProverError::ParseArgs);
    // ...
}

If this structure supported borsh, I could've store and parse it directly as:

#[near(serializers=[borsh])]
#[derive(Debug, Clone)]
pub struct MpcVerifyProofArgs {
    pub proof_kind: ProofKind,
    pub sign_payload: Vec<u8>,
    pub mpc_response: VerifyForeignTransactionResponse,
}

[DSharifi]

Yes, we can add this. But just be aware that we haven't published this crate yet for external usage (it's being discussed), so there is no guarantee of maintaining semver compatiblity.

[gilcu3]

LGTM

[netrome]

I think we need to clean up the error type now when we don't do signature verification