Normalize ECDSA signature v parameter for Solidity ecrecover compatibility#4107
Normalize ECDSA signature v parameter for Solidity ecrecover compatibility#4107squadgazzz merged 8 commits intomainfrom
v parameter for Solidity ecrecover compatibility#4107Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly normalizes the v parameter of ECDSA signatures to ensure compatibility with Solidity's ecrecover. The changes are well-contained, and the added tests thoroughly verify the new normalization logic for various scenarios. I have one suggestion to improve the robustness of the EcdsaSignature type by enforcing the normalization invariant at the type level, and another to restore test coverage that was lost during refactoring.
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the v parameter incompatibility between modern EIP-2 signatures and Solidity's ecrecover precompile by normalizing v values (0 to 27, 1 to 28) during signature parsing. The implementation is robust, including proper error handling for invalid v values and updating the Default implementation for EcdsaSignature. The changes are thoroughly covered by comprehensive new and updated test cases, ensuring correctness and reliability. No critical issues found.
jmg-duarte
left a comment
There was a problem hiding this comment.
Super minor nits. Amazing work with the investigation 🕵️🔍
m-sz
left a comment
There was a problem hiding this comment.
Great catch! Looks good to me and the comments describe the relevant section well for posterity.
MartinquaXD
left a comment
There was a problem hiding this comment.
Really nice PR!
- good investigation
- great description
- good test coverage
Just minor nits.
Description
This PR fixes an issue where EIP-712 signatures with
v = 0or1(modern EIP-2 format) pass off-chain validation but fail on-chain settlement withGPv2: invalid signature.Problem
Some wallets (e.g., Bitget Wallet) produce ECDSA signatures using the modern EIP-2 format, where
v ∈ {0, 1}, while Solidity'secrecoverprecompile expects the legacy format wherev ∈ {27, 28}.Off-chain (Alloy library): The https://github.com/alloy-rs/core/blob/main/crates/primitives/src/signature/sig.rs internally normalizes
vto a boolean parity before recovery, so signatures withv = 0or1recover correctly.On-chain (Solidity): The
ecrecoverprecompile https://coders-errand.com/ecrecover-signature-verification-ethereum/. When givenv = 0, it returnsaddress(0), which triggers thehttps://github.com/cowprotocol/contracts/blob/main/src/contracts/mixins/GPv2Signing.sol#L207-L208:
This mismatch causes orders to pass orderbook validation but fail at settlement.
Solution
Normalize
vto the legacy format (27/28) at signature parsing time inEcdsaSignature::from_bytes():This ensures:
vvaluesecrecoverreceive compatible parametersSignature::from_bytes, JSON deserialization)Reproducing the Issue
The issue can be verified using a real failed order and Foundry's cast tool to call the
ecrecoverprecompile directly:Failed order:
0xb8e19962dd762067afb9f169684abfcbf2cb13bdc7a62ae2e680ebd5ce18c9bcca0c9c4a650cc4ed406d4a6dd031cdd9d4ebf0dc697a06860xb8e19962dd762067afb9f169684abfcbf2cb13bdc7a62ae2e680ebd5ce18c9bc0xca0c9c4a650cc4ed406d4a6dd031cdd9d4ebf0dc0xAB2E74AA0D67233ADC7B52C3B832357ED35F2052338D820D4DA66210EFA7A9684601726CB76BD26DDD958EFE291CFB57E02C39B3F60FBB8BBED1E891FB14CB5D000xAB2E74AA0D67233ADC7B52C3B832357ED35F2052338D820D4DA66210EFA7A9680x4601726CB76BD26DDD958EFE291CFB57E02C39B3F60FBB8BBED1E891FB14CB5D0x00← the problemStep 1: Compute the EIP-712 message hash
To avoid computing it manually, I grabbed it from a Tenderly simulation[URL].
Step 2: Test ecrecover with
v=0(returns zero address - FAILS on-chain)Returns:
0x0000000000000000000000000000000000000000000000000000000000000000Step 3: Test ecrecover with
v=27(returns correct signer - WORKS)Returns:
0x000000000000000000000000ca0c9c4a650cc4ed406d4a6dd031cdd9d4ebf0dc✅