Fix tests account#101
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
Remove executable bit from deploy/execute_config.sh, deploy/verify_config.sh, and prep/check-bytecode-changes.js (mode 100755 → 100644). Update lib/forge-std submodule from commit c2cf7017d27c1d20e74ace4dacb6c5ce4bbbe899 to 07853315f998f94dc724e464b1bab1270888ee64. No other content changes.
* Dargon789 legion rouge (#61) * feat: add merkle sigs natively into the account * fix: tests * test: add more sophisticated fuzz test * chore: bump contract versions due to bytecode changes - Contracts updated: IthacaAccount * Add .circleci/config.yml (#1) Add CircleCI configuration file to set up a basic pipeline with a say-hello job and workflow CI: Add .circleci/config.yml to define a say-hello job that checks out the code and prints a greeting using the cimg/base Docker image Add a workflow to orchestrate the say-hello job under the CircleCI 2.1 engine * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#2) CI: Update Forge test command to use --rerun and increase verbosity to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#4) Enhance the CI test step to automatically rerun failed tests and increase verbosity in Forge. CI: Enable the --rerun flag for Forge tests to retry failures automatically Increase Forge test verbosity from -vvv to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#5) Change Forge test invocation from "forge test --rerun -vvvvv" to "forge test -vvv" Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Create CNAME * Revert "Merge branch 'master'" This reverts commit 6c02fbf, reversing changes made to a317ddb. * Create CNAME (#9) * Update ci.yaml (#2) CI: Update Forge test command to use --rerun and increase verbosity to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#4) Enhance the CI test step to automatically rerun failed tests and increase verbosity in Forge. CI: Enable the --rerun flag for Forge tests to retry failures automatically Increase Forge test verbosity from -vvv to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#5) Change Forge test invocation from "forge test --rerun -vvvvv" to "forge test -vvv" Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Create CNAME * Revert "Merge branch 'master'" This reverts commit 6c02fbf, reversing changes made to a317ddb. --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Delete .circleci directory (#27) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * pre-commit * deploy execute_config.sh * Update forge-std * test: add tests for getContextKeyHash in Account.t.sol (ithacaxyz#412) * Normalize file modes and update forge-std submodule Remove executable bit from deploy/execute_config.sh, deploy/verify_config.sh, and prep/check-bytecode-changes.js (mode 100755 → 100644). Update lib/forge-std submodule from commit c2cf7017d27c1d20e74ace4dacb6c5ce4bbbe899 to 07853315f998f94dc724e464b1bab1270888ee64. No other content changes. * Update LayerZero-v2 * Update LayerZero-v2 --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: Tanishk Goyal <goyaltanishk02@gmail.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@google.com> * Ithaca (#58) * feat: add ERC20 transfer benchmark for Porto with passkey 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> * chore: use `auto-assign-pr.yml` org action * feat: add merkle sigs natively into the account * fix: tests * test: add more sophisticated fuzz test * chore: bump contract versions due to bytecode changes - Contracts updated: IthacaAccount * # Default ignored files * .snapshot_worktree * Delete .idea directory (#35) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Normalize file modes and update forge-std submodule Remove executable bit from deploy/execute_config.sh, deploy/verify_config.sh, and prep/check-bytecode-changes.js (mode 100755 → 100644). Update lib/forge-std submodule from commit c2cf7017d27c1d20e74ace4dacb6c5ce4bbbe899 to 07853315f998f94dc724e464b1bab1270888ee64. No other content changes. * Revert "Delete .idea directory (#35)" (#55) This reverts commit 512c204. --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tanishk Goyal <legion2002@users.noreply.github.com> Co-authored-by: o-az <omaraziz.dev@proton.me> Co-authored-by: Tanishk Goyal <goyaltanishk02@gmail.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@google.com> * Chore bump contract versions due to bytecode changes (#60) * feat: add merkle sigs natively into the account * fix: tests * test: add more sophisticated fuzz test * chore: bump contract versions due to bytecode changes - Contracts updated: IthacaAccount * pre-commit * deploy execute_config.sh * Update forge-std * Normalize file modes and update forge-std submodule Remove executable bit from deploy/execute_config.sh, deploy/verify_config.sh, and prep/check-bytecode-changes.js (mode 100755 → 100644). Update lib/forge-std submodule from commit c2cf7017d27c1d20e74ace4dacb6c5ce4bbbe899 to 07853315f998f94dc724e464b1bab1270888ee64. No other content changes. * Update LayerZero-v2 * Update LayerZero-v2 --------- Co-authored-by: Tanishk Goyal <goyaltanishk02@gmail.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@google.com> * Legion rouge (#63) * feat: add merkle sigs natively into the account * fix: tests * test: add more sophisticated fuzz test * chore: bump contract versions due to bytecode changes - Contracts updated: IthacaAccount * Add .circleci/config.yml (#1) Add CircleCI configuration file to set up a basic pipeline with a say-hello job and workflow CI: Add .circleci/config.yml to define a say-hello job that checks out the code and prints a greeting using the cimg/base Docker image Add a workflow to orchestrate the say-hello job under the CircleCI 2.1 engine * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#2) CI: Update Forge test command to use --rerun and increase verbosity to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#4) Enhance the CI test step to automatically rerun failed tests and increase verbosity in Forge. CI: Enable the --rerun flag for Forge tests to retry failures automatically Increase Forge test verbosity from -vvv to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#5) Change Forge test invocation from "forge test --rerun -vvvvv" to "forge test -vvv" Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Create CNAME * Revert "Merge branch 'master'" This reverts commit 6c02fbf, reversing changes made to a317ddb. * Create CNAME (#9) * Update ci.yaml (#2) CI: Update Forge test command to use --rerun and increase verbosity to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#4) Enhance the CI test step to automatically rerun failed tests and increase verbosity in Forge. CI: Enable the --rerun flag for Forge tests to retry failures automatically Increase Forge test verbosity from -vvv to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#5) Change Forge test invocation from "forge test --rerun -vvvvv" to "forge test -vvv" Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Create CNAME * Revert "Merge branch 'master'" This reverts commit 6c02fbf, reversing changes made to a317ddb. --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Delete .circleci directory (#27) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * test: add tests for getContextKeyHash in Account.t.sol (ithacaxyz#412) * Normalize file modes and update forge-std submodule Remove executable bit from deploy/execute_config.sh, deploy/verify_config.sh, and prep/check-bytecode-changes.js (mode 100755 → 100644). Update lib/forge-std submodule from commit c2cf7017d27c1d20e74ace4dacb6c5ce4bbbe899 to 07853315f998f94dc724e464b1bab1270888ee64. No other content changes. * Update LayerZero-v2 --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: Tanishk Goyal <goyaltanishk02@gmail.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@google.com> * feat: add merkle sigs natively into the account legion rouge (#70) * feat: add merkle sigs natively into the account * fix: tests * test: add more sophisticated fuzz test * chore: bump contract versions due to bytecode changes - Contracts updated: IthacaAccount * Add .circleci/config.yml (#1) Add CircleCI configuration file to set up a basic pipeline with a say-hello job and workflow CI: Add .circleci/config.yml to define a say-hello job that checks out the code and prints a greeting using the cimg/base Docker image Add a workflow to orchestrate the say-hello job under the CircleCI 2.1 engine * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#2) CI: Update Forge test command to use --rerun and increase verbosity to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#4) Enhance the CI test step to automatically rerun failed tests and increase verbosity in Forge. CI: Enable the --rerun flag for Forge tests to retry failures automatically Increase Forge test verbosity from -vvv to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#5) Change Forge test invocation from "forge test --rerun -vvvvv" to "forge test -vvv" Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Create CNAME * Revert "Merge branch 'master'" This reverts commit 6c02fbf, reversing changes made to a317ddb. * Create CNAME (#9) * Update ci.yaml (#2) CI: Update Forge test command to use --rerun and increase verbosity to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#4) Enhance the CI test step to automatically rerun failed tests and increase verbosity in Forge. CI: Enable the --rerun flag for Forge tests to retry failures automatically Increase Forge test verbosity from -vvv to -vvvvv Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml (#5) Change Forge test invocation from "forge test --rerun -vvvvv" to "forge test -vvv" Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update ci.yaml Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Create CNAME * Revert "Merge branch 'master'" This reverts commit 6c02fbf, reversing changes made to a317ddb. --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Delete .circleci directory (#27) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * pre-commit * deploy execute_config.sh * Update forge-std * test: add tests for getContextKeyHash in Account.t.sol (ithacaxyz#412) * Normalize file modes and update forge-std submodule Remove executable bit from deploy/execute_config.sh, deploy/verify_config.sh, and prep/check-bytecode-changes.js (mode 100755 → 100644). Update lib/forge-std submodule from commit c2cf7017d27c1d20e74ace4dacb6c5ce4bbbe899 to 07853315f998f94dc724e464b1bab1270888ee64. No other content changes. * Update LayerZero-v2 * Update LayerZero-v2 --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: Tanishk Goyal <goyaltanishk02@gmail.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@google.com> * Potential fix for code scanning alert no. 3: Workflow does not contain permissions (#66) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Potential fix for code scanning alert no. 2: Workflow does not contain permissions (#67) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Potential fix for code scanning alert no. 1: Workflow does not contain permissions (#68) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Ithaca (#80) * feat: add ERC20 transfer benchmark for Porto with passkey 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> * chore: use `auto-assign-pr.yml` org action * feat: add merkle sigs natively into the account * fix: tests * test: add more sophisticated fuzz test * chore: bump contract versions due to bytecode changes - Contracts updated: IthacaAccount * # Default ignored files * .snapshot_worktree * Delete .idea directory (#35) Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Normalize file modes and update forge-std submodule Remove executable bit from deploy/execute_config.sh, deploy/verify_config.sh, and prep/check-bytecode-changes.js (mode 100755 → 100644). Update lib/forge-std submodule from commit c2cf7017d27c1d20e74ace4dacb6c5ce4bbbe899 to 07853315f998f94dc724e464b1bab1270888ee64. No other content changes. * Revert "Delete .idea directory (#35)" (#55) This reverts commit 512c204. --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tanishk Goyal <legion2002@users.noreply.github.com> Co-authored-by: o-az <omaraziz.dev@proton.me> Co-authored-by: Tanishk Goyal <goyaltanishk02@gmail.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@google.com> * Delete CNAME * ci(Mergify): configuration update (#81) Signed-off-by: Dargon789 <null> * Revert "Ithaca (#58)" This reverts commit 0cb7a77. --------- Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Signed-off-by: Dargon789 <null> Co-authored-by: Tanishk Goyal <goyaltanishk02@gmail.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@google.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tanishk Goyal <legion2002@users.noreply.github.com> Co-authored-by: o-az <omaraziz.dev@proton.me> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
clear && forge fmt && forge snapshot --isolate --match-contract Benchmark --via-ir && git add snapshots
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideAdds Merkle tree–based signature verification support and corresponding tests, updates the wrapped signature format to include a merkle flag across helpers and tests, tightens CI workflows, and introduces minor version and workflow configuration updates. Sequence diagram for wrapped and Merkle-based signature validationsequenceDiagram
actor User
participant ExternalCaller
participant IthacaAccount
participant MerkleProofLib
User ->> ExternalCaller : submitTransaction(digest, signature)
ExternalCaller ->> IthacaAccount : unwrapAndValidateSignature(digest, signature)
alt signature_length_is_65
IthacaAccount ->> IthacaAccount : ECDSA.recoverCalldata(digest, signature)
IthacaAccount -->> ExternalCaller : isValid, keyHash=0
else wrapped_signature
IthacaAccount ->> IthacaAccount : parse keyHash, prehashFlag, merkleFlag
alt prehashFlag_true
IthacaAccount ->> IthacaAccount : digest = EfficientHashLib.sha2(digest)
end
alt merkleFlag_true
IthacaAccount ->> IthacaAccount : _verifyMerkleSig(digest, innerSignature)
IthacaAccount ->> MerkleProofLib : verifyCalldata(proof, root, digest)
alt merkle_proof_valid
IthacaAccount ->> IthacaAccount : unwrapAndValidateSignature(root, rootSig)
IthacaAccount -->> ExternalCaller : isValid, keyHash_from_rootSig
else merkle_proof_invalid
IthacaAccount -->> ExternalCaller : isValid=false, keyHash=0
end
else merkleFlag_false
IthacaAccount ->> IthacaAccount : getKey(keyHash) and validate innerSignature
IthacaAccount -->> ExternalCaller : isValid, keyHash
end
end
Class diagram for updated IthacaAccount signature handlingclassDiagram
class IthacaAccount {
+unwrapAndValidateSignature(digest, signature)
-_verifyMerkleSig(digest, signature)
+getDomainDetails()
}
class MerkleProofLib
class LibBytes
class EfficientHashLib
class ECDSA
IthacaAccount ..> MerkleProofLib : uses
IthacaAccount ..> LibBytes : uses
IthacaAccount ..> EfficientHashLib : uses
IthacaAccount ..> ECDSA : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ 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. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
unwrapAndValidateSignature, you truncatesignaturebefore reading the prehash and merkle flag bytes (LibBytes.truncatedCalldata(signature, n)thenloadCalldata(signature, n + 1/2)), which means those loads are out of bounds / reading from the wrong slice; you likely want to read both flags before truncating the calldata view. - The custom ABI decoding in
_verifyMerkleSigassumes a very specificabi.encode(proof, root, rootSig)layout; consider adding an explicit length/offset sanity check (e.g. minimumsignature.lengthand ensuring offsets are within bounds) so that future changes to the inner encoding fail loudly instead of mis-decoding.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `unwrapAndValidateSignature`, you truncate `signature` before reading the prehash and merkle flag bytes (`LibBytes.truncatedCalldata(signature, n)` then `loadCalldata(signature, n + 1/2)`), which means those loads are out of bounds / reading from the wrong slice; you likely want to read both flags before truncating the calldata view.
- The custom ABI decoding in `_verifyMerkleSig` assumes a very specific `abi.encode(proof, root, rootSig)` layout; consider adding an explicit length/offset sanity check (e.g. minimum `signature.length` and ensuring offsets are within bounds) so that future changes to the inner encoding fail loudly instead of mis-decoding.
## Individual Comments
### Comment 1
<location path="src/IthacaAccount.sol" line_range="525" />
<code_context>
/// @dev Returns if the signature is valid, along with its `keyHash`.
/// The `signature` is a wrapped signature, given by
- /// `abi.encodePacked(bytes(innerSignature), bytes32(keyHash), bool(prehash))`.
+ /// `abi.encode(bytes(innerSignature), bytes32(keyHash), bool(prehash), bool(merkle))`.
function unwrapAndValidateSignature(bytes32 digest, bytes calldata signature)
public
</code_context>
<issue_to_address>
**issue (bug_risk):** The documented encoding (`abi.encode`) doesn’t match how the bytes are parsed (as if they were packed).
`unwrapAndValidateSignature` decodes `signature` as if it ends with a fixed `[keyHash (32)][prehash (1)][merkle (1)]` tail at `length - 0x22`, which corresponds to `abi.encodePacked(innerSignature, keyHash, prehash, merkle)`. With `abi.encode(...)`, these fields would be placed according to the ABI (with dynamic offsets/padding), so the current indexing would be wrong and decoding would fail. Please either revert to `abi.encodePacked` (and update call sites accordingly) or change the decoding logic to follow the ABI-encoded layout so signatures are interpreted correctly.
</issue_to_address>
### Comment 2
<location path="src/IthacaAccount.sol" line_range="542-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):** Flag offsets and truncation order in `unwrapAndValidateSignature` look incorrect for the intended layout.
For the packed layout `[...innerSignature..., keyHash (32), prehash (1), merkle (1)]`, the correct offsets are:
- `keyHash`: `length - 34`
- `prehash`: `length - 2`
- `merkle`: `length - 1`
With `n = length - 0x22`, `keyHash` is correct, but:
- `prehash` at `n + 1 = length - 0x21` matches the old 33-byte trailer layout, not the second‑to‑last byte of a 34‑byte trailer.
- `merkle` at `n + 2 = length - 0x20` is 32 bytes before `prehash`, so it never reads the last byte.
You’re also truncating to `n` before reading `n + 1`/`n + 2`, which implicitly relies on `LibBytes.loadCalldata` ignoring `length`. To avoid subtle bugs, read flags using `length - 2` and `length - 1` before truncation, then truncate once to remove the 34‑byte trailer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces Merkle signature verification to the IthacaAccount contract, enabling the validation of standard digests against a Merkle root. The signature format has been updated to include a Merkle flag, and corresponding test cases and benchmarks have been added. Feedback focuses on critical safety issues in the signature parsing logic, specifically a missing length check that could lead to underflows and out-of-bounds reads. Additionally, it is recommended to use abi.decode instead of manual assembly for better maintainability and to correct documentation that misidentifies the signature encoding as abi.encode instead of abi.encodePacked.
I am having trouble creating individual review comments. Click here to see my feedback.
src/IthacaAccount.sol (541-550)
The signature parsing logic has a few issues:
- Missing Length Check: The calculation
signature.length - 0x22inside theuncheckedblock will underflow if the signature is shorter than 34 bytes (and not exactly 65 bytes), leading to out-of-bounds reads. A length check should be added. - Confusing Byte Extraction: Using
LibBytes.loadCalldata(signature, n + 1)andn + 2to extract single bytes by relying on the last byte of a 32-byte word is obscure. Also, these reads occur after thesignaturehas been truncated, which is brittle and relies on the underlying library not enforcing slice bounds. - Documentation Mismatch: The docstring for
unwrapAndValidateSignaturewas updated to mentionabi.encode, but the implementation still expects a packed format (innerSignaturefollowed by a fixed-size suffix). It should remainabi.encodePackedin the documentation to avoid confusion.
It is recommended to validate the length and extract all suffix components (keyHash, prehash, and merkle flags) before truncating the signature.
if (signature.length < 0x22) return (false, bytes32(0));
bool merkle;
unchecked {
uint256 n = signature.length - 0x22;
keyHash = LibBytes.loadCalldata(signature, n);
// Extract flags before truncation for clarity and safety.
if (uint8(signature[n + 32]) != 0) {
digest = EfficientHashLib.sha2(digest);
}
merkle = uint8(signature[n + 33]) != 0;
signature = LibBytes.truncatedCalldata(signature, n);
}
src/IthacaAccount.sol (498-512)
Using manual assembly to decode ABI-encoded data is error-prone and lacks the automatic bounds checks provided by the Solidity compiler. Since the signature is expected to be abi.encode(bytes32[] proof, bytes32 root, bytes rootSig), it is safer and more maintainable to use abi.decode. This approach automatically handles offset validation and ensures that the decoded slices stay within the bounds of the input signature.
(bytes32[] calldata proof, bytes32 root, bytes calldata rootSig) =
abi.decode(signature, (bytes32[], bytes32, bytes));
Summary by Sourcery
Add Merkle-based signature support to IthacaAccount and update tests, benchmarks, and CI/workflows accordingly.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests:
Chores:
accountmerge queue and commit various IDE and hook configuration files.