Security findings in pq-account: isValid() attack surface, dead code, no key rotation, VERSION mutability #256

We did a quick review of packages/pq-account and found a few issues worth addressing before production readiness.

Finding 1 — isValid() is public with arbitrary logic contract addresses

Severity: High

isValid() is public view and accepts arbitrary preQLogicContractAddress / postQLogicContractAddress as parameters. An attacker can pass in a contract that always returns the verify selector, bypassing both signature checks. The function is misleading as a public API and dangerous for any integrator who calls it directly.

Fix: Make it internal and remove the address parameters — always use the stored addresses:

function _isValid(bytes32 digest, bytes memory preQuantumSig, bytes memory postQuantumSig) internal view returns (bool) {
    ISigVerifier preQ = ISigVerifier(preQuantumLogicContractAddress);
    if (preQ.verify(preQuantumPubKey, digest, preQuantumSig) != preQ.verify.selector) return false;
    ISigVerifier postQ = ISigVerifier(postQuantumLogicContractAddress);
    if (postQ.verify(postQuantumPubKey, digest, postQuantumSig) != postQ.verify.selector) return false;
    return true;
}

Finding 2 — Dead digest.length > 32 guard on a bytes32 parameter

Severity: Low

function isValid(..., bytes32 digest, ...) public view returns (bool) {
    if (digest.length > 32) { return false; } // ← always false, bytes32 is always 32 bytes

bytes32 is a fixed value type — digest.length always equals 32. This check never fires. It looks like a leftover from when digest was bytes memory. The intent (validate input size) was correct but silently disappeared during a type change.

Fix: Remove the dead check. If raw message validation is needed, do it before hashing.

Finding 3 — No key rotation mechanism

Severity: Medium

Keys are set once in the constructor with no update path. If either key is compromised the account is permanently locked. For a PQ account this is critical — NIST PQ schemes are still being evaluated and real deployments need upgrade paths.

Suggested addition:

function rotateKeys(bytes memory newPreQKey, bytes memory newPostQKey) external {
    require(msg.sender == address(_entryPoint), "Only callable via EntryPoint");
    preQuantumPubKey = ISigVerifier(preQuantumLogicContractAddress).setKey(newPreQKey);
    postQuantumPubKey = ISigVerifier(postQuantumLogicContractAddress).setKey(newPostQKey);
    emit KeysRotated(keccak256(newPreQKey), keccak256(newPostQKey));
}
event KeysRotated(bytes32 indexed preQuantumKeyHash, bytes32 indexed postQuantumKeyHash);

Finding 4 — VERSION not immutable in factory

Severity: Low

VERSION is mutable storage in the factory. Since CREATE2 salt includes VERSION, mutating it post-deployment would silently break address derivation for any wallet that pre-computed its address.

Fix: string public immutable VERSION;

Summary

Happy to submit PRs for any of these. The codebase is clean and well-structured — these are expected at alpha stage.