Ithaca account#62
Closed
Dargon789 wants to merge 12 commits into
Closed
Conversation
Add testERC20TransferViaPortoOrchestratorWithPasskey() benchmark to isolate passkey authentication costs from spend limit enforcement costs. - Uses secp256k1 passkey for transaction signing - Sets execution permissions for ERC20 transfers - Requires spend limits (set to max) for passkey operations - Gas cost: 116,094 (vs 97,030 without passkey, 117,083 with restrictive limits) - Provides clean measurement of passkey overhead (~19k gas) Resolves ithacaxyz#272 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Tanishk Goyal <legion2002@users.noreply.github.com>
…ated: IthacaAccount
…' into fix-tests-account
Reviewer's GuideAdds Merkle tree–based signature support to IthacaAccount, updates the wrapped signature format to include a merkle flag, and introduces tests (including fuzz tests and benchmarks) for Merkle signatures and passkey-based orchestrator flows, along with minor EIP712 domain and versioning adjustments. Sequence diagram for Merkle-based signature verification in IthacaAccountsequenceDiagram
actor Caller
participant IthacaAccount
participant MerkleProofLib
participant ECDSA
Caller->>IthacaAccount: unwrapAndValidateSignature(digest, wrappedSignature)
activate IthacaAccount
IthacaAccount->>IthacaAccount: parse keyHash, prehash flag, merkle flag
alt merkle flag is set
IthacaAccount->>IthacaAccount: _verifyMerkleSig(digest, innerMerkleSignature)
activate IthacaAccount
IthacaAccount->>MerkleProofLib: verifyCalldata(proof, root, digest)
activate MerkleProofLib
MerkleProofLib-->>IthacaAccount: isProofValid
deactivate MerkleProofLib
alt isProofValid
IthacaAccount->>IthacaAccount: unwrapAndValidateSignature(root, rootSig)
activate IthacaAccount
IthacaAccount->>ECDSA: recoverCalldata(root, rootSig)
activate ECDSA
ECDSA-->>IthacaAccount: signerAddress
deactivate ECDSA
IthacaAccount-->>IthacaAccount: validate signerAddress
deactivate IthacaAccount
IthacaAccount-->>Caller: (isValid, keyHash)
else invalid proof
IthacaAccount-->>Caller: (false, 0x0)
end
deactivate IthacaAccount
else merkle flag not set
IthacaAccount->>ECDSA: recoverCalldata(digestOrPrehashed, innerSignature)
activate ECDSA
ECDSA-->>IthacaAccount: signerAddress
deactivate ECDSA
IthacaAccount-->>Caller: (isValid, keyHash)
end
deactivate IthacaAccount
Class diagram for updated IthacaAccount signature handlingclassDiagram
class IthacaAccount {
+unwrapAndValidateSignature(digest bytes32, signature bytes) bool isValid
+unwrapAndValidateSignature(digest bytes32, signature bytes) bytes32 keyHash
-_verifyMerkleSig(digest bytes32, signature bytes) bool isValid
-_verifyMerkleSig(digest bytes32, signature bytes) bytes32 keyHash
-getKey(keyHash bytes32) Key
}
class Key {
<<struct>>
}
class MerkleProofLib {
<<library>>
+verifyCalldata(proof bytes32[], root bytes32, leaf bytes32) bool
}
class LibBytes {
<<library>>
+loadCalldata(data bytes, index uint256) bytes32
+truncatedCalldata(data bytes, newLength uint256) bytes
}
class EfficientHashLib {
<<library>>
+sha2(data bytes32) bytes32
}
class ECDSA {
<<library>>
+recoverCalldata(digest bytes32, signature bytes) address
}
IthacaAccount ..> MerkleProofLib : uses
IthacaAccount ..> LibBytes : uses
IthacaAccount ..> EfficientHashLib : uses
IthacaAccount ..> ECDSA : uses
IthacaAccount o--> Key : returns
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
unwrapAndValidateSignature, theprehashandmerkleflags are read after truncatingsignatureto lengthn, so theloadCalldata(signature, n + 1)/n + 2accesses are out-of-bounds; compute the flags before truncating, or adjust the offsets to the new length. - The merkle signature wrapper format has effectively changed (an extra boolean/byte at the end), but
_verifyMerkleSigassumes the innersignatureis a cleanabi.encode(proof, root, rootSig)blob; consider validating or documenting (in-code) that no extra trailer bytes are present whenmerkleis set to avoid subtle encoding mismatch bugs in other callers. - The
.ideadirectory and related IDE project files have been added to the repo; these are typically local-only and should be removed from version control or added to.gitignore.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `unwrapAndValidateSignature`, the `prehash` and `merkle` flags are read *after* truncating `signature` to length `n`, so the `loadCalldata(signature, n + 1)` / `n + 2` accesses are out-of-bounds; compute the flags before truncating, or adjust the offsets to the new length.
- The merkle signature wrapper format has effectively changed (an extra boolean/byte at the end), but `_verifyMerkleSig` assumes the inner `signature` is a clean `abi.encode(proof, root, rootSig)` blob; consider validating or documenting (in-code) that no extra trailer bytes are present when `merkle` is set to avoid subtle encoding mismatch bugs in other callers.
- The `.idea` directory and related IDE project files have been added to the repo; these are typically local-only and should be removed from version control or added to `.gitignore`.
## Individual Comments
### Comment 1
<location path="src/IthacaAccount.sol" line_range="541-549" />
<code_context>
}
+ bool merkle;
unchecked {
- uint256 n = signature.length - 0x21;
+ uint256 n = signature.length - 0x22;
keyHash = LibBytes.loadCalldata(signature, n);
signature = LibBytes.truncatedCalldata(signature, n);
// Do the prehash if last byte is non-zero.
if (uint256(LibBytes.loadCalldata(signature, n + 1)) & 0xff != 0) {
digest = EfficientHashLib.sha2(digest); // `sha256(abi.encode(digest))`.
}
+ merkle = uint256(LibBytes.loadCalldata(signature, n + 2)) & 0xff != 0;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reading `prehash` and `merkle` after truncating `signature` is likely out-of-bounds / incorrect.
With the new layout `[inner][keyHash(32)][prehash(1)][merkle(1)]`, `n` is the start of `keyHash`. After loading `keyHash`, you truncate `signature` to length `n`, then read `prehash` and `merkle` at `n + 1` and `n + 2` using this truncated slice. Those indices are now past `signature.length`, so these loads will be out of bounds / revert. You should read `prehash` and `merkle` from the original `signature` before truncating, or compute their positions from the original offset and keep a separate variable for the inner-sig slice.
</issue_to_address>
### Comment 2
<location path="src/IthacaAccount.sol" line_range="552-553" />
<code_context>
+ merkle = uint256(LibBytes.loadCalldata(signature, n + 2)) & 0xff != 0;
+ }
+
+ if (merkle) {
+ return _verifyMerkleSig(digest, signature);
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Recursive use of `unwrapAndValidateSignature` via merkle root signatures can be abused for gas griefing if nesting is unbounded.
When `merkle` is true, `_verifyMerkleSig` calls `unwrapAndValidateSignature(root, rootSig)` again, allowing arbitrarily nested merkle signatures as long as each inner `rootSig` also sets `merkle = true`. An attacker can exploit this to create very deep nesting and inflate verification gas, leading to griefing or DoS for callers that must handle arbitrary signatures.
If unbounded nesting is not a required feature, consider either enforcing a maximum nesting depth (e.g., via a depth counter) or rejecting merkle-wrapped signatures inside another merkle verification to bound worst-case gas cost.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 implements Merkle signature verification within the IthacaAccount contract, enabling efficient validation of batched digests. The update includes a new internal verification function, modifications to the signature unwrapping logic to support a Merkle flag, and updated tests and benchmarks. Feedback identifies a potential underflow vulnerability when processing short signatures and a lack of bounds validation in the assembly-based decoding logic. Additionally, there is a documentation inconsistency regarding signature encoding, and it is advised to remove IDE-specific configuration files from the repository.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add Merkle tree–based signature support to IthacaAccount and update tests and helpers to exercise the new wrapped signature format and Merkle verification, including new benchmark coverage for ERC20 transfers via the orchestrator with passkeys.
New Features:
Enhancements:
Tests:
Chores: