eth-infinitism/account-abstraction

Expose `userOpHash` of the user op currently being executed

NIC619 opened this issue · 5 comments

NIC619 commented

During the execution phase, it would be helpful for the account contract to know the transaction hash of the transaction currently being executed. Like in StarkNet AA, you can use get_tx_info to get the transaction hash or in zkSync AA, the transaction hash is also passed into executeTransaction.

In ERC-4337, we tried to keep the same "separation of layers" as in normal transactions.
Just like normal execution can't tell its nonce, hash, etc, the same is true for erc4337 execution: as an account, you shouldn't care about the transaction that triggered this call.
This is also the reason "execute()" doesn't receive the entire UserOp, but only the "callData".

Do you have explicit use-cases where you need the above information ?

NIC619 commented

In our contract wallet design, we have selfAuthorized pattern to protect important functionalities like upgrade or ownership management in the wallet contract. For example in Safe's addOwner function:

abstract contract SelfAuthorized {
    function requireSelfCall() private view {
        require(msg.sender == address(this), "GS031");
    }

    modifier authorized() {
        // Modifiers are copied around during compilation. This is a function call as it minimized the bytecode size
        requireSelfCall();
        _;
    }
}

function addOwnerWithThreshold(address owner, uint256 _threshold) public authorized {
    ...
}

But we would like to improve on this pattern as selfAuthorized only checks that the wallet called itself but does not check if the validation step actually approved this call to perform important functionalities on the wallet contract. For example, if we enable third party modules to be added to the wallet contract and the module can instruct the wallet contract to call a given contract, then the module can instruct the wallet contract to call itself to pass the SelfAuthorized check and perform important functionalities. This result may not be what the wallet's owner had in mind when he added the module.

So what we are thinking is, to add a more granular check on top of SelfAuthorized. During validation, we store bool validatedOrNot into a mapping mapping(bytes32 userOpHash => mapping(CoreOps => bool)) isCoreOpValidated for each CoreOp validated for the given userOpHash. Then in execution phase, the important functionality check if the CoreOp corresponding to its functionality is validated.

Enum CoreOps {
    Upgrade,
    Ownership,
}

modifier authorized(bytes32 userOpHash, CoreOps op) {
    require(msg.sender == address(this));
    require(isCoreOpValidated[userOpHash][op]);
    _;
}

function validateUserOp(...) {
    ...
    if (selector == this.upgradeTo.selector) {
        // validate
        ...
        (isCoreOpValidated[userOpHash][CoreOps.Upgrade] = true;
    } else if
    ...
}

function execute(...) {
    ...
    address(to).call(data); // `to` would be the wallet contract itself if the user is to perform core ops like upgrade
    ...
}

function upgradeTo(...) authorized(entrypoint.getUserOpHash(), CoreOps.Upgrade) {
    ...
}

What you suggest is that the admin function is called by "execute" method, and is protected by "authorizeSelf", but then you find this authorization too wide, and want to add state info to check if it was validated.

My suggestion:

  • admin function should make sure it is called directly from entrypoint (that is, it validates it was called immediately after "validateUserOp"
  • the validateUserOp will verify the methodSig (userOp.callData[0:4]), and if it is the admin operation, it will perform extra check (e.g. the rightful signers)
  • it can go even one step further: if you have different "roles" for admin operations, then you could validate them too:
    • the authorizedOp gets a first parameter "role"
    • the validateUserOP validates not only the above methodsig, but also this first parameter (uint256(userOp.callData[4:36])), to be valid for this specific signer(s)
NIC619 commented

admin function should make sure it is called directly from entrypoint

Yes, that was initially our implementation but then we discovered that in native AA like StarkNet and zkSync, there's a fixed entry function during execution phase, i.e., the AA OS will always call the __execute__ (or executeTransaction function) at the beginning of execution phase, you can not tell AA OS what function to call. And in __execute__ function, you design your own control flow.

So in order to have an architecture that we can use across 4337 and native AAs, we decided to use the selfAuthorized approach and that's why we proposed to expose userOpHash in 4337 Entrypoint contract so that we can improve on selfAuthorized.

@NIC619 please stake a look at #380 - see if it solves your use case. We're seeking feedback before adding this functionality.