feat: [v0.8-develop] account self call restrictions

[adamegyed]

Motivation

In ERC-6900 v0.8, we’re planning on explicitly defining account self-calls to bypass validation. Previously in 6900 v0.7, this behavior was left up to the runtime validation function, but we depended on this behavior for things that the core account needed to do - like batching internal calls by setting the target to self.

Self-calls are also necessary for 6900 v0.8 to efficiently support two new workflows: executeUserOp, the function introduced in EntryPoint v0.7 to allow accounts to access user op contents in the execution phase, and executeWithAuthorization, a workflow to allow selecting between different runtime validation functions & to provide data to the runtime validation function and its hooks.

In both of those functions, a self-call is used after the “validation” checks are complete. Then, the account is able to execute the function(s) as regular native functions or functions routed through the fallback. Doing so lets us avoid having internal routing for every external function. Internal routing is difficult, un-idiomatic solidity because it involves reimplementing the automatically generated function dispatcher. This would also increase implementation complexity of 6900 v0.8.

However, there is a notable issue with self-call authorization: If a validation function is added to either standard execute functions (execute or executeBatch), then that validation function effectively gets “root” access. Any execution function, including execution functions that the validation function is not allowed to call directly, may instead be called by performing a self-call and encoding the execution function invocation into calldata.

We want to prevent the privilege escalation, while still allowing self-calling for the purposes of batching actions, and handling executeWithAuthorization and executeUserOp.

Solution

When running user op and runtime validation, add an extra check:

• If the function is execute, disallow a target address of the account. This is unnecessary wrapping, and the inner call may instead be pulled up to the entire calldata itself.
• If the function is executeBatch, then:
  • for each Call where the target is the account, inspect the selector in calldata:
    • the validation currently being used must apply to that selector
    • the selector must not be another recursing call to the account's execute or executeBatch functions.

This preserves the ability to use self-calls for batched actions, executeUserOp, and executeWIthAuthorization, while preventing privilege escalation.

Also add a test that shows these different use cases with ComprehensivePlugin.

Design Questions

In earlier attempts to solve this problem, I suggested allowing arbitrarily-deep recursion via execute/executeBatch. This would pose a problem to pre-validation hooks looking to inspect calldata, wherein we must either:

1. require the hooks to also implement arbitrary-depth recursion
2. have the account unpack all inner calls, and fire the hooks for each invocation.

However, this PR proposes an approach of entirely disallowing execute to self, and only allowing a max recursion depth of 1 for executeBatch. This simplifies things by capping the depth of any required calldata-inspecting pre validation hooks to max 1, and only for the executeBatch selector. Given this constraint, I think it is reasonable to expect pre-validation hooks that fall into this category of "inspecting calldata selectors + params" to either:

• restrict what selectors a validation may call to exclude executeBatch, and not have to worry about this.
• allow executeBatch, and unpack + handle the logic for each call in the batch within the hook itself.

Does that seem like a reasonable expectation?

[fangting-alchemy]

This effectively prevents execute make self-call, right?

[0xrubes]

I think so, because there isn't really a scenario where execute() would do a single self call except for attempted shenanigans, because the call could just be executed regularly too

[fangting-alchemy]

The solution is simple and makes sense to me. I wonder if there are scenarios we missed where a 2 levels down nested self-calls necessary.
I am for this simplicity approach.

[howydev]

Hmm, don't think the runtime path is protected. Someone with callPermitted permissions can call any msg.sig. We're deprecating the callPermitted path so not necessary to have a solution here, but if we add any other forms of runtime validation, it would also need to do this selector check. If thats the case, does it make sense to add the check in wrapNativeFunction instead?

Check I was thinking of:
if (msg.sender == address(this) && selector == execute/executeBatch)) => revert

[adamegyed]

Someone with callPermitted permissions can call any msg.sig

Ahh shoot, good catch on this. Yes we're deprecating that path, but this PR still has it. I'll make a note to hold off on merging this until we get that change in from #67, and that fix is patched.

As for the inner check - I think within the scope of this PR's change, we're OK without that check, but if we ever re-introduce something like callPermitted (a way to authorize directly calling the account, not through executeWithAuthorization), then we'll need to add a check like that.

[howydev]

Makes sense, agree that considering the shape of runtime validation is out of scope of this PR. Additions here LGTM

[huaweigu]

lgtm

[huaweigu]

isGlobal? I guess it depends on if renaming PR gets in first

[adamegyed]

Yeah, the rename is on another branch, will merge as soon as possible

[huaweigu]

Should we explicitly check executeWithAuthorization and executeUserOp and only allow these to pass?

[adamegyed]

• executeUserOp doesn't yet exist on this branch, but on the branch where it does, it requires msg.sender to be the entrypoint, so we don't need extra handling here.
• executeWithAuthorization will re-run a new validation function, so it should be OK to allow - it doesn't really make sense to call it in a nested way via execute/executeBatch due it just adding gas, but it won't break any security guarantees.

[huaweigu]

Thanks! I meant reverting on line 609 if random function selector is accidentally calling but not in (executeWithAuthorization, executeUserOp). Maybe it's a bit too defensive.

[huaweigu]

lgtm

[huaweigu]

Thanks! I meant reverting on line 609 if random function selector is accidentally calling but not in (executeWithAuthorization, executeUserOp). Maybe it's a bit too defensive.

[huaweigu]

@adamegyed Is this line redundant (same as line 626)? Or do you want to pull the actual selector from bytes4(userOp.callData[4:8])?

[adamegyed]

We do want to pull the actual selector from bytes4(userOp.callData[4:8]), because this is the calling convention of 4337's IAccountExecute interface. This is in this format during validation, but it will look a bit different during execution. Specifically, when using executeUserOp:

• During validation, userOp.callData will contain:
  • bytes [0:4]: executeUserOp selector
  • bytes[4:end]: actual callData
• During execution, the EntryPoint will send a call to executeUserOp with the full PackedUserOp struct, and the user op hash.
  • userOp.callData will have the same format as above, which is why we need to skip the first 4 bytes before doing the self-call in the account's implementation of executeUserOp.

It's a bit non-standard, but you can see how it gets encoded here: https://github.com/eth-infinitism/account-abstraction/blob/v0.7.0/contracts/core/EntryPoint.sol#L101-L107

[huaweigu]

Thanks, I think I overlooked the line callData = callData[4:]; (I was probably just following the order in comment here :) ) when I made the previous comment. The current impl is essentially equivalent to

outerSelector = bytes4(callData[4:8]);
callData = callData[4:];