Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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": "128100",
"testERC20Transfer_IthacaAccountWithSpendLimits": "193563",
"testERC20Transfer_IthacaAccount_AppSponsor": "138636",
"testERC20Transfer_IthacaAccount_AppSponsor_ERC20": "143937",
"testERC20Transfer_IthacaAccount_ERC20SelfPay": "128589",
"testERC20Transfer_IthacaAccount": "117971",
"testERC20Transfer_IthacaAccountWithSpendLimits": "184283",
"testERC20Transfer_IthacaAccount_AppSponsor": "128966",
"testERC20Transfer_IthacaAccount_AppSponsor_ERC20": "134255",
"testERC20Transfer_IthacaAccount_ERC20SelfPay": "118460",
"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": "7535892",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor": "8144196",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor_ERC20": "7970208",
"testERC20Transfer_batch100_IthacaAccount_ERC20SelfPay": "7357152",
"testERC20Transfer_batch100_IthacaAccount": "6669544",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor": "7324144",
"testERC20Transfer_batch100_IthacaAccount_AppSponsor_ERC20": "7150348",
"testERC20Transfer_batch100_IthacaAccount_ERC20SelfPay": "6490744",
"testERC20Transfer_batch100_ZerodevKernel": "12631318",
"testERC20Transfer_batch100_ZerodevKernel_ERC20SelfPay": "14149937",
"testNativeTransfer_AlchemyModularAccount": "180829",
"testNativeTransfer_CoinbaseSmartWallet": "178916",
"testNativeTransfer_IthacaAccount": "129456",
"testNativeTransfer_IthacaAccount_AppSponsor": "140023",
"testNativeTransfer_IthacaAccount_ERC20SelfPay": "137245",
"testNativeTransfer_IthacaAccount": "119355",
"testNativeTransfer_IthacaAccount_AppSponsor": "130351",
"testNativeTransfer_IthacaAccount_ERC20SelfPay": "127144",
"testNativeTransfer_Safe4337": "198595",
"testNativeTransfer_ZerodevKernel": "208635",
"testUniswapV2Swap_AlchemyModularAccount": "238647",
"testUniswapV2Swap_CoinbaseSmartWallet": "237451",
"testUniswapV2Swap_ERC4337MinimalAccount": "230691",
"testUniswapV2Swap_IthacaAccount": "187244",
"testUniswapV2Swap_IthacaAccount_AppSponsor": "197744",
"testUniswapV2Swap_IthacaAccount_ERC20SelfPay": "192533",
"testUniswapV2Swap_IthacaAccount": "177059",
"testUniswapV2Swap_IthacaAccount_AppSponsor": "188054",
"testUniswapV2Swap_IthacaAccount_ERC20SelfPay": "182348",
"testUniswapV2Swap_Safe4337": "257333",
"testUniswapV2Swap_ZerodevKernel": "266367"
}
98 changes: 63 additions & 35 deletions src/IthacaAccount.sol
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 @@ -478,38 +479,80 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
)
);
}
bool isMultichain = nonce >> 240 == MULTICHAIN_NONCE_PREFIX;
bytes32 structHash = EfficientHashLib.hash(
uint256(EXECUTE_TYPEHASH), LibBit.toUint(isMultichain), uint256(a.hash()), nonce
);
return isMultichain ? _hashTypedDataSansChainId(structHash) : _hashTypedData(structHash);
bytes32 structHash =
EfficientHashLib.hash(uint256(EXECUTE_TYPEHASH), uint256(a.hash()), nonce);
return nonce >> 240 == MULTICHAIN_NONCE_PREFIX
? _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.encodePacked(bytes32[] proof, bytes32 root, bytes rootSig)
function _verifyMerkleSig(bytes32 digest, bytes calldata signature)
internal
view
returns (bool isValid, bytes32 keyHash)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

continuing convo from this: https://github.com/ithacaxyz/account/pull/388/files#r2387828490

I think the outer one is entirely ignored, so theres no security issue

return _verifyMerkleSig(digest, signature) -> this overrides the keyHash = ... parsed in the top level call frame

On prehash - IIUC there should only be 1 prehash that's set, in either the outer or inner wrapper. There's a difference between the 2 since the digest has to be in the merkle tree, so which one we want to set depends whether the merkle leaf is sha-ed or not. I don't know which one would be shaed in practice, but the other one would probably be redundant

Copy link
Contributor

Choose a reason for hiding this comment

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

Something that I just thought to check was whether we'll run into issues around second preimage attacks

I don't see anything thats obvious so far, bringing it up just in case im missing something

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.encodePacked(bytes(innerSignature), bytes32(keyHash), bool(prehash), bool(merkle))`.
function unwrapAndValidateSignature(bytes32 digest, bytes calldata signature)
public
view
virtual
returns (bool isValid, bytes32 keyHash)
{
// Early return if unable to unwrap the signature.
if (signature.length < 0x21) return (false, 0);
if (signature.length < 0x22) return (false, 0);

// If the signature's length is 64 or 65, treat it like an secp256k1 signature.
if (LibBit.or(signature.length == 64, signature.length == 65)) {
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);
}

Key memory key = getKey(keyHash);
Expand Down Expand Up @@ -603,41 +646,26 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
uint256 paymentAmount,
bytes32 keyHash,
bytes32 intentDigest,
bytes calldata encodedIntent
address eoa,
address payer,
address paymentToken,
address paymentRecipient,
bytes calldata paymentSignature
) public virtual {
Intent calldata intent;
// Equivalent Solidity Code: (In the assembly intent is stored in calldata, instead of memory)
// Intent memory intent = abi.decode(encodedIntent, (Intent));
// Gas Savings:
// ~2.5-3k gas for general cases, by avoiding duplicated bounds checks, and avoiding writing the intent to memory.
// Extracts the Intent from the calldata bytes, with minimal checks.
// NOTE: Only use this implementation if you are sure that the encodedIntent is coming from a trusted source.
// We can avoid standard bound checks here, because the Orchestrator already does these checks when it interacts with ALL the fields in the intent using solidity.
assembly ("memory-safe") {
let t := calldataload(encodedIntent.offset)
intent := add(t, encodedIntent.offset)
// Bounds check. We don't need to explicitly check the fields here.
// In the self call functions, we will use regular Solidity to access the
// dynamic fields like `signature`, which generate the implicit bounds checks.
if or(shr(64, t), lt(encodedIntent.length, 0x20)) { revert(0x00, 0x00) }
}

if (!LibBit.and(
msg.sender == ORCHESTRATOR,
LibBit.or(intent.eoa == address(this), intent.payer == address(this))
msg.sender == ORCHESTRATOR, LibBit.or(eoa == address(this), payer == address(this))
)) {
revert Unauthorized();
}

// If this account is the paymaster, validate the paymaster signature.
if (intent.payer == address(this)) {
if (payer == address(this)) {
if (_getAccountStorage().paymasterNonces[intentDigest]) {
revert PaymasterNonceError();
}
_getAccountStorage().paymasterNonces[intentDigest] = true;

(bool isValid, bytes32 k) =
unwrapAndValidateSignature(intentDigest, intent.paymentSignature);
(bool isValid, bytes32 k) = unwrapAndValidateSignature(intentDigest, paymentSignature);

// Set the target key hash to the payer's.
keyHash = k;
Expand All @@ -654,13 +682,13 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
}
}

TokenTransferLib.safeTransfer(intent.paymentToken, intent.paymentRecipient, paymentAmount);
TokenTransferLib.safeTransfer(paymentToken, paymentRecipient, paymentAmount);

// Increase spend.
if (!(keyHash == bytes32(0) || _isSuperAdmin(keyHash))) {
SpendStorage storage spends = _getGuardedExecutorKeyStorage(keyHash).spends;
_incrementSpent(spends.spends[intent.paymentToken], intent.paymentToken, paymentAmount);
_incrementSpent(spends.spends[paymentToken], paymentToken, paymentAmount);
}

// Done to avoid compiler warnings.
intentDigest = intentDigest;
}
Expand Down Expand Up @@ -747,6 +775,6 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
returns (string memory name, string memory version)
{
name = "IthacaAccount";
version = "0.5.10";
version = "0.5.11";
}
}
Loading