Conversation
📝 WalkthroughWalkthroughAdds smart-contract-wallet signature support: new SmartWalletValidator and SwSignatureUtils (ERC-1271 + ERC-6492), updates SessionKeyValidator and ECDSA utilities to handle smart-wallet formats, removes several Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SmartWalletValidator
participant SwSignatureUtils
participant Factory
participant Wallet
participant ERC1271
Caller->>SmartWalletValidator: validateSignature(channelId, signingData, sig, participant)
SmartWalletValidator->>SwSignatureUtils: validateSmartWalletSigner(message, sig, participant)
alt participant has on-chain code
SwSignatureUtils->>ERC1271: isValidSignature(hash, sig) on participant
ERC1271-->>SwSignatureUtils: magic value / reject
else participant has no code
SwSignatureUtils->>SwSignatureUtils: detect ERC-6492 format in sig
SwSignatureUtils->>Factory: extract factory + calldata, call deploy
Factory-->>Wallet: deployment (CREATE2)
SwSignatureUtils->>ERC1271: isValidSignature(hash, sig) on deployed wallet
ERC1271-->>SwSignatureUtils: magic value / reject
end
SwSignatureUtils-->>SmartWalletValidator: validation result (bool)
SmartWalletValidator-->>Caller: VALIDATION_SUCCESS or VALIDATION_FAILURE
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 @nksazonov, 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 significantly upgrades the system's signature validation capabilities by introducing robust support for smart contract wallets. It integrates ERC-4337 account abstraction standards, allowing both deployed and undeployed smart accounts to participate in signature flows. This enhancement broadens compatibility and future-proofs the system for evolving blockchain account models. 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. Changelog
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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/sigValidators/SessionKeyValidator.sol (1)
17-25:⚠️ Potential issue | 🟡 MinorStale
@param authSignaturedoc: still says "65 bytes ECDSA" but now accepts smart wallet signatures.With
_validateSignersupporting ERC-1271/ERC-6492,authSignatureis no longer restricted to 65-byte ECDSA. The struct-level NatDoc should be updated to match.📝 Proposed fix
* `@param` sessionKey The address of the delegated session key * `@param` metadataHash Hashed application-specific data (e.g., expiration timestamp, nonce, permissions) - * `@param` authSignature The participant's signature authorizing this session key (65 bytes ECDSA) + * `@param` authSignature The participant's signature authorizing this session key (ECDSA, ERC-1271, or ERC-6492)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/sigValidators/SessionKeyValidator.sol` around lines 17 - 25, Update the struct NatSpec for SessionKeyAuthorization to remove the stale "65 bytes ECDSA" remark for authSignature and instead state that authSignature may be a signer signature or a contract signature (supports ERC-1271/ERC-6492 / smart wallet signatures) and therefore can be variable length; reference the related _validateSigner method in the comment to indicate the supported validation mechanisms. Ensure the param description for authSignature clarifies it accepts both ECDSA and contract-auth formats rather than a fixed 65-byte signature.
🧹 Nitpick comments (5)
contracts/signature-validators.md (1)
175-175: Minor grammar nit: "Variable length" → "Variable-length".Use a hyphen when used as a compound adjective before a noun.
📝 Fix
-Variable length signature that depends on the smart contract wallet's implementation. The validator handles both deployed and undeployed contracts. +Variable-length signature that depends on the smart contract wallet's implementation. The validator handles both deployed and undeployed contracts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/signature-validators.md` at line 175, Minor grammar nit: change the compound adjective "Variable length signature that depends on the smart contract wallet's implementation." to "Variable-length signature that depends on the smart contract wallet's implementation." Update the phrase wherever it appears (e.g., in the descriptive sentence referencing the validator that handles both deployed and undeployed contracts) to use the hyphenated form "Variable-length".contracts/src/sigValidators/SwSignatureUtils.sol (2)
106-112: Gas-expensive byte-by-byte copy for signature data stripping.The loop copies each byte individually, which is significantly more expensive than a 32-byte-word-at-a-time copy. For typical ERC-6492 signatures this could waste meaningful gas. Consider using assembly
mcopy/mstoreloop or reusingisValidErc1271Signatureto avoid the copy entirely.♻️ Alternative: decode in-place using assembly slice
- // Extract the signature data (remove the magic suffix) - uint256 dataLength = sig.length - 32; - bytes memory signatureData = new bytes(dataLength); - - for (uint256 i = 0; i < dataLength; i++) { - signatureData[i] = sig[i]; - } - - (address create2Factory, bytes memory factoryCalldata, bytes memory originalSig) = - abi.decode(signatureData, (address, bytes, bytes)); + // Strip the 32-byte magic suffix and decode the remaining data + bytes memory signatureData; + assembly { + let dataLength := sub(mload(sig), 32) + signatureData := mload(0x40) + mstore(signatureData, dataLength) + let src := add(sig, 32) + let dst := add(signatureData, 32) + for { let i := 0 } lt(i, dataLength) { i := add(i, 32) } { + mstore(add(dst, i), mload(add(src, i))) + } + mstore(0x40, add(dst, dataLength)) + } + + (address create2Factory, bytes memory factoryCalldata, bytes memory originalSig) = + abi.decode(signatureData, (address, bytes, bytes));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/sigValidators/SwSignatureUtils.sol` around lines 106 - 112, The byte-by-byte loop that builds signatureData from sig (variables: signatureData, dataLength, sig) is gas-inefficient; replace it with a 32-byte word copy using inline assembly (use mload from sig + 32-byte offset, mstore to the signatureData memory pointer in a loop, then handle the final partial word with a mask) or avoid copying entirely by adapting/reusing isValidErc1271Signature to accept an offset/length into sig and validate in-place; update only the block that presently does the for-loop so signatureData is created/copied word-at-a-time (or removed) and ensure the free-memory pointer and length are set correctly in assembly.
41-58: Validation order is correct but note potential wasted gas for deployed wallets with ERC-6492 signatures.When
expectedSignerhas code AND the signature is in ERC-6492 format, the code first tries ERC-1271 with the full ERC-6492-wrapped signature (which will almost certainly fail), then falls through to the ERC-6492 path. This is functionally correct but wastes gas on the failed ERC-1271 call.Consider checking
isERC6492Signaturefirst to short-circuit:♻️ Reorder to check ERC-6492 format first
function validateSmartWalletSigner(bytes memory message, bytes memory signature, address expectedSigner) internal returns (bool) { bytes32 messageHash = keccak256(message); - if (expectedSigner.code.length > 0) { - if (isValidErc1271Signature(expectedSigner, messageHash, signature)) { - return true; - } - } - if (isERC6492Signature(signature)) { return isValidERC6492Signature(messageHash, signature, expectedSigner); } + if (expectedSigner.code.length > 0) { + return isValidErc1271Signature(expectedSigner, messageHash, signature); + } + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/sigValidators/SwSignatureUtils.sol` around lines 41 - 58, The function validateSmartWalletSigner currently calls isValidErc1271Signature on deployed contracts before checking isERC6492Signature, causing wasted gas when the signature is ERC-6492; change the control flow to first test isERC6492Signature(signature) and, if true, call isValidERC6492Signature(messageHash, signature, expectedSigner) immediately, otherwise proceed to check if expectedSigner.code.length > 0 and call isValidErc1271Signature(expectedSigner, messageHash, signature); keep the same return semantics and keep references to validateSmartWalletSigner, isERC6492Signature, isValidERC6492Signature, isValidErc1271Signature and expectedSigner.contracts/test/sigValidators/SwTestUtils.sol (1)
21-40:ecrecovercan returnaddress(0)— consider guarding against it.If
ecrecoverfails (e.g., due to a malformed signature that happens to pass the length check), it returnsaddress(0). Ifownerwere everaddress(0), the check on Line 35 would incorrectly pass. This is unlikely in tests (owner comes fromvm.addr(1)), but adding a guard is a defensive best practice that also makes the mock more realistic.🛡️ Suggested guard
address recovered = ecrecover(hash, v, r, s); - if (recovered == owner) { + if (recovered != address(0) && recovered == owner) { return ERC1271_MAGIC_VALUE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/sigValidators/SwTestUtils.sol` around lines 21 - 40, In isValidSignature, guard against ecrecover returning address(0) by checking the recovered address is non-zero before comparing to owner: after calling _splitSignature and ecrecover in isValidSignature, ensure recovered != address(0) && recovered == owner before returning ERC1271_MAGIC_VALUE, otherwise return the 0xffffffff fallback; this uses the existing isValidSignature, _splitSignature, ecrecover, owner, and ERC1271_MAGIC_VALUE symbols.contracts/test/sigValidators/SessionKeyValidator.t.sol (1)
366-557: Consider adding a mixed deployed/non-deployed smart wallet test.The ERC-6492 tests thoroughly cover: non-deployed participant + EOA session key, EOA participant + non-deployed session key, and both non-deployed. However, there's no test for the combination where the participant is a deployed smart wallet (ERC-1271 path) while the session key is a non-deployed smart wallet (ERC-6492 path), or vice versa. This mixed scenario exercises both validation paths in a single call and could catch ordering or state-related issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/sigValidators/SessionKeyValidator.t.sol` around lines 366 - 557, Add two tests covering mixed deployed/non-deployed combos: (1) deployed participant smart wallet + non-deployed session-key smart wallet, and (2) non-deployed participant smart wallet + deployed session-key smart wallet. For (1) deploy the participant address returned by MockSmartWalletFactory.getAddress(user, salt) before calling validator.validateSignature, then create an ERC-6492 signature for the session key using SwTestUtils.createERC6492Signature(address(factory), factoryCalldata, stateSig) and sign the authorization with TestUtils.signEip191 or TestUtils.signRaw as in existing tests; call validator.validateSignature and assert VALIDATION_SUCCESS/FAILURE and that the non-deployed session-key wallet gets deployed. For (2) deploy the session-key address first and use an ERC-1271-style authorization for the participant (sign auth with TestUtils.signRaw + create ERC-6492 for participant if needed) or reuse existing patterns (see test_success_participantIsNonDeployedSmartWallet_sessionKeyIsEoa, test_success_bothAreNonDeployedSmartWallets) to mirror the opposite mix; assert validation result and that the previously non-deployed wallet is deployed after validation. Ensure to reuse factory.getAddress, MockSmartWalletFactory.deploy, SwTestUtils.createERC6492Signature, TestUtils.signRaw/TestUtils.signEip191, signStateWithSk and validator.validateSignature so tests match existing patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/signature-validators.md`:
- Around line 178-181: The documentation's described validation order is wrong —
update the docs to match the actual implementation in
SwSignatureUtils.validateSmartWalletSigner which checks ERC-1271 first for
deployed wallets and only falls back to ERC-6492 (counterfactual) afterwards;
change the numbered steps to reflect: 1) Try ERC-1271 (`isValidSignature`) for
deployed contracts, 2) If not applicable, try ERC-6492 counterfactual
verification, 3) Return VALIDATION_SUCCESS or VALIDATION_FAILURE based on the
contract verification result.
In `@contracts/src/sigValidators/SmartWalletValidator.sol`:
- Around line 13-20: The NatSpec is misleading: update the contract-level and
function-level comments in SmartWalletValidator to accurately state that it
validates only smart contract wallets via ERC-1271 and ERC-6492 (remove or
correct the ERC-4337 reference), and either (A) implement EOA signature support
by adding ECDSA recovery validation (e.g., add a validateEoaSigner flow
alongside SwSignatureUtils.validateSmartWalletSigner and call both in
validateSignature), or (B) explicitly document that SmartWalletValidator only
supports smart contract wallets and instruct callers to use a separate EOA
validator; reference SwSignatureUtils.validateSmartWalletSigner and the
validateSignature entry point when making these changes so the implementation
and docs remain consistent.
In `@contracts/src/sigValidators/SwSignatureUtils.sol`:
- Around line 102-124: The ERC-1271 call inside isValidERC6492Signature
currently invokes IERC1271(expectedSigner).isValidSignature directly and can
bubble up a revert; wrap that call in a try/catch similar to
isValidErc1271Signature so any revert returns false instead of reverting the
whole transaction. Specifically, replace the direct
IERC1271(expectedSigner).isValidSignature(msgHash, originalSig) invocation with
a try IERC1271(expectedSigner).isValidSignature(msgHash, originalSig) returns
(bytes4 res) { return res == ERC1271_MAGIC_VALUE; } catch { return false; } to
mirror the safe behavior of isValidErc1271Signature.
---
Outside diff comments:
In `@contracts/src/sigValidators/SessionKeyValidator.sol`:
- Around line 17-25: Update the struct NatSpec for SessionKeyAuthorization to
remove the stale "65 bytes ECDSA" remark for authSignature and instead state
that authSignature may be a signer signature or a contract signature (supports
ERC-1271/ERC-6492 / smart wallet signatures) and therefore can be variable
length; reference the related _validateSigner method in the comment to indicate
the supported validation mechanisms. Ensure the param description for
authSignature clarifies it accepts both ECDSA and contract-auth formats rather
than a fixed 65-byte signature.
---
Nitpick comments:
In `@contracts/signature-validators.md`:
- Line 175: Minor grammar nit: change the compound adjective "Variable length
signature that depends on the smart contract wallet's implementation." to
"Variable-length signature that depends on the smart contract wallet's
implementation." Update the phrase wherever it appears (e.g., in the descriptive
sentence referencing the validator that handles both deployed and undeployed
contracts) to use the hyphenated form "Variable-length".
In `@contracts/src/sigValidators/SwSignatureUtils.sol`:
- Around line 106-112: The byte-by-byte loop that builds signatureData from sig
(variables: signatureData, dataLength, sig) is gas-inefficient; replace it with
a 32-byte word copy using inline assembly (use mload from sig + 32-byte offset,
mstore to the signatureData memory pointer in a loop, then handle the final
partial word with a mask) or avoid copying entirely by adapting/reusing
isValidErc1271Signature to accept an offset/length into sig and validate
in-place; update only the block that presently does the for-loop so
signatureData is created/copied word-at-a-time (or removed) and ensure the
free-memory pointer and length are set correctly in assembly.
- Around line 41-58: The function validateSmartWalletSigner currently calls
isValidErc1271Signature on deployed contracts before checking
isERC6492Signature, causing wasted gas when the signature is ERC-6492; change
the control flow to first test isERC6492Signature(signature) and, if true, call
isValidERC6492Signature(messageHash, signature, expectedSigner) immediately,
otherwise proceed to check if expectedSigner.code.length > 0 and call
isValidErc1271Signature(expectedSigner, messageHash, signature); keep the same
return semantics and keep references to validateSmartWalletSigner,
isERC6492Signature, isValidERC6492Signature, isValidErc1271Signature and
expectedSigner.
In `@contracts/test/sigValidators/SessionKeyValidator.t.sol`:
- Around line 366-557: Add two tests covering mixed deployed/non-deployed
combos: (1) deployed participant smart wallet + non-deployed session-key smart
wallet, and (2) non-deployed participant smart wallet + deployed session-key
smart wallet. For (1) deploy the participant address returned by
MockSmartWalletFactory.getAddress(user, salt) before calling
validator.validateSignature, then create an ERC-6492 signature for the session
key using SwTestUtils.createERC6492Signature(address(factory), factoryCalldata,
stateSig) and sign the authorization with TestUtils.signEip191 or
TestUtils.signRaw as in existing tests; call validator.validateSignature and
assert VALIDATION_SUCCESS/FAILURE and that the non-deployed session-key wallet
gets deployed. For (2) deploy the session-key address first and use an
ERC-1271-style authorization for the participant (sign auth with
TestUtils.signRaw + create ERC-6492 for participant if needed) or reuse existing
patterns (see test_success_participantIsNonDeployedSmartWallet_sessionKeyIsEoa,
test_success_bothAreNonDeployedSmartWallets) to mirror the opposite mix; assert
validation result and that the previously non-deployed wallet is deployed after
validation. Ensure to reuse factory.getAddress, MockSmartWalletFactory.deploy,
SwTestUtils.createERC6492Signature, TestUtils.signRaw/TestUtils.signEip191,
signStateWithSk and validator.validateSignature so tests match existing
patterns.
In `@contracts/test/sigValidators/SwTestUtils.sol`:
- Around line 21-40: In isValidSignature, guard against ecrecover returning
address(0) by checking the recovered address is non-zero before comparing to
owner: after calling _splitSignature and ecrecover in isValidSignature, ensure
recovered != address(0) && recovered == owner before returning
ERC1271_MAGIC_VALUE, otherwise return the 0xffffffff fallback; this uses the
existing isValidSignature, _splitSignature, ecrecover, owner, and
ERC1271_MAGIC_VALUE symbols.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
contracts/signature-validators.mdcontracts/src/ChannelHub.solcontracts/src/interfaces/ISignatureValidator.solcontracts/src/sigValidators/EcdsaSignatureUtils.solcontracts/src/sigValidators/SessionKeyValidator.solcontracts/src/sigValidators/SmartWalletValidator.solcontracts/src/sigValidators/SwSignatureUtils.solcontracts/test/sigValidators/SessionKeyValidator.t.solcontracts/test/sigValidators/SmartWalletValidator.t.solcontracts/test/sigValidators/SwSignatureUtils.t.solcontracts/test/sigValidators/SwTestUtils.sol
|
|
||
| 1. Try ERC-6492 validation first (supports undeployed contracts via counterfactual verification) | ||
| 2. If contract is deployed, use ERC-1271 validation (`isValidSignature` method) | ||
| 3. Return `VALIDATION_SUCCESS` if signature is valid according to the contract's logic, `VALIDATION_FAILURE` otherwise |
There was a problem hiding this comment.
Documentation describes validation order incorrectly.
The doc states ERC-6492 is tried first, then ERC-1271. The actual code in SwSignatureUtils.validateSmartWalletSigner does the opposite: it checks ERC-1271 first (for deployed wallets), then falls back to ERC-6492.
📝 Proposed fix to match the actual code
### Validation Logic
-1. Try ERC-6492 validation first (supports undeployed contracts via counterfactual verification)
-2. If contract is deployed, use ERC-1271 validation (`isValidSignature` method)
+1. If contract is deployed, try ERC-1271 validation (`isValidSignature` method)
+2. If signature is in ERC-6492 format, try ERC-6492 validation (supports undeployed contracts via counterfactual deployment)
3. Return `VALIDATION_SUCCESS` if signature is valid according to the contract's logic, `VALIDATION_FAILURE` otherwise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/signature-validators.md` around lines 178 - 181, The
documentation's described validation order is wrong — update the docs to match
the actual implementation in SwSignatureUtils.validateSmartWalletSigner which
checks ERC-1271 first for deployed wallets and only falls back to ERC-6492
(counterfactual) afterwards; change the numbered steps to reflect: 1) Try
ERC-1271 (`isValidSignature`) for deployed contracts, 2) If not applicable, try
ERC-6492 counterfactual verification, 3) Return VALIDATION_SUCCESS or
VALIDATION_FAILURE based on the contract verification result.
| /** | ||
| * @title SmartWalletValidator | ||
| * @notice Signature validator supporting smart contract wallets via ERC-4337 standards | ||
| * @dev Supports ERC-1271 and ERC-6492 signatures. | ||
| * | ||
| * The validator prepends channelId to the signingData to construct the full message, | ||
| * then attempts validation in the order listed above. | ||
| */ |
There was a problem hiding this comment.
NatDoc mentions EOA support but the validator only handles smart wallets.
Line 23 says "Validates a signature from either an EOA or smart contract wallet" but the implementation only calls SwSignatureUtils.validateSmartWalletSigner, which only supports ERC-1271/ERC-6492 — no ECDSA recovery for EOAs. Also, Line 15 references "ERC-4337 standards" but the actual standards used are ERC-1271 and ERC-6492, which are distinct from ERC-4337.
📝 Fix misleading documentation
/**
* `@title` SmartWalletValidator
- * `@notice` Signature validator supporting smart contract wallets via ERC-4337 standards
+ * `@notice` Signature validator supporting smart contract wallets via ERC-1271 and ERC-6492 standards
* `@dev` Supports ERC-1271 and ERC-6492 signatures.
*
...
/**
- * `@notice` Validates a signature from either an EOA or smart contract wallet
+ * `@notice` Validates a signature from a smart contract wallet🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/sigValidators/SmartWalletValidator.sol` around lines 13 - 20,
The NatSpec is misleading: update the contract-level and function-level comments
in SmartWalletValidator to accurately state that it validates only smart
contract wallets via ERC-1271 and ERC-6492 (remove or correct the ERC-4337
reference), and either (A) implement EOA signature support by adding ECDSA
recovery validation (e.g., add a validateEoaSigner flow alongside
SwSignatureUtils.validateSmartWalletSigner and call both in validateSignature),
or (B) explicitly document that SmartWalletValidator only supports smart
contract wallets and instruct callers to use a separate EOA validator; reference
SwSignatureUtils.validateSmartWalletSigner and the validateSignature entry point
when making these changes so the implementation and docs remain consistent.
| function isValidERC6492Signature(bytes32 msgHash, bytes memory sig, address expectedSigner) | ||
| internal | ||
| returns (bool) | ||
| { | ||
| // Extract the signature data (remove the magic suffix) | ||
| uint256 dataLength = sig.length - 32; | ||
| bytes memory signatureData = new bytes(dataLength); | ||
|
|
||
| for (uint256 i = 0; i < dataLength; i++) { | ||
| signatureData[i] = sig[i]; | ||
| } | ||
|
|
||
| (address create2Factory, bytes memory factoryCalldata, bytes memory originalSig) = | ||
| abi.decode(signatureData, (address, bytes, bytes)); | ||
|
|
||
| if (expectedSigner.code.length == 0) { | ||
| (bool success,) = create2Factory.call(factoryCalldata); | ||
| require(success, ERC6492DeploymentFailed(create2Factory, factoryCalldata)); | ||
| require(expectedSigner.code.length != 0, ERC6492NoCode(expectedSigner)); | ||
| } | ||
|
|
||
| return IERC1271(expectedSigner).isValidSignature(msgHash, originalSig) == ERC1271_MAGIC_VALUE; | ||
| } |
There was a problem hiding this comment.
Missing try/catch around the ERC-1271 call makes isValidERC6492Signature revert instead of returning false.
isValidErc1271Signature (Line 68) wraps the external call in try/catch, gracefully returning false on revert. But isValidERC6492Signature (Line 123) calls IERC1271.isValidSignature directly — if the deployed contract reverts (e.g., doesn't implement ERC-1271, or has an internal error), the entire transaction reverts instead of returning false.
This asymmetry means a malformed ERC-6492 signature pointing to an incompatible contract will revert the whole on-chain operation (channel creation, deposit, etc.) rather than just failing validation.
🐛 Proposed fix: wrap in try/catch like the ERC-1271 path
- return IERC1271(expectedSigner).isValidSignature(msgHash, originalSig) == ERC1271_MAGIC_VALUE;
+ return isValidErc1271Signature(expectedSigner, msgHash, originalSig);📝 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.
| function isValidERC6492Signature(bytes32 msgHash, bytes memory sig, address expectedSigner) | |
| internal | |
| returns (bool) | |
| { | |
| // Extract the signature data (remove the magic suffix) | |
| uint256 dataLength = sig.length - 32; | |
| bytes memory signatureData = new bytes(dataLength); | |
| for (uint256 i = 0; i < dataLength; i++) { | |
| signatureData[i] = sig[i]; | |
| } | |
| (address create2Factory, bytes memory factoryCalldata, bytes memory originalSig) = | |
| abi.decode(signatureData, (address, bytes, bytes)); | |
| if (expectedSigner.code.length == 0) { | |
| (bool success,) = create2Factory.call(factoryCalldata); | |
| require(success, ERC6492DeploymentFailed(create2Factory, factoryCalldata)); | |
| require(expectedSigner.code.length != 0, ERC6492NoCode(expectedSigner)); | |
| } | |
| return IERC1271(expectedSigner).isValidSignature(msgHash, originalSig) == ERC1271_MAGIC_VALUE; | |
| } | |
| function isValidERC6492Signature(bytes32 msgHash, bytes memory sig, address expectedSigner) | |
| internal | |
| returns (bool) | |
| { | |
| // Extract the signature data (remove the magic suffix) | |
| uint256 dataLength = sig.length - 32; | |
| bytes memory signatureData = new bytes(dataLength); | |
| for (uint256 i = 0; i < dataLength; i++) { | |
| signatureData[i] = sig[i]; | |
| } | |
| (address create2Factory, bytes memory factoryCalldata, bytes memory originalSig) = | |
| abi.decode(signatureData, (address, bytes, bytes)); | |
| if (expectedSigner.code.length == 0) { | |
| (bool success,) = create2Factory.call(factoryCalldata); | |
| require(success, ERC6492DeploymentFailed(create2Factory, factoryCalldata)); | |
| require(expectedSigner.code.length != 0, ERC6492NoCode(expectedSigner)); | |
| } | |
| return isValidErc1271Signature(expectedSigner, msgHash, originalSig); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/sigValidators/SwSignatureUtils.sol` around lines 102 - 124, The
ERC-1271 call inside isValidERC6492Signature currently invokes
IERC1271(expectedSigner).isValidSignature directly and can bubble up a revert;
wrap that call in a try/catch similar to isValidErc1271Signature so any revert
returns false instead of reverting the whole transaction. Specifically, replace
the direct IERC1271(expectedSigner).isValidSignature(msgHash, originalSig)
invocation with a try IERC1271(expectedSigner).isValidSignature(msgHash,
originalSig) returns (bytes4 res) { return res == ERC1271_MAGIC_VALUE; } catch {
return false; } to mirror the safe behavior of isValidErc1271Signature.
![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
The pull request introduces support for smart contract wallets (ERC-1271 and ERC-6492) in signature validation. This is a significant enhancement that enables account abstraction wallets to participate in the protocol. The changes correctly update the validator interfaces and the main ChannelHub contract to handle potential state changes during validation (required for ERC-6492 contract deployment). I have identified a few critical and high-severity issues related to reentrancy protection and robustness in the new validation logic.
| uint256 dataLength = sig.length - 32; | ||
| bytes memory signatureData = new bytes(dataLength); | ||
|
|
||
| for (uint256 i = 0; i < dataLength; i++) { | ||
| signatureData[i] = sig[i]; | ||
| } |
There was a problem hiding this comment.
Manual byte-by-byte copying in memory is very gas-inefficient in Solidity. Consider using assembly or a more optimized approach to slice the signature.
| require(expectedSigner.code.length != 0, ERC6492NoCode(expectedSigner)); | ||
| } | ||
|
|
||
| return IERC1271(expectedSigner).isValidSignature(msgHash, originalSig) == ERC1271_MAGIC_VALUE; |
There was a problem hiding this comment.
This call to isValidSignature should be wrapped in a try/catch block or use a low-level call. If the target contract reverts (e.g., it doesn't implement ERC-1271), the entire validation will revert, which can block protocol operations like challenges. For consistency, it should behave like isValidErc1271Signature on line 67.
try IERC1271(expectedSigner).isValidSignature(msgHash, originalSig) returns (bytes4 magicValue) {
return magicValue == ERC1271_MAGIC_VALUE;
} catch {
return false;
}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/sigValidators/SessionKeyValidator.sol (1)
19-19:⚠️ Potential issue | 🟡 MinorStale
authSignatureNatSpec — drop the "65 bytes ECDSA" constraint.The struct comment still describes
authSignatureas(65 bytes ECDSA), but with the new_validateSignerdispatch this field can now carry ERC-1271 or ERC-6492 formatted bytes as well.✏️ Suggested fix
- * `@param` authSignature The participant's signature authorizing this session key (65 bytes ECDSA) + * `@param` authSignature The participant's signature authorizing this session key (ECDSA, ERC-1271, or ERC-6492)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/sigValidators/SessionKeyValidator.sol` at line 19, Update the NatSpec for the SessionKeyValidator struct parameter `authSignature` to remove the fixed "65 bytes ECDSA" description and reflect that it may contain different signature formats (e.g., ECDSA, ERC-1271, ERC-6492) handled by `_validateSigner`; locate the `authSignature` mention in SessionKeyValidator.sol and replace the size-specific comment with a generic description such as "signature or authorization data for the session (format validated by _validateSigner)" so the comment matches the new dispatch behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/sigValidators/SessionKeyValidator.sol`:
- Line 67: Update the NatSpec for validateSignature and _validateSigner to
accurately describe the actual conditional routing: explain that the
implementation selects the smart-wallet validation path when the signer has code
(signer.code.length != 0) or when the signature has the ERC-6492 magic suffix,
and otherwise uses ECDSA validation; do not imply an attempted-then-fallback
flow or that ECDSA is tried first—document the decision criteria and the
mutually exclusive branches instead.
---
Outside diff comments:
In `@contracts/src/sigValidators/SessionKeyValidator.sol`:
- Line 19: Update the NatSpec for the SessionKeyValidator struct parameter
`authSignature` to remove the fixed "65 bytes ECDSA" description and reflect
that it may contain different signature formats (e.g., ECDSA, ERC-1271,
ERC-6492) handled by `_validateSigner`; locate the `authSignature` mention in
SessionKeyValidator.sol and replace the size-specific comment with a generic
description such as "signature or authorization data for the session (format
validated by _validateSigner)" so the comment matches the new dispatch behavior.
| * 2. sessionKey signed the full state message (channelId + signingData) | ||
| * Tries EIP-191 recovery first, then raw ECDSA for both signatures. | ||
| * Supports both EOA (ECDSA) and smart wallet (ERC-1271/ERC-6492) signatures. | ||
| * Tries ECDSA validation first, then smart wallet validation for both signatures. |
There was a problem hiding this comment.
Inaccurate "tries ECDSA first" description in two NatSpec comments.
Both the validateSignature NatSpec (line 67) and the _validateSigner NatSpec (line 102) state "Tries ECDSA validation first, then smart wallet validation", implying a sequential attempt-then-fallback pattern. The actual implementation is a conditional router: it takes the smart-wallet path when signer.code.length != 0 or an ERC-6492 magic suffix is detected, and only otherwise falls through to ECDSA. No speculative ECDSA attempt is made for smart wallets.
✏️ Suggested fixes
- * Tries ECDSA validation first, then smart wallet validation for both signatures.
+ * Routes to smart wallet validation (ERC-1271/ERC-6492) if the signer has deployed code
+ * or the signature carries the ERC-6492 magic suffix; otherwise uses ECDSA.- * `@dev` Tries ECDSA validation first, then smart wallet validation
+ * `@dev` Routes to ERC-1271/ERC-6492 validation if the signer has deployed code or the signature
+ * is ERC-6492 formatted; otherwise falls back to ECDSA validation.Also applies to: 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/sigValidators/SessionKeyValidator.sol` at line 67, Update the
NatSpec for validateSignature and _validateSigner to accurately describe the
actual conditional routing: explain that the implementation selects the
smart-wallet validation path when the signer has code (signer.code.length != 0)
or when the signature has the ERC-6492 magic suffix, and otherwise uses ECDSA
validation; do not imply an attempted-then-fallback flow or that ECDSA is tried
first—document the decision criteria and the mutually exclusive branches
instead.
1 participant
Summary by CodeRabbit