zerodevapp / kernel

https://docs.zerodev.app/
MIT License
178 stars 63 forks source link

Delayed user ops #33

Closed nebolax closed 1 year ago

nebolax commented 1 year ago

Hi, team! Sorry for bothering you with issues, but I have one question.

It is not possible right now to make a delayed (valid only after a certain timestamp) user op with kernel signed via ECDSA validator, right? What would be the easiest approach to implement it?

leekt commented 1 year ago

Hi :) sorry for the late response. "Delay" is quite tricky question, because of the 4337 rule, block.timestamp cannot be accessed on validation phase.

And also, making a delay might also make some vulnerabilities or UX problems when you make everything on chain

For example if we have this

contract DelayECDSAValidator {
    mapping(address kernel => address) public owner;
    mapping(address kernel => ValidAfter) public delay;
    mapping(bytes32 userOpHash => mapping(address kernel => ValidAfter)) public validAfter;

    function approve(address kernel, bytes32 userOpHash) external {
        /// do some check for msg.sender
        validAfter[userOpHash][kernel] = block.timestamp + delay[kernel];
    }

    function validateUserOp(...) external returns(ValidationData validationData) {
        validationData = packValidationData(validAfter[userOpHash][msg.sender], ValidUntil.wrap(0));
        if(signer != owner[msg.sender]) {
            return SIG_VALIDATION_FAILED;
        } 
    }
}

this does not seems to introduce any vulnerability and looks fine BUT it will be hard for this to be actually "executed" since you does not include the gas data (preVerificationGas, verificationGasLimit, callDataGaslimit, baseFeePerGas, ...) which is critical to submitting to the bundler.

it is also a case when you use a offchain signature that includes validAfter(eg. use keccak256(userOpHash | validAfter) for signing instead of userOpHash)

if you allow only the calldata to be signed by the user, then it will open vulnerability to drain the eth. for example, if we modify the code above to user callDataHash instead of userOpHash,

contract DelayECDSAValidator {
    mapping(address kernel => address) public owner;
    mapping(address kernel => ValidAfter) public delay;
    mapping(bytes32 callDataHash => mapping(address kernel => ValidAfter)) public validAfter;

    function approve(address kernel, bytes32 callDataHash) external {
        /// do some check for msg.sender
        validAfter[callDataHash][kernel] = block.timestamp + delay[kernel];
    }

    function validateUserOp(...) external returns(ValidationData validationData) {
        bytes32 callDataHash = keccak256(userOp.callData);
        validationData = packValidationData(validAfter[callDataHash][msg.sender], ValidUntil.wrap(0));
    }
}

this does look fine but this can be exploited because if attacker user same callData with high preverificationGas and high priority gas fee, then wallet can be drained.(applies to the offchain keccak256(callDataHash | validAfter) too)

One potential idea is forcing the paymasterAndData to prevent wallet for paying the gas

contract DelayECDSAValidator {
    mapping(address kernel => address) public owner;
    mapping(address kernel => ValidAfter) public delay;
    mapping(bytes32 callDataHash => mapping(address kernel => ValidAfter)) public validAfter;

    function approve(address kernel, bytes32 callDataHash) external {
        /// do some check for msg.sender
        validAfter[callDataHash][kernel] = block.timestamp + delay[kernel];
    }

    function validateUserOp(...) external returns(ValidationData validationData) {
        if(userOp.paymasterAndData.length == 0) {
            return SIG_VALIDATION_FAILED;
        }
        bytes32 callDataHash = keccak256(userOp.callData);
        validationData = packValidationData(validAfter[callDataHash][msg.sender], ValidUntil.wrap(0));
    }
}

Also can you provide some usecases for the delayed ecdsa? I can probably come up with some solution here if you want.

nebolax commented 1 year ago

Thanks a lot for the reply! I think I've found a solution actually and made a plugin. I am using it to allow subscriptions functionality.

I did not get this message btw.

"Delay" is quite tricky question, because of the 4337 rule, block.timestamp cannot be accessed on validation phase.

What I've implemented right now is a plugin that accepts a signature of validAfter | signature(userOpHash | validAfter) and then returns the validAfter timestamp to the entry point to be validate.

leekt commented 1 year ago

What I've implemented right now is a plugin that accepts a signature of validAfter | signature(userOpHash | validAfter) and then returns the validAfter timestamp to the entry point to be validate.

If you are happy with it totally fine with me :), let me know if you need any review on the plugin. DM me in twitter if you need privacy on the details.

"Delay" is quite tricky question, because of the 4337 rule, block.timestamp cannot be accessed on validation phase.

When people say "Delay" for userOp, the word itself makes people to think this delay is enforced on-chain, like you submit userOp and it does nothing for given "Delay" and can be executed afterwards. But this is not the case in offchain solution like signing the (userOpHash | validAfter) since validator has to believe that signer signed the delay. Which is totally fine for me but it may be quite confusing to some people :)

nebolax commented 1 year ago

Aha, got it, thanks! Will reach you out about the plugin review!