Non-compliance with EIP-4337 spec

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/CoinbaseSmartWallet.sol#L137-L167

Vulnerability details

Impact

When validating signatures, the validateUserOp function must always return SIG_VALIDATION_FAILED if signature validation fails.

According to EIP 4337 specifications: 'If the account does not support signature aggregation, it MUST validate the signature is a valid signature of the userOpHash, and SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error MUST revert.'

https://eips.ethereum.org/EIPS/eip-4337

Proof of Concept

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/CoinbaseSmartWallet.sol#L137-L167

function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds)
        public
        payable
        virtual
        onlyEntryPoint
        payPrefund(missingAccountFunds)
        returns (uint256 validationData)
    {
        uint256 key = userOp.nonce >> 64;

        // 0xbf6ba1fc = bytes4(keccak256("executeWithoutChainIdValidation(bytes)"))
        if (userOp.callData.length >= 4 && bytes4(userOp.callData[0:4]) == 0xbf6ba1fc) {
            userOpHash = getUserOpHashWithoutChainId(userOp);
            if (key != REPLAYABLE_NONCE_KEY) {
                revert InvalidNonceKey(key);
            }
        } else {
            if (key == REPLAYABLE_NONCE_KEY) {
                revert InvalidNonceKey(key);
            }
        }

        // Return 0 if the recovered address matches the owner.
        if (_validateSignature(userOpHash, userOp.signature)) {
            return 0;
        }

        // Else return 1, which is equivalent to:
        // `(uint256(validAfter) << (160 + 48)) | (uint256(validUntil) << 160) | (success ? 0 : 1)`
        // where `validUntil` is 0 (indefinite) and `validAfter` is 0.
        return 1;
    }

Tools Used

Recommended Mitigation Steps

change

     revert InvalidNonceKey(key);

by

return SIG_VALIDATION_FAILED;

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #10

[raymondfam]

This would require refactoring that might interfere with the intended logic if revert isn't allowed.

[3docSec]

Starting from the same quote reported in the finding:

If the account does not support signature aggregation, it MUST validate the signature is a valid signature of the userOpHash, and SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error MUST revert.

The protocol complies strictly with that statement:

• if the signature does not match, it returns 1 which is the value of SIG_VALIDATION_FAILED, as it SHOULD do
• for any other error (replay not allowed) it reverts as it MUST do
• validUntil and validAfter are always 0, which is OK because the wallet's UserOperation does not allow specifying validity windows

[c4-judge]

3docSec marked the issue as unsatisfactory: Insufficient proof