Conversation
* Implement Reednaa's Stowaway * fmt * Remove embedded calls and replace by storage config.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@solidity/README.md`:
- Line 308: Fix the typo in the README sentence: change "valdiated" to
"validated" in the line that reads about signed `Outcome`s being validated by
recording balances before spending `Allowance` and comparing after the external
call (references: `Outcome`, `Allowance`).
- Line 302: The README contains a typo in the sentence describing CATValidator:
change the misspelled word "execue" to "execute" in the line referencing
CATValidator, CallProxy and CATValidator::CALL_PROXY so the sentence reads that
CallProxy "safely execute arbitrary calldata" (retain references to
CATValidator, CallProxy, and CALL_PROXY exactly as-is).
In `@solidity/src/CallProxy.sol`:
- Around line 15-24: The fallback() assembly uses calldatacopy with
sub(calldatasize(), 32) which underflows if calldata < 32 bytes; add a guard at
the start of the assembly block to check calldatasize() >= 32 (e.g., use
iszero(lt(calldatasize(), 32))) and revert or return early if the check fails,
so the subsequent calldatacopy and calldataload of the 32-byte target cannot
underflow; reference the fallback() function, the calldatasize(), calldatacopy,
and the sub(calldatasize(), 32) expression when applying this change.
In `@typescript/src/transaction/constrainedtransaction.spec.ts`:
- Around line 17-25: The test helper waitForTransaction currently sleeps 50ms
then calls createPublicClient(...).getTransactionReceipt({ hash }), which is
flaky; replace this with viem's polling helper by calling
waitForTransactionReceipt on the created publicClient (or import the standalone
waitForTransactionReceipt) with the transaction hash and desired
confirmations/timeout so it polls until mined; update the function
waitForTransaction to return await publicClient.waitForTransactionReceipt({
hash, confirmations?: number, timeout?: number }) (or equivalent options)
instead of the fixed sleep + single getTransactionReceipt call.
- Around line 88-94: The mint call via executor.writeContract is not awaited,
causing races before calling getTransactionCount and subsequent sendTransaction;
update the test to await the mint transaction (and wait for it to be mined) when
calling executor.writeContract for token1 (and any other mint calls), then only
call getTransactionCount() and perform sendTransaction after the awaited/minted
transaction completes to ensure balances and nonces are finalized.
🧹 Nitpick comments (13)
typescript/scripts/rewrite-dist-imports.mjs (1)
1-5: Resolvedist/relative to the script to avoid CWD dependence.In monorepos/workspaces, npm/pnpm/yarn may run scripts with the repo root as CWD.
path.resolve("dist")would then point at the wrong directory or fail. Prefer resolving relative to this script.Proposed change
import fs from "node:fs/promises"; import path from "node:path"; +import { fileURLToPath } from "node:url"; -const distDir = path.resolve("dist"); +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const distDir = path.resolve(__dirname, "..", "dist");solidity/README.md (2)
25-27: Clarify "Multiple Keys" table entry.The value "1" in the "Multiple Keys" row is inconsistent with the Yes/No pattern used in other rows. Consider using "Single" or "No (single key)" for clarity.
📝 Suggested clarification
-| Multiple Keys | 1 | Yes | Yes (K1Validator) | With Module | +| Multiple Keys | Single Key | Yes | Yes (K1Validator) | With Module |
108-108: Clarify ERC-7821 compatibility statement.The statement "Catapultar is not
ERC-7821compatible but it followsERC-7821specification" appears contradictory. If it follows the specification, it should be compatible. Consider clarifying whether this means:
- Partial implementation of the spec
- Implementation without formal compatibility claim
- A specific interpretation difference
solidity/src/CATValidator.sol (3)
228-233: Dead code in assembly error handling block.The inner
if iszero(success)check on line 230 is redundant since this code is already inside theif iszero(success)block from line 228. The revert will always execute.🔧 Proposed fix to remove redundant check
if iszero(success) { returndatacopy(0x00, 0x00, returndatasize()) - if iszero(success) { - revert(0x00, returndatasize()) - } + revert(0x00, returndatasize()) }
99-101: Nonce setting is correct but could be clearer.Setting
spentNonces[account][nonce] = !spentworks becausespentis guaranteed to befalseat this point (otherwise we'd have reverted). However, using= truewould be more explicit and readable.✨ Suggested improvement for clarity
function _checkNonce( address account, uint256 nonce ) internal { bool spent = spentNonces[account][nonce]; if (spent) revert NonceAlreadySpent(); - spentNonces[account][nonce] = !spent; + spentNonces[account][nonce] = true; }
165-171: Underflow on balance decrease will revert correctly.Line 168 performs
newBalance - recordedBalances[i]which will revert on underflow (Solidity 0.8+) if the balance decreased. This is the intended behavior per the contract's purpose, but worth documenting that a balance decrease causes a revert with an arithmetic underflow error rather thanInvalidTokenAmount.Consider adding a comment or a custom error for balance decrease scenarios for clearer error messages:
function _compareOutcomes( address account, Outcome[] calldata outcomes, uint256[] memory recordedBalances ) internal view { for (uint256 i; i < outcomes.length; ++i) { Outcome calldata outcome = outcomes[i]; uint256 newBalance = _balanceOf(account, outcome); + // Reverts with arithmetic underflow if balance decreased uint256 diff = newBalance - recordedBalances[i]; if (diff < outcome.amount) revert InvalidTokenAmount(outcome.amount, diff); } }solidity/script/setupanvil.s.sol (1)
4-4: Remove unusedconsoleimport.The
consoleimport fromforge-std/src/Script.solis not used in this script.🧹 Remove unused import
-import { Script, console } from "forge-std/src/Script.sol"; +import { Script } from "forge-std/src/Script.sol";typescript/src/types/types.ts (1)
159-159: Consider using a more explicit type forMaybeFactory.The
{} | Factoryunion means any object matches{}. This is technically correct but could lead to subtle bugs where an object with unrelated properties passes the type check.Consider a discriminated union or explicit partial:
export type MaybeFactory = { factory?: undefined; template?: undefined } | Factory;This makes it explicit that when factory/template aren't provided, they should be undefined rather than any arbitrary object matching.
solidity/test/CATValidator.t.sol (1)
226-234: Consider using a named constant for the magic value.The value
57896044618658097711785492504343953926634992332820282019728792003956564819968is1 << 255(SPEND_BALANCE_OF_MAGIC from CATValidator). Using a reference to the constant would improve readability.✨ Suggested improvement
+ uint256 constant SPEND_BALANCE_OF_MAGIC = 1 << 255; + function test_handleAllowances_0_spend_balanceOf( uint248 amount ) external { // ... AllowanceSpend[] memory targets = new AllowanceSpend[](1); targets[0] = AllowanceSpend({ token: token, allocated: 1 << 255, - spend: 57896044618658097711785492504343953926634992332820282019728792003956564819968 + spend: SPEND_BALANCE_OF_MAGIC });typescript/src/catapultar/account.spec.ts (1)
16-24: Consider making the polling delay configurable or using viem's built-in waitForTransactionReceipt.The hardcoded 50ms delay before checking the receipt may be insufficient under load or on slower systems, potentially causing flaky tests. Consider using viem's
waitForTransactionReceiptwith appropriate timeout settings instead.♻️ Suggested improvement
async function waitForTransaction(hash: `0x${string}`) { - await new Promise((resolve) => setTimeout(resolve, 50)); - // We need to wait for the transaction to be finalised. const publicClient = createPublicClient({ chain: anvil, transport: http(rpcUrl()), }); - await publicClient.getTransactionReceipt({ hash }); + return await publicClient.waitForTransactionReceipt({ hash }); }typescript/src/transaction/transaction.ts (1)
124-141: Unexpected side effect:getOpDatamutatesthis.signature.The
getOpDatamethod has a side effect at lines 132-134 where it reassignsthis.signatureto its compact form. This is unexpected for a getter-style method and could cause issues if called multiple times or in different contexts.Consider either:
- Renaming to indicate mutation (e.g.,
buildOpData)- Using a local variable instead of mutating
this.signature♻️ Proposed fix to avoid mutation
getOpData(options?: { compactSignature: boolean }): `0x${string}` { if (this.nonce === 0n) throw new Error( "Nonce 0 is not allowed. It cannot be differentiated from an invalid nonce.", ); if (!this.nonce) throw new Error("No nonce has been set"); const { compactSignature = true } = options ?? {}; + let sig = this.signature; - if (this.signature) { - this.signature = compactSignature - ? this.asCompactSignature() - : this.signature; - } - if (this.signature) { - return `0x${asHex(this.nonce, 32)}${this.signature.replace("0x", "")}`; + if (sig) { + sig = compactSignature ? this.asCompactSignature(sig) : sig; + return `0x${asHex(this.nonce, 32)}${sig.replace("0x", "")}`; } else { return asHex(this.nonce, 32, "0x"); } }typescript/src/catapultar/catapultar.spec.ts (1)
364-365: Remove unused code.These lines create an
Accountinstance and accesssignTypedDatabut the result is never used. This appears to be leftover debugging code.♻️ Proposed fix
- const a = Account.fromP256(privateKey); - a.signTypedData; - const wallet = privateKeyToAccount(PUBLIC_DEFAULT_ANVIL_ACCOUNT_0);typescript/src/catapultar/account.ts (1)
322-345: Consider using bitwise OR instead of addition for mask accumulation.Line 329 uses
mask += 1n << bitwhich works correctly when bits are unique, but usingmask |= 1n << bitis more semantically correct for bitmap operations and would be resilient against any potential duplicate bits.♻️ Suggested improvement
let mask = 0n; for (const bit of bits) { - mask += 1n << bit; + mask |= 1n << bit; }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@solidity/script/setupanvil.s.sol`:
- Around line 36-40: The low-level call to
0x4e59b44847b379578588920cA78FbF26c0B4956C in the deployment block currently
ignores the (bool, bytes) return value, so failures go unnoticed; change the
invocation in the setup script (the call that uses .call{ gas: 1000000 }(...))
to capture the return values (e.g., bool success, bytes memory data) and add a
require(success) check (or revert with a clear message) after the call to ensure
deployment succeeded.
In `@solidity/src/CATValidator.sol`:
- Around line 228-233: The assembly block contains a redundant nested `if
iszero(success)`—remove the inner unreachable check and directly revert after
copying return data: when `iszero(success)` in the outer guard, call
`returndatacopy(0x00, 0x00, returndatasize())` followed by `revert(0x00,
returndatasize())`; update the block that uses the `success` flag (the assembly
section referencing `returndatacopy`, `returndatasize`, and `revert`) to
eliminate the inner `if` and perform the copy+revert unconditionally within the
outer failure branch.
🧹 Nitpick comments (17)
typescript/scripts/rewrite-dist-imports.mjs (1)
55-62: Avoid unbounded concurrency when rewriting files.
Promise.allover every file can hitEMFILE(too many open files) on large builds. Consider sequential processing or a small concurrency limit.♻️ Safe sequential alternative
-const files = await walk(distDir); -await Promise.all( - files.map(async (file) => { - const source = await fs.readFile(file, "utf8"); - const updated = rewrite(source); - if (updated !== source) await fs.writeFile(file, updated); - }), -); +const files = await walk(distDir); +for (const file of files) { + const source = await fs.readFile(file, "utf8"); + const updated = rewrite(source); + if (updated !== source) await fs.writeFile(file, updated); +}typescript/src/catapultar/account.spec.ts (1)
16-24: UsewaitForTransactionReceiptfor robust, deterministic finality.
The current approach with a fixed 50ms sleep and single receipt fetch is unreliable. viem'swaitForTransactionReceiptprovides built-in polling with configurable timeout, retry logic with exponential backoff, and automatic replacement detection—all of which eliminate the need for arbitrary sleep durations.🔁 Suggested change
async function waitForTransaction(hash: `0x${string}`) { - await new Promise((resolve) => setTimeout(resolve, 50)); - // We need to wait for the transaction to be finalised. const publicClient = createPublicClient({ chain: anvil, transport: http(rpcUrl()), }); - await publicClient.getTransactionReceipt({ hash }); + await publicClient.waitForTransactionReceipt({ hash }); }typescript/src/transaction/transaction.ts (2)
131-135: Mutatingthis.signaturein a getter-style method is a side effect.
getOpDatamutatesthis.signaturewhencompactSignatureis true. This can cause unexpected behavior if the method is called multiple times or if the caller expects the original signature to remain unchanged.Consider returning the compact signature without mutating the instance:
♻️ Proposed fix to avoid mutation
getOpData(options?: { compactSignature: boolean }): `0x${string}` { if (this.nonce === 0n) throw new Error( "Nonce 0 is not allowed. It cannot be differentiated from an invalid nonce.", ); if (!this.nonce) throw new Error("No nonce has been set"); const { compactSignature = true } = options ?? {}; + const sig = this.signature + ? compactSignature + ? this.asCompactSignature() + : this.signature + : undefined; - if (this.signature) { - this.signature = compactSignature - ? this.asCompactSignature() - : this.signature; - } - if (this.signature) { - return `0x${asHex(this.nonce, 32)}${this.signature.replace("0x", "")}`; + if (sig) { + return `0x${asHex(this.nonce, 32)}${sig.replace("0x", "")}`; } else { return asHex(this.nonce, 32, "0x"); } }
167-180: Clarify whyvalueis hardcoded to0ninasCallData.The method returns
value: 0nwhile the transaction may contain calls with non-zero values (retrievable viagetTotalValue()). If this is intentional because the value is encoded withinexecutionData, consider adding a brief doc comment to clarify this design choice for future maintainers.typescript/src/transaction/constrainedtransaction.ts (2)
132-142: Variabletypehashshadows outer scope declaration.The
typehashvariable declared on line 132 shadows thetypehashfrom line 98. While this works correctly due to block scoping, using a distinct name (e.g.,refundTypehash) would improve readability and prevent accidental misuse.♻️ Proposed rename for clarity
- const typehash = hashTypedData({ + const refundTypehash = hashTypedData({ types: ExecutionConstraintTyped, primaryType: "ExecutionConstraint", message: refundExecutionConstraint, domain: { chainId: this.chainId, name: "CAT Validator", version: "1", verifyingContract: validator, }, }); calls.push({ to: zeroAddress, value: 0n, data: encodeFunctionData({ abi: CATAPULTAR_V0_1_0_ABI, functionName: "setSignature", - args: [typehash, DigestApproval.Signature], + args: [refundTypehash, DigestApproval.Signature], }), });
154-158: Document why nonce is hardcoded to1n.The transaction is always created with
setNonce(1n). If this is intentional (e.g., first transaction on a newly deployed account), add a comment explaining this constraint. Otherwise, consider making the nonce configurable via theoptparameter.solidity/test/CallProxy.t.sol (1)
38-40: **Unchecked return value from low-level call.**The return value from the low-levelc.call(payload)is not captured or asserted. "When using expectRevert with low-level calls, the status boolean indicates whether the expectation was met, not whether the call reverted." Whilevm.expectCallverifies the call happened, the success status should still be captured and asserted for consistency with the other test functions.♻️ Proposed fix to capture and assert return value
function test_forwardCall( address target, bytes calldata cdata ) external { vm.assume(target.code.length == 0); bytes memory payload = abi.encodePacked(bytes32(uint256(uint160(target))), cdata); vm.expectCall(target, cdata); - c.call(payload); + (bool success,) = c.call(payload); + assertEq(success, true); }typescript/src/catapultar/catapultar.spec.ts (1)
364-365: Remove unused variablea.The variable
acreated fromAccount.fromP256(privateKey)is never used. The referencea.signTypedDataon line 365 appears to be a stray statement with no effect.🧹 Proposed removal of unused code
- const a = Account.fromP256(privateKey); - a.signTypedData; - const wallet = privateKeyToAccount(PUBLIC_DEFAULT_ANVIL_ACCOUNT_0);typescript/src/config.ts (1)
10-11: Document that this is a well-known Anvil test private key.
PUBLIC_DEFAULT_ANVIL_ACCOUNT_0is the first default Anvil account private key. Consider adding a comment clarifying this is intentionally public test data to prevent future security concerns.📝 Proposed documentation addition
+/** Well-known first Anvil account private key - intentionally public for local testing only */ export const PUBLIC_DEFAULT_ANVIL_ACCOUNT_0 = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";solidity/src/libs/LibExecutionConstraint.sol (1)
46-58: Clarify thatallowancesHashusesallocatednotspend.The function hashes
allowance.allocated(line 54) rather thanallowance.spend. This appears intentional for constraint validation (the constraint is pre-signed with the allocated amount, while spend can vary at execution time). Consider adding a NatSpec comment to clarify this design decision.typescript/src/types/types.ts (1)
47-52: Remove or document commented-outSignabletype.The commented-out
Signabletype has no explanation. If it's planned for future use, add a TODO comment; otherwise, remove it to keep the codebase clean.solidity/test/CATValidator.t.sol (2)
304-329: Hardcoded indexi == 2may cause test to skip native token path.When
ad.length < 3, the native token (ETH) branch atif (i == 2)is never exercised. Consider using a modulo-based approach or ensuring minimum array length.💡 Suggested approach
- if (i == 2) { + if (i == ad.length / 2 || (ad.length > 0 && i == 0)) {Or add
vm.assume(ad.length >= 3)to ensure the native token path is always tested.Also applies to: 331-376
398-399: Non-specific revert expectation.Using
vm.expectRevert()without a selector makes the test fragile - it will pass for any revert reason. Consider specifying the expected error selector for more precise testing.💡 Suggested fix
- vm.expectRevert(); + vm.expectRevert(abi.encodeWithSelector(CATValidator.InvalidTokenAmount.selector, 1, 0));solidity/test/helpers/CATValidator.t.sol (1)
314-339: Same hardcoded index issue as the other test file.The
if (i == 2)pattern for native token testing has the same fragility issue - skipped whenad.length < 3.Also applies to: 341-386
solidity/src/CATValidator.sol (2)
99-102: Consider simplifying nonce spend logic.The expression
!spentwhenspentis guaranteed to befalse(due to the revert on line 100) could be simplified totruefor clarity.💡 Suggested simplification
function _checkNonce( address account, uint256 nonce ) internal { bool spent = spentNonces[account][nonce]; if (spent) revert NonceAlreadySpent(); - spentNonces[account][nonce] = !spent; + spentNonces[account][nonce] = true; }
165-170: Underflow possible if balance decreases unexpectedly.Line 168 computes
newBalance - recordedBalances[i]. If the balance decreased (e.g., due to fees, reentrancy, or edge cases), this will revert with a panic rather than the descriptiveInvalidTokenAmounterror. Consider using checked subtraction with a clear error path.💡 Suggested approach
- uint256 diff = newBalance - recordedBalances[i]; - if (diff < outcome.amount) revert InvalidTokenAmount(outcome.amount, diff); + if (newBalance < recordedBalances[i]) revert InvalidTokenAmount(outcome.amount, 0); + uint256 diff = newBalance - recordedBalances[i]; + if (diff < outcome.amount) revert InvalidTokenAmount(outcome.amount, diff);typescript/src/catapultar/account.ts (1)
327-330: Consider using bitwise OR for mask accumulation.Using
+=works for non-overlapping bits but|=is the idiomatic choice for bitmask operations and is safer if bits could overlap (e.g., duplicate nonces in input).💡 Suggested fix
let mask = 0n; for (const bit of bits) { - mask += 1n << bit; + mask |= 1n << bit; }
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@solidity/src/CATValidator.sol`:
- Around line 179-194: _handleAllowances currently calls
SafeTransferLib.safeTransferFrom for every AllowanceSpend, which will revert for
native ETH (token == address(0)); update _handleAllowances to explicitly detect
allowance.token == address(0) and revert with a clear descriptive error (e.g.,
NativeTokenNotSupported or revert with a message) before computing spend or
calling SafeTransferLib.safeTransferFrom, referencing AllowanceSpend,
SPEND_BALANCE_OF_MAGIC and SafeTransferLib.safeTransferFrom so reviewers can
locate the change.
- Around line 67-87: The entry function currently treats nonce==0 as "never
checked" allowing unlimited replay; add an on-chain cancellation mechanism by
(a) implementing a public revokeApproval or cancelNonce function (e.g.,
revokeApproval(uint256 nonce) / cancelNonceFor(address signer, uint256 nonce) or
a dedicated cancelNonce0(address signer)) that marks a nonce as used/invalid in
the same nonce tracking storage used by _checkNonce, and (b) update _checkNonce
and _validateApproval (or add a new internal path) so that when a signer has
revoked nonce 0 the entry call will revert; reference the entry, _checkNonce,
_validateApproval symbols when modifying logic and the nonce-tracking storage so
revocations for nonce 0 are honored by entry.
- Around line 160-171: _guard the subtraction in _compareOutcomes to avoid
underflow: inside _compareOutcomes (which calls _balanceOf) first check that
newBalance >= recordedBalances[i] and revert with a clear error if not, then
compute diff = newBalance - recordedBalances[i] and keep the existing check that
reverts InvalidTokenAmount(outcome.amount, diff) when diff < outcome.amount;
this prevents the Solidity underflow panic and preserves your custom
InvalidTokenAmount semantics.
- Around line 206-232: The assembly call currently forwards the contract's
entire balance via selfbalance(), which can unintentionally drain ETH; update
the call in the assembly block that constructs calldata for callProxy (the block
using execTarget, execPayload, callProxy) to pass 0 as the value parameter
instead of selfbalance() (or alternatively refactor the surrounding function to
accept an explicit uint256 value parameter and use that value in place of
selfbalance()); ensure the new value is used in the call(...) invocation and
adjust any related NatSpec/comments to reflect the change.
In `@typescript/README.md`:
- Line 107: Fix the grammar in the README sentence that currently reads
"Catapultar can be configured for this use case by embedded a call (or
signature) digest" by changing "by embedded a call" to "by embedding a call" so
the line becomes "Catapultar can be configured for this use case by embedding a
call (or signature) digest"; locate the sentence in the README.md (the line
containing that exact phrase) and update it accordingly.
- Around line 141-148: The example mistakenly declares `const tx =
tx.asCatapultarAllowanceTransaction()` which self-references an undefined `tx`;
change the call to use the previously created `cat` instance (i.e. call
`cat.asCatapultarAllowanceTransaction()`), then use that resulting `tx` for the
subsequent `tx.asAccount(...)` and `tx.asExecuteCall(...)` calls so
`ConstrainedAssetTransaction`, `cat`, and the methods
`asCatapultarAllowanceTransaction`, `asAccount`, and `asExecuteCall` are
referenced correctly.
In `@typescript/src/catapultar/account.ts`:
- Around line 322-345: In getSpendNoncesCalls, the bitmap construction uses mask
+= 1n << bit which incorrectly double-counts when nonces repeat; change the
update to use bitwise OR (mask |= 1n << bit) so setting a bit is idempotent;
locate mask in getSpendNoncesCalls where bits are iterated, replace the += with
|= and leave the rest (CATAPULTAR_V0_1_0_ABI and invalidateUnorderedNonces
encoding) unchanged.
In `@typescript/src/catapultar/catapultar.spec.ts`:
- Around line 129-134: The two identical assertions calling
tx.hasValidSignature({ noSignatureIsValid: true }) are redundant; update the
second assertion to call tx.hasValidSignature({ noSignatureIsValid: false }) so
you test both flag cases (i.e., keep the first assertion as-is and change the
duplicate to expect(await tx.hasValidSignature({ noSignatureIsValid: false
})).toBe(true)) referencing the tx.hasValidSignature(...) call in the spec.
- Around line 20-21: Remove the dead/unused Account usage: delete the import of
Account from "viem/tempo" and remove the code that constructs a via
Account.fromP256(privateKey) and accesses a.signTypedData (the unused variable
`a` and its signTypedData access are no-ops). Keep the necessary imports Base64
and P256 intact and ensure no other references to Account remain in the file.
In `@typescript/src/transaction/transaction.ts`:
- Around line 124-141: getOpData currently mutates this.signature when
compactSignature is true; instead compute the compacted signature into a local
variable and leave this.signature unchanged. In getOpData, introduce a local sig
(or similar) that is set to this.signature ? (compactSignature ?
this.asCompactSignature() : this.signature) : undefined and use sig when
building the returned hex string; do not reassign this.signature. Ensure
asCompactSignature() is still used to produce the compact form and that
downstream callers (e.g., asParameters or direct .signature access) observe the
original instance field unchanged.
🧹 Nitpick comments (21)
typescript/src/catapultar/catapultar.spec.ts (1)
32-40: UsewaitForTransactionReceiptinstead of a fixed delay + single receipt fetch.A 50ms sleep followed by
getTransactionReceiptis brittle and can cause flaky tests if the transaction isn't mined within that window. Viem provideswaitForTransactionReceiptwhich polls until mined, with configurable retries and timeout.Proposed fix
async function waitForTransaction(hash: `0x${string}`) { - await new Promise((resolve) => setTimeout(resolve, 50)); - // We need to wait for the transaction to be finalised. const publicClient = createPublicClient({ chain: anvil, transport: http(rpcUrl()), }); - return await publicClient.getTransactionReceipt({ hash }); + return await publicClient.waitForTransactionReceipt({ hash }); }solidity/src/libs/LibExecutionConstraint.sol (1)
44-44: Naming inconsistency:OUTPUT_TYPE_HASHvsOutcomestruct.The constant is named
OUTPUT_TYPE_HASHbut it hashes the string"Outcome(...)"and corresponds to theOutcomestruct. Consider renaming toOUTCOME_TYPE_HASHfor consistency with the other constants (ALLOWANCE_TYPE_HASHmaps toAllowance,EXECUTION_CONSTRAINT_TYPE_HASHmaps toExecutionConstraint).Proposed fix
- bytes32 constant OUTPUT_TYPE_HASH = keccak256(bytes("Outcome(address token,uint256 amount,address destination)")); + bytes32 constant OUTCOME_TYPE_HASH = keccak256(bytes("Outcome(address token,uint256 amount,address destination)"));Then update references in
outcomesHash(line 67) and the test file accordingly.solidity/src/libs/KeyedOwnable.sol (1)
187-199: Stale key slot not cleared when downgrading from a 2-slot key to a 1-slot key.When
_transferOwnershipswitches from P256/WebAuthn (2 slots) to ECDSA (1 slot), slot index 1 (_OWNER_SLOT - 1) retains the old Y-coordinate. WhilepublicKeyTypegates which slots are semantically read, this leaves residual data in storage. Consider zeroing extra slots of the previous key type if the new key is shorter, especially as a defense-in-depth measure given the type confusion issue in_validateSignature.Suggested approach
function _transferOwnership( PublicKeyType ktp, bytes32[] calldata nextKey ) internal { if (!_isValidKey(ktp, nextKey)) revert InvalidKey(); + // Clear stale slots if downgrading key length + uint256 prevLength = _keyTypeLength(publicKeyType); uint256 nextKeyLength = nextKey.length; for (uint256 i; i < nextKeyLength; ++i) { _setPublicKeySlice(i, nextKey[i]); } + for (uint256 i = nextKeyLength; i < prevLength; ++i) { + _setPublicKeySlice(i, bytes32(0)); + } publicKeyType = ktp; emit OwnershipTransferred(ktp, nextKey); }solidity/test/Integration.t.sol (1)
348-358:swapmock function uses fragile cumulative balance tracking.The
balanceOfSwapstate variable tracks cumulative balance diffs, butdiffwill be 0 on subsequent calls if no new tokens are transferred between calls. Additionally, the function mintsamountof outcome tokens regardless of actualdiffreceived — this could mask bugs where the allowance transfer didn't send the expected amount.For test purposes this works for a single invocation, but consider asserting
diff == amountto validate that the correct amount was forwarded.typescript/src/types/types.ts (1)
47-52: Remove commented-outSignabletype.Dead code that adds noise. If it's planned for future use, consider tracking it in an issue instead.
typescript/src/transaction/transaction.ts (1)
167-180:asCallDataalways returnsvalue: 0n, ignoringgetTotalValue().If any calls contain non-zero
value, the outer transaction must forward ETH. Returningvalue: 0nhere means the caller must separately account for ETH. This may be intentional (ETH is forwarded viamsg.valueat a higher level), but it could mislead callers who expect the returnedCallto be self-contained.Consider at minimum adding a doc comment noting this intentional behavior.
typescript/src/config.ts (1)
10-11:PUBLIC_DEFAULT_ANVIL_ACCOUNT_0is the well-known Anvil/Hardhat test private key.The naming makes it clear this is a public test key. Ensure this is never referenced in production deployment paths. If it's only used in test files, consider moving it to a test-only config module to reduce the risk surface.
typescript/src/catapultar/account.spec.ts (2)
12-14: Minor duplication:PUBLIC_DEFAULT_ANVIL_ACCOUNT_0is redefined locally.This constant is already exported from
typescript/src/config.ts. Consider importing it instead.-const PUBLIC_DEFAULT_ANVIL_ACCOUNT_0 = - "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"; +import { PUBLIC_DEFAULT_ANVIL_ACCOUNT_0 } from "../config";
16-24:waitForTransactiondoes not handle the case where the receipt is unavailable.
getTransactionReceiptcan throw if the transaction hasn't been mined yet. The 50ms delay is short and may not always suffice. Consider usingwaitForTransactionReceiptfrom viem, which polls until the receipt is available.Suggested improvement
async function waitForTransaction(hash: `0x${string}`) { - await new Promise((resolve) => setTimeout(resolve, 50)); - // We need to wait for the transaction to be finalised. const publicClient = createPublicClient({ chain: anvil, transport: http(rpcUrl()), }); - await publicClient.getTransactionReceipt({ hash }); + await publicClient.waitForTransactionReceipt({ hash }); }typescript/src/transaction/constrainedtransaction.ts (3)
109-117: Shadowedtypehashvariable name in refund block.The
const typehashdeclared at line 98 and the one at line 132 share the same name. While JavaScript scoping rules make this work (the inner one is in theifblock), it reduces readability.Also applies to: 143-151
109-111:to: zeroAddressfor self-calls is a non-obvious convention — consider a named constant or comment.Using
zeroAddressas thetofield forsetSignaturecalls relies on the Catapultar smart account interpretingaddress(0)as a self-call. A brief inline comment explaining this convention (or a named constant likeSELF_CALL_TARGET) would improve discoverability.
20-33: Consider makingaddAllowancesandaddOutcomeschainable.Both
addAllowancesandaddOutcomesmutate state but returnvoid. Returningthiswould enable fluent-style chaining, consistent withBaseTransaction.addCall(...).solidity/test/CallProxy.t.sol (1)
31-40: Return value ofc.call(payload)is discarded in the success path.In
test_forwardCall, the(bool success, bytes memory)return from the low-level call is not captured. Whilevm.expectCallverifies the outbound call, assertingsuccess == truewould strengthen the test.solidity/test/helpers/CATValidator.t.sol (2)
155-191:vm.assumeinside a loop over fuzz array may cause excessive test rejections.Line 163 uses
vm.assume(amounts[i] >= 2)within a loop over the fuzzedamountsarray. Each element must satisfy the constraint, and a single failure rejects the entire input. For large arrays, this significantly reduces the hit rate of valid fuzz inputs. Consider bounding the values usingvm.bound(amounts[i], 2, type(uint256).max)instead if Foundry supports it, or moving to a bounded fuzz approach.
314-339: Hardcodedi == 2for native ETH testing limits coverage.The native ETH path (line 322:
if (i == 2)) only runs when the fuzz array has at least 3 elements. Fuzz runs withad.length <= 2skip ETH handling entirely. Consider a separate dedicated test for ETH outputs rather than embedding it in a conditional on a fuzz index.typescript/src/catapultar/account.ts (2)
220-226:publicClient()creates a new viemPublicClienton every invocation.Methods like
validateNonces,getNextValidNonce, andvalidateOwnercallthis.publicClient()which allocates a new transport and client each time. For methods that issue multiple RPC calls (e.g.,getNextValidNonceloops up toattemptstimes), this is wasteful.Consider caching the client instance (keyed on
rpc+chainId), or at minimum reusing a single client within multi-call methods.
258-265:attachRpcmutatesthiswhile also returning a re-typed reference.This is a surprise for callers who might expect immutability. The mutation of
this.rpcandthis.chainIdmeans the original reference is also modified. Consider either documenting this clearly or returning a new instance.solidity/src/CATValidator.sol (1)
95-102: Nit:!spentis alwaystruehere — prefer explicit= true.Since line 100 reverts when
spentistrue, the value ofspenton line 101 is guaranteedfalse, making!spentalwaystrue. Writing= truedirectly is clearer and avoids the cognitive indirection.♻️ Suggested simplification
function _checkNonce( address account, uint256 nonce ) internal { - bool spent = spentNonces[account][nonce]; - if (spent) revert NonceAlreadySpent(); - spentNonces[account][nonce] = !spent; + if (spentNonces[account][nonce]) revert NonceAlreadySpent(); + spentNonces[account][nonce] = true; }solidity/test/CATValidator.t.sol (3)
304-329: Hardcodedi == 2for native-token branch may leave the path untested.In
test_fuzz_recordBalances(and identically intest_fuzz_compareOutcomes), the native-token code path is only exercised whenad.length >= 3. If the fuzzer generates arrays of length 0, 1, or 2, theaddress(0)token branch is never reached. Consider addingvm.assume(ad.length >= 3)or using a separate dedicated test for the native-token path.
225-230: Replace magic literal with a named constant for clarity.The value
57896044618658097711785492504343953926634992332820282019728792003956564819968is1 << 255(i.e.,SPEND_BALANCE_OF_MAGIC). Since the contract's constant isinternal, the test can't reference it directly, but defining a local constant in the test contract would improve readability.♻️ Suggested approach
contract CATValidatorTest is LibExecutionConstraintTest { CATValidatorMock validator; + uint256 constant SPEND_BALANCE_OF_MAGIC = 1 << 255;Then replace the literals:
- targets[0] = AllowanceSpend({ - token: token, - allocated: 1 << 255, - spend: 57896044618658097711785492504343953926634992332820282019728792003956564819968 - }); + targets[0] = AllowanceSpend({ + token: token, + allocated: SPEND_BALANCE_OF_MAGIC, + spend: SPEND_BALANCE_OF_MAGIC + });Also applies to: 249-254
378-400:test_entry_decrease_balances: genericvm.expectRevert()weakens the assertion.Line 398 uses
vm.expectRevert()without specifying an expected error selector. This means the test passes on any revert, including unexpected ones (e.g., a signature validation failure, reentrancy guard, or an unrelated panic). Consider asserting the specific revert reason to ensure the test validates the intended outcome-checking logic.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@solidity/src/CATValidator.sol`:
- Around line 168-175: The unchecked block in _compareOutcomes allows
recordedBalances[i] + outcome.amount to wrap and silently bypass validation;
change this to a checked operation by removing the unchecked block (or
explicitly require outcome.amount <= type(uint256).max - recordedBalances[i]) so
the addition reverts on overflow, then compute the expected sum and use that to
validate newBalance and compute diff for InvalidTokenAmount; reference
recordedBalances, outcome.amount, newBalance, and the InvalidTokenAmount revert.
In `@typescript/README.md`:
- Around line 87-101: Remove the stray bare expression "CatapultarTx;" from the
README example; locate the example block that declares account and then creates
new CatapultarTx (symbols: CatapultarTx, tx, account) and delete that standalone
statement, and if the intent was to show an import, replace it with a proper
import line or a brief comment explaining where CatapultarTx comes from so the
example remains correct and non-confusing.
In `@typescript/src/catapultar/account.ts`:
- Around line 535-545: In validateOwner (method validateOwner on
CatapultarAccount), the ECDSA branch compares this.pubkey to the on-chain
actualAccountOwner using strict equality which is case-sensitive; normalize both
values (e.g., call .toLowerCase() on this.pubkey and actualAccountOwner) before
comparing so checksummed/mixed-case addresses match, keeping the same error
message and throwing behavior if they differ.
- Around line 180-193: ownerInSalt currently omits the keyType when computing
the owner hash and compares the full 32-byte keccak256 hex to a 20-byte salt
slice, so include the keyType in the keccak256 input and truncate the computed
hash to 20 bytes before comparing. Specifically, when computing saltPrefix in
ownerInSalt use keccak256 over an encodePacked that includes the key type
(AccountPublicKeyType value) and the keccak256 of the owner pubkey (use
pubkeyAsArray as before), then take the first 20 bytes (first 42 hex chars
including "0x") of that keccak result to produce saltPrefix; finally compare
that 20-byte hex string to the saltSlice (or convert both to BigInt
consistently) so the comparison between the computed saltPrefix and
opt.salt.slice(0, 42) is correct.
In `@typescript/src/transaction/transaction.ts`:
- Line 182: Fix the typo in the JSDoc comment that reads "Return the calls as an
approval diget. This can be used to "embed" the calls into an account" by
changing "diget" to "digest" so it reads "Return the calls as an approval
digest..." — update the comment near the relevant function in transaction.ts
where this exact sentence appears to correct the spelling.
🧹 Nitpick comments (2)
typescript/README.md (1)
81-83: TODO: Account deploy section is incomplete.The "Account deploy" section only contains a TODO placeholder. If this is meant to be documented before release, consider filling it in.
Would you like me to draft the Account deploy documentation based on the
CatapultarAccount.deploystatic method?typescript/src/catapultar/account.ts (1)
99-161: GenericVondeployis unused at runtime — the constructed account always gets version"0.1.0".The return type claims
CatapultarAccount<V, undefined, AKT>, but the constructor call on line 155 doesn't pass aversion, so it defaults to"0.1.0"regardless of whatVresolves to. This makes the generic parameter misleading. Consider either removingVfromdeploy's signature or accepting aversionin the options and forwarding it.
| private static ownerInSalt<AKT extends AccountPublicKeyType>( | ||
| opt: { | ||
| salt: `0x${string}`; | ||
| } & Pubkey<AKT>, | ||
| ) { | ||
| const pubkeyArray = pubkeyAsArray(opt); | ||
| const saltPrefix = | ||
| opt.keyType === AccountPublicKeyType.ECDSAOrSmartContract | ||
| ? opt.pubkey | ||
| : keccak256(encodePacked(["bytes32[]"], [pubkeyArray])); | ||
|
|
||
| const saltSlice = opt.salt.slice(0, 20 * 2 + 2); | ||
| return BigInt(saltSlice) === 0n || saltSlice === saltPrefix; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual implementation of ownerInSalt
cat -n typescript/src/catapultar/account.ts | sed -n '180,193p'Repository: lifinance/catapultar
Length of output: 630
🏁 Script executed:
# Check the full context around this function
cat -n typescript/src/catapultar/account.ts | sed -n '175,200p'Repository: lifinance/catapultar
Length of output: 921
🏁 Script executed:
# Look at the Solidity factory to understand how salt is handled
find . -name "CatapultarFactory.sol" -o -name "*Factory*.sol" | head -5Repository: lifinance/catapultar
Length of output: 140
🏁 Script executed:
# Search for how salt is validated in Solidity
rg -n "salt" --type sol -A 5 -B 5 | grep -A 5 -B 5 -i "owner\|pubkey" | head -100Repository: lifinance/catapultar
Length of output: 91
🏁 Script executed:
# Check if there are tests that show expected behavior for P256 salts
fd -e test.ts -e test.js | xargs grep -l "P256\|WebAuthn" | head -5Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Look for usage examples of ownerInSalt with P256 keys
rg -n "ownerInSalt" typescript/ -A 3 -B 3Repository: lifinance/catapultar
Length of output: 1021
🏁 Script executed:
# Check the Solidity factory implementation
cat solidity/src/CatapultarFactory.sol | head -150Repository: lifinance/catapultar
Length of output: 6682
🏁 Script executed:
# Look for salt-related validation in Solidity factory
rg -n "salt" solidity/src/CatapultarFactory.sol -B 3 -A 3Repository: lifinance/catapultar
Length of output: 5919
🏁 Script executed:
# Check test files for P256 or WebAuthn salt examples
fd -e "*.ts" -e "*.js" typescript/test | head -10Repository: lifinance/catapultar
Length of output: 429
🏁 Script executed:
# Search for P256 test cases
rg -n "P256\|WebAuthn" typescript/ -lRepository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Get the rest of the _saltPrefix function in Solidity
cat -n solidity/src/CatapultarFactory.sol | sed -n '147,170p'Repository: lifinance/catapultar
Length of output: 856
🏁 Script executed:
# Look for the checkStartsWith function to understand what it does
rg -n "checkStartsWith" solidity/ -B 2 -A 5Repository: lifinance/catapultar
Length of output: 565
🏁 Script executed:
# Check if there are any tests showing P256 salt encoding
fd . typescript/test --type f | head -5Repository: lifinance/catapultar
Length of output: 88
🏁 Script executed:
# Look more carefully at the salt prefix computation in Solidity
rg -n "_saltPrefix\|checkStartsWith" solidity/src/CatapultarFactory.sol -A 10Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Let me check if pubkeyAsArray includes the keyType or just the pubkey data
cat -n typescript/src/catapultar/account.ts | sed -n '1,50p'Repository: lifinance/catapultar
Length of output: 1929
🏁 Script executed:
# Find the pubkeyAsArray function definition
rg -n "pubkeyAsArray" typescript/src/catapultar/ -B 2 -A 8Repository: lifinance/catapultar
Length of output: 5390
🏁 Script executed:
# Check how P256 pubkeys are encoded
rg -n "P256\|WebAuthn" typescript/src/catapultar/ -B 2 -A 2Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Find the pubkeyAsArray helper function
rg -n "export.*pubkeyAsArray\|function pubkeyAsArray" typescript/src/utils/ -A 15Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Look at the helpers file
cat typescript/src/utils/helpers.ts | head -100Repository: lifinance/catapultar
Length of output: 3307
🏁 Script executed:
# Look at test cases to see if P256 salt validation is tested
cat -n typescript/src/catapultar/account.spec.ts | grep -A 30 "P256\|WebAuthn"Repository: lifinance/catapultar
Length of output: 2800
🏁 Script executed:
# Check what the test parameters are
cat -n typescript/src/catapultar/account.spec.ts | sed -n '80,130p'Repository: lifinance/catapultar
Length of output: 1958
🏁 Script executed:
# Look for how the Solidity hash is computed - search for EfficientHashLib
rg -n "EfficientHashLib" solidity/Repository: lifinance/catapultar
Length of output: 2394
🏁 Script executed:
# Compare what the TypeScript encodes vs what Solidity encodes
# Look more carefully at the encodePacked vs EfficientHashLib.hash
cat -n typescript/src/catapultar/account.ts | sed -n '185,193p'Repository: lifinance/catapultar
Length of output: 466
🏁 Script executed:
# Check if the keyType is supposed to be included in the hash
# Look for any encoding of keyType in the TypeScript
rg -n "keyType" typescript/src/catapultar/account.ts | head -20Repository: lifinance/catapultar
Length of output: 381
🏁 Script executed:
# Let me verify what EfficientHashLib.hash does with the test to understand the expected behavior better
cat -n typescript/src/catapultar/account.spec.ts | sed -n '133,150p'Repository: lifinance/catapultar
Length of output: 807
🏁 Script executed:
# Check the Solidity test to see how P256 salts are constructed there
cat solidity/test/CatapultarFactory.t.sol | head -200Repository: lifinance/catapultar
Length of output: 7709
🏁 Script executed:
# Check if there are tests with non-zero P256 salt prefixes
rg -n "P256\|WebAuthn" solidity/test/ -A 20 -B 5Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Let me trace through what the actual hash should be by looking at the imports and understanding how EfficientHashLib works
python3 << 'EOF'
# Verify the mismatch between Solidity and TypeScript hashing
print("SOLIDITY SIDE:")
print("_saltPrefix for P256/WebAuthn:")
print(" EfficientHashLib.hash(bytes32(uint256(uint8(ktp))), EfficientHashLib.hash(owner))")
print(" - Includes keyType in the hash")
print(" - Result passed to address(bytes20(...))")
print("")
print("TYPESCRIPT SIDE:")
print("ownerInSalt for P256/WebAuthn:")
print(" keccak256(encodePacked(['bytes32[]'], [pubkeyArray]))")
print(" - Does NOT include keyType in the hash")
print(" - Result is full 32-byte hash (66 chars with 0x)")
print(" - saltSlice is first 20 bytes (42 chars with 0x)")
print(" - String comparison: 42 chars === 66 chars will always be false")
print("")
print("TEST BEHAVIOR:")
print(" P256 test uses salt: 0x${asHex(0n, 20)}${random(12)...}")
print(" This is: 0x + 40 zeros + random = zero-prefixed salt")
print(" ownerInSalt check: BigInt('0x' + 40 zeros) === 0n → TRUE")
print(" Test passes because of the 0n check, not the string comparison")
EOFRepository: lifinance/catapultar
Length of output: 836
🏁 Script executed:
# Confirm that the keyType is missing from the TypeScript hash by looking at what EfficientHashLib does
# Search for how the keyType is used in Solidity
rg -n "uint8(ktp)" solidity/src/ -B 3 -A 3Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Let me trace through a concrete example to confirm the bug
python3 << 'EOF'
from hashlib import sha256, keccak
# Simulate what happens with P256 keys
# Example: P256 pubkey = [0x1234...abcd, 0x5678...ef01]
# TypeScript: keccak256(encodePacked(["bytes32[]"], [pubkeyArray]))
# This hashes the two 32-byte values
example_hash = "0xabcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"
print(f"TypeScript saltPrefix (P256): {example_hash}")
print(f"Length: {len(example_hash)} chars (32 bytes = 66 chars with 0x)")
print()
# TypeScript: saltSlice = opt.salt.slice(0, 42)
salt = "0x0000000000000000000000000000000000000000" + "123456abcdef"
saltSlice = salt[0:42]
print(f"Example salt: {salt}")
print(f"saltSlice: {saltSlice}")
print(f"Length: {len(saltSlice)} chars (20 bytes = 42 chars with 0x)")
print()
# The comparison
print(f"String comparison: saltSlice === saltPrefix")
print(f" '{saltSlice}' === '{example_hash}'")
print(f" Result: {saltSlice == example_hash}")
print()
# What the test passes with:
print("Current test behavior:")
print(f" Test uses zero-prefixed salt: 0x{'0'*40}... (40 zeros)")
print(f" BigInt('0x' + '0'*40) === 0n: True")
print(f" So test passes via first condition (BigInt check), bypassing string comparison")
EOFRepository: lifinance/catapultar
Length of output: 225
🏁 Script executed:
# Now verify that the Solidity side DOES include keyType by checking the actual hash computation
cat -n solidity/src/CatapultarFactory.sol | sed -n '156,165p'Repository: lifinance/catapultar
Length of output: 572
🏁 Script executed:
# Search for any comments or documentation about why keyType might be excluded
rg -n "keyType\|ktp" typescript/src/catapultar/account.ts -B 2 -A 2 | grep -A 5 -B 5 "ownerInSalt"Repository: lifinance/catapultar
Length of output: 46
🏁 Script executed:
# Check the git history or comments around ownerInSalt
rg -n "ownerInSalt" solidity/src/CatapultarFactory.sol -B 5 -A 15Repository: lifinance/catapultar
Length of output: 5454
🏁 Script executed:
# Confirm the Solidity comment about the hash computation
cat -n solidity/src/CatapultarFactory.sol | sed -n '138,154p'Repository: lifinance/catapultar
Length of output: 959
🏁 Script executed:
# Verify the Solidity specification one more time
cat -n solidity/src/CatapultarFactory.sol | sed -n '138,165p'Repository: lifinance/catapultar
Length of output: 1476
ownerInSalt fails for P256/WebAuthn keys due to missing keyType in hash computation.
The TypeScript implementation omits keyType from the hash computation, deviating from the Solidity specification (line 142: "keccak256(ktp, keccak256(owner))") and implementation (line 163). Additionally, the resulting 32-byte hash (66 chars) is compared against a 20-byte salt slice (42 chars), causing the string comparison to always fail.
P256/WebAuthn tests pass only because they use zero-prefixed salts that satisfy BigInt(saltSlice) === 0n, bypassing the broken validation. Non-zero-prefixed salts will always fail validation regardless of correctness.
The fix requires including keyType in the hash and properly truncating to 20 bytes:
Proposed fix
const pubkeyArray = pubkeyAsArray(opt);
const saltPrefix =
opt.keyType === AccountPublicKeyType.ECDSAOrSmartContract
? opt.pubkey
- : keccak256(encodePacked(["bytes32[]"], [pubkeyArray]));
+ : ("0x" + keccak256(encodePacked(
+ ["uint8", "bytes32[]"],
+ [opt.keyType, pubkeyArray]
+ )).slice(2, 42)) as `0x${string}`;🤖 Prompt for AI Agents
In `@typescript/src/catapultar/account.ts` around lines 180 - 193, ownerInSalt
currently omits the keyType when computing the owner hash and compares the full
32-byte keccak256 hex to a 20-byte salt slice, so include the keyType in the
keccak256 input and truncate the computed hash to 20 bytes before comparing.
Specifically, when computing saltPrefix in ownerInSalt use keccak256 over an
encodePacked that includes the key type (AccountPublicKeyType value) and the
keccak256 of the owner pubkey (use pubkeyAsArray as before), then take the first
20 bytes (first 42 hex chars including "0x") of that keccak result to produce
saltPrefix; finally compare that 20-byte hex string to the saltSlice (or convert
both to BigInt consistently) so the comparison between the computed saltPrefix
and opt.salt.slice(0, 42) is correct.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@solidity/src/CATValidator.sol`:
- Around line 169-174: The unchecked block formatting around the balance check
(the unchecked { ... } containing uint256 diff = newBalance >
recordedBalances[i] ? newBalance - recordedBalances[i] : 0; if (diff <
outcome.amount) { revert InvalidTokenAmount(outcome.amount, diff); }) is not
compliant with forge fmt; run `forge fmt` (or reformat that block) so
spacing/braces match project style and CI passes, keeping the logic and
identifiers newBalance, recordedBalances[i], diff, outcome.amount and the
InvalidTokenAmount revert unchanged.
In `@solidity/test/CATValidator.t.sol`:
- Around line 378-396: The failing CI check is due to formatting issues in the
test function test_compareOutcomes_revert_on_wrapped_required_amount; run `forge
fmt` (or apply the same formatter rules) to reformat
solidity/test/CATValidator.t.sol so the block containing
test_compareOutcomes_revert_on_wrapped_required_amount and its surrounding code
matches the project's solidity style; ensure the spacing and alignment around
the Outcome[] declaration, recordedBalances initialization, and vm.expectRevert
call are corrected so `forge fmt --check` passes.
In `@typescript/src/transaction/transaction.ts`:
- Around line 96-97: Fix the JSDoc typo in the comment block that currently
reads "@returns Whether the a multichain execution mode is set." — remove the
extra "the" so it reads "@returns Whether a multichain execution mode is set."
Update the JSDoc above the function (the JSDoc return line in the transaction.ts
comment block) to the corrected phrasing.
🧹 Nitpick comments (5)
typescript/src/transaction/transaction.ts (1)
199-234:asAccounthardcodesisSignature: false— document the intent.Line 213 always passes
isSignature: falsewhen predicting the address. This meansasAccountonly supports embedding call digests, not signature digests. The distinction is important since it affects the derived address. Consider adding a brief JSDoc note clarifying this constraint.solidity/src/CATValidator.sol (1)
95-102: Minor:!spentis alwaystruehere — consider using a literal for clarity.Since line 100 reverts when
spent == true, the assignmentspentNonces[account][nonce] = !spenton line 101 is always equivalent to= true. Using!spentis a micro-gas trick (avoids a freshPUSH1 1) but harms readability.✏️ Optional clarity improvement
function _checkNonce( address account, uint256 nonce ) internal { bool spent = spentNonces[account][nonce]; if (spent) revert NonceAlreadySpent(); - spentNonces[account][nonce] = !spent; + spentNonces[account][nonce] = true; }solidity/test/CATValidator.t.sol (3)
214-234: Consider using a named constant for the magic value.Lines 229 and 253 use the raw literal
57896044618658097711785492504343953926634992332820282019728792003956564819968which is1 << 255(SPEND_BALANCE_OF_MAGIC). Referencing the constant directly would improve readability and reduce fragility.✏️ Suggested improvement
You could expose
SPEND_BALANCE_OF_MAGICvia the mock or reference it as a test constant:- targets[0] = AllowanceSpend({ - token: token, - allocated: 1 << 255, - spend: 57896044618658097711785492504343953926634992332820282019728792003956564819968 - }); + uint256 SPEND_BALANCE_OF_MAGIC = 1 << 255; + targets[0] = AllowanceSpend({ + token: token, + allocated: SPEND_BALANCE_OF_MAGIC, + spend: SPEND_BALANCE_OF_MAGIC + });
304-329: Hardcodedi == 2for native ETH only tests that path when the fuzz array has 3+ elements.If the fuzzer generates arrays with fewer than 3 entries, the native ETH balance path (
token = address(0)) is never exercised. Consider adding a dedicated non-fuzz test for native ETH balance recording to guarantee coverage.
63-68: Consider adding anentry()happy-path integration test with non-zero nonce.The current integration test (
test_entry_decrease_balances) uses nonce 0 and an empty allowance array to test the revert path. A complementary test with a non-zero nonce, actual allowances, a real external call, and satisfied outcomes would exercise the full happy path and nonce-replay protection end-to-end.
* chore: address comments Signed-off-by: georgi <georgi@li.finance> * chore: address comments Signed-off-by: georgi <georgi@li.finance> * chore: address comments Signed-off-by: georgi <georgi@li.finance> * test: improve coverage Signed-off-by: georgi <georgi@li.finance> --------- Signed-off-by: georgi <georgi@li.finance>
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 |
There was a problem hiding this comment.
3rd party Github Actions should be pinned - high severity
A third-party GitHub Action was imported, and is not pinned via a hash. This leaves your CI/CD at risk for potential supply chain attacks, if the affected GitHub Action is compromised.
| uses: oven-sh/setup-bun@v2 | |
| uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| working-directory: typescript | ||
|
|
||
| - name: Install Anvil | ||
| uses: foundry-rs/foundry-toolchain@v1 |
There was a problem hiding this comment.
3rd party Github Actions should be pinned - high severity
A third-party GitHub Action was imported, and is not pinned via a hash. This leaves your CI/CD at risk for potential supply chain attacks, if the affected GitHub Action is compromised.
| uses: foundry-rs/foundry-toolchain@v1 | |
| uses: foundry-rs/foundry-toolchain@8789b3e21e6c11b2697f5eb56eddae542f746c10 # v1.7.0 |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| working-directory: typescript | ||
|
|
||
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v5 |
There was a problem hiding this comment.
3rd party Github Actions should be pinned - high severity
A third-party GitHub Action was imported, and is not pinned via a hash. This leaves your CI/CD at risk for potential supply chain attacks, if the affected GitHub Action is compromised.
| uses: codecov/codecov-action@v5 | |
| uses: codecov/codecov-action@75cd11691c0faa626561e295848008c8a7dddffe # v5 |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| submodules: recursive | ||
|
|
||
| - name: Install Foundry | ||
| uses: foundry-rs/foundry-toolchain@v1 |
There was a problem hiding this comment.
3rd party Github Actions should be pinned - high severity
A third-party GitHub Action was imported, and is not pinned via a hash. This leaves your CI/CD at risk for potential supply chain attacks, if the affected GitHub Action is compromised.
| uses: foundry-rs/foundry-toolchain@v1 | |
| uses: foundry-rs/foundry-toolchain@8789b3e21e6c11b2697f5eb56eddae542f746c10 # v1.7.0 |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| working-directory: solidity | ||
|
|
||
| - name: Upload coverage to Codecov | ||
| uses: codecov/codecov-action@v5 |
There was a problem hiding this comment.
3rd party Github Actions should be pinned - high severity
A third-party GitHub Action was imported, and is not pinned via a hash. This leaves your CI/CD at risk for potential supply chain attacks, if the affected GitHub Action is compromised.
| uses: codecov/codecov-action@v5 | |
| uses: codecov/codecov-action@75cd11691c0faa626561e295848008c8a7dddffe # v5 |
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| const files = []; | ||
|
|
||
| for (const entry of entries) { | ||
| const fullPath = path.join(dir, entry.name); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
| const fullPath = path.join(dir, entry.name); | |
| const fullPath = path.resolve(dir, entry.name); | |
| const relative = path.relative(distDir, fullPath); | |
| if (relative.startsWith("..") || path.isAbsolute(relative)) { | |
| continue; | |
| } |
There was a problem hiding this comment.
4 Open source vulnerabilities detected - medium severity
Aikido detected 4 vulnerabilities across 2 packages, it includes 4 medium vulnerabilities.
Details
Remediation Aikido suggests bumping the vulnerable packages to a safe version.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
|
|
||
| // forge-lint: disable-next-line(unchecked-call) | ||
| // wake-disable-next-line: unchecked-return-value | ||
| 0x4e59b44847b379578588920cA78FbF26c0B4956C.call{ gas: 1000000 }( |
There was a problem hiding this comment.
Large inline hex blob passed to low-level .call hides executable bytecode; replace with an imported/verifiable contract artifact or add provenance and explicit decoding to make intent auditable.
Details
✨ AI Reasoning
A newly added script contains an inline, very large hex literal that is passed directly to a low-level .call on a hardcoded address. This embeds executable bytecode/data in a way that obscures intent and behavior. Such embedded hex blobs are opaque to reviewers and can hide arbitrary logic or backdoors. The pattern is not standard build artifact inclusion (it's a raw literal executed at runtime), which increases risk. Replacing with a referenced, named artifact or adding clear provenance would improve transparency.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| bytes calldata execPayload | ||
| ) internal { | ||
| address callProxy = CALL_PROXY; | ||
| assembly ("memory-safe") { |
There was a problem hiding this comment.
Inline assembly builds and forwards calldata via a proxy (calldatacopy, call, manual returndatacopy). This low-level construction obscures execution flow and can hide effects; avoid or add thorough justification and tests.
Details
✨ AI Reasoning
The _call function constructs calldata manually and performs a low-level call through a proxy using assembly, copying calldata and using dynamic parameters (selfbalance, codesize()) and manual returndatacopy/revert handling. This manual, byte-level construction and handling obscures the exact external call flow and returned data handling, making it easy to hide or obfuscate side effects. Such patterns are legitimate in optimized low-level code, but they also enable concealed execution paths and should be minimized or clearly justified when introduced.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| /// The storage slot is intentionally chosen such that it overlaps with Solady's storage slot. | ||
| /// If an upgradeable contract by misfortune upgrades to an Ownable and not KeyedOwnable contract, then if the key | ||
| /// used is ECDSAOrSmartContract, there is a chance that the account is still in control of someone. | ||
| bytes32 internal constant _OWNER_SLOT = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffff74873927; |
There was a problem hiding this comment.
A nonstandard hard-coded owner storage slot (_OWNER_SLOT) is used to hide owner data; this reduces transparency and can conceal control. Use explicit state variables or document and rationalize thoroughly.
Details
✨ AI Reasoning
The code introduces a hard-coded, non-obvious storage slot constant used as the base for owner data. Choosing an intentionally overlapping and obfuscated slot makes it difficult to reason about ownership and increases risk that ownership or key data is hidden from straightforward static analysis. Although comments reference the intent, the pattern still reduces transparency and can conceal backdoors or surprises in upgrades.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| ) private { | ||
| assembly ("memory-safe") { | ||
| // Subtract the index so we get away from the "small" space. | ||
| sstore(sub(_OWNER_SLOT, index), val) |
There was a problem hiding this comment.
Direct sstore to a computed slot (sstore(sub(_OWNER_SLOT, index), val)) hides where ownership data is stored; this obscures control and makes audits harder. Avoid computed storage slots or clearly document and justify the layout.
Details
✨ AI Reasoning
The code stores owner public key slices using direct sstore into a computed slot (subtracting an internal constant by index). This intentionally places ownership data in an unusual location and uses low-level storage arithmetic instead of explicit named state variables. While comments assert the choice is intentional, the pattern makes the storage layout non-obvious and can hide ownership control, complicating review and potentially masking malicious state changes. The change introduces direct, computed storage writes that obscure where owner data is kept and who controls the contract.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
typescript/src/transaction/transaction.ts (1)
33-37: Consider defensively cloningcallsin the constructor.
this.calls = callskeeps external aliasing alive; mutating the original array later mutates transaction state implicitly.Proposed fix
- this.calls = calls; + this.calls = [...calls];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/src/transaction/transaction.ts` around lines 33 - 37, The constructor currently assigns the incoming array directly (const { calls = [] } = opt ?? {}; this.calls = calls) which preserves external aliasing; change the assignment in the Transaction constructor to defensively clone the calls array (e.g., use a shallow copy like Array.from(calls) or [...calls]) when setting this.calls to prevent external mutations from affecting transaction state, ensuring you update the assignment site that references calls in the transaction.ts constructor.solidity/src/CATValidator.sol (1)
95-102: Consider simplifying the nonce toggle.The expression
spentNonces[account][nonce] = !spentis equivalent to= truesincespentis guaranteedfalseat this point (we just checked). While correct, the negation adds unnecessary cognitive overhead.♻️ Suggested simplification
function _checkNonce( address account, uint256 nonce ) internal { bool spent = spentNonces[account][nonce]; if (spent) revert NonceAlreadySpent(); - spentNonces[account][nonce] = !spent; + spentNonces[account][nonce] = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/src/CATValidator.sol` around lines 95 - 102, The _checkNonce function currently reads spentNonces[account][nonce] into local bool spent, checks and reverts if true, then sets spentNonces[account][nonce] = !spent; simplify this by directly setting spentNonces[account][nonce] = true after the revert check to remove the unnecessary negation—update the _checkNonce function (and keep the NonceAlreadySpent() revert logic) so it explicitly writes true to the spentNonces mapping.solidity/test/CATValidator.t.sol (2)
304-329: Native ETH outcome testing is limited by index-based condition.The
test_fuzz_recordBalancesandtest_fuzz_compareOutcomestests only assign native ETH wheni == 2. With fuzzed array lengths, many runs will have fewer than 3 elements, meaning native ETH balance recording is rarely exercised.Consider using a modulo check or separate dedicated test:
// Example: test native ETH for last element of any array if (i == outcomes.length - 1 && outcomes.length > 0) { vm.deal(dest, ad[i].amount); outcomes[i] = Outcome({ amount: ad[i].amount, destination: ad[i].destination, token: address(0) }); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/test/CATValidator.t.sol` around lines 304 - 329, The tests test_fuzz_recordBalances and test_fuzz_compareOutcomes currently only assign native ETH when i == 2, which is skipped for fuzzed arrays shorter than 3; update the native-ETH selection logic in both functions (the loop building outcomes from AmountAndDestination[] ad and the outcomes array) to pick an element that always exists for any length — for example, use the last element (i == outcomes.length - 1 && outcomes.length > 0) or a modulo-based selection (e.g., i % max(1, outcomes.length) == someIndex) instead of the hardcoded i == 2 so ETH balance recording is exercised for all fuzz sizes.
414-436: Good coverage of entry-level balance decrease revert.The test correctly validates that the full
entryflow reverts when the external call decreases the balance, exercising the outcome comparison guard. Consider specifying the expected revert selector for clearer failure diagnostics.♻️ More specific revert expectation
- vm.expectRevert(); + vm.expectRevert(abi.encodeWithSelector(CATValidator.InvalidTokenAmount.selector, 1, 0)); validator.entry(token, call, address(this), 0, allowances, outcomes, hex"");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/test/CATValidator.t.sol` around lines 414 - 436, Replace the generic vm.expectRevert() in test_entry_decrease_balances with a specific revert expectation that matches the validator.entry outcome-comparison guard's error selector; locate the actual revert identifier emitted by validator.entry (the outcome comparison error/exception in the Validator contract) and call vm.expectRevert(abi.encodeWithSelector(<ValidatorError>.selector)) or vm.expectRevert(<encoded error bytes>) so the test asserts the exact revert reason instead of any revert.solidity/test/libs/IntentExecutor.t.sol (1)
295-324: Consider adding a USDT-style token test for safeApproveWithRetry.The
test_repeatApprovalSavesGasvalidates gas savings for repeat approvals, butMockERC20from Solady doesn't implement USDT-style "revert on non-zero-to-non-zero" approval behavior. ThesafeApproveWithRetryretry logic is never exercised.Consider adding a
MockUSDTthat reverts on non-zero-to-non-zero approvals to validate thatsafeApproveWithRetrycorrectly handles this edge case:contract MockUSDT is MockERC20 { function approve(address spender, uint256 amount) public override returns (bool) { if (amount != 0 && allowance(msg.sender, spender) != 0) { revert("USDT: approve from non-zero to non-zero"); } return super.approve(spender, amount); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solidity/test/libs/IntentExecutor.t.sol` around lines 295 - 324, Add a USDT-style mock and a test that exercises safeApproveWithRetry: create a MockUSDT that overrides approve to revert on non-zero->non-zero (similar to the provided snippet), deploy MockUSDT in the test (or setUp), then add a test (e.g., test_repeatApprovalWithUSDTRetries) that first sets a non-zero allowance for address(this) on the MockUSDT and then calls executor.executeAndSweep with an IntentExecutor.Approval for MockUSDT to a non-zero target so the executor's safeApproveWithRetry path runs; finally assert the executor's allowance is set to type(uint256).max (or expected) and that no revert occurred. Reference symbols: MockERC20, MockUSDT, IntentExecutor.Approval, executor.executeAndSweep, safeApproveWithRetry, and the existing test_repeatApprovalSavesGas for placement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@typescript/src/transaction/transaction.ts`:
- Around line 27-37: The constructor currently assigns opt.nonce directly to
this.nonce allowing invalid values to bypass checks; update the constructor to
validate the nonce the same way setNonce does: require a bigint, enforce
non-negative and ensure it is within the allowed upper bound (e.g., < 2**64) and
throw a RangeError/TypeError for invalid values instead of assigning; likewise
tighten setNonce to explicitly reject negative or out-of-range values and
document the invariant on the nonce property to keep both constructor and
setNonce consistent (refer to constructor, setNonce, and this.nonce).
---
Nitpick comments:
In `@solidity/src/CATValidator.sol`:
- Around line 95-102: The _checkNonce function currently reads
spentNonces[account][nonce] into local bool spent, checks and reverts if true,
then sets spentNonces[account][nonce] = !spent; simplify this by directly
setting spentNonces[account][nonce] = true after the revert check to remove the
unnecessary negation—update the _checkNonce function (and keep the
NonceAlreadySpent() revert logic) so it explicitly writes true to the
spentNonces mapping.
In `@solidity/test/CATValidator.t.sol`:
- Around line 304-329: The tests test_fuzz_recordBalances and
test_fuzz_compareOutcomes currently only assign native ETH when i == 2, which is
skipped for fuzzed arrays shorter than 3; update the native-ETH selection logic
in both functions (the loop building outcomes from AmountAndDestination[] ad and
the outcomes array) to pick an element that always exists for any length — for
example, use the last element (i == outcomes.length - 1 && outcomes.length > 0)
or a modulo-based selection (e.g., i % max(1, outcomes.length) == someIndex)
instead of the hardcoded i == 2 so ETH balance recording is exercised for all
fuzz sizes.
- Around line 414-436: Replace the generic vm.expectRevert() in
test_entry_decrease_balances with a specific revert expectation that matches the
validator.entry outcome-comparison guard's error selector; locate the actual
revert identifier emitted by validator.entry (the outcome comparison
error/exception in the Validator contract) and call
vm.expectRevert(abi.encodeWithSelector(<ValidatorError>.selector)) or
vm.expectRevert(<encoded error bytes>) so the test asserts the exact revert
reason instead of any revert.
In `@solidity/test/libs/IntentExecutor.t.sol`:
- Around line 295-324: Add a USDT-style mock and a test that exercises
safeApproveWithRetry: create a MockUSDT that overrides approve to revert on
non-zero->non-zero (similar to the provided snippet), deploy MockUSDT in the
test (or setUp), then add a test (e.g., test_repeatApprovalWithUSDTRetries) that
first sets a non-zero allowance for address(this) on the MockUSDT and then calls
executor.executeAndSweep with an IntentExecutor.Approval for MockUSDT to a
non-zero target so the executor's safeApproveWithRetry path runs; finally assert
the executor's allowance is set to type(uint256).max (or expected) and that no
revert occurred. Reference symbols: MockERC20, MockUSDT,
IntentExecutor.Approval, executor.executeAndSweep, safeApproveWithRetry, and the
existing test_repeatApprovalSavesGas for placement.
In `@typescript/src/transaction/transaction.ts`:
- Around line 33-37: The constructor currently assigns the incoming array
directly (const { calls = [] } = opt ?? {}; this.calls = calls) which preserves
external aliasing; change the assignment in the Transaction constructor to
defensively clone the calls array (e.g., use a shallow copy like
Array.from(calls) or [...calls]) when setting this.calls to prevent external
mutations from affecting transaction state, ensuring you update the assignment
site that references calls in the transaction.ts constructor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e55c2ad-0036-48dc-9827-70358169b40a
📒 Files selected for processing (6)
solidity/script/deploy.s.solsolidity/src/CATValidator.solsolidity/src/libs/IntentExecutor.solsolidity/test/CATValidator.t.solsolidity/test/libs/IntentExecutor.t.soltypescript/src/transaction/transaction.ts
| constructor(opt?: { | ||
| mode?: ExecutionMode; | ||
| nonce?: bigint; | ||
| calls?: Call[]; | ||
| signature?: `0x${string}`; | ||
| }) { | ||
| const { mode, nonce, calls = [], signature } = opt ?? {}; | ||
| this.mode = mode; | ||
| this.nonce = nonce; | ||
| this.calls = calls; | ||
| this.signature = signature; |
There was a problem hiding this comment.
Enforce nonce invariants consistently (constructor + setter).
nonce can bypass validation in the constructor, and setNonce currently allows negative/out-of-range values. This can generate invalid opdata and fail downstream at execution time.
Proposed fix
export class BaseTransaction {
+ private static readonly MAX_UINT256 = (1n << 256n) - 1n;
+
+ private assertValidNonce(nonce: bigint) {
+ if (nonce <= 0n)
+ throw new Error(
+ "Nonce must be greater than 0. 0 cannot be differentiated from an invalid nonce.",
+ );
+ if (nonce > BaseTransaction.MAX_UINT256)
+ throw new Error("Nonce exceeds uint256 range.");
+ }
+
constructor(opt?: {
@@
}) {
const { mode, nonce, calls = [], signature } = opt ?? {};
this.mode = mode;
- this.nonce = nonce;
+ if (nonce !== undefined) this.assertValidNonce(nonce);
+ this.nonce = nonce;
this.calls = calls;
this.signature = signature;
}
@@
setNonce(nonce: bigint) {
- if (nonce === 0n)
- throw new Error(
- `Nonce 0 is not allowed. It cannot be differentiated from an invalid nonce.`,
- );
+ this.assertValidNonce(nonce);
this.nonce = nonce;
return this;
}Also applies to: 45-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@typescript/src/transaction/transaction.ts` around lines 27 - 37, The
constructor currently assigns opt.nonce directly to this.nonce allowing invalid
values to bypass checks; update the constructor to validate the nonce the same
way setNonce does: require a bigint, enforce non-negative and ensure it is
within the allowed upper bound (e.g., < 2**64) and throw a RangeError/TypeError
for invalid values instead of assigning; likewise tighten setNonce to explicitly
reject negative or out-of-range values and document the invariant on the nonce
property to keep both constructor and setNonce consistent (refer to constructor,
setNonce, and this.nonce).
3 participants
Fixes: https://linear.app/lifi-linear/issue/API-174/create-catapultar-library-with-webauth-and-viem-support
Implements:
CALLS_TYPE_HASH#7New Features
Adds support for new key types:
Along with supporting an additionally SHA256 hash of the payload before signature validation.
Set Signatures & execution payloads as pre-validated. This allows the ability to schedule calls where one calls execution allows a follow up call to be executed along with other composability features.
Multichain actions without the chainId set in signed domain separators using: Solady's
_hashTypedDataSansChainId.Mono Repo
This repository now uses a mono-repo for both the smart account and a companion typescript library.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores