Conversation
PR SummaryUpdated the EVM signature generation process to use a V4 signature approach. The changes modify how the EVM signature is created by using Keccak256 hashing on the JWT data and signing with the Ethereum key directly, rather than using the previous Changes
autogenerated by presubmit.ai |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 91af1db: feat: v4 signature
Files Processed (2)
- FRW/Foundation/Bridge/RNBridgeError.swift (1 hunk)
- FRW/Foundation/Bridge/TurboModule+Onboard.swift (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [170-170]
possible bug: "Potential inconsistent hex formatting between signatures."
-
FRW/Foundation/Bridge/RNBridgeError.swift [20-20]
maintainability: "New error case is defined but not used in the PR."
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- bcdce96: feat: update sign for eoa
Files Processed (1)
- FRW/Foundation/Bridge/TurboModule+Onboard.swift (1 hunk)
Actionable Comments (1)
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [172-172]
possible issue: "Potential signature format issue for Ethereum V4 signatures."
Skipped Comments (1)
-
FRW/Foundation/Bridge/TurboModule+Onboard.swift [166-166]
enhancement: "Missing HUD error display for ethAddress failure."
| HUD.error(WalletError.invalidSignData) | ||
| throw WalletError.invalidSignData | ||
| } | ||
| let digest = Hash.keccak256(data: jwtData) |
There was a problem hiding this comment.
The code signs the raw Keccak256 hash of the JWT data directly. For Ethereum V4 (EIP-712) typed data signatures, the standard requires a specific prefix (\x19Ethereum Signed Message:\n + message length) to be prepended before hashing, or proper EIP-712 domain separator handling. Signing raw hashes without proper prefixing can lead to signature replay attacks or incompatibility with standard Ethereum signature verification.
Related Issue
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)