Skip to content

Add native merkle sigs to the Account #388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
legion2002 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add native merkle sigs to the Account #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ecce31
feat: add merkle sigs natively into the account
legion2002 Sep 24, 2025
5577747
fix: tests
legion2002 Sep 24, 2025
995a3e1
test: add more sophisticated fuzz test
legion2002 Sep 24, 2025
4918a12
chore: bump contract versions due to bytecode changes - Contracts upd…
actions-user Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions snapshots/BenchmarkTest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
"testERC20Transfer_ERC4337MinimalAccount": "171509",
"testERC20Transfer_ERC4337MinimalAccount_AppSponsor": "168500",
"testERC20Transfer_ERC4337MinimalAccount_ERC20SelfPay": "199831",
"testERC20Transfer_IthacaAccount": "128059",
"testERC20Transfer_IthacaAccountWithSpendLimits": "193602",
"testERC20Transfer_IthacaAccount_AppSponsor": "138595",
"testERC20Transfer_IthacaAccount_AppSponsor_ERC20": "143884",
"testERC20Transfer_IthacaAccount_ERC20SelfPay": "128548",
"testERC20Transfer_IthacaAccount": "128100",
"testERC20Transfer_IthacaAccountWithSpendLimits": "193589",
"testERC20Transfer_IthacaAccount_AppSponsor": "138636",
"testERC20Transfer_IthacaAccount_AppSponsor_ERC20": "143937",
"testERC20Transfer_IthacaAccount_ERC20SelfPay": "128589",
"testERC20Transfer_Safe4337": "197561",
"testERC20Transfer_Safe4337_AppSponsor": "191725",
"testERC20Transfer_Safe4337_ERC20SelfPay": "221464",
Expand All @@ -29,25 +29,25 @@
"testERC20Transfer_ZerodevKernel_ERC20SelfPay": "235489",
"testERC20Transfer_batch100_AlchemyModularAccount": "10109066",
"testERC20Transfer_batch100_AlchemyModularAccount_ERC20SelfPay": "11609298",
"testERC20Transfer_batch100_IthacaAccount": "7531912",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor": "8140216",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor_ERC20": "7966120",
"testERC20Transfer_batch100_IthacaAccount_ERC20SelfPay": "7353064",
"testERC20Transfer_batch100_IthacaAccount": "7535892",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor": "8144196",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor_ERC20": "7970208",
"testERC20Transfer_batch100_IthacaAccount_ERC20SelfPay": "7357152",
"testERC20Transfer_batch100_ZerodevKernel": "12631318",
"testERC20Transfer_batch100_ZerodevKernel_ERC20SelfPay": "14149937",
"testNativeTransfer_AlchemyModularAccount": "180829",
"testNativeTransfer_CoinbaseSmartWallet": "178916",
"testNativeTransfer_IthacaAccount": "129415",
"testNativeTransfer_IthacaAccount_AppSponsor": "139982",
"testNativeTransfer_IthacaAccount_ERC20SelfPay": "137204",
"testNativeTransfer_IthacaAccount": "129456",
"testNativeTransfer_IthacaAccount_AppSponsor": "140023",
"testNativeTransfer_IthacaAccount_ERC20SelfPay": "137245",
"testNativeTransfer_Safe4337": "198595",
"testNativeTransfer_ZerodevKernel": "208635",
"testUniswapV2Swap_AlchemyModularAccount": "238647",
"testUniswapV2Swap_CoinbaseSmartWallet": "237451",
"testUniswapV2Swap_ERC4337MinimalAccount": "230691",
"testUniswapV2Swap_IthacaAccount": "187179",
"testUniswapV2Swap_IthacaAccount_AppSponsor": "197691",
"testUniswapV2Swap_IthacaAccount_ERC20SelfPay": "192480",
"testUniswapV2Swap_IthacaAccount": "187244",
"testUniswapV2Swap_IthacaAccount_AppSponsor": "197744",
"testUniswapV2Swap_IthacaAccount_ERC20SelfPay": "192533",
"testUniswapV2Swap_Safe4337": "257333",
"testUniswapV2Swap_ZerodevKernel": "266367"
}
47 changes: 44 additions & 3 deletions src/IthacaAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {LibNonce} from "./libraries/LibNonce.sol";
import {TokenTransferLib} from "./libraries/TokenTransferLib.sol";
import {LibTStack} from "./libraries/LibTStack.sol";
import {IIthacaAccount} from "./interfaces/IIthacaAccount.sol";
import {MerkleProofLib} from "solady/utils/MerkleProofLib.sol";

/// @title Account
/// @notice A account contract for EOAs with EIP7702.
Expand Down Expand Up @@ -490,9 +491,43 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
return isMultichain ? _hashTypedDataSansChainId(structHash) : _hashTypedData(structHash);
}

/// @dev Verifies the merkle sig
/// - Note: Each leaf of the merkle tree should be a standard digest.
/// - The signature for using merkle verification is encoded as:
/// - bytes signature = abi.encode(bytes32[] proof, bytes32 root, bytes rootSig)
function _verifyMerkleSig(bytes32 digest, bytes calldata signature)
internal
view
returns (bool isValid, bytes32 keyHash)
{
bytes32[] calldata proof;
bytes32 root;
bytes calldata rootSig;

assembly ("memory-safe") {
let proofOffset := add(signature.offset, calldataload(signature.offset))
proof.length := calldataload(proofOffset)
proof.offset := add(proofOffset, 0x20)

root := calldataload(add(signature.offset, 0x20))

let rootSigOffset := add(signature.offset, calldataload(add(signature.offset, 0x40)))
rootSig.length := calldataload(rootSigOffset)
rootSig.offset := add(rootSigOffset, 0x20)
}

if (MerkleProofLib.verifyCalldata(proof, root, digest)) {
(isValid, keyHash) = unwrapAndValidateSignature(root, rootSig);

return (isValid, keyHash);
}

return (false, bytes32(0));
}

/// @dev Returns if the signature is valid, along with its `keyHash`.
/// The `signature` is a wrapped signature, given by
/// `abi.encodePacked(bytes(innerSignature), bytes32(keyHash), bool(prehash))`.
/// `abi.encode(bytes(innerSignature), bytes32(keyHash), bool(prehash), bool(merkle))`.
Copy link
Contributor

@howydev howydev Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encode would result in this:

[0-0x20] - sig offset, likely pointing to 0x60
[0x20-0x40] - keyhash
[0x40-0x60] - bool
[0x60-...] sig

Copy link
Contributor

@howydev howydev Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in tests we're doing abi.encodePacked so we just need to update this comment

function unwrapAndValidateSignature(bytes32 digest, bytes calldata signature)
public
view
Expand All @@ -507,14 +542,20 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
return (ECDSA.recoverCalldata(digest, signature) == address(this), 0);
}

bool merkle;
unchecked {
uint256 n = signature.length - 0x21;
uint256 n = signature.length - 0x22;
keyHash = LibBytes.loadCalldata(signature, n);
signature = LibBytes.truncatedCalldata(signature, n);
// Do the prehash if last byte is non-zero.
if (uint256(LibBytes.loadCalldata(signature, n + 1)) & 0xff != 0) {
digest = EfficientHashLib.sha2(digest); // `sha256(abi.encode(digest))`.
}
merkle = uint256(LibBytes.loadCalldata(signature, n + 2)) & 0xff != 0;
}

if (merkle) {
return _verifyMerkleSig(digest, signature);
Copy link
Contributor

@howydev howydev Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we're skipping some checks by short circuiting here like key expiry

are we checking expiry anywhere else? otherwise this might allow an expired session key to execute valid transactions

Copy link
Contributor

@howydev howydev Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nvm, i just saw that _verifyMerkleSig pipes back into the unwrapAndValidateSignature flow

Copy link
Contributor

@howydev howydev Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we use the user input keyHash instead of the value returned by _verifyMerkleSig

and would there be issues here? e.g. could a user pass in the owner keyHash in the sig, but use a session key to validate the merkle sig?

Copy link
Contributor

@howydev howydev Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, i see that we truncate the sig before calling _verifyMerkle

signature = LibBytes.truncatedCalldata(signature, n);

why do we not require two instances of bytes32(keyHash), bool(prehash), bool(merkle) in the sig?

Copy link
Collaborator Author

@legion2002 legion2002 Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer keyhash is ignored right now, because we do a recurisve call into the unwrapAndValidateSignature function which then unwraps the inner signature.
But I think there should be a check here that outer keyHash == inner keyhash, so someone cannot trick the getContextKeyHash TStack into believing the call is coming from an arbitrary keyhash. Good Point.

Copy link
Contributor

@howydev howydev Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yep, adding the eq check should make it work
  2. Ok I must have missed it... That said, is there any way we can avoid having 2 sets?

Copy link
Contributor

@howydev howydev Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on second review, the outer keyhash is not used at all, there shouldn’t be a security issue, just a redundancy

In the first call frame for unwrapAndValidate, it parses out the outer keyHash from the sig but they’re not used, since it enters the if(merkle) branch and returns after

The second call frame does all the checks using the inner keyHash. That’s the keyhash that’s returned here, and is bubbled up into the top level call frame and out

If we move the isMerkle check and the merkle = loadCalldata to right after calculating n, we would only need to duplicate the isMerkle bool and not the keyhash/prehash

Copy link
Collaborator Author

@legion2002 legion2002 Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer keyhash is used for the getContextKeyHash TStack, which aims to inform external parties about the keyhash that signed the latest execution.
But currently this has an issue, because someone could spoof the outer keyhash to be something else, but the actual verification is done with the inner keyhash.

Copy link
Collaborator Author

@legion2002 legion2002 Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could implement your suggestion of directly picking the inner keyhash, if we detect an isMerkle signature. But, this logic for getting the correct keyhash from the signature would have to be replicated by external onchain and offchain integrators.

So everyone would basically have to do an isMerkle if statement for decoding. If we duplicate the keyhash inside the signature, and just checking if both are the same, then we would be able to maintain standard outer signature encoding for both flows.

Will think about it some more.

Copy link
Collaborator Author

@legion2002 legion2002 Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we only ignore the keyhash in the outer sig. The prehash config is actually applied to the digest.
So we can't ignore that.

}

Key memory key = getKey(keyHash);
Expand Down Expand Up @@ -755,6 +796,6 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
returns (string memory name, string memory version)
{
name = "IthacaAccount";
version = "0.5.10";
version = "0.5.11";
}
}
69 changes: 67 additions & 2 deletions test/Account.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import "./Base.t.sol";
import {MockSampleDelegateCallTarget} from "./utils/mocks/MockSampleDelegateCallTarget.sol";
import {LibEIP7702} from "solady/accounts/LibEIP7702.sol";

import {Merkle} from "murky/Merkle.sol";

contract AccountTest is BaseTest {
struct _TestExecuteWithSignatureTemps {
TargetFunctionPayload[] targetFunctionPayloads;
Expand Down Expand Up @@ -68,6 +70,70 @@ contract AccountTest is BaseTest {
}
}

function testMerkleSignature(uint256 seed) public {
DelegatedEOA memory d = _randomEIP7702DelegatedEOA();
PassKey memory k = _randomSecp256k1PassKey();
k.k.isSuperAdmin = true;

vm.prank(d.eoa);
d.d.authorize(k.k);

// Fuzz number of leaves (2 to 256)
uint256 numLeaves = bound(seed, 2, 256);
bytes32[] memory leaves = new bytes32[](numLeaves);

// Generate random leaves
for (uint256 i = 0; i < numLeaves; i++) {
leaves[i] = keccak256(abi.encodePacked("leaf", i, seed));
}

// Pick a random valid index
uint256 validIndex = seed % numLeaves;
bytes32 validDigest = leaves[validIndex];

Merkle merkle = new Merkle();
bytes32 root = merkle.getRoot(leaves);
bytes32[] memory proof = merkle.getProof(leaves, validIndex);

// Test valid merkle proof
{
bytes memory rootSig = abi.encode(proof, root, _sig(k, root));
bytes memory sig = abi.encodePacked(rootSig, bytes32(0), uint8(0), uint8(1));

(bool isValid, bytes32 keyHash) = d.d.unwrapAndValidateSignature(validDigest, sig);
assertEq(isValid, true);
assertEq(keyHash, k.keyHash);

// Test invalid digest not in tree
bytes32 invalidDigest = keccak256(abi.encodePacked("not_in_tree", seed));
(isValid, keyHash) = d.d.unwrapAndValidateSignature(invalidDigest, sig);
assertEq(isValid, false);
assertEq(keyHash, bytes32(0));
}

// Test tampered proof (only if proof has elements to tamper)
if (proof.length > 0) {
bytes32[] memory tamperedProof = new bytes32[](proof.length);
for (uint256 i = 0; i < proof.length; i++) {
tamperedProof[i] = i == 0 ? bytes32(uint256(proof[i]) ^ 1) : proof[i];
}
bytes memory tamperedRootSig = abi.encode(tamperedProof, root, _sig(k, root));
bytes memory tamperedSig =
abi.encodePacked(tamperedRootSig, bytes32(0), uint8(0), uint8(1));
(bool isValid,) = d.d.unwrapAndValidateSignature(validDigest, tamperedSig);
assertEq(isValid, false);
}

// Test wrong root (tampered root should fail verification)
{
bytes32 wrongRoot = bytes32(uint256(root) ^ 1);
bytes memory wrongRootSig = abi.encode(proof, wrongRoot, _sig(k, wrongRoot));
bytes memory wrongSig = abi.encodePacked(wrongRootSig, bytes32(0), uint8(0), uint8(1));
(bool isValid,) = d.d.unwrapAndValidateSignature(validDigest, wrongSig);
assertEq(isValid, false);
}
}

function testSignatureCheckerApproval(bytes32) public {
DelegatedEOA memory d = _randomEIP7702DelegatedEOA();
PassKey memory k = _randomSecp256k1PassKey();
Expand All @@ -93,8 +159,7 @@ contract AccountTest is BaseTest {

bytes32 replaySafeDigest = keccak256(abi.encode(d.d.SIGN_TYPEHASH(), digest));

(, string memory name, string memory version,, address verifyingContract,,) =
d.d.eip712Domain();
(,,,, address verifyingContract,,) = d.d.eip712Domain();
bytes32 domain = keccak256(
abi.encode(
0x035aff83d86937d35b32e04f0ddc6ff469290eef2f1b692d8a815c89404d4749, // DOMAIN_TYPEHASH with only verifyingContract
Expand Down
11 changes: 6 additions & 5 deletions test/Base.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ contract BaseTest is SoladyTest {
{
(bytes32 r, bytes32 s) = vm.signP256(privateKey, digest);
s = P256.normalized(s);
return abi.encodePacked(abi.encode(r, s), keyHash, uint8(prehash ? 1 : 0));
return abi.encodePacked(abi.encode(r, s), keyHash, uint8(prehash ? 1 : 0), uint8(0));
}

function _secp256k1Sig(uint256 privateKey, bytes32 keyHash, bytes32 digest)
Expand All @@ -232,7 +232,8 @@ contract BaseTest is SoladyTest {
returns (bytes memory)
{
(uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest);
return abi.encodePacked(abi.encodePacked(r, s, v), keyHash, uint8(prehash ? 1 : 0));
return
abi.encodePacked(abi.encodePacked(r, s, v), keyHash, uint8(prehash ? 1 : 0), uint8(0));
}

function _multiSig(MultiSigKey memory k, bytes32 keyHash, bool preHash, bytes32 digest)
Expand All @@ -245,7 +246,7 @@ contract BaseTest is SoladyTest {
signatures[i] = _sig(k.owners[i], digest);
}

return abi.encodePacked(abi.encode(signatures), keyHash, uint8(preHash ? 1 : 0));
return abi.encodePacked(abi.encode(signatures), keyHash, uint8(preHash ? 1 : 0), uint8(0));
}

function _estimateGasForEOAKey(Orchestrator.Intent memory i)
Expand Down Expand Up @@ -277,15 +278,15 @@ contract BaseTest is SoladyTest {
{
(uint8 v, bytes32 r, bytes32 s) =
vm.sign(uint128(_randomUniform()), bytes32(_randomUniform()));
i.signature = abi.encodePacked(abi.encodePacked(r, s, v), keyHash, uint8(0));
i.signature = abi.encodePacked(abi.encodePacked(r, s, v), keyHash, uint8(0), uint8(0));
return _estimateGas(i);
}

function _estimateGasForSecp256r1Key(bytes32 keyHash, Orchestrator.Intent memory i)
internal
returns (uint256 gExecute, uint256 gCombined, uint256 gUsed)
{
i.signature = abi.encodePacked(keccak256("a"), keccak256("b"), keyHash, uint8(0));
i.signature = abi.encodePacked(keccak256("a"), keccak256("b"), keyHash, uint8(0), uint8(0));

return _estimateGas(i);
}
Expand Down
5 changes: 3 additions & 2 deletions test/Orchestrator.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,8 +924,9 @@ contract OrchestratorTest is BaseTest {
if (_randomChance(16)) {
u.combinedGas += 10_000;
// Fill with some junk signature, but with the session `keyHash`.
u.signature =
abi.encodePacked(keccak256("a"), keccak256("b"), kSession.keyHash, uint8(0));
u.signature = abi.encodePacked(
keccak256("a"), keccak256("b"), kSession.keyHash, uint8(0), uint8(0)
);

(t.gExecute, t.gCombined, t.gUsed) = _estimateGas(u);

Expand Down
3 changes: 2 additions & 1 deletion test/SimulateExecute.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,8 @@ contract SimulateExecuteTest is BaseTest {
// it needs to add the variance for non-precompile P256 verification.
// We need the `keyHash` in the signature so that the simulation is able
// to hit all the gas for the GuardedExecutor stuff for the `keyHash`.
i.signature = abi.encodePacked(keccak256("a"), keccak256("b"), k.keyHash, uint8(0));
i.signature =
abi.encodePacked(keccak256("a"), keccak256("b"), k.keyHash, uint8(0), uint8(0));

uint256 snapshot = vm.snapshotState();
vm.deal(_ORIGIN_ADDRESS, type(uint192).max);
Expand Down