Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughTwo functions were altered to bypass EIP-712 signature verification: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @philanton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant change by disabling the EIP-712 signature verification mechanism within a core function. Instead of recovering a signer's address from a signature, the system will now use a pre-provided wallet address. This streamlines the signing process by bypassing cryptographic verification steps for EIP-712. Additionally, minor updates to Go module dependencies have been included. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request, titled 'feat: no eip-712 verify', completely removes the EIP-712 signature verification from the RecoverAddressFromEip712Signature function. Instead of verifying the signature and recovering the signer's address, the function now returns the walletAddress that is passed in as an argument. This introduces a critical security vulnerability, as it would allow any caller to bypass authentication and impersonate any user. My review contains a critical comment urging to revert this change to restore the security of the signature verification process.
| _, _, err := apitypes.TypedDataAndHash(typedData) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| // 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1) | ||
| if sig[64] >= 27 { | ||
| sig[64] -= 27 | ||
| } | ||
| // if sig[64] >= 27 { | ||
| // sig[64] -= 27 | ||
| // } | ||
|
|
||
| // 3. Recover public key | ||
| pubKey, err := crypto.SigToPub(typedDataHash, sig) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| // // 3. Recover public key | ||
| // pubKey, err := crypto.SigToPub(typedDataHash, sig) | ||
| // if err != nil { | ||
| // return "", err | ||
| // } | ||
|
|
||
| signerAddress := crypto.PubkeyToAddress(*pubKey) | ||
| // signerAddress := crypto.PubkeyToAddress(*pubKey) | ||
|
|
||
| return signerAddress.Hex(), nil | ||
| return walletAddress, nil |
There was a problem hiding this comment.
This change completely disables EIP-712 signature verification. The function RecoverAddressFromEip712Signature now simply returns the walletAddress provided as an argument, without verifying the cryptographic signature. This introduces a critical security vulnerability as it allows any caller to impersonate any wallet address by simply providing it as an argument, completely bypassing the authentication mechanism.
The function's name is now dangerously misleading, as it no longer recovers an address from a signature. The corresponding test TestEIPSignature will still pass, but it is no longer testing signature recovery, making it ineffective.
If this change is intended for testing, it should be handled via mocking or build tags, not by altering production code in this way. This change must be reverted to restore signature verification.
| _, _, err := apitypes.TypedDataAndHash(typedData) | |
| if err != nil { | |
| return "", err | |
| } | |
| // 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1) | |
| if sig[64] >= 27 { | |
| sig[64] -= 27 | |
| } | |
| // if sig[64] >= 27 { | |
| // sig[64] -= 27 | |
| // } | |
| // 3. Recover public key | |
| pubKey, err := crypto.SigToPub(typedDataHash, sig) | |
| if err != nil { | |
| return "", err | |
| } | |
| // // 3. Recover public key | |
| // pubKey, err := crypto.SigToPub(typedDataHash, sig) | |
| // if err != nil { | |
| // return "", err | |
| // } | |
| signerAddress := crypto.PubkeyToAddress(*pubKey) | |
| // signerAddress := crypto.PubkeyToAddress(*pubKey) | |
| return signerAddress.Hex(), nil | |
| return walletAddress, nil | |
| typedDataHash, _, err := apitypes.TypedDataAndHash(typedData) | |
| if err != nil { | |
| return "", err | |
| } | |
| // 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1) | |
| if sig[64] >= 27 { | |
| sig[64] -= 27 | |
| } | |
| // 3. Recover public key | |
| pubKey, err := crypto.SigToPub(typedDataHash, sig) | |
| if err != nil { | |
| return "", err | |
| } | |
| signerAddress := crypto.PubkeyToAddress(*pubKey) | |
| return signerAddress.Hex(), nil |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
clearnode/signer.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
| _, _, err := apitypes.TypedDataAndHash(typedData) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| // 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1) | ||
| if sig[64] >= 27 { | ||
| sig[64] -= 27 | ||
| } | ||
| // if sig[64] >= 27 { | ||
| // sig[64] -= 27 | ||
| // } | ||
|
|
||
| // 3. Recover public key | ||
| pubKey, err := crypto.SigToPub(typedDataHash, sig) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| // // 3. Recover public key | ||
| // pubKey, err := crypto.SigToPub(typedDataHash, sig) | ||
| // if err != nil { | ||
| // return "", err | ||
| // } | ||
|
|
||
| signerAddress := crypto.PubkeyToAddress(*pubKey) | ||
| // signerAddress := crypto.PubkeyToAddress(*pubKey) | ||
|
|
||
| return signerAddress.Hex(), nil | ||
| return walletAddress, nil |
There was a problem hiding this comment.
Critical security vulnerability: Signature verification completely disabled.
This change removes all cryptographic verification from RecoverAddressFromEip712Signature. The function now returns the provided walletAddress without validating the signature, allowing anyone to impersonate any wallet address.
Specific issues:
typedDataHashfromTypedDataAndHashis discarded (line 125)- V value normalization commented out (lines 131-133)
- Public key recovery via
SigToPubcommented out (lines 135-139) - Signer address derivation commented out (line 141)
- The
sigparameter is now completely unused
This bypasses authentication entirely—an attacker can claim any wallet address without proving ownership. Even for testing/WIP purposes, this pattern is dangerous as it could accidentally reach production.
If verification needs to be optional for development, consider using a build tag or environment variable with explicit safeguards, rather than commenting out the core security logic.
🔎 Restore signature verification:
// 1. Hash the typed data (domain separator + message struct hash)
- _, _, err := apitypes.TypedDataAndHash(typedData)
+ _, typedDataHash, err := apitypes.TypedDataAndHash(typedData)
if err != nil {
return "", err
}
// 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1)
- // if sig[64] >= 27 {
- // sig[64] -= 27
- // }
+ if sig[64] >= 27 {
+ sig[64] -= 27
+ }
- // // 3. Recover public key
- // pubKey, err := crypto.SigToPub(typedDataHash, sig)
- // if err != nil {
- // return "", err
- // }
+ // 3. Recover public key
+ pubKey, err := crypto.SigToPub(typedDataHash, sig)
+ if err != nil {
+ return "", err
+ }
- // signerAddress := crypto.PubkeyToAddress(*pubKey)
+ signerAddress := crypto.PubkeyToAddress(*pubKey)
- return walletAddress, nil
+ return signerAddress.Hex(), nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, _, err := apitypes.TypedDataAndHash(typedData) | |
| if err != nil { | |
| return "", err | |
| } | |
| // 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1) | |
| if sig[64] >= 27 { | |
| sig[64] -= 27 | |
| } | |
| // if sig[64] >= 27 { | |
| // sig[64] -= 27 | |
| // } | |
| // 3. Recover public key | |
| pubKey, err := crypto.SigToPub(typedDataHash, sig) | |
| if err != nil { | |
| return "", err | |
| } | |
| // // 3. Recover public key | |
| // pubKey, err := crypto.SigToPub(typedDataHash, sig) | |
| // if err != nil { | |
| // return "", err | |
| // } | |
| signerAddress := crypto.PubkeyToAddress(*pubKey) | |
| // signerAddress := crypto.PubkeyToAddress(*pubKey) | |
| return signerAddress.Hex(), nil | |
| return walletAddress, nil | |
| _, typedDataHash, err := apitypes.TypedDataAndHash(typedData) | |
| if err != nil { | |
| return "", err | |
| } | |
| // 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1) | |
| if sig[64] >= 27 { | |
| sig[64] -= 27 | |
| } | |
| // 3. Recover public key | |
| pubKey, err := crypto.SigToPub(typedDataHash, sig) | |
| if err != nil { | |
| return "", err | |
| } | |
| signerAddress := crypto.PubkeyToAddress(*pubKey) | |
| return signerAddress.Hex(), nil |
🤖 Prompt for AI Agents
In clearnode/signer.go around lines 125 to 143, signature verification was
removed causing the function to always return the provided walletAddress;
restore verification by capturing the typedDataHash from
apitypes.TypedDataAndHash, ensure sig is normalized (if sig[64] >= 27 subtract
27), call crypto.SigToPub with the typedDataHash and sig to recover the public
key, derive the signer address via crypto.PubkeyToAddress and compare it to the
provided walletAddress, returning an error when they don't match; preserve
proper error returns on each failure and, if an optional bypass is desired for
development only, gate it behind a clear build tag or environment flag rather
than commenting out verification.
| // if sig[64] >= 27 { | ||
| // sig[64] -= 27 | ||
| // } |
There was a problem hiding this comment.
| // if sig[64] >= 27 { | |
| // sig[64] -= 27 | |
| // } |
| // // 3. Recover public key | ||
| // pubKey, err := crypto.SigToPub(typedDataHash, sig) | ||
| // if err != nil { | ||
| // return "", err | ||
| // } |
There was a problem hiding this comment.
| // // 3. Recover public key | |
| // pubKey, err := crypto.SigToPub(typedDataHash, sig) | |
| // if err != nil { | |
| // return "", err | |
| // } |
| // } | ||
|
|
||
| signerAddress := crypto.PubkeyToAddress(*pubKey) | ||
| // signerAddress := crypto.PubkeyToAddress(*pubKey) |
There was a problem hiding this comment.
| // signerAddress := crypto.PubkeyToAddress(*pubKey) |
alessio
left a comment
alessio
left a comment
There was a problem hiding this comment.
Minor remark: I'd get rid of unused code rather than commenting it out. Looks good otherwise
alessio
left a comment
alessio
left a comment
There was a problem hiding this comment.
Blocking to prevent merge
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
clearnode/rpc_router_auth.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.378Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.378Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.378Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
clearnode/rpc_router_auth.go
📚 Learning: 2025-11-13T10:32:14.541Z
Learnt from: nksazonov
Repo: erc7824/nitrolite PR: 428
File: clearnode/rpc_router_auth.go:233-244
Timestamp: 2025-11-13T10:32:14.541Z
Learning: In clearnode/rpc_router_auth.go, when CheckSessionKeyExists returns ErrSessionKeyExistsAndExpired during authentication, the error should be returned to the client and block re-authentication. This is intentional behavior - expired session keys should not allow automatic re-authentication and replacement in the auth flow.
Applied to files:
clearnode/rpc_router_auth.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
| // recoveredAddress, err := RecoverAddressFromEip712Signature( | ||
| // challenge.Address, | ||
| // challenge.Token.String(), | ||
| // challenge.SessionKey, | ||
| // challenge.Application, | ||
| // challenge.Allowances, | ||
| // challenge.Scope, | ||
| // challenge.SessionKeyExpiresAt, | ||
| // sig) | ||
| // if err != nil { | ||
| // logger.Error("failed to recover address from signature", "error", err) | ||
| // return nil, nil, RPCErrorf("invalid signature") | ||
| // } | ||
| recoveredAddress := challenge.Address // TODO: implement EIP-712 signature recovery |
There was a problem hiding this comment.
Critical security vulnerability: Authentication bypass allows any user to impersonate any wallet.
By assigning recoveredAddress := challenge.Address, the signature verification is completely disabled. The sig parameter is accepted but never validated, meaning:
- Any client can authenticate as any wallet address without proving key ownership
- An attacker only needs to know a target wallet address to impersonate it
- The entire authentication security model is broken
Even for development/testing purposes, this approach is dangerous because:
- The code could accidentally be merged to production
- There's no build-time or runtime guard preventing this from running in production environments
If this is intended for local development only, consider using a build tag or environment-based feature flag:
🔎 Suggested safer approach for development bypass:
- // recoveredAddress, err := RecoverAddressFromEip712Signature(
- // challenge.Address,
- // challenge.Token.String(),
- // challenge.SessionKey,
- // challenge.Application,
- // challenge.Allowances,
- // challenge.Scope,
- // challenge.SessionKeyExpiresAt,
- // sig)
- // if err != nil {
- // logger.Error("failed to recover address from signature", "error", err)
- // return nil, nil, RPCErrorf("invalid signature")
- // }
- recoveredAddress := challenge.Address // TODO: implement EIP-712 signature recovery
+ recoveredAddress, err := RecoverAddressFromEip712Signature(
+ challenge.Address,
+ challenge.Token.String(),
+ challenge.SessionKey,
+ challenge.Application,
+ challenge.Allowances,
+ challenge.Scope,
+ challenge.SessionKeyExpiresAt,
+ sig)
+ if err != nil {
+ logger.Error("failed to recover address from signature", "error", err)
+ return nil, nil, RPCErrorf("invalid signature")
+ }If you need a dev-only bypass, use a separate file with //go:build dev or check an explicit environment variable like DISABLE_SIG_VERIFY=true with prominent logging warnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // recoveredAddress, err := RecoverAddressFromEip712Signature( | |
| // challenge.Address, | |
| // challenge.Token.String(), | |
| // challenge.SessionKey, | |
| // challenge.Application, | |
| // challenge.Allowances, | |
| // challenge.Scope, | |
| // challenge.SessionKeyExpiresAt, | |
| // sig) | |
| // if err != nil { | |
| // logger.Error("failed to recover address from signature", "error", err) | |
| // return nil, nil, RPCErrorf("invalid signature") | |
| // } | |
| recoveredAddress := challenge.Address // TODO: implement EIP-712 signature recovery | |
| recoveredAddress, err := RecoverAddressFromEip712Signature( | |
| challenge.Address, | |
| challenge.Token.String(), | |
| challenge.SessionKey, | |
| challenge.Application, | |
| challenge.Allowances, | |
| challenge.Scope, | |
| challenge.SessionKeyExpiresAt, | |
| sig) | |
| if err != nil { | |
| logger.Error("failed to recover address from signature", "error", err) | |
| return nil, nil, RPCErrorf("invalid signature") | |
| } |
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.