Skip to content

feat: no eip-712 verify #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
philanton wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: no eip-712 verify #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb97f8d
feat: no eip-712 verify
philanton Dec 15, 2025
7afb8b2
no domain
philanton Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions clearnode/rpc_router_auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,19 +192,20 @@ func (r *RPCRouter) handleAuthSigVerify(ctx context.Context, sig Signature, auth
logger.Error("failed to get challenge", "error", err)
return nil, nil, RPCErrorf("invalid challenge")
}
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, 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
Comment on lines +195 to +208
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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:

  1. Any client can authenticate as any wallet address without proving key ownership
  2. An attacker only needs to know a target wallet address to impersonate it
  3. 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.

Suggested change
// 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 err := r.AuthManager.ValidateChallenge(authParams.Challenge, recoveredAddress); err != nil {
logger.Debug("challenge verification failed", "error", err)
Expand Down
22 changes: 11 additions & 11 deletions clearnode/signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,25 +122,25 @@ func RecoverAddressFromEip712Signature(
}

// 1. Hash the typed data (domain separator + message struct hash)
typedDataHash, _, err := apitypes.TypedDataAndHash(typedData)
_, _, 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
// }
Comment on lines +131 to +133
Copy link
Contributor

@alessio alessio Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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
// }
Comment on lines +135 to +139
Copy link
Contributor

@alessio alessio Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// // 3. Recover public key
// pubKey, err := crypto.SigToPub(typedDataHash, sig)
// if err != nil {
// return "", err
// }


signerAddress := crypto.PubkeyToAddress(*pubKey)
// signerAddress := crypto.PubkeyToAddress(*pubKey)
Copy link
Contributor

@alessio alessio Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// signerAddress := crypto.PubkeyToAddress(*pubKey)


return signerAddress.Hex(), nil
return walletAddress, nil
Comment on lines +125 to +143
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

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.

Suggested change
_, _, 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

Comment on lines +125 to +143
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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:

  1. typedDataHash from TypedDataAndHash is discarded (line 125)
  2. V value normalization commented out (lines 131-133)
  3. Public key recovery via SigToPub commented out (lines 135-139)
  4. Signer address derivation commented out (line 141)
  5. The sig parameter 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.

Suggested change
_, _, 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.

}

func convertAllowances(input []Allowance) []map[string]interface{} {
Expand Down
4 changes: 4 additions & 0 deletions go.work.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,7 @@ github.com/status-im/keycard-go v0.2.0/go.mod h1:wlp8ZLbsmrF6g6WjugPAx+IzoLrkdf9
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=
Expand Down Expand Up @@ -562,6 +563,7 @@ go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
go.uber.org/automaxprocs v1.5.2/go.mod h1:eRbA25aqJrxAbsLO0xy5jVwPt7FQnRgjW+efnwa1WM0=
go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
go.uber.org/multierr v1.10.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
go.uber.org/zap v1.10.0/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
go.uber.org/zap v1.17.0/go.mod h1:MXVU+bhUf/A7Xi2HNOnopQOrmycQ5Ih87HtOu4q5SSo=
golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
Expand Down Expand Up @@ -632,6 +634,7 @@ golang.org/x/mod v0.25.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww=
golang.org/x/mod v0.26.0/go.mod h1:/j6NAhSk8iQ723BGAUyoAcn7SlD7s15Dp9Nd/SfeaFQ=
golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc=
golang.org/x/mod v0.28.0/go.mod h1:yfB/L0NOf/kmEbXjzCPOx1iK1fRutOydrCMsqRhEBxI=
golang.org/x/mod v0.29.0/go.mod h1:NyhrlYXJ2H4eJiRy/WDBO6HMqZQ6q9nk4JzS3NuCK+w=
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20181023162649-9b4f9f5ad519/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
Expand Down Expand Up @@ -841,6 +844,7 @@ golang.org/x/tools v0.34.0/go.mod h1:pAP9OwEaY1CAW3HOmg3hLZC5Z0CCmzjAF2UQMSqNARg
golang.org/x/tools v0.35.0/go.mod h1:NKdj5HkL/73byiZSJjqJgKn3ep7KjFkBOkR/Hps3VPw=
golang.org/x/tools v0.36.0/go.mod h1:WBDiHKJK8YgLHlcQPYQzNCkUxUypCaa5ZegCVutKm+s=
golang.org/x/tools v0.37.0/go.mod h1:MBN5QPQtLMHVdvsbtarmTNukZDdgwdwlO5qGacAzF0w=
golang.org/x/tools v0.38.0/go.mod h1:yEsQ/d/YK8cjh0L6rZlY8tgtlKiBNTL14pGDJPJpYQs=
gonum.org/v1/gonum v0.0.0-20180816165407-929014505bf4/go.mod h1:Y+Yx5eoAFn32cQvJDxZx5Dpnq+c3wtXuadVZAcxbbBo=
gonum.org/v1/gonum v0.8.2/go.mod h1:oe/vMfY3deqTw+1EZJhuvEW2iwGF1bW9wwu7XCu0+v0=
gonum.org/v1/gonum v0.9.3/go.mod h1:TZumC3NeyVQskjXqmyWt4S3bINhy7B4eYwW69EbyX+0=
Expand Down
Loading