feat: add native timelock to account keys #329

[Jds-23]

No description provided.

[Jds-23]

@legion2002 @gakonst

[legion2002]

I think this PR is on the right track. Requested a few implementation detail changes.
Also can you please rebase with latest main to remove old dependencies like devtools and safe-singleton deployer, and also update to latest foundry nightly linting.

[legion2002]

This approach has an intricacy that the relayer payment and the spending limit check happens at the time of adding the timelock.
Not at the time of actually executing it.
I think this could lead to some weird kinds of attacks and inconsistencies. Like the relayer takes the payments, but never executes the action after the timelock.
Or the spending limit changes by the time the timelock is executed, so you can bypass spending limits.

A cleaner way to do this would be, to add a separate prepTimelock function. Where you can pass in just the calls and nonce ( or maybe user can directly pass in the digest ), and it adds the digest to the timelock mapping.

Then this execute checks that for timelocked keys, the timelock should already have passed. If not it reverts.

[Jds-23]

The spend limit checks happens at the time of executing. As the function executeTimelock calls the GuardedExecutor._execute(Call[] calldata calls, bytes32 keyHash) in line 658.

[Jds-23]

@legion2002

[legion2002]

I don't like the idea of adding another execution input with executeTimelock since it is not 7821 compliant.

We should keep the same execute() function, but instead create a new function like startTimelock which adds the timelock key to the storage, and then someone can call the normal execute function later to do the actual execution.

In this case we would just check the timelock status in the main execute function

[Jds-23]

Hey @legion2002, updated the execution flow. Now, the new prepTimelock function is used to add a timelocked and validateTimelockIsExecutable helps unwrapAndValidateSignature validate the timelock, and execution goes through execute.

The new optimisations will be with:

1. Handle storage bloat from timelocks, as execute is used to execute the timelock, we are not removing the timelock from storage. Either add a manual cleaning process or somehow trigger a cleanup by detecting in the execute function itself.
2. Add a way to cancel a pending timelock. We could do it by invalidating the nonce.

[legion2002]

Good work with this! Having the validateTimelock function in unwrapAndValidateSignature makes sense, specially because you don't want a key to transfer the money out of your account using some kind of permit magic.
Having said that, we only allow super admin keys to do isValidSignature so maybe it is a better idea to validate timelock just during execution, and allow the key to make any unwrapAndValidateSignature signatures, without timelock.

I am leaning towards the latter, because this means we can reduce storage bloat, by checking timelock inside the execution and then removing it.

Also for cancelling a pending timelock, we can just allow the key itself, or any admin key to cancel any timelock.

ButI think the PR is at a good place right for now.
We'll figure out what's the right time to get this in, depending on how we'll schedule the next few releases.

Thanks for your contribution! Will let you know when/if we plan to schedule the next release.