`_validateSignature` doesn't check for deadline

[hats-bug-reporter[bot]]

Github username: @0x3b33 Twitter username: -- Submission hash (on-chain): 0xae191bb514c98b3f23817179d23e734dd39b9fed14fd1acd8880a73fa134c7a4 Severity: medium

Description: Description\ _validateSignature, as documented by the EIP-4337 docs, MUST perform specific tasks to be compliant and safe.

One of these tasks is checking validUntil and validAfter, or in simple terms, determining from when and up to when the TX can be executed.

Without these checks, the signature is always valid, which can be dangerous as some executions are time-dependent.

Example:\

1. Alice sends a UserOperation to execute a swap, deposit, or withdraw.
2. The transaction sits in the mempool for a while.
3. The price of the asset changes.
4. The transaction finally gets executed, but at a worse price than Alice wanted.

In this case, Alice can lose some, if not all, of her funds if the transaction is executed too far into the future. Same can be said for validAfter in cases where the user wants to execute his TX after some X period of time.

Attachments\ Implement a deadline check that verifies if the signature is valid within the specified timeframe and reverts if it is not.

[mihailo-maksa]

While the suggestion to include validUntil and validAfter fields is noted, it is not relevant to our current implementation for the following reasons:

1. Non-Dependence on Time-Ranges: Our contract design does not rely on time-sensitive operations. The primary use case does not require transactions to be valid only within specific timeframes. Therefore, the absence of validUntil and validAfter checks does not introduce a vulnerability.
2. Compliance with Standards: The BaseAccount contract includes provisions for incorporating time-range checks if needed. However, as our implementation does not require scheduled or delayed operations, we have chosen not to use these fields.
3. Transaction Handling: Our focus is on ensuring that transactions are executed promptly. If a transaction remains in the mempool for an extended period, it is up to the user or the network conditions, not a flaw in our contract design.

In conclusion, the need for validUntil and validAfter checks depends on the specific requirements of the contract. Since our implementation does not involve time-based constraints, the current design is appropriate and secure. Finally, the comments in the BaseAccount contract, inherited directly by our AtomWallet contract clearly state that if no scheduled or delayed transactions are needed, it is enough to just return the SIG_VALIDATION_FAILED value, which we already do, and therefore, this issue is not a vulnerability but rather a feature that is not applicable to our use case, and for that reason, we consider this issue to be invalid.

[0x3b33]

Good points, however I still think that this issue is not invalid.

The point given here can be considered valid for system contracts (like EthMultiVault), however the AtomWallet is made as a *wallet and it seems like it's design is made to handle normal user operations in the whole DEFI (lending, borrowing, swapping, depositing, staking, etc.). However with the current lack of time validation most of these actions will be dangerous to execute, as if gone wrong they would cause the user to lose some or most of their funds.

Non-Dependence on Time-Ranges: Our contract design does not rely on time-sensitive operations. The primary use case does not require transactions to be valid only within specific timeframes. Therefore, the absence of validUntil and validAfter checks does not introduce a vulnerability.

The third case is incorrect. The user should not be to blame if the code cannot handle system congestion (heavy chain usage). Such mechanics are done by the biggest protocols, as they optimize for user safety first. Examples are deadline on UNI or deadline on GMX.

Transaction Handling: Our focus is on ensuring that transactions are executed promptly. If a transaction remains in the mempool for an extended period, it is up to the user or the network conditions, not a flaw in our contract design.

Do you think the above points are fair ?

[mihailo-maksa]

Our detailed perspective on the issue:

This issue is not directly critical for the Intuition protocol itself but affects users individually based on their responsibility. We recognize that the AtomWallets are designed to eventually support unrestricted participation in various DeFi functionalities. Thus, implementing time validation is indeed a valuable safeguard to protect users from potential losses due to delayed transactions.

The lack of time validation does not harm the entire protocol, but it is a helpful feature to prevent users from inadvertently making poor decisions. While it’s primarily the user's responsibility, incorporating this safeguard would enhance the overall user experience and security.

In summary, while the absence of time validation does not currently present a vulnerability to the protocol, it is a beneficial enhancement for protecting users from potential negative outcomes in their transactions. For that reason, we consider it a low severity vulnerability.

For reference, it's not accepted as medium since it doesn't satisfy the following criterion: "An exploit that in some way compromises the experience of other Intuition users," with emphasis on the "other users" as the most critical point. You can find more details about severity levels here.

[0x3b33]

I agree with the decision. Thank you again for revisiting and making the change.

[mihailo-maksa]

Hi @0x3b33, do you have any suggestion or recommendation on how to resolve this issue?

[0x3b33]

It would requite a little bit of code refactoring. For example we can package PackedUserOperation.callData to have it's first 12 bytes split into validUntil and validAfter and the next bytes to the real callData, where we would allow the user to input some valid times for them.

Another option is to pack the 12 bytes into accountGasLimits, this will mean that the real accountGasLimits would be 20 bytes, but I think 20 bytes are enough for a small uint like gas limit.

Packing the time may be hard as PackedUserOperation doesn't give us a specific slot for time management.

struct PackedUserOperation {
    address sender;
    uint256 nonce;
    bytes initCode;
    bytes callData;
    bytes32 accountGasLimits;
    uint256 preVerificationGas;
    bytes32 gasFees;
    bytes paymasterAndData;
    bytes signature;
}

What do you think ?

[mihailo-maksa]

Hi @0x3b33,

Your suggestions sound very interesting and practical. If I understand correctly, refactoring the PackedUserOperation.callData to include validUntil and validAfter shouldn't be too much of a lift. Here's an example implementation based on your recommendation. Does this look like what we're aiming for?

Example Implementation

Extract ValidUntil and ValidAfter

/// @notice Extract the validUntil and validAfter from the call data
/// @param callData the call data
///
/// @return validUntil the valid until timestamp
/// @return validAfter the valid after timestamp
/// @return actualCallData the actual call data of the user operation
function extractValidUntilAndValidAfter(bytes calldata callData)
    internal
    pure
    returns (uint256 validUntil, uint256 validAfter, bytes memory actualCallData)
{
    require(callData.length >= 24, "Invalid callData length");

    validUntil = abi.decode(callData[:12], (uint256));
    validAfter = abi.decode(callData[12:24], (uint256));
    actualCallData = callData[24:];

    return (validUntil, validAfter, actualCallData);
}

Validate Signature

/// @notice Validate the signature of the user operation
///
/// @param userOp the user operation
/// @param userOpHash the hash of the user operation
///
/// @return validationData the validation data (0 if successful)
/// NOTE: Implements the template method of BaseAccount
function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash)
    internal
    virtual
    override
    returns (uint256 validationData)
{
    (uint256 validUntil, uint256 validAfter,) = extractValidUntilAndValidAfter(userOp.callData);

    if (block.timestamp < validAfter || block.timestamp > validUntil) {
        return SIG_VALIDATION_FAILED;
    }

    bytes32 hash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", userOpHash));

    (address recovered, ECDSA.RecoverError recoverError, bytes32 errorArg) =
        ECDSA.tryRecover(hash, userOp.signature);

    if (recoverError == ECDSA.RecoverError.InvalidSignature) {
        revert Errors.AtomWallet_InvalidSignature();
    } else if (recoverError == ECDSA.RecoverError.InvalidSignatureLength) {
        revert Errors.AtomWallet_InvalidSignatureLength(uint256(errorArg));
    } else if (recoverError == ECDSA.RecoverError.InvalidSignatureS) {
        revert Errors.AtomWallet_InvalidSignatureS(errorArg);
    }

    if (recovered != owner()) {
        return SIG_VALIDATION_FAILED;
    }

    return 0;
}

Let me know if this meets the requirements or if any adjustments are needed, and thanks once again for your efforts to make our protocol better and more secure, we really appreciate it.

[0x3b33]

That's a great implementation. Only a few minor things (not sure if they are gonna be needed, but to mention them just in case):

validUntil is 6-byte timestamp value, or zero for “infinite”. The UserOp is valid only up to this time. validAfter is 6-byte timestamp. The UserOp is valid only after this time.

• validUntil can be 0, meaning there won't be an expiration
• also both terms are exclusive, meaning they need to be block.timestamp <= validAfter || block.timestamp >= validUntil

Here is the slight change:

    if (block.timestamp <= validAfter || (block.timestamp >= validUntil && validUntil != 0)) {
        return SIG_VALIDATION_FAILED;
    }

and thanks once again for your efforts to make our protocol better and more secure, we really appreciate it.

No problem, it's my job to (and life) to help protocols and developers. If you need any help with other issues feel free to tag me in 🫡.

[mihailo-maksa]

I really appreciate your advice regarding this issue. This new change makes a lot more sense now. We truly value your dedication and mission-driven attitude. Thanks once again, and we hope to work together with you again soon!