Add native merkle sigs to the Account

[legion2002]

This allows us to do multichain operations using account.execute(), and will also be useful for getting the user to sign precalls for specific chains.

[github-actions]

🤖 Bytecode changes detected! EIP-712 domain versions have been automatically updated for: IthacaAccount

[howydev]

encode would result in this:

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

[howydev]

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

[howydev]

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

[howydev]

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

[howydev]

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?

[howydev]

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?

[legion2002]

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.

[howydev]

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?

[howydev]

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

[legion2002]

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.

[legion2002]

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.

[legion2002]

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.