From 0d788ce993e31e9ffec565c2d8e929fcf59b378f Mon Sep 17 00:00:00 2001 From: hazim-j Date: Fri, 1 Aug 2025 17:17:26 +1000 Subject: [PATCH] Findings 1: Consistent custom errors everywhere --- src/account/KeystoreAccountFactory.sol | 4 +++- src/lib/OnlyKeystore.sol | 17 +++++++++++++++++ src/verifier/UserOpECDSAVerifier.sol | 14 +++----------- src/verifier/UserOpMultiSigVerifier.sol | 13 +++---------- src/verifier/UserOpWebAuthnCosignVerifier.sol | 13 +++---------- src/verifier/UserOpWebAuthnVerifier.sol | 14 +++----------- test/account/KeystoreAccountFactory.t.sol | 2 +- test/verifier/UserOpECDSAVerifier.t.sol | 3 ++- test/verifier/UserOpMultiSigVerifier.t.sol | 3 ++- .../verifier/UserOpWebAuthnCosignVerifier.t.sol | 3 ++- test/verifier/UserOpWebAuthnVerifier.t.sol | 3 ++- 11 files changed, 41 insertions(+), 48 deletions(-) create mode 100644 src/lib/OnlyKeystore.sol diff --git a/src/account/KeystoreAccountFactory.sol b/src/account/KeystoreAccountFactory.sol index 59f7576..4acf33d 100644 --- a/src/account/KeystoreAccountFactory.sol +++ b/src/account/KeystoreAccountFactory.sol @@ -10,6 +10,8 @@ import {IKeystore} from "../interface/IKeystore.sol"; import {KeystoreAccount} from "./KeystoreAccount.sol"; contract KeystoreAccountFactory { + error NotFromSenderCreator(); + KeystoreAccount public immutable accountImplementation; IEntryPoint public immutable entryPoint; ISenderCreator public immutable senderCreator; @@ -21,7 +23,7 @@ contract KeystoreAccountFactory { } function createAccount(bytes32 refHash, uint256 salt) public returns (KeystoreAccount ret) { - require(msg.sender == address(senderCreator), "only callable from SenderCreator"); + require(msg.sender == address(senderCreator), NotFromSenderCreator()); address addr = getAddress(refHash, salt); uint256 codeSize = addr.code.length; if (codeSize > 0) { diff --git a/src/lib/OnlyKeystore.sol b/src/lib/OnlyKeystore.sol new file mode 100644 index 0000000..b18c50a --- /dev/null +++ b/src/lib/OnlyKeystore.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.28; + +abstract contract OnlyKeystore { + error NotFromKeystore(); + + address public immutable keystore; + + constructor(address aKeystore) { + keystore = aKeystore; + } + + modifier onlyKeystore() { + require(msg.sender == keystore, NotFromKeystore()); + _; + } +} diff --git a/src/verifier/UserOpECDSAVerifier.sol b/src/verifier/UserOpECDSAVerifier.sol index 1a1bd83..d97bc3d 100644 --- a/src/verifier/UserOpECDSAVerifier.sol +++ b/src/verifier/UserOpECDSAVerifier.sol @@ -6,18 +6,10 @@ import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOper import {ECDSA} from "solady/utils/ECDSA.sol"; import {IVerifier} from "../interface/IVerifier.sol"; +import {OnlyKeystore} from "../lib/OnlyKeystore.sol"; -contract UserOpECDSAVerifier is IVerifier { - address public immutable keystore; - - modifier onlyKeystore() { - require(msg.sender == keystore, "verifier: not from Keystore"); - _; - } - - constructor(address aKeystore) { - keystore = aKeystore; - } +contract UserOpECDSAVerifier is IVerifier, OnlyKeystore { + constructor(address aKeystore) OnlyKeystore(aKeystore) {} function validateData(bytes32 message, bytes calldata data, bytes calldata config) external diff --git a/src/verifier/UserOpMultiSigVerifier.sol b/src/verifier/UserOpMultiSigVerifier.sol index f409a4a..5bd4524 100644 --- a/src/verifier/UserOpMultiSigVerifier.sol +++ b/src/verifier/UserOpMultiSigVerifier.sol @@ -6,24 +6,17 @@ import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOper import {ECDSA} from "solady/utils/ECDSA.sol"; import {IVerifier} from "../interface/IVerifier.sol"; +import {OnlyKeystore} from "../lib/OnlyKeystore.sol"; -contract UserOpMultiSigVerifier is IVerifier { +contract UserOpMultiSigVerifier is IVerifier, OnlyKeystore { bytes1 public constant SIGNATURES_ONLY_TAG = 0xff; - address public immutable keystore; struct SignerData { uint8 index; bytes signature; } - modifier onlyKeystore() { - require(msg.sender == keystore, "verifier: not from Keystore"); - _; - } - - constructor(address aKeystore) { - keystore = aKeystore; - } + constructor(address aKeystore) OnlyKeystore(aKeystore) {} function validateData(bytes32 message, bytes calldata data, bytes calldata config) external diff --git a/src/verifier/UserOpWebAuthnCosignVerifier.sol b/src/verifier/UserOpWebAuthnCosignVerifier.sol index 7b897ae..e08581c 100644 --- a/src/verifier/UserOpWebAuthnCosignVerifier.sol +++ b/src/verifier/UserOpWebAuthnCosignVerifier.sol @@ -8,19 +8,12 @@ import {LibBytes} from "solady/utils/LibBytes.sol"; import {WebAuthn} from "solady/utils/WebAuthn.sol"; import {IVerifier} from "../interface/IVerifier.sol"; +import {OnlyKeystore} from "../lib/OnlyKeystore.sol"; -contract UserOpWebAuthnCosignVerifier is IVerifier { +contract UserOpWebAuthnCosignVerifier is IVerifier, OnlyKeystore { bytes1 public constant SIGNATURES_ONLY_TAG = 0xff; - address public immutable keystore; - modifier onlyKeystore() { - require(msg.sender == keystore, "verifier: not from Keystore"); - _; - } - - constructor(address aKeystore) { - keystore = aKeystore; - } + constructor(address aKeystore) OnlyKeystore(aKeystore) {} function validateData(bytes32 message, bytes calldata data, bytes calldata config) external diff --git a/src/verifier/UserOpWebAuthnVerifier.sol b/src/verifier/UserOpWebAuthnVerifier.sol index 41a0244..2639f7d 100644 --- a/src/verifier/UserOpWebAuthnVerifier.sol +++ b/src/verifier/UserOpWebAuthnVerifier.sol @@ -6,18 +6,10 @@ import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOper import {WebAuthn} from "solady/utils/WebAuthn.sol"; import {IVerifier} from "../interface/IVerifier.sol"; +import {OnlyKeystore} from "../lib/OnlyKeystore.sol"; -contract UserOpWebAuthnVerifier is IVerifier { - address public immutable keystore; - - modifier onlyKeystore() { - require(msg.sender == keystore, "verifier: not from Keystore"); - _; - } - - constructor(address aKeystore) { - keystore = aKeystore; - } +contract UserOpWebAuthnVerifier is IVerifier, OnlyKeystore { + constructor(address aKeystore) OnlyKeystore(aKeystore) {} function validateData(bytes32 message, bytes calldata data, bytes calldata config) external diff --git a/test/account/KeystoreAccountFactory.t.sol b/test/account/KeystoreAccountFactory.t.sol index b6e5df5..a5e4315 100644 --- a/test/account/KeystoreAccountFactory.t.sol +++ b/test/account/KeystoreAccountFactory.t.sol @@ -58,7 +58,7 @@ contract KeystoreAccountFactoryTest is Test { vm.assume(caller != address(entryPoint.senderCreator())); vm.prank(caller); - vm.expectRevert("only callable from SenderCreator"); + vm.expectRevert(KeystoreAccountFactory.NotFromSenderCreator.selector); factory.createAccount(refHash, salt); } } diff --git a/test/verifier/UserOpECDSAVerifier.t.sol b/test/verifier/UserOpECDSAVerifier.t.sol index 6f3fe44..d9710b9 100644 --- a/test/verifier/UserOpECDSAVerifier.t.sol +++ b/test/verifier/UserOpECDSAVerifier.t.sol @@ -6,6 +6,7 @@ import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOper import {Test} from "forge-std/Test.sol"; import {ECDSA} from "solady/utils/ECDSA.sol"; +import {OnlyKeystore} from "../../src/lib/OnlyKeystore.sol"; import {UserOpECDSAVerifier} from "../../src/verifier/UserOpECDSAVerifier.sol"; contract UserOpECDSAVerifierTest is Test { @@ -50,7 +51,7 @@ contract UserOpECDSAVerifierTest is Test { function testFuzz_validateDataInvalidCaller(address keystore) public { vm.assume(keystore != address(this)); vm.prank(keystore); - vm.expectRevert("verifier: not from Keystore"); + vm.expectRevert(OnlyKeystore.NotFromKeystore.selector); verifier.validateData(0, "", ""); } diff --git a/test/verifier/UserOpMultiSigVerifier.t.sol b/test/verifier/UserOpMultiSigVerifier.t.sol index f70dd83..3e7ae45 100644 --- a/test/verifier/UserOpMultiSigVerifier.t.sol +++ b/test/verifier/UserOpMultiSigVerifier.t.sol @@ -7,6 +7,7 @@ import {Test} from "forge-std/Test.sol"; import {ECDSA} from "solady/utils/ECDSA.sol"; import {LibString} from "solady/utils/LibString.sol"; +import {OnlyKeystore} from "../../src/lib/OnlyKeystore.sol"; import {UserOpMultiSigVerifier} from "../../src/verifier/UserOpMultiSigVerifier.sol"; contract UserOpMultiSigVerifierTest is Test { @@ -64,7 +65,7 @@ contract UserOpMultiSigVerifierTest is Test { function testFuzz_validateDataInvalidCaller(address keystore) public { vm.assume(keystore != address(this)); vm.prank(keystore); - vm.expectRevert("verifier: not from Keystore"); + vm.expectRevert(OnlyKeystore.NotFromKeystore.selector); verifier.validateData(0, "", ""); } diff --git a/test/verifier/UserOpWebAuthnCosignVerifier.t.sol b/test/verifier/UserOpWebAuthnCosignVerifier.t.sol index c2e57fb..1db638d 100644 --- a/test/verifier/UserOpWebAuthnCosignVerifier.t.sol +++ b/test/verifier/UserOpWebAuthnCosignVerifier.t.sol @@ -11,6 +11,7 @@ import {LibString} from "solady/utils/LibString.sol"; import {P256} from "solady/utils/P256.sol"; import {WebAuthn} from "solady/utils/WebAuthn.sol"; +import {OnlyKeystore} from "../../src/lib/OnlyKeystore.sol"; import {UserOpWebAuthnCosignVerifier} from "../../src/verifier/UserOpWebAuthnCosignVerifier.sol"; contract UserOpWebAuthnCosignVerifierTest is Test { @@ -97,7 +98,7 @@ contract UserOpWebAuthnCosignVerifierTest is Test { function testFuzz_validateDataInvalidCaller(address keystore) public { vm.assume(keystore != address(this)); vm.prank(keystore); - vm.expectRevert("verifier: not from Keystore"); + vm.expectRevert(OnlyKeystore.NotFromKeystore.selector); verifier.validateData(0, "", ""); } diff --git a/test/verifier/UserOpWebAuthnVerifier.t.sol b/test/verifier/UserOpWebAuthnVerifier.t.sol index 5e1a9aa..6d2152d 100644 --- a/test/verifier/UserOpWebAuthnVerifier.t.sol +++ b/test/verifier/UserOpWebAuthnVerifier.t.sol @@ -11,6 +11,7 @@ import {LibString} from "solady/utils/LibString.sol"; import {P256} from "solady/utils/P256.sol"; import {WebAuthn} from "solady/utils/WebAuthn.sol"; +import {OnlyKeystore} from "../../src/lib/OnlyKeystore.sol"; import {UserOpWebAuthnVerifier} from "../../src/verifier/UserOpWebAuthnVerifier.sol"; contract UserOpWebAuthnVerifierTest is Test { @@ -70,7 +71,7 @@ contract UserOpWebAuthnVerifierTest is Test { function testFuzz_validateDataInvalidCaller(address keystore) public { vm.assume(keystore != address(this)); vm.prank(keystore); - vm.expectRevert("verifier: not from Keystore"); + vm.expectRevert(OnlyKeystore.NotFromKeystore.selector); verifier.validateData(0, "", ""); }