Add support for other ownable key systems including P256.#1
Add support for other ownable key systems including P256.#1
Conversation
* Implement Reednaa's Stowaway * fmt * Remove embedded calls and replace by storage config.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/Catapultar.sol
Outdated
| asUnsafeBytes32(address(this)), | ||
| hash | ||
| ); | ||
| if (signature.length == 0 && approvedDigest[digest] == type(uint256).max) return bytes4(0xffffffff); |
Check notice
Code scanning / Olympix Integrated Security
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
WalkthroughReplace single-address ownership with a KeyedOwnable multi-key model; add per-digest approvals and digest-based deploy/signature flows; update factory APIs and salt derivation to accept KeyType+keys; integrate Stowaway submodule and fallback; adjust nonce/Calls layout; update docs, tests, and snapshots. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Factory as CatapultarFactory
participant Deployer as CREATE2/Cloner
participant Proxy as Catapultar
participant KeyStore as KeyedOwnable
participant Stowaway
User->>Factory: deployWithDigest(ktp, owner[], salt, digest, isSignature)
Factory->>Factory: compute _saltPrefix(ktp, owner[]) & ownerInSalt check
Factory->>Deployer: create2/clone using saltWithDigest
Deployer->>Proxy: deploy proxy
Factory->>Proxy: init(ktp, owner[]) (forwards msg.value)
Proxy->>KeyStore: store owner key slice / set ownerKeyType
Factory->>Proxy: setSignature(digest, DigestApproval)
User->>Proxy: submit tx referencing digest
Proxy->>Proxy: isValidSignature — short-circuit if approvedDigest==Signature
alt not pre-approved
Proxy->>KeyStore: validate signature via KeyedOwnable
end
Proxy->>Stowaway: fallback -> searchAndCall(ERC7821.execute.selector)
Stowaway-->>Proxy: result (call performed / none)
alt no Stowaway result
Proxy->>Proxy: LibZip.cdFallback()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
| ) external initializer { | ||
| _initializeOwner(owner); | ||
| if (ALLOW_EMBEDDED_CALLS && !_notUpgradeable()) revert CannotBeUpgradeable(); | ||
| function setSignature( |
Check failure
Code scanning / Olympix Integrated Security
The contract contains functions with inadequate validation of input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/insufficient-parameter-assertion
| /// @param salt The first 20 bytes of salt has to be the owner or 0. | ||
| function deploy(address owner, bytes32 salt) external ownerInSalt(salt, owner) returns (address proxy) { | ||
| proxy = LibClone.cloneDeterministic_PUSH0(address(EXECUTOR_NO_EMBEDDED_CALLS), salt); | ||
| function deploy( |
Check failure
Code scanning / Olympix Integrated Security
The contract contains functions with inadequate validation of input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/insufficient-parameter-assertion
| /// @param salt The first 20 bytes of salt has to be the owner or 0. | ||
| function deployUpgradeable(address owner, bytes32 salt) external ownerInSalt(salt, owner) returns (address proxy) { | ||
| proxy = LibClone.deployDeterministicERC1967(address(EXECUTOR_NO_EMBEDDED_CALLS), salt); | ||
| function deployUpgradeable( |
Check failure
Code scanning / Olympix Integrated Security
The contract contains functions with inadequate validation of input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/insufficient-parameter-assertion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
.gitmodules(1 hunks)README.md(2 hunks)lib/stowaway(1 hunks)remappings.txt(1 hunks)snapshots/CatapultarCallsTest.json(1 hunks)snapshots/CatapultarFactoryTest.json(1 hunks)snapshots/CatapultarMinimalTest.json(1 hunks)snapshots/CatapultarUpgradeableTest.json(1 hunks)src/Catapultar.sol(9 hunks)src/CatapultarFactory.sol(5 hunks)src/libs/BitmapNonce.sol(1 hunks)src/libs/ERC7821LIFI.sol(4 hunks)src/libs/KeyedOwnable.sol(1 hunks)test/Catapultar/Catapultar.base.t.sol(15 hunks)test/Catapultar/Catapultar.calls.t.sol(0 hunks)test/Catapultar/Catapultar.minimal.t.sol(1 hunks)test/Catapultar/Catapultar.upgradable.t.sol(1 hunks)test/CatapultarFactory.t.sol(3 hunks)test/Integration.t.sol(9 hunks)test/libs/ERC7821LIFI.t.sol(2 hunks)test/libs/LibCalls.t.sol(1 hunks)test/mocks/MockCatapultar.sol(1 hunks)test/mocks/MockERC20.sol(2 hunks)test/solady/ERC7821.t.sol(1 hunks)test/solady/mocks/MockERC7821.sol(2 hunks)
💤 Files with no reviewable changes (1)
- test/Catapultar/Catapultar.calls.t.sol
🧰 Additional context used
🪛 GitHub Actions: CI
src/libs/KeyedOwnable.sol
[error] 262-262: forge fmt --check: formatting mismatch detected. This is simply abi.decode(signature, (WebAuthn.WebAuthnAuth)).
src/libs/ERC7821LIFI.sol
[error] 151-151: forge fmt --check: formatting mismatch detected. Compute the padded length for ABI alignment. (rdsize + rdsize % 32)
🪛 GitHub Check: Olympix Integrated Security
src/Catapultar.sol
[failure] 106-106:
The contract contains functions with inadequate validation of input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/insufficient-parameter-assertion
src/CatapultarFactory.sol
[failure] 46-46:
The contract contains functions with inadequate validation of input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/insufficient-parameter-assertion
[failure] 95-95:
The contract contains functions with inadequate validation of input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/insufficient-parameter-assertion
🪛 LanguageTool
README.md
[grammar] ~185-~185: Ensure spelling is correct
Context: ...` call into a bytes field which will be delievered to the account. You can optionally LibZ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~191-~191: Ensure spelling is correct
Context: ... bypassed if called from itself: - The exeute endpoint does not require a signature I...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~192-~192: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...f the underlying SCA if the contract is upgradable. - The SCA can upgrade the contract own...
(EN_WORD_COHERENCY)
[style] ~195-~195: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...es themselves are not structured. It is very important to parse and validate that ALL sign...
(EN_WEAK_ADJECTIVE)
[grammar] ~210-~210: Use a hyphen to join words.
Context: ... remember that token allowances are long lived. If a token allowance is set on a ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
204-204: Hard tabs
Column: 80
(MD010, no-hard-tabs)
⏰ 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). (1)
- GitHub Check: Security Check
🔇 Additional comments (26)
test/libs/ERC7821LIFI.t.sol (2)
64-96: Function signature and call formatting improvements look good.The parameter list split across multiple lines (64–66) and the
abi.encodeWithSignaturearguments split across lines (73–74) improve readability and maintain consistency with updated test style across the suite. No logic changes; test behavior remains identical.
122-128: Commented test block reformatting is clean.The commented-out test maintains proper indentation and structure. Readability improves without affecting any active test logic.
remappings.txt (1)
3-3: LGTM!The remapping correctly aligns with the Stowaway submodule addition in
.gitmodulesand follows the standard remapping pattern.test/solady/ERC7821.t.sol (1)
97-100: LGTM!The multiline function signature formatting improves readability without altering semantics.
snapshots/CatapultarUpgradeableTest.json (1)
4-7: Verify the gas cost increases are expected.All ERC-20 transfer operations show increased gas costs:
- Single transfers: +372 gas
- Double transfers: +501 gas
These increases likely result from the KeyedOwnable integration and updated signature validation mechanisms. Please confirm these overhead costs are acceptable for the added functionality.
test/mocks/MockCatapultar.sol (2)
7-7: LGTM!The simplified constructor signature aligns with the refactored base
Catapultarinitialization mechanism described in the PR.
9-13: LGTM!The multiline function signature formatting improves readability without altering semantics.
snapshots/CatapultarCallsTest.json (1)
4-7: Verify the gas cost increases are expected.All ERC-20 transfer operations show increased gas costs:
- Single transfers: +329 gas
- Double transfers: +458 gas
These increases likely result from the KeyedOwnable integration and updated signature validation mechanisms. Please confirm these overhead costs are acceptable for the added functionality.
test/mocks/MockERC20.sol (1)
15-19: LGTM!The multiline function signature formatting throughout the file improves readability without altering semantics or behavior.
Also applies to: 42-45, 49-52, 56-60, 64-68, 72-75, 79-83
README.md (2)
183-211: Excellent security documentation!The Usage Warnings section provides critical security guidance about the self-calling bypass mechanism and potential ERC-1271 attack vectors. This documentation is essential for users to understand the security implications of batch operations.
248-248: LGTM!The Stowaway library is correctly added to the license notice section.
.gitmodules (1)
7-9: Verify trust before integrating this new submodule dependency.The Stowaway repository (created 2025-10-04, pushed only once) lacks indicators of established maintenance or security review. With 0 stars and an unknown maintainer "reednaa," this dependency has no community validation. Before merging, manually confirm:
- The maintainer's identity and trustworthiness
- Whether this code has undergone security review
- Your organization's risk tolerance for unproven dependencies in your build/deployment pipeline
test/libs/LibCalls.t.sol (1)
40-44: LGTM! Formatting improvement.The multiline parameter layout improves readability without altering behavior.
test/solady/mocks/MockERC7821.sol (2)
34-40: LGTM! Formatting improvement.The multiline parameter layout improves readability without altering behavior.
51-56: LGTM! Formatting improvement.The multiline parameter layout improves readability.
test/Catapultar/Catapultar.upgradable.t.sol (2)
7-7: LGTM! Import added for KeyedOwnable support.The import aligns with the PR's multi-key ownership refactoring.
14-14: LGTM! Constructor updated to parameterless form.This change aligns with the removal of the embedded calls parameter from MockCatapultar, consistent with the PR's refactoring away from embedded-call mechanisms.
snapshots/CatapultarFactoryTest.json (1)
2-4: LGTM! Gas snapshots updated.The increased gas values reflect the KeyedOwnable-based ownership model introduced in this PR. The changes are expected and reasonable.
snapshots/CatapultarMinimalTest.json (1)
4-7: LGTM! Gas snapshots updated.The gas increases are consistent across operations and reflect the new signature validation mechanisms introduced with KeyedOwnable. The pattern of increases is logical.
test/Catapultar/Catapultar.minimal.t.sol (1)
11-11: LGTM! Constructor updated to parameterless form.This change is consistent with the removal of the embedded calls parameter from MockCatapultar across all test files.
test/Integration.t.sol (5)
11-11: LGTM! KeyedOwnable import added.This import enables the test to use the new multi-key ownership model.
62-65: LGTM! Migration to KeyedOwnable ownership model.The deployment now uses
KeyedOwnable.KeyType.ECDSAOrSmartContractwith a keys array instead of a direct owner address. This properly demonstrates the new multi-key ownership API introduced in this PR.
87-130: LGTM! Nonce values updated to start at 1.The encoded nonce/index values now range from 1-6 instead of (presumably) 0-5. This change coordinates with the new guard in
BitmapNonce.solthat disallows nonce 0, ensuring test consistency across the refactoring.
197-247: LGTM! Test expectations updated for new nonce values.The
assembleExtraDatacalls have been updated to reflect the new nonce values (3-6), maintaining consistency with the encoded call data above and the BitmapNonce zero guard.
255-272: LGTM! Formatting improvements.The multiline parameter layouts improve readability without altering behavior.
src/libs/BitmapNonce.sol (1)
26-31: Code change is well-tested and documented—no action needed.Verification confirms:
- ✓ NatSpec documentation present:
/// @dev Disallows nonce 0(line 26)- ✓ Guard implemented:
if (nonce == 0) revert InvalidNonce();(line 31)- ✓ Explicit test coverage:
test_useUnorderedNonce()in Catapultar.base.t.sol expects revert for nonce 0- ✓ Fuzz tests exclude nonce 0 with
vm.assumeconditionsThe nonce 0 restriction is properly documented in code comments and thoroughly tested. This is an appropriate implementation detail.
| @@ -0,0 +1 @@ | |||
| Subproject commit bf862027851229fa6b00cdf625872a6f2f74b3a6 | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify the submodule commit is pinned to a stable, documented version.
Adding an external submodule introduces a supply-chain dependency. Ensure the commit hash is:
- Tagged or documented in the Stowaway repository (e.g., as a release or version tag)
- From a maintained, trustworthy source (reednaa/stowaway)
- Compatible with the Catapultar architecture and the new KeyedOwnable/ERC7821 systems
To verify the commit and integration, run the following script:
Additionally, verify that the Stowaway library integrates correctly with the KeyedOwnable and ERC7821 systems by confirming:
- The fallback routing via
Stowaway.searchAndCalldoes not conflict with signature validation - The interaction with embedded-call mechanisms and LibZip is well-tested
🏁 Script executed:
#!/bin/bash
# Description: Verify the Stowaway submodule commit is tagged/documented and check repository health.
# 1. Clone the Stowaway repository to inspect the commit
git clone https://github.com/reednaa/stowaway.git /tmp/stowaway_check 2>/dev/null || true
# 2. Check if commit exists and retrieve commit metadata
cd /tmp/stowaway_check 2>/dev/null && \
git show bf862027851229fa6b00cdf625872a6f2f74b3a6 --no-patch --format="%H %ai %s" 2>/dev/null || \
echo "Commit not found or repository unavailable"
# 3. Check if commit is tagged
cd /tmp/stowaway_check 2>/dev/null && \
git describe --exact-match --tags bf862027851229fa6b00cdf625872a6f2f74b3a6 2>/dev/null || \
echo "Commit is not tagged"
# 4. Check repository metadata (stars, last update)
curl -s https://api.github.com/repos/reednaa/stowaway | jq '{full_name, description, stars: .stargazers_count, forks: .forks_count, last_push: .pushed_at}'
# Cleanup
rm -rf /tmp/stowaway_checkLength of output: 324
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if any tags exist in the Stowaway repository and list them
git clone https://github.com/reednaa/stowaway.git /tmp/stowaway_tags 2>/dev/null || true
cd /tmp/stowaway_tags 2>/dev/null && {
echo "=== All tags in repository ===" && \
git tag -l || echo "No tags found"
echo -e "\n=== Repository commit history (last 10 commits) ===" && \
git log --oneline -10 || echo "Cannot retrieve history"
echo -e "\n=== Repository age (first commit) ===" && \
git log --oneline --reverse | head -1 || echo "Cannot determine"
}
rm -rf /tmp/stowaway_tags
Length of output: 244
Do not merge without addressing the supply-chain risk of this unversioned dependency.
The Stowaway repository has:
- No release tags or versioning (zero tags exist)
- Only 4 commits spanning ~5 weeks
- Zero stars, zero forks, zero adoption
- Pinned commit is untagged, arbitrary
Pinning to an untagged commit of a brand-new, unadopted library introduces material supply-chain risk. The maintainer can delete the repository, rewrite history, or abandon the project without warning.
Required before merge:
- Request the Stowaway maintainer create a release tag (e.g.,
v0.1.0) for this commit - Update the submodule to reference the tag instead of a raw commit hash, or
- Re-evaluate whether this dependency is production-ready and warranted
If the library is critical and the maintainer is unresponsive, consider inlining the functionality to avoid external dependency risk.
🤖 Prompt for AI Agents
In lib/stowaway around lines 1 to 1, the PR pins an unversioned submodule commit
from a very new/unadopted repository which creates a supply-chain risk; request
the upstream maintainer to create a release tag (e.g., v0.1.0) for the current
commit and update the submodule to reference that tag instead of the raw commit
hash, or alternatively remove/replace the dependency (inline the needed code or
choose a vetted library) if the maintainer is unresponsive or the project is not
production-ready; ensure the submodule update and commit reference use the tag
or document the justification and risk acceptance in the repo if you must
temporarily keep the untagged pin.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/libs/KeyedOwnable.sol (1)
274-283: Fix dirty-address detection before returning owner.This is the same critical issue flagged in the previous review. The assembly block computes
dirtyfromaddrbeforeaddris assigned fromelem, sodirtyalways remains false and theDirtyEthereumAddresscheck never triggers. When a non-ECDSA key (e.g., P256) is stored,owner()will silently return a truncated address instead of reverting.Additionally, there's a typo in the comment on line 279: "addres" should be "address".
Apply this diff to fix both issues:
assembly ("memory-safe") { - // Shift away 20 bytes of the addres. + // Shift away 20 bytes of the address. // Check if not 0. - dirty := iszero(eq(shr(mul(8, 20), addr), 0)) + dirty := iszero(eq(shr(mul(8, 20), elem), 0)) addr := elem }
🧹 Nitpick comments (1)
src/libs/KeyedOwnable.sol (1)
12-12: Remove unused debug import.The
consoleimport from forge-std is a testing/debugging utility and should not be present in production contract code. There are noconsole.logcalls in this file, making this import unused.Apply this diff to remove the unused import:
-import { console } from "forge-std/console.sol"; - /**
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/libs/KeyedOwnable.sol(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
src/libs/KeyedOwnable.sol
[error] 262-262: forge fmt --check failed: formatting issues detected in the diff (comment alignment for abi.decode statement).
⏰ 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). (1)
- GitHub Check: Security Check
🔇 Additional comments (5)
src/libs/KeyedOwnable.sol (5)
35-42: LGTM: Storage layout is well-designed.The storage slot is intentionally chosen to overlap with Solady's Ownable for upgrade compatibility, which is a thoughtful design decision. The comment clearly explains the rationale and edge case handling.
48-79: LGTM: Key storage implementation is correct.The key slice storage and retrieval functions correctly use assembly for efficient storage slot access. The comments appropriately document the lack of bounds checking, which is acceptable since getOwnerKey properly bounds reads using ownerKeyType.
99-128: LGTM: Access control logic is correct.The owner() function correctly validates that the stored value is a clean address. The onlyOwnerOrSelf modifier appropriately restricts access to either the contract itself or the stored owner address, with correct handling for non-ECDSA key types (which would fail the equality check due to dirty upper bits).
130-180: LGTM: Key validation logic is sound.The validation correctly enforces constraints for each key type:
- ECDSAOrSmartContract: clean 20-byte address (upper bits zero, lower bits non-zero)
- P256/WebAuthnP256: both coordinate words must be non-zero
187-227: LGTM: Ownership transfer implementation is correct.The ownership transfer functions properly validate keys, update storage, and emit events. The design intentionally does not clear old key slices when transferring to shorter keys, which is acceptable since all read operations use ownerKeyType to determine the valid slice count.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
42-60: Update the Integration Example to the new factory and execute signatures.The “Account Deployment” section documents
factory.deploy(ktp, owner, salt), but the later Integration Example still showsfactory.deploy(owner, salt)and an olderproxy.execute(...)shape. Please update that example to useKeyedOwnable.KeyTypeplus abytes32[]keys array and the current execute/opData wiring so README users don’t copy a stale interface.Also applies to: 165-179
♻️ Duplicate comments (2)
README.md (1)
62-68: Clean up remaining typos and wording in the Stowaway / warnings docs.There are a few small issues that were already noted previously but are still present:
- “delievered” → “delivered”.
- “exeute” → “execute”.
- “upgradable” vs “Upgradeable” – pick one spelling and use it consistently (the table row/header vs later bullet currently differ).
- “long lived” → “long‑lived”.
- Optional: remove hard tabs in lists/tables to satisfy markdownlint.
These are non‑functional, but worth tightening given this section is security‑sensitive documentation.
Also applies to: 183-211, 248-249
src/libs/KeyedOwnable.sol (1)
78-84: Correct small NatSpec typo in_keyTypeLengthcomment.The comment currently says “occopy”; please change it to “occupy”:
- /// @notice Returns the number of words that a key should occopy. + /// @notice Returns the number of words that a key should occupy.
🧹 Nitpick comments (1)
test/CatapultarFactory.t.sol (1)
24-26: Consider extracting the keys construction to a helper function.The pattern
keys[0] = bytes32(uint256(uint160(owner)))is repeated in every test function. Extracting it to a helper would improve maintainability.Apply this diff to add a helper function:
+ function _makeKeys(address owner) internal pure returns (bytes32[] memory keys) { + keys = new bytes32[](1); + keys[0] = bytes32(uint256(uint160(owner))); + } + /// forge-config: default.isolate = true function test_deploy() external { address owner = makeAddr("owner"); bytes32 salt = bytes32(bytes20(uint160(owner))); - bytes32[] memory keys = new bytes32[](1); - keys[0] = bytes32(uint256(uint160(owner))); + bytes32[] memory keys = _makeKeys(owner);Then update all test functions to use
_makeKeys(owner).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
README.md(4 hunks)script/deploy.s.sol(2 hunks)snapshots/CatapultarFactoryTest.json(1 hunks)snapshots/CatapultarMinimalTest.json(1 hunks)snapshots/CatapultarUpgradeableTest.json(1 hunks)src/Catapultar.sol(9 hunks)src/CatapultarFactory.sol(4 hunks)src/libs/BitmapNonce.sol(1 hunks)src/libs/KeyedOwnable.sol(1 hunks)src/libs/LibCalls.sol(1 hunks)test/Catapultar/Catapultar.base.t.sol(16 hunks)test/Catapultar/Catapultar.upgradable.t.sol(1 hunks)test/CatapultarFactory.t.sol(2 hunks)test/Integration.t.sol(9 hunks)test/mocks/MockERC20.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/Catapultar/Catapultar.upgradable.t.sol
- snapshots/CatapultarMinimalTest.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
test/Integration.t.soltest/CatapultarFactory.t.soltest/Catapultar/Catapultar.base.t.solsrc/Catapultar.solsrc/CatapultarFactory.sol
🪛 GitHub Actions: CI
src/libs/KeyedOwnable.sol
[error] 259-259: Formatting differences detected by forge fmt --check in KeyedOwnable.sol. Run 'forge fmt' to auto-format.
🪛 LanguageTool
README.md
[uncategorized] ~64-~64: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...--------------: | :---------------: | | Upgradable | No | No...
(EN_WORD_COHERENCY)
[grammar] ~185-~185: Ensure spelling is correct
Context: ...` call into a bytes field which will be delievered to the account. You can optionally LibZ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~191-~191: Ensure spelling is correct
Context: ... bypassed if called from itself: - The exeute endpoint does not require a signature I...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~192-~192: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...f the underlying SCA if the contract is upgradable. - The SCA can upgrade the contract own...
(EN_WORD_COHERENCY)
[style] ~195-~195: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...es themselves are not structured. It is very important to parse and validate that ALL sign...
(EN_WEAK_ADJECTIVE)
[grammar] ~210-~210: Use a hyphen to join words.
Context: ... remember that token allowances are long lived. If a token allowance is set on a ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
204-204: Hard tabs
Column: 80
(MD010, no-hard-tabs)
🔇 Additional comments (22)
snapshots/CatapultarUpgradeableTest.json (1)
4-7: Verify gas increase justification for KeyedOwnable-based changes.The snapshot shows consistent gas increases across four metrics (+435 gas for single transfers, +564 gas for double transfers). This correlates with the introduction of KeyedOwnable multi-key ownership and digest-based approval flows. However, confirm that:
- These increases are intentional and reflect necessary changes (not unintended complexity bloat)
- The digest-based flow and nonce tightening account for the observed gas deltas
- No further optimization opportunities exist in the KeyType key handling or signature validation paths
test/mocks/MockERC20.sol (1)
4-4: Formatting-only changes and lint directive look goodThe added forge-lint directive and the multiline reformatting of the constructor and function signatures are non-functional changes: parameter order/types, visibility, overrides, and call sites remain the same. No issues from a behavior or testing standpoint.
Also applies to: 17-21, 44-47, 51-54, 58-62, 66-70, 74-77, 81-85
src/libs/LibCalls.sol (1)
7-40: Calls struct layout and typehash remain consistent.The
Callsfield order andCALLS_TYPE_HASHstring are aligned, andtypehashstill hashes(nonce, mode, calls)in that order. No issues from this refactor.snapshots/CatapultarFactoryTest.json (1)
2-4: Snapshot updates look consistent with new factory APIs.The updated gas snapshots and
deployWithDigestkey align with the new deployment paths; nothing to change here.src/libs/BitmapNonce.sol (1)
21-40: Nonce‑0 guard in unordered nonce usage is sound.Rejecting nonce
0up front in_useUnorderedNoncematches the bitmap layout and updated tests, and avoids reserving a “valid” zero nonce. The extra forge‑lint suppressions around the casts/shifts are reasonable here.test/Integration.t.sol (1)
65-69: Integration test wiring matches new ownership and extraData semantics.Using
KeyedOwnable.KeyType.ECDSAOrSmartContractwith a 1‑elementkeysarray and shifting nonces to start at1keeps the integration test consistent withKeyedOwnableand the new BitmapNonce guard. The updatedassembleExtraDatahelper also still matches the documentedbytes1 | bytes23 | bytes8packing.Also applies to: 197-251, 259-277
test/Catapultar/Catapultar.base.t.sol (1)
34-41: Tests correctly exercise new KeyedOwnable + BitmapNonce behavior.Switching initialization to
KeyedOwnable.KeyType.ECDSAOrSmartContractwith a single‑word key and moving all unordered‑nonce tests to start at1lines up with the newBitmapNoncezero‑nonce guard. The added digest and token‑transfer‑fallback tests give good coverage of the new approval and callback paths.Also applies to: 60-66, 115-217, 237-320, 338-348, 348-370, 372-478
src/libs/KeyedOwnable.sol (1)
226-263: Runforge fmton KeyedOwnable.sol to resolve CI formatting check.The file has formatting issues with inline comments in the
WebAuthn.verify()function call (around line 251-252) that are causingforge fmt --checkto fail in CI.test/CatapultarFactory.t.sol (2)
27-27: LGTM! Tests properly exercise the new KeyedOwnable-based API.The migration from owner address to
KeyType/keys[]parameters is consistent across all test functions, properly exercising the new initialization flow.Also applies to: 85-87
124-136: LGTM! Salt validation tests properly updated.All salt validation fuzz tests correctly construct the
keys[]array and exercise theownerInSaltmodifier with the new KeyedOwnable-based parameters.Also applies to: 138-150, 152-164, 166-178, 180-192, 194-206
src/Catapultar.sol (6)
4-4: LGTM! New imports support the multi-key ownership and Stowaway integration.The imports of
Stowaway,ERC7821, andKeyedOwnableare properly used throughout the contract to enable the new digest-based approval flow and fallback routing.Also applies to: 6-6, 15-15
65-71: LGTM! Digest approval mechanism properly defined.The
DigestApprovalenum andapprovedDigestmapping correctly implement the three-state approval system (Unset/Call/Signature), with enum values that align with the factory's assembly logic.
95-100: LGTM! Initialization properly migrated to KeyedOwnable.The
initfunction correctly acceptsKeyTypeandbytes32[]owner parameters and delegates to_transferOwnership, maintaining the initializer pattern.
223-232: LGTM! Digest-based approval properly integrated into validation.The refactored
_validateOpDatacorrectly handles three cases:
- Self-calls with 32-byte opData (batch execution)
- Pre-approved Call digests with 32-byte opData
- Standard signature validation for longer opData
This aligns with the new digest approval mechanism.
73-76: LGTM! Constructor properly simplified for proxy pattern.The removal of the
allowOneTimeCallparameter and unconditional initialization disabling correctly supports the unified executor approach in the factory. The version bump to0.1.0appropriately reflects the breaking changes.Also applies to: 80-80
252-252: Stowaway integration is intentionally designed and documented.The fallback routing is correctly implemented:
Stowaway.searchAndCall(ERC7821.execute.selector)routes encoded execute calls, followed byLibZip.cdFallback()for decompression. This two-layer pattern is documented in the README as the intended design—Stowaway catches stray fallback functions while LibZip provides gas efficiency on expensive calldata chains. The ordering is intentional and the integration is complete.src/CatapultarFactory.sol (6)
4-4: LGTM! New imports support the KeyedOwnable-based deployment.The imports of
EfficientHashLibandKeyedOwnableare properly used for salt derivation and key type handling throughout the factory.Also applies to: 8-8
37-42: LGTM! Consolidation to single executor simplifies deployment.The removal of separate
EXECUTOR_NO_EMBEDDED_CALLSandEXECUTOR_EMBEDDED_CALLSin favor of a singleEXECUTORaligns with the unified digest-based approval mechanism, reducing complexity and deployment cost.
46-54: LGTM! Deploy function properly migrated to KeyedOwnable API.The
deployfunction correctly acceptsKeyTypeandbytes32[]owner parameters, validates them via theownerInSaltmodifier, and initializes the proxy with the new ownership model.
75-88: Assembly logic correctly maps isSignature to DigestApproval enum.The assembly blocks correctly compute:
nonce = isSignature + 1→ 1 (false) or 2 (true)- This maps to
DigestApproval.Call(1) andDigestApproval.Signature(2)The salt derivation includes the nonce to ensure distinct addresses for Call vs. Signature approvals.
59-65: LGTM! Predict functions properly mirror deploy functions.All
predictDeploy*functions correctly use the sameownerInSaltvalidation and salt derivation logic as their correspondingdeploy*functions, ensuring consistent address prediction.Also applies to: 97-113, 131-137
159-170: The concern raised in this review comment is unfounded. Evidence from the codebase shows thatEfficientHashLibexplicitly supportsbytes32[]arrays through the using statementusing EfficientHashLib for bytes32[];insrc/libs/LibCalls.sol. The code at line 167 correctly hashes the entireownerarray as a single input, which is the intended behavior. There are no compilation issues or incorrect hashing logic.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (1)
201-229: Clean up typos and wording in Stowaway “Usage Warnings” section.The content is good, but there are a few remaining typos and minor style nits (many of which were flagged in a previous review):
- Line 203:
delievered→delivered; optionally clarify casing: “You can optionallyLibZipthe call” or “you can optionally libzip the call” depending on project convention.- Line 209:
exeute→execute; also consider spelling out “if and only if (IFF)” or just “if and only if” instead of bare “IFF”.- Lines 97 & 210: avoid mixing
Upgradable/upgradeablevariants; pick one spelling and use it consistently.- Line 213: “very important” can be softened if you want to follow the style suggestion (e.g., “crucial to parse and validate”).
- Line 219: “If A implement ERC-1271” → “If A implements ERC-1271”.
- Line 222: remove the stray tab in “signed (by A).” → “signed (by A).”
- Line 228: “long lived” → “long‑lived”.
- Line 266: consider adding a short license note for Stowaway (MIT/other), similar to the Solady/Permit2 entries.
These are all editorial and don’t affect behavior, but tightening them up will make the security guidance easier to read.
Also applies to: 266-266
src/Catapultar.sol (1)
63-72: Pre‑approved signature digests are treated as invalid inisValidSignature.Right now this branch:
if (signature.length == 0 && approvedDigest[digest] == DigestApproval.Signature) return bytes4(0xffffffff);returns the ERC‑1271 “invalid” value for an empty signature even when the digest has been explicitly approved via
setSignature(..., DigestApproval.Signature). That contradicts the docstring onsetSignature(“Allow setting a signed object as validated through owner or self call”) and defeats the purpose of pre‑approving a signing digest.To align behavior with ERC‑1271 and your
setSignaturesemantics, the empty‑signature + approved‑Signature case should return the magic “valid” selector (0x1626ba7e), not0xffffffff. For example:- if (signature.length == 0 && approvedDigest[digest] == DigestApproval.Signature) return bytes4(0xffffffff); + if (signature.length == 0 && approvedDigest[digest] == DigestApproval.Signature) { + return bytes4(0x1626ba7e); + }The rest of the function can continue to delegate to
_validateSignaturefor non‑empty signatures.What are the ERC-1271 magic return values for `isValidSignature(bytes32,bytes)` (valid vs invalid), and is returning `0x1626ba7e` for valid and `0xffffffff` for invalid consistent with the specification and common implementations?Also applies to: 102-120, 131-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
README.md(4 hunks)foundry.toml(1 hunks)src/Catapultar.sol(7 hunks)src/CatapultarFactory.sol(4 hunks)src/libs/ERC7821LIFI.sol(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libs/ERC7821LIFI.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
src/CatapultarFactory.solsrc/Catapultar.sol
🪛 LanguageTool
README.md
[uncategorized] ~97-~97: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...--------------: | :---------------: | | Upgradable | No | No...
(EN_WORD_COHERENCY)
[grammar] ~203-~203: Ensure spelling is correct
Context: ...` call into a bytes field which will be delievered to the account. You can optionally LibZ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~209-~209: Ensure spelling is correct
Context: ... bypassed if called from itself: - The exeute endpoint does not require a signature I...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~210-~210: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...f the underlying SCA if the contract is upgradable. - The SCA can upgrade the contract own...
(EN_WORD_COHERENCY)
[style] ~213-~213: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...es themselves are not structured. It is very important to parse and validate that ALL sign...
(EN_WEAK_ADJECTIVE)
[grammar] ~228-~228: Use a hyphen to join words.
Context: ... remember that token allowances are long lived. If a token allowance is set on a ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 80
(MD010, no-hard-tabs)
🔇 Additional comments (5)
foundry.toml (1)
9-9: Configuration change aligns with reproducible builds best practice.The addition of
bytecode_hash = 'none'enables reproducible builds by excluding the bytecode hash from compiled artifacts. This is a standard practice for smart contract projects.Note: Only the foundry configuration file was provided for review. The actual smart contract implementation changes (KeyedOwnable contract, factory updates, Stowaway integration, signature approval mechanism) mentioned in the PR summary are not visible in this review. Please ensure those changes are thoroughly reviewed separately.
README.md (1)
75-86: Docs correctly reflect new factory deployment signatures.The deployment examples and the new proxy feature table match the updated factory APIs (
deploy(ktp, owner, salt)anddeployWithDigest(ktp, owner, salt, digest, isSignature)plus Digest Proxy row), so integrators see the right parameter shapes and capabilities.Also applies to: 93-101
src/Catapultar.sol (2)
55-56: KeyedOwnable integration and initializer wiring look sound.Inheriting
KeyedOwnableand exposing a singleinit(KeyType ktp, bytes32[] owner)initializer that just delegates to_transferOwnershipmatches the new factory deployment flow, and the constructor’s_disableInitializers()correctly protects the implementation contract. Domain name/version ("Catapultar","0.1.0") are consistent with the@custom:versiontag.Also applies to: 73-76, 78-81, 95-101
211-233: _validateOpData and fallback integration accurately implement documented behavior.
_validateOpDatacleanly supports three cases: (a) self-call with 32-byteopData(line 223), (b) pre-approved call digests viaapprovedDigest[callTypeHash] == DigestApproval.Call(line 227), and (c) standard EIP‑712 signature validation via_hashTypedData+_validateSignature(lines 229-230). Nonce consumption via_useUnorderedNonceat line 220 is consistent with unordered nonce semantics.- The fallback function correctly chains
Stowaway.searchAndCall(ERC7821.execute.selector)beforeLibZip.cdFallback(), matching the README's documented intent to catch stray fallback functions and optionally handle zipped calldata.src/CatapultarFactory.sol (1)
4-8: Multi-key deploy APIs and digest-salt flow are correct and atomic.
- The factory APIs consistently take
KeyedOwnable.KeyType ktpandbytes32[] owner, forwarding them intoCatapultar.init{value: msg.value}(ktp, owner), matching the new initializer signature.deployWithDigest/predictDeployWithDigestderive deterministicsaltWithDigest = EfficientHashLib.hash(salt, digest, bytes32(nonce))wherenonceencodesDigestApproval.Callvs.Signature, keeping the user-controlledsaltwhile namespacing by digest and approval kind.- Within a single
deployWithDigesttransaction, the sequencecloneDeterministic_PUSH0→setSignature→initexecutes synchronously and atomically; EVM transactions are sequential at the call-stack level, so no external transaction can interleave before the factory call completes.deployUpgradeable/predictDeployUpgradeablemirror the minimal deploy semantics for the ERC1967 path, threading throughktp/ownerandownerInSalt.Factory wiring aligns with the KeyedOwnable/DigestApproval model.
Also applies to: 13-21, 39-43, 45-55, 59-65, 67-93, 115-137
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
97-105: Clarify P256 salt derivation to match_saltPrefiximplementation.The text says “for P256 accounts, the first 20 bytes should be the last 20 bytes of the hash of the entire key”, but
_saltPrefixderives an address asbytes20(keccak256(ktp, keccak256(owner[]))), i.e. effectively “the 20 bytes taken viabytes20from that hash.” To avoid confusion for off‑chain tooling, consider rephrasing this to something like “the 20 bytes obtained frombytes20(keccak256(ktp, keccak256(owner)))” (or at least drop “last/first”) so the README matches the actual factory logic.src/Catapultar.sol (1)
95-119:setSignatureaccess control is safe under current invariants; treat “no external calls” as part of the design contract.Allowing
setSignatureto be callable by anyone while_getInitializedVersion() == 0is safe here because:
- The proxy is created and
setSignature+initare invoked sequentially within a single transaction from the factory.- Neither
setSignaturenorinitperforms external calls, so there is no reentrancy window for third‑party injection of digests before initialization completes.However, this safety depends on those functions remaining free of external calls (including via hooks or delegatecalls). If either ever starts calling out, you’ll want to re‑evaluate this pattern (e.g., by tightening the guard or adding an explicit non‑reentrancy invariant) to avoid digest pollution on uninitialized proxies.
src/libs/KeyedOwnable.sol (1)
92-121:owner()semantics for non‑ECDSA key types may surprise integrators.
owner()always reads_OWNER_SLOTand passes it through_asAddressNotDirty, which will revert for P256 / WebAuthn keys (the high 12 bytes are not zero). This is consistent with your access‑control design (P256/WebAuthn ownership is enforced via_validateSignatureandownerOrSelfusingaddress(this)), but it means:
- For non‑ECDSA keys,
owner()cannot be used off‑chain to discover a controlling EOA/contract; callers will either see a revert or have to special‑case based onownerKeyType.If that behavior is intentional, consider documenting it explicitly in the NatSpec for
owner()and/orownerKeyTypeso downstream tooling doesn’t assumeowner()is always callable and meaningful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
README.md(7 hunks)script/deploy.s.sol(2 hunks)src/Catapultar.sol(7 hunks)src/CatapultarFactory.sol(4 hunks)src/libs/ERC7821LIFI.sol(2 hunks)src/libs/KeyedOwnable.sol(1 hunks)test/CatapultarFactory.t.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/libs/ERC7821LIFI.sol
- test/CatapultarFactory.t.sol
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
script/deploy.s.solsrc/CatapultarFactory.solsrc/Catapultar.solsrc/libs/KeyedOwnable.sol
🪛 LanguageTool
README.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...d user. It should provide durable double spend protection ensuring dispatched tra...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...0000078210001` can be used. It is a once callable, revert ignoring batch call. It...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...n be used. It is a once callable, revert ignoring batch call. It allows a set of ...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...cking others. To provide durable double spend protections, execution mode `0x010...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...0000078210001` can be used. It is a once executable, revert raising batch call. I...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...be used. It is a once executable, revert raising batch call. It allows a set of t...
(QB_NEW_EN_HYPHEN)
[grammar] ~17-~17: Use a hyphen to join words.
Context: ...10001` calling itself allowing for a one time callable batch with inner unsigned ...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~101-~101: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...--------------: | :---------------: | | Upgradable | No | No...
(EN_WORD_COHERENCY)
[style] ~217-~217: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...es themselves are not structured. It is very important to parse and validate that ALL sign...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
README.md
38-38: Hard tabs
Column: 83
(MD010, no-hard-tabs)
69-69: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 80
(MD010, no-hard-tabs)
🔇 Additional comments (3)
script/deploy.s.sol (1)
33-51: Salt and owner key packing inaccount()are consistent with the factory’s expectations.
ownerArray[0] = bytes32(uint256(uint160(owner)))matchesKeyedOwnable’s ECDSA key format (address in the low 20 bytes, high 12 bytes zero), andsalt = bytes32(bytes20(owner))encodes the address into the high 20 bytes of the salt, which aligns with the “first 20 bytes” requirement enforced viaownerInSalt/_saltPrefixandLibClone.checkStartsWith. The predict‑then‑deploy flow plus the post‑deploy equality check give a good safety net around deterministic deployment.src/CatapultarFactory.sol (2)
64-110: Digest‑based deploy path correctly couples salt derivation andDigestApprovalenum values.
deployWithDigest/predictDeployWithDigestderivenonce = isSignature ? 2 : 1, fold(salt, digest, bytes32(nonce))intosaltWithDigest, and then castnonceback toCatapultar.DigestApprovalfor thesetSignaturecall. GivenDigestApproval { Unset, Call, Signature }, this keeps:
- Salt derivation and approval flag in sync (1 → Call, 2 → Signature).
- Address prediction deterministic and collision‑free across digest/type combinations.
With
setSignatureandinitboth remaining internal‑only (no external calls), this deploy path stays atomic and does not introduce a new reentrancy surface.
138-167:_saltPrefixdesign matches the multi‑key model and keeps the owner‑in‑salt invariant.The updated
ownerInSaltmodifier now defers to_saltPrefix(ktp, owner), which:
- For
ECDSAOrSmartContractwith a single word key, uses the literal address encoded intoowner[0].- For larger keys (P256/WebAuthn or multi‑word), derives a 20‑byte prefix as
bytes20(keccak256(ktp, keccak256(owner[])))).This preserves the “salt starts with an owner‑derived address” invariant across all key types and is compatible with the script’s
salt = bytes32(bytes20(owner))pattern for the ECDSA case. The trade‑off (hash‑based prefix for large keys) is reasonable and well‑documented in the modifier comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
214-241: Clean up remaining grammar/whitespace issues in the Stowaway warning example.There are still a couple of small issues in this section:
- Line 232: “If A implement ERC-1271” → “If A implements ERC-1271”
- Line 235: remove the stray tab: “set custom calldata as signed (by A).” → “set custom calldata as signed (by A).”
- Optional: “It is very important to parse and validate…” could drop “very” to keep the tone a bit crisper.
These are minor, but this section is security-sensitive documentation, so it’s worth keeping it as polished as possible.
🧹 Nitpick comments (2)
README.md (2)
9-20: Tighten wording around design/intro for readability.Minor language nits you may want to clean up:
- “durable double spend protection” → “durable double-spend protection”
- “execute a complex actions on behalf of a user” → “execute a complex action on behalf of a user” (or “execute complex actions”)
- “deterministic deployed with a pre-configured call” → “deterministically deployed with a pre-configured call”
These don’t affect meaning but make the intro read more smoothly.
35-40: Fix markdown hard tabs and list indentation flagged by markdownlint.On several of the changed lines (e.g., the feature tables, bullet lists, and code examples), there are literal tab characters and slightly off indentation (
MD010,MD007). This can cause noisy diffs and tooling warnings.Consider normalizing these to spaces only, for example:
- Replace tab-aligned cells like
Yes\t\t\t\t |with space-based padding.- Replace leading tabs before list items and fenced code (e.g., under “Signature Validation”, “Account Deployment”, “Execution Models”) with 2–4 spaces, consistently.
This should clear the reported
MD010andMD007issues.Also applies to: 69-70, 79-90, 97-105, 116-116, 131-135, 198-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
README.md(9 hunks)snapshots/CatapultarMinimalTest.json(1 hunks)snapshots/CatapultarUpgradeableTest.json(1 hunks)src/Catapultar.sol(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- snapshots/CatapultarMinimalTest.json
- snapshots/CatapultarUpgradeableTest.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
src/Catapultar.sol
🪛 LanguageTool
README.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...d user. It should provide durable double spend protection ensuring dispatched tra...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...0000078210001` can be used. It is a once callable, revert ignoring batch call. It...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...n be used. It is a once callable, revert ignoring batch call. It allows a set of ...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...cking others. To provide durable double spend protections, execution mode `0x010...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...0000078210001` can be used. It is a once executable, revert raising batch call. I...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...be used. It is a once executable, revert raising batch call. It allows a set of t...
(QB_NEW_EN_HYPHEN)
[grammar] ~17-~17: Use a hyphen to join words.
Context: ...10001` calling itself allowing for a one time callable batch with inner unsigned ...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~101-~101: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...--------------: | :---------------: | | Upgradable | No | No...
(EN_WORD_COHERENCY)
[style] ~226-~226: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...es themselves are not structured. It is very important to parse and validate that ALL sign...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
README.md
38-38: Hard tabs
Column: 83
(MD010, no-hard-tabs)
69-69: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
116-116: Hard tabs
Column: 40
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 80
(MD010, no-hard-tabs)
🔇 Additional comments (3)
README.md (1)
69-70: New key-type and digest-proxy documentation looks consistent and helpful.The additions for:
- Supported signature types (ECDSA, ERC-1271, P256, WebAuth P256),
- Updated
CatapultarFactorydeployment signatures (deploy(ktp, owner, salt)anddeployWithDigest(ktp, owner, salt, callsTypeHash, isSignature)),- The digest-proxy feature table,
all read coherently and match the described KeyedOwnable / per-digest approval model in the contracts.
No changes requested here.
Also applies to: 79-90, 97-105, 198-200
src/Catapultar.sol (2)
58-72: Per-digest approval wiring (setSignature,approvedDigest,isValidSignature,_validateOpData) looks consistent.A quick sanity pass over the new approval/digest flow:
setSignature:
- Allows calls on uninitialized clones (version 0), but restricts to
ownerOrSelf()once initialized.- Writes the
approvedDigestflag and emitsSignatureSet.- Contains no external calls, so it remains safe to use before
initin the factory’s atomic deployment sequence.
isValidSignature:
- Wraps the incoming
hashusingREPLAY_PROTECTIONandaddress(this), matching the documented replay-protection envelope.- Supports pre-approved signature digests via the
signature.length == 0 && approvedDigest[digest] == DigestApproval.Signaturefast path, returning the ERC‑1271 “valid” selector.- Otherwise delegates to
_validateSignature(digest, signature)as expected.
_validateOpData:
- Always consumes the unordered nonce (
_useUnorderedNonce(nonce)), including for self-calls, which is consistent with the replay model.- Permits no-signature execution only when
opData.length == 32 && msg.sender == address(this), or when there is a matchingDigestApproval.Callentry for the computedcallTypeHash.- For all other cases, it falls back to signature validation using the digest selected via the (fixed)
isMultichainflag.This all matches the intended per-digest approval model and the README’s description of embedded calls and replay protection. No changes requested here. Based on learnings, I also agree with keeping this logic lean and not adding extra
requirechecks that just guard obviously malformed input.Also applies to: 95-101, 111-120, 131-141, 211-230
259-264: Fallback composition with Stowaway and LibZip is straightforward.The updated
fallbackthat callsStowaway.searchAndCall(ERC7821.execute.selector)followed byLibZip.cdFallback()is clear and keeps all logic on-chain in a single entry point. There are no additional external calls or state writes here, so this doesn’t introduce new reentrancy or ownership concerns withinCatapultaritself.No action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/CatapultarFactory.sol (1)
64-85: LGTM: Digest-based deployment is correctly implemented.The assembly logic correctly maps
isSignatureto the enum values (1 forCall, 2 forSignature), and the salt derivation through triple-hashing ensures distinct addresses per digest/approval-type. The initialization sequence (deploy → setSignature → init) executes atomically within a single transaction.Optional: The assembly block (lines 73–78) is duplicated in
predictDeployWithDigest(lines 98–103). Consider extracting to a private pure function:function _digestApprovalNonce(bool isSignature) private pure returns (uint256 nonce) { assembly ("memory-safe") { // Catapultar.DigestApproval.Call == 1 // Catapultar.DigestApproval.Signature == 2 nonce := add(isSignature, 1) } }Then replace both blocks with:
uint256 nonce = _digestApprovalNonce(isSignature);This reduces duplication and simplifies future maintenance, though the gas impact is negligible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
snapshots/CatapultarFactoryTest.json(1 hunks)src/CatapultarFactory.sol(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snapshots/CatapultarFactoryTest.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
src/CatapultarFactory.sol
🔇 Additional comments (7)
src/CatapultarFactory.sol (7)
4-4: LGTM: Appropriate imports and version bump.The new imports support the multi-key ownership model, and the version bump correctly reflects the breaking API changes.
Also applies to: 8-8, 13-13
43-51: LGTM: Clean multi-key deployment.The function correctly integrates the new KeyedOwnable model, forwards
msg.valuesafely, and maintains deterministic deployment via the updatedownerInSaltmodifier.
56-62: LGTM: Consistent prediction API.The signature changes align with
deploy, and the deterministic address prediction remains correct.
90-106: LGTM: Prediction logic mirrors deployment.The function correctly replicates the salt derivation and address prediction for digest-based deployments.
109-130: LGTM: Upgradeable deployment functions updated consistently.Both
deployUpgradeableandpredictDeployUpgradeablecorrectly integrate the new multi-key parameters and maintain proper value forwarding.
143-150: LGTM: Clean salt validation.The modifier correctly delegates to
_saltPrefixfor computing the expected prefix and validates the salt accordingly.
152-161: LGTM: Efficient salt prefix computation.The helper correctly handles the ECDSA single-address optimization (lines 156–157) and hashes arbitrary key types with their owner arrays (line 159). The implementation prioritizes gas efficiency by omitting explicit validation in the else branch, consistent with the team's established approach. Invalid inputs will fail downstream or result in unusable deployments, which is acceptable given users control the input data.
Based on learnings, explicit length checks are intentionally omitted to avoid wasting gas on malformed user input.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
69-105: Replace hard tabs with spaces for consistent formatting.The code blocks and list items (lines 69–95) use hard tabs instead of spaces, which violates Markdown best practices and causes linting warnings. Replace all tabs with spaces (typically 2 or 4 spaces per indentation level depending on context).
Also review line 38 which contains a hard tab in the table; this should also be converted to spaces.
🧹 Nitpick comments (2)
src/libs/KeyedOwnable.sol (1)
146-171: Duplicate validation logic for P256 and WebAuthnP256.Cases 1 and 2 have identical validation logic (checking both key words are non-zero). Consider consolidating:
case 1 { valid := and( valid, not( or( eq(calldataload(key.offset), 0), eq(calldataload(add(key.offset, 0x20)), 0) ) ) ) } - case 2 { - valid := and( - valid, - not( - or( - // Check if first word of key is 0 - eq(calldataload(key.offset), 0), - // Check if second word of key is 0. - eq(calldataload(add(key.offset, 0x20)), 0) - ) - ) - ) - } + // case 2: same validation as case 1 (falls through)Or use
defaultwith an early return for case 0. This is a minor readability/gas nit.test/libs/KeyedOwnable.t.sol (1)
326-353: Consider adding WebAuthnP256 signature validation test.The test suite covers ECDSA, ERC-1271, and P256 signature validation but lacks WebAuthnP256 coverage. Given the complexity of WebAuthn authentication (challenge encoding, authenticator data, client data JSON), a dedicated test would catch integration issues with Solady's
WebAuthn.verify.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
README.md(9 hunks)snapshots/CatapultarFactoryTest.json(1 hunks)src/Catapultar.sol(8 hunks)src/CatapultarFactory.sol(4 hunks)src/libs/KeyedOwnable.sol(1 hunks)test/libs/KeyedOwnable.t.sol(1 hunks)test/mocks/MockKeyedOwnable.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snapshots/CatapultarFactoryTest.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
src/libs/KeyedOwnable.solsrc/CatapultarFactory.solsrc/Catapultar.sol
🪛 LanguageTool
README.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...d user. It should provide durable double spend protection ensuring dispatched tra...
(QB_NEW_EN_HYPHEN)
[grammar] ~11-~11: Ensure spelling is correct
Context: ...execute a complex action on behalf of a use. To scale a transaction dispatch envir...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...0000078210001` can be used. It is a once callable, revert ignoring batch call. It...
(QB_NEW_EN_HYPHEN)
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...n be used. It is a once callable, revert ignoring batch call. It allows a set of ...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...0000078210001` can be used. It is a once executable, revert raising batch call. I...
(QB_NEW_EN_HYPHEN)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...be used. It is a once executable, revert raising batch call. It allows a set of t...
(QB_NEW_EN_HYPHEN)
[grammar] ~17-~17: Use a hyphen to join words.
Context: ...10001` calling itself allowing for a one time callable batch with inner unsigned ...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~101-~101: Do not mix variants of the same word (‘upgradable’ and ‘upgradeable’) within a single text.
Context: ...--------------: | :---------------: | | Upgradable | No | No...
(EN_WORD_COHERENCY)
[style] ~226-~226: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...es themselves are not structured. It is very important to parse and validate that ALL sign...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
README.md
38-38: Hard tabs
Column: 83
(MD010, no-hard-tabs)
69-69: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
116-116: Hard tabs
Column: 40
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 80
(MD010, no-hard-tabs)
🔇 Additional comments (12)
README.md (1)
1-282: Approve README updates for multi-key and P256 support.The documentation updates effectively communicate the new capabilities: KeyedOwnable multi-key support, digest-based deployments (deployWithDigest), Stowaway integration for fallback handling, and expanded signature support (ECDSA, ERC-1271, P256, WebAuth P256). The execution mode descriptions, deployment strategies, and salt handling guidance for P256 accounts are clear and accurate.
Key areas verified:
- New deployment methods and parameters (KeyType, salt format for P256)
- Execution mode specifications with multi-chain signing support
- Feature comparison tables updated consistently
- Stowaway integration documented with appropriate security warnings
Minor formatting issues (hard tabs) and one remaining style issue (line 226) have been flagged separately above.
src/libs/KeyedOwnable.sol (4)
81-90: _keyTypeLength assembly logic is correct but could benefit from a brief inline comment.The assembly computes
len = keyType + (keyType < 2 ? 1 : 0), yielding:
- ECDSAOrSmartContract (0): 0 + 1 = 1
- P256 (1): 1 + 1 = 2
- WebAuthnP256 (2): 2 + 0 = 2
This is correct. Consider adding a brief inline comment explaining the arithmetic for future maintainers.
206-222: Verify intentional design:transferOwnership(address)allows zero-address owner.The comment mentions resigning ownership by setting to 0, but this bypasses
_isValidKeyvalidation (which rejects zero addresses forECDSAOrSmartContract). This creates an inconsistency:
transferOwnership(KeyType.ECDSAOrSmartContract, [0x0])→ reverts withInvalidKey()transferOwnership(address(0))→ succeedsIf renunciation is intentional, consider:
- Adding explicit documentation that this is the only way to renounce
- Adding a dedicated
renounceOwnership()function for clarity- Emitting a distinct event for renunciation
If unintentional, add validation:
function transferOwnership( address newOwner ) public payable onlyOwnerOrSelf { + if (newOwner == address(0)) revert InvalidKey(); // ... }
271-274: Verify: ECDSAOrSmartContract fallback receives truncated signature.When
ownerKeyType == ECDSAOrSmartContractbut signature length is not 64/65 bytes (e.g., 66+ bytes), the code:
- Truncates the signature by 1 byte (line 245)
- Falls through to line 271-273, passing the truncated signature to
SignatureCheckerLibThis seems intentional (allowing ECDSA signatures with an appended prehash byte), but:
- The prehash is applied (SHA256) before ECDSA verification
- Standard ECDSA/EIP-1271 verifiers don't expect prehashed digests
Is this path intentional for smart contract wallets that support prehashed digests, or should line 237 include other ECDSA-compatible lengths?
283-294: LGTM!The dirty-address detection correctly examines
elembefore assignment. The shift-and-compare logic properly identifies when upper 96 bits are non-zero.test/mocks/MockKeyedOwnable.sol (1)
10-37: LGTM!Test mock correctly exposes internal functions for testing. The warning comment appropriately signals this is not for production use. ERC-1271 magic values are correct.
src/Catapultar.sol (4)
111-119: LGTM!The access control correctly allows
setSignatureduring atomic factory deployment (when uninitialized) while requiring owner/self authorization post-initialization. The extensive prior review discussion confirmed this pattern is secure.
131-147: LGTM!The replay protection envelope correctly prevents cross-account signature replay. The pre-approved digest path now correctly returns the valid magic value.
231-237: Multichain flag detection is now correct.The assembly extracts byte index 2 (0-indexed from MSB) of
mode:
shl(16, mode)shifts away the 2 MSB bytesshr(248, ...)isolates the new MSB (originally byte 2)- Comparison with 1 correctly identifies the multichain flag
This addresses the previously flagged issue where the wrong shift amounts were used.
261-264: LGTM!The fallback correctly integrates Stowaway for dynamic call resolution before falling back to LibZip's calldata decompression. This maintains backward compatibility while adding the new execution routing.
src/CatapultarFactory.sol (2)
65-86: LGTM!The digest-based deployment correctly:
- Derives a unique salt from (salt, digest, approvalType) preventing address collisions
- Executes setSignature and init atomically within a single transaction
- Properly forwards msg.value to init
The prior review discussion confirmed the reentrancy annotation is appropriate.
153-162: LGTM!The salt prefix logic correctly handles both cases:
- ECDSA: Uses the owner address directly (efficient, backward-compatible)
- P256/WebAuthn: Hashes the full key data to derive a 20-byte prefix
The double-hashing approach for multi-word keys prevents length-extension attacks and ensures consistent prefix derivation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/libs/KeyedOwnable.t.sol (2)
238-243: Bug:owner[1]is never assigned.Line 241 assigns
owner[0]twice, leavingowner[1]as 0. This causes the test to fail for the wrong reason (zero key instead of "too long").Apply this diff:
// too long owner = new bytes32[](3); owner[0] = keccak256(bytes("x")); - owner[0] = keccak256(bytes("y")); + owner[1] = keccak256(bytes("y")); vm.expectRevert(KeyedOwnable.InvalidKey.selector); ownable.transferOwnership(KeyedOwnable.KeyType.P256, owner);
267-272: Same bug:owner[1]not assigned.// too long owner = new bytes32[](3); owner[0] = keccak256(bytes("x")); - owner[0] = keccak256(bytes("y")); + owner[1] = keccak256(bytes("y")); vm.expectRevert(KeyedOwnable.InvalidKey.selector); ownable.transferOwnership(KeyedOwnable.KeyType.WebAuthnP256, owner);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/libs/KeyedOwnable.sol(1 hunks)test/libs/KeyedOwnable.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
test/libs/KeyedOwnable.t.solsrc/libs/KeyedOwnable.sol
🔇 Additional comments (7)
test/libs/KeyedOwnable.t.sol (1)
15-81: LGTM!The
P256VerifierEtcherhelper provides robust test infrastructure for managing P256 verification bytecode across different execution environments (native RIP precompile vs. etched verifier).src/libs/KeyedOwnable.sol (6)
28-94: LGTM!The storage slot design provides upgrade safety by overlapping with Solady's
Ownableslot, ensuring that if an ECDSAOrSmartContract key is in use, accidental upgrades to non-KeyedOwnable contracts may still maintain access control.
74-90: LGTM!Clever branchless arithmetic to compute key lengths. The expression
add(keyType, lt(keyType, 2))correctly produces 1 for ECDSA and 2 for P256/WebAuthnP256.
123-173: LGTM!The key validation correctly enforces constraints for each KeyType: clean 20-byte addresses for ECDSA, and non-zero coordinate pairs for P256/WebAuthnP256.
Based on learnings, the absence of explicit length checks before array indexing is intentional for gas efficiency—out-of-bounds access will panic.
175-222: LGTM!Ownership transfer logic correctly validates keys, writes storage slices, updates
ownerKeyType, and emits events. The address-based overload provides a convenient path for ECDSA transfers.
224-278: LGTM!The signature validation flow correctly handles all three key types:
- Lines 237-240: Fast path for standard Ethereum signatures
- Lines 242-251: Prehash flag extraction is correct—
truncatedCalldataadjusts length but preserves offset, soloadCalldata(signature, n-32)reads a 32-byte word whose last byte (atsignature.offset + n - 1) is the prehash flag- Lines 253-277: Proper routing to P256, WebAuthnP256, or ECDSA/1271 verification
280-296: LGTM!The dirty-address detection correctly validates that the upper 12 bytes are zero before returning the address, protecting against returning truncated addresses when non-ECDSA keys are stored.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
60-60: Fix typo: "dangourus" should be "dangerous".Apply this diff:
-- Catapultar does not support external delegate calls. Delegate calls are dangourus, particularly for upgradeable contracts. They can change the owner of Catapultar but also the implementation of a proxy (ERC-1967). +- Catapultar does not support external delegate calls. Delegate calls are dangerous, particularly for upgradeable contracts. They can change the owner of Catapultar but also the implementation of a proxy (ERC-1967).
♻️ Duplicate comments (1)
README.md (1)
235-235: Remove hard tab character before "(by A)."Line 235 contains a hard tab character that should be replaced with a space for consistent formatting.
Apply this diff:
-2. Store custom calldata in transient storage and set custom calldata as signed (by A). +2. Store custom calldata in transient storage and set custom calldata as signed (by A).
🧹 Nitpick comments (1)
test/libs/KeyedOwnable.t.sol (1)
330-330: Optional: Comment refers to "WebAuthnP256Owner" but test uses P256.The comment on line 330 mentions "WebAuthnP256Owner" but the test is actually for P256 (not WebAuthnP256), as evidenced by the KeyType.P256 used on line 332. Consider updating the comment for clarity.
Apply this diff:
- // We need to switch the account to a P256 owner. - (uint256 privatekeyP256, bytes32[] memory publickeyP256) = makeP256("WebAuthnP256Owner"); + // We need to switch the account to a P256 owner. + (uint256 privatekeyP256, bytes32[] memory publickeyP256) = makeP256("P256Owner");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(6 hunks)test/libs/KeyedOwnable.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
test/libs/KeyedOwnable.t.sol
🪛 markdownlint-cli2 (0.18.1)
README.md
69-69: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
72-72: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 80
(MD010, no-hard-tabs)
🔇 Additional comments (5)
test/libs/KeyedOwnable.t.sol (5)
14-81: LGTM: P256 verifier etching infrastructure is well-structured.The P256VerifierEtcher contract provides clean test infrastructure for conditionally etching verifier bytecode and handling native RIP precompile detection. The logic correctly:
- Detects native RIP precompile support
- Conditionally etches or clears bytecode based on the active flag
- Provides helper methods for common etching patterns
83-110: LGTM: Test setup and P256 key generation are correct.The setup properly:
- Generates deterministic P256 key pairs with validation (ensuring privateKey is non-zero and within P256.N)
- Etches the necessary verifier infrastructure
- Initializes the mock contract with an ECDSA owner
112-177: LGTM: Comprehensive ownership transfer test.The test thoroughly validates:
- Ownership transfers across all key types (ECDSA, P256, WebAuthnP256)
- Event emissions with correct parameters
- Dirty slot behavior when switching from P256/WebAuthnP256 back to ECDSA
- The revert when calling owner() with a dirty Ethereum address slot
179-273: LGTM: Invalid key test cases are comprehensive and correct.The test validates all invalid key scenarios:
- Zero/empty keys for all key types
- Dirty Ethereum addresses for ECDSA
- Invalid array lengths (too short, too long) for all key types
- Both x and y coordinates being non-zero for P256/WebAuthnP256
The previously flagged array assignment bugs have been fixed.
275-407: LGTM: Signature validation tests are comprehensive and correct.The test suite thoroughly validates:
- ECDSA signatures (both compact and standard formats)
- ERC-1271 signature delegation to smart contract owners
- P256 signatures with optional sha256 rehashing
- WebAuthn signatures with optional rehashing
- Negative test cases (invalid digests, wrong rehash flags)
The previously flagged typos ("choise" → "choice") have been corrected.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
snapshots/CatapultarMinimalTest.json(1 hunks)snapshots/CatapultarUpgradeableTest.json(1 hunks)src/Catapultar.sol(8 hunks)test/Catapultar/Catapultar.base.t.sol(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snapshots/CatapultarMinimalTest.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
src/Catapultar.sol
🪛 GitHub Actions: CI
test/Catapultar/Catapultar.base.t.sol
[error] 424-435: forge fmt --check failed. Formatting differences detected in test/Catapultar/Catapultar.base.t.sol. Run 'forge fmt' (or 'forge fmt --check' after formatting) to fix code style issues. Process completed with exit code 1.
🔇 Additional comments (11)
src/Catapultar.sol (6)
4-16: LGTM! Clean integration of KeyedOwnable and digest-based approval system.The migration from Ownable to KeyedOwnable enables support for multiple key systems (including P256), and the DigestApproval enum with approvedDigest mapping provides a type-safe mechanism for per-digest authorizations.
Also applies to: 55-71
95-100: LGTM! Init function properly integrates KeyedOwnable.The function signature correctly accepts KeyType and keys array, delegates to KeyedOwnable's
_transferOwnership, and is protected by theinitializermodifier.
111-119: LGTM! Access control correctly implements factory-safe initialization pattern.The condition on line 115 allows
setSignatureto be called during factory deployment (when uninitialized) or by owner/self post-initialization. This is safe because factory deployment is atomic (clone → setSignature → init all execute in one transaction).Based on learnings, the design has been thoroughly vetted for reentrancy and front-running concerns.
131-147: LGTM! Signature validation correctly handles pre-approved digests.The replay protection envelope prevents cross-account signature replay, and line 140 now correctly returns the valid selector (0x1626ba7e) for pre-approved signatures with empty signature data.
231-242: LGTM! Multichain flag extraction logic is correct.The assembly correctly extracts byte 2 of the mode:
shl(16, mode)shifts byte 2 into byte 0 positionshr(248, ...)isolates that byte in the least significant positionFor multichain mode (byte 2 = 0x01),
isMultichainwill be truthy and_hashTypedDataSansChainIdis used. For standard mode (byte 2 = 0x00),_hashTypedDatais used with chain ID binding.
261-264: LGTM! Fallback properly integrates Stowaway for ERC7821 support.The fallback first attempts Stowaway-based selector lookup for ERC7821.execute, then falls back to LibZip for calldata decompression. Clean integration.
snapshots/CatapultarUpgradeableTest.json (1)
4-7: Gas cost increases are expected given the new features.The increases (507-636 gas per operation) align with the added functionality: KeyedOwnable-based ownership, per-digest approvals, and multichain mode detection. The increases are consistent across operations and reasonable for the added capabilities.
test/Catapultar/Catapultar.base.t.sol (4)
60-91: LGTM! Tests properly migrated to KeyedOwnable-based initialization.The init helper and related tests correctly construct the keys array and use
KeyType.ECDSAOrSmartContractfor initialization. The migration is complete and consistent.
259-267: LGTM! New test validates deterministic cloning produces distinct addresses.The test confirms that different embeds (salts) produce different clone addresses, which is essential for the digest-based deployment model.
348-370: LGTM! Comprehensive test of digest-based approval flow.The test validates that pre-approved digests enable validation without signatures, exercising both the rejection path (unapproved) and success path (approved via
setSignature).
484-521: LGTM! Fallback test correctly validates ERC721/1155 callback support.The test confirms that Stowaway-based fallback properly handles token transfer callbacks (onERC721Received, onERC1155Received, onERC1155BatchReceived), enabling the contract to receive NFTs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/Catapultar/Catapultar.base.t.sol (3)
4-7: Reasonable use of forge-lint disables, but consider narrowing scope.Disabling
unsafe-typecast,erc20-unchecked-transfer, andunchecked-callfor the whole file is understandable for a low-level test harness, but you might later want to confine these to the few callsites that actually need them to keep lint signal high elsewhere in the file.
348-370: Digest-based validateOpData flow is covered end-to-end.
test_validateOpData_digestnicely captures the “default (no digest approval) returns false” path and the “after setSignature + approvedDigest, the same opData validates” path, including an explicit check of theDigestApprovalvalue. One optional improvement would be to initializecalls[0]with a non‑zero target / data so the digest reflects a realistic call bundle instead of an all‑zero placeholder, but that’s not required for correctness.
491-523: Improved token-transfer fallback test assertions; consider also checking return selectors.Capturing the
successflag from the low-levelcalland assertingtruefor each of the ERC721 / ERC1155 / ERC1155Batch callbacks is a solid improvement over relying solely on lack of revert. For even stronger guarantees, you could additionally decode the return data and assert it equals the respective selector, but that’s optional given the current test already catches regressions where the call fails outright.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/Catapultar/Catapultar.base.t.sol(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
test/Catapultar/Catapultar.base.t.sol
🔇 Additional comments (5)
test/Catapultar/Catapultar.base.t.sol (5)
16-18: KeyedOwnable-based init wiring looks consistent with previous owner model.The switch to
KeyedOwnable.KeyType.ECDSAOrSmartContractplusbytes32(uint256(uint160(owner)))keys, and the corresponding updates ininit,test_template_init_disabled, andtest_init, all line up with the prior “single EOA owner” semantics while exercising the new keyed API. UsingmakeAddrAndKey("owner")keeps the on-chain address and signing key aligned across tests.Also applies to: 60-66, 69-73, 80-84, 90-91
259-267: Good coverage for embedded clone address differentiation.
test_embed_new_addressnicely asserts that different embed payloads (embedAvsembedB) lead to distinct deterministic clone addresses off the sameexecutorTemplate. That directly guards regressions in yourLibClone.cloneDeterministicembedding behavior.If there are multiple
cloneDeterministicoverloads in yourLibClone, double‑check that the argument order here matches the “embedded data + salt” variant you intend to exercise.
306-307: Updated unordered nonce invalidation behavior is exercised well.Adjusting the invalidation mask from
3to7and explicitly checking that nonces1and2now revert while a higher nonce (12) still validates gives clearer coverage of the expanded bitmap behavior. This matches the intent of broader invalidation without over-constraining the implementation.Also applies to: 309-309, 317-317, 319-320
387-395: Stronger assertions on signature-based validation semantics.Wrapping the first successful call in
assertTrueand explicitly assertingassertFalsewhen the nonce inopDatadoesn’t match the signed nonce makes the intended behavior ofvalidateOpDatamuch clearer: booleanfalsefor bad signatures, revert for reused nonces. This is a good tightening of the test.
397-443: Multichain signature semantics and EIP712 domain handling are well tested; domain type string is a coupling point.This test gives excellent coverage of:
- Chain‑bound mode: signature bound to chain 100 failing on chain 101.
- Multichain mode: recomputing a domain without
chainIdand verifying that the same signature works on both chain 100 and 101 over shared storage.The main subtle coupling is the hard‑coded domain type string
"EIP712Domain(string name,string version,address verifyingContract)"and the assumption thatexecutor.eip712Domain()’s layout matches that exact shape. If the underlying contract’s EIP‑712 domain fields or type string ever change, this test will silently become out of sync.Please confirm that:
executor.eip712Domain()returns(uint8, string, string, address, ...)in the order assumed here, and- The contract’s multichain hashing really uses the same
EIP712Domain(string name,string version,address verifyingContract)type string.If those ever diverge, consider centralizing the domain hashing helper in the contract/library and calling that from the test instead of re‑encoding the domain here.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(7 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~137-~137: Ensure spelling is correct
Context: ...10001**: Executing a set of conditional transactons. If 1 transaction in a set fails, t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
69-69: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
72-72: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 80
(MD010, no-hard-tabs)
🔇 Additional comments (1)
README.md (1)
1-283: Documentation updates properly capture KeyedOwnable multi-key model and Digest Proxy functionality.The content changes effectively document:
- The shift to KeyedOwnable multi-key support (shown in feature table as "1" entry instead of "Multiple Keys" comparison)
- New
deployWithDigest()deployment method for digest-based proxy initialization- Introduction of Digest Proxy as a deployment strategy
- Comprehensive Stowaway integration section with security warnings about self-call bypass scenarios
- Consistent usage of
ktpparameter across deployment examples- ERC-1271 abuse scenarios and token allowance risks in Usage Warnings
These additions align well with the PR objectives of supporting P256 and other key systems through KeyedOwnable, along with digest-based deployment patterns.
|
Closes: https://linear.app/lifi-linear/issue/API-173/finish-p256-and-webauth-pr |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
README.md (1)
97-97: Clarify salt derivation guidance for multi-key systems.Line 97 provides guidance on salt structure for P256 accounts, but the phrasing could be slightly clearer. The instruction "For P256 accounts, the first 20 bytes should be the last 20 bytes of the hash of the entire key" is precise but dense. Consider whether additional context (e.g., a brief note about why hashing is needed for longer keys, or a reference to
predictDeploy*functions) would help users implement this correctly.test/Catapultar/Catapultar.base.t.sol (5)
4-7: Scopeforge-lintdisables as narrowly as possible.
These disables apply to the whole file; consider wrapping only the specific blocks that need them to avoid masking future issues.
60-66: Reduce duplication when buildingbytes32[] keysfrom an owner address.
The same “address → bytes32” packing is repeated in multiple tests; a small helper (e.g.,_singleAddressKey(address)) would keep future KeyType expansions cleaner.Also applies to: 69-73, 80-84
305-320: Avoid magic-number brittleness for nonce invalidation mask/value (7).
If7is derived from a specific bitmap layout change, consider computing it from the intended nonces (or documenting why7is correct) so future nonce encoding changes don’t silently break this test’s intent.
397-443: Multichain domain-separator reconstruction is brittle—prefer a contract/helper-derived “sans chainId” hash if available.
Manually rebuilding the domain separator (Line 424-433) can diverge from the implementation if the domain fields/typehash differ (e.g., extra fields, different EIP-5267 handling). If Catapultar exposes a helper (or internal logic can be mirrored in a shared test util), prefer that to reduce false negatives.
477-500: Good: usingvm.prank(address(executor))makessetSignatureauthorization explicit.
Optional: add one negative assertion that a non-self caller reverts (if that’s the intended access control) to lock it in.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(7 hunks)test/Catapultar/Catapultar.base.t.sol(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T15:05:38.106Z
Learnt from: reednaa
Repo: lifinance/catapultar PR: 1
File: src/CatapultarFactory.sol:132-137
Timestamp: 2025-11-12T15:05:38.106Z
Learning: In src/CatapultarFactory.sol, explicit length validation checks for the owner array parameter should not be added before indexing, as the out-of-bounds panic provides sufficient protection and adding require() checks would waste gas when users provide invalid data.
Applied to files:
test/Catapultar/Catapultar.base.t.sol
🔇 Additional comments (5)
README.md (2)
69-73: Clear and well-documented new signature and deployment features.The Key Features section now properly documents multi-key support, P256 signatures, and the new Digest Proxy deployment strategy. The updates align well with the PR objectives to add support for other ownable key systems.
214-241: Stowaway integration and usage warnings are well-documented.The new Stowaway section clearly explains the fallback integration and its optional LibZip support. The Usage Warnings section provides important security guidance about self-call bypass scenarios and ERC-1271 abuse risks. The documentation appropriately emphasizes the critical nature of validating signed batches.
test/Catapultar/Catapultar.base.t.sol (3)
259-267: Nice regression test for embed-dependent deterministic clone addresses.
This guards against accidental salt/embed collisions inLibClone.cloneDeterministic.
446-475:isValidSignaturereplay protection coverage looks solid.
Good check that the same signature can’t be replayed across a newly deployed proxy.
521-553: Fallback callback test remains clear after the refactor.
The three interface callbacks are still exercised end-to-end via low-level calls.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
69-69: Consider adding P256-specific signature validation documentation.Line 69 advertises "P256, and WebAuth P256 signatures" support, but the signature validation section (lines 150–163) only documents ERC-1271 validation. If P256 validation differs meaningfully from ECDSA/ERC-1271 (e.g., different hashing, different replayability properties, or different message formats), consider adding a parallel section or subsection explaining P256-specific validation steps. Otherwise, clarify in a brief note that P256 validation follows the same replay-protection pattern as documented.
Also applies to: 150-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(7 hunks)
🔇 Additional comments (1)
README.md (1)
214-241: ✅ Stowaway and usage warnings sections are well-documented.The Stowaway integration documentation is comprehensive and appropriately security-focused. The usage warnings clearly explain the implications of self-calls and token allowances. Previous typo fixes have been properly applied. No changes needed in this section.
| - **Minimal Proxy:** | ||
| ```solidity | ||
| factory.deploy(owner, salt); | ||
| ``` | ||
| Deploys a minimal proxy for batch execution. | ||
| ```solidity | ||
| factory.deploy(ktp, owner, salt); | ||
| ``` | ||
| Deploys a minimal proxy for batch execution. | ||
|
|
||
| - **Proxy with Embedded Call:** | ||
| ```solidity | ||
| factory.deployWithDigest(ktp, owner, salt, callsTypeHash, isSignature); | ||
| ``` | ||
| Deploys a proxy with a pre-configured allowed call. | ||
|
|
||
| - **Upgradeable Proxy:** | ||
| ```solidity | ||
| factory.deployUpgradeable(owner, salt); | ||
| ``` | ||
| Deploys an ERC1967 upgradeable proxy. Ownership can be transferred and logic upgraded. | ||
| ```solidity | ||
| factory.deployUpgradeable(owner, salt); | ||
| ``` | ||
| Deploys an ERC1967 upgradeable proxy. Ownership can be transferred and logic upgraded. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src -name "*Factory*.sol" -type fRepository: lifinance/catapultar
Length of output: 89
🏁 Script executed:
cat -n src/CatapultarFactory.sol | head -150Repository: lifinance/catapultar
Length of output: 7780
🏁 Script executed:
sed -n '79,95p' README.mdRepository: lifinance/catapultar
Length of output: 547
Correct deployment method signatures in documentation.
The documented function calls contain two parameter errors:
- Line 87:
deployWithDigestuses incorrect parameter namecallsTypeHash— should bedigest - Line 93:
deployUpgradeableis missing the requiredktpparameter — should befactory.deployUpgradeable(ktp, owner, salt)
Both methods require the ktp (KeyType) parameter according to the actual factory implementation.
🤖 Prompt for AI Agents
README.md around lines 79 to 95: update the documented factory method signatures
to match the actual implementation: change the second example to use
deployWithDigest(ktp, owner, salt, digest, isSignature) (replace callsTypeHash
with digest and include ktp) and change the third example to include the ktp
parameter as factory.deployUpgradeable(ktp, owner, salt); ensure both examples
list parameters in the correct order and names to reflect the real factory API.
| Deploys an ERC1967 upgradeable proxy. Ownership can be transferred and logic upgraded. | ||
|
|
||
| For all deployments, the first 20 bytes of `salt` should be the owner address or zero. Use the `predictDeploy*` functions to precompute addresses before deployment. | ||
| For all deployments, the first 20 bytes of `salt` should be the owner address or zero. For P256 accounts, the first 20 bytes should be the last 20 bytes of the hash of the entire key. Use the `predictDeploy*` functions to precompute addresses before deployment. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for P256-related salt derivation logic in CatapultarFactory.sol
rg -n 'P256|publicKey' src/CatapultarFactory.sol -A 5 -B 2Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Look for salt handling and key hashing logic
rg -n 'salt|hash' src/CatapultarFactory.sol | head -40Repository: lifinance/catapultar
Length of output: 2541
🏁 Script executed:
# Search for specific deployment functions related to P256
fd 'CatapultarFactory.sol' | xargs cat | grep -n 'deploy\|predictDeploy' -A 10Repository: lifinance/catapultar
Length of output: 5350
Update P256 salt derivation guidance to match factory implementation.
Line 97 contains inaccurate guidance. The actual implementation in CatapultarFactory.sol (lines 139-160) derives the salt prefix using the first 20 bytes of keccak256(ktp, keccak256(owner)) for all key types including P256, not "the last 20 bytes of the hash of the entire key." This applies equally to ECDSA and P256 accounts—the same ownerInSalt modifier enforces the salt format regardless of key type. Update the README to reflect the correct derivation method to prevent deployment address mismatches.
🤖 Prompt for AI Agents
In README.md around line 97, the guidance about P256 salt derivation is
incorrect; update the sentence to state that for all deployments (including P256
and ECDSA) the salt prefix is the first 20 bytes of keccak256(ktp,
keccak256(owner)) as implemented by CatapultarFactory.sol (lines 139–160) and
enforced by the ownerInSalt modifier; replace the existing line that mentions
"last 20 bytes of the hash of the entire key" with this corrected derivation and
keep the note to use the predictDeploy* functions to precompute addresses.
1 participant
Closes: https://linear.app/lifi-linear/issue/API-173/finish-p256-and-webauth-pr
Closes: API-173
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.