(audit 6.1.2) CatapultarFactory allows _saltPrefix() collisions#20
(audit 6.1.2) CatapultarFactory allows _saltPrefix() collisions#20reednaa wants to merge 2 commits intots-libraryfrom
Conversation
…avoiding non-obvious reuse behaviour
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughMajor refactor introducing Catapultar v0.1.0 smart account architecture. Reorganizes Solidity into Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Factory as CatapultarFactory
participant Clone as Catapultar (Clone)
participant Owner as KeyedOwnable
participant Validator as CATValidator
participant Token as ERC20
User->>Factory: deploy(ktp, owner[], salt)
Factory->>Clone: CREATE2 (deterministic)
activate Clone
Factory->>Clone: init(ktp, owner[])
Clone->>Owner: _transferOwnership(ktp, owner[])
Owner->>Owner: store owner in keyed slots
deactivate Clone
Factory-->>User: address payable proxy
User->>Clone: execute(calls)
activate Clone
Clone->>Clone: validateOpData(opData)
Clone->>Clone: _execute(calls)
deactivate Clone
User->>Validator: entry(target, payload, account, nonce, allowances, outcomes, sig)
activate Validator
Validator->>Validator: _checkNonce(account, nonce)
Validator->>Validator: _validateApproval(digest, sig)
Validator->>Validator: _handleAllowances(allowances)
Validator->>Token: transferFrom(source, dest, amount)
Validator->>Validator: _call(target, payload)
Validator->>Validator: _compareOutcomes(outcomes)
deactivate Validator
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This pull request introduces substantial new architecture across three domains (CI/CD, Solidity contracts, TypeScript SDK) with high-density logic including multi-signature verification schemes (ECDSA/P256/WebAuthn), deterministic CREATE2 deployments with complex salt encoding, execution constraint validation with EIP-712 hashing, and a comprehensive transaction builder API. The changes span 100+ new files with interconnected dependencies requiring careful review of cryptographic implementations, contract state management, and SDK API design. Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ts-library #20 +/- ##
===========================================
Coverage 95.82% 95.82%
===========================================
Files 14 14
Lines 2446 2446
===========================================
Hits 2344 2344
Misses 102 102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Refine factory salt computation
Replaces the
ownerInSaltmodifier and_saltPrefixbranching logic in bothCatapultarFactoryandCatapultarFactoryTronwith a unified_salt(preSalt, ktp, owner)function.Why: The old
_saltPrefixhad a collision vulnerability (audit 6.1.2) — an attacker could register anECDSAOrSmartContractowner whose raw value matched the hash-derived prefix of a victim's P256/WebAuthn key, front-running their deployment and bricking the address.What changed:
_saltnow hashes all inputs together:keccak256(preSalt ‖ ktp ‖ numOwners ‖ owners...), making cross-key-type collisions computationally infeasiblepreSaltis included so different user-provided salts always yield different addressesTooManyOwnersguard added (>255 owners reverts)SaltDoesNotStartWithrevert tests removed; new tests added covering salt encoding correctness and the TooManyOwners pathSummary by CodeRabbit
Release Notes
New Features
CATValidator) for constrained asset transaction approval.Documentation
Tests