Skip to content
Merged
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
73 changes: 61 additions & 12 deletions src/account/UpgradeableModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ contract UpgradeableModularAccount is
error RequireUserOperationContext();
error RuntimeValidationFunctionMissing(bytes4 selector);
error RuntimeValidationFunctionReverted(address plugin, uint8 functionId, bytes revertReason);
error SelfCallRecursionDepthExceeded();
error SignatureValidationInvalid(address plugin, uint8 functionId);
error UnexpectedAggregator(address plugin, uint8 functionId, address aggregator);
error UnrecognizedFunction(bytes4 selector);
Expand Down Expand Up @@ -216,14 +217,12 @@ contract UpgradeableModularAccount is
payable
returns (bytes memory)
{
bytes4 execSelector = bytes4(data[:4]);

// Revert if the provided `authorization` less than 21 bytes long, rather than right-padding.
FunctionReference runtimeValidationFunction = FunctionReference.wrap(bytes21(authorization[:21]));

// Check if the runtime validation function is allowed to be called
bool isDefaultValidation = uint8(authorization[21]) == 1;
_checkIfValidationApplies(execSelector, runtimeValidationFunction, isDefaultValidation);
_checkIfValidationAppliesCallData(data, runtimeValidationFunction, isDefaultValidation);

_doRuntimeValidation(runtimeValidationFunction, data, authorization[22:]);

Expand Down Expand Up @@ -388,16 +387,12 @@ contract UpgradeableModularAccount is
if (userOp.callData.length < 4) {
revert UnrecognizedFunction(bytes4(userOp.callData));
}
bytes4 selector = bytes4(userOp.callData);
if (selector == this.executeUserOp.selector) {
selector = bytes4(userOp.callData[4:8]);
}

// Revert if the provided `authorization` less than 21 bytes long, rather than right-padding.
FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21]));
bool isDefaultValidation = uint8(userOp.signature[21]) == 1;

_checkIfValidationApplies(selector, userOpValidationFunction, isDefaultValidation);
_checkIfValidationAppliesCallData(userOp.callData, userOpValidationFunction, isDefaultValidation);

// Check if there are permission hooks associated with the validator, and revert if the call isn't to
// `executeUserOp`
Expand Down Expand Up @@ -623,10 +618,64 @@ contract UpgradeableModularAccount is
// solhint-disable-next-line no-empty-blocks
function _authorizeUpgrade(address newImplementation) internal override {}

function _checkIfValidationApplies(bytes4 selector, FunctionReference validationFunction, bool isDefault)
internal
view
{
function _checkIfValidationAppliesCallData(
bytes calldata callData,
FunctionReference validationFunction,
bool isDefault
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

) internal view {
bytes4 outerSelector = bytes4(callData[:4]);
if (outerSelector == this.executeUserOp.selector) {
// If the selector is executeUserOp, pull the actual selector from the following data,
// and trim the calldata to ensure the self-call decoding is still accurate.
callData = callData[4:];
outerSelector = bytes4(callData[:4]);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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:];

}

_checkIfValidationAppliesSelector(outerSelector, validationFunction, isDefault);

if (outerSelector == IStandardExecutor.execute.selector) {
(address target,,) = abi.decode(callData[4:], (address, uint256, bytes));

if (target == address(this)) {
// There is no point to call `execute` to recurse exactly once - this is equivalent to just having
Copy link
Collaborator

Choose a reason for hiding this comment

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

This effectively prevents execute make self-call, right?

Copy link

Choose a reason for hiding this comment

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

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

// the calldata as a top-level call.
revert SelfCallRecursionDepthExceeded();
}
} else if (outerSelector == IStandardExecutor.executeBatch.selector) {
// executeBatch may be used to batch account actions together, by targetting the account itself.
// If this is done, we must ensure all of the inner calls are allowed by the provided validation
// function.

(Call[] memory calls) = abi.decode(callData[4:], (Call[]));

for (uint256 i = 0; i < calls.length; ++i) {
if (calls[i].target == address(this)) {
bytes4 nestedSelector = bytes4(calls[i].data);

if (
nestedSelector == IStandardExecutor.execute.selector
|| nestedSelector == IStandardExecutor.executeBatch.selector
) {
// To prevent arbitrarily-deep recursive checking, we limit the depth of self-calls to one
// for the purposes of batching.
// This means that all self-calls must occur at the top level of the batch.
// Note that plugins of other contracts using `executeWithAuthorization` may still
// independently call into this account with a different validation function, allowing
// composition of multiple batches.
revert SelfCallRecursionDepthExceeded();
}

_checkIfValidationAppliesSelector(nestedSelector, validationFunction, isDefault);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

function _checkIfValidationAppliesSelector(
bytes4 selector,
FunctionReference validationFunction,
bool isDefault
) internal view {
AccountStorage storage _storage = getAccountStorage();

// Check that the provided validation function is applicable to the selector
Expand Down
Loading