Merge pull request #36 from zerodevapp/fix/tob-kernel-1

fix: timelock policy create proposal using erc4337 with no-op calldata

Author: leekt

src/policies/CallerPolicy.sol

@@ -28,6 +28,11 @@ import {
  *      If you need to validate who signed, use a signer module instead.
  */
 contract CallerPolicy is PolicyBase, IStatelessValidatorWithSender {
+    error PolicyAlreadyInstalled();
+    error PolicyNotLive();
+    error EmptyCallers();
+    error ZeroAddressCaller();
+
     mapping(bytes32 id => mapping(address => Status)) public status;
     /// @notice Maps policy ID => requesting protocol => wallet => whether protocol is allowed
     mapping(bytes32 id => mapping(address caller => mapping(address wallet => bool))) public allowedCaller;
@@ -82,18 +87,18 @@ contract CallerPolicy is PolicyBase, IStatelessValidatorWithSender {
     }
 
     function _policyOninstall(bytes32 id, bytes calldata _data) internal override {
-        require(status[id][msg.sender] == Status.NA, "Already installed");
+        require(status[id][msg.sender] == Status.NA, PolicyAlreadyInstalled());
         address[] memory callers = abi.decode(_data, (address[]));
-        require(callers.length > 0, "Empty callers array");
+        require(callers.length > 0, EmptyCallers());
         for (uint256 i = 0; i < callers.length; i++) {
-            require(callers[i] != address(0), "Zero address caller");
+            require(callers[i] != address(0), ZeroAddressCaller());
             allowedCaller[id][callers[i]][msg.sender] = true;
         }
         status[id][msg.sender] = Status.Live;
     }
 
     function _policyOnUninstall(bytes32 id, bytes calldata _data) internal override {
-        require(status[id][msg.sender] == Status.Live);
+        require(status[id][msg.sender] == Status.Live, PolicyNotLive());
         status[id][msg.sender] = Status.Deprecated;
     }
 

src/policies/TimelockPolicy.sol

@@ -17,6 +17,8 @@ import {
 contract ECDSASigner is SignerBase, IStatelessValidator, IStatelessValidatorWithSender {
     error InvalidDataLength();
     error ZeroAddressSigner();
+    error SignerAlreadySet();
+    error SignerNotSet();
 
     mapping(bytes32 id => mapping(address wallet => address)) public signer;
 
@@ -48,6 +50,12 @@ contract ECDSASigner is SignerBase, IStatelessValidator, IStatelessValidatorWith
             : SIG_VALIDATION_FAILED_UINT;
     }
 
+    /// @notice Validate an ERC-1271 signature
+    /// @dev The `sender` parameter (requesting protocol) is intentionally unused.
+    ///      This signer authenticates the SIGNER (owner), not the requesting protocol.
+    ///      WARNING: Because sender is ignored, any protocol can request signature
+    ///      validation. If you need to restrict which protocols can request signatures,
+    ///      pair this signer with a CallerPolicy.
     function checkSignature(bytes32 id, address sender, bytes32 hash, bytes calldata sig)
         external
         view
@@ -61,15 +69,15 @@ contract ECDSASigner is SignerBase, IStatelessValidator, IStatelessValidatorWith
     }
 
     function _signerOninstall(bytes32 id, bytes calldata _data) internal override {
-        require(signer[id][msg.sender] == address(0), "Already installed");
-        if (_data.length != 20) revert InvalidDataLength();
+        require(signer[id][msg.sender] == address(0), SignerAlreadySet());
+        require(_data.length == 20, InvalidDataLength());
         address signerAddr = address(bytes20(_data[0:20]));
-        if (signerAddr == address(0)) revert ZeroAddressSigner();
+        require(signerAddr != address(0), ZeroAddressSigner());
         signer[id][msg.sender] = signerAddr;
     }
 
     function _signerOnUninstall(bytes32 id, bytes calldata) internal override {
-        require(signer[id][msg.sender] != address(0));
+        require(signer[id][msg.sender] != address(0), SignerNotSet());
         delete signer[id][msg.sender];
     }
 

src/signers/ECDSASigner.sol

@@ -36,6 +36,15 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel
         keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)");
 
     error ZeroWeightSigner();
+    error LengthMismatch();
+    error EmptyGuardians();
+    error ZeroThreshold();
+    error GuardianCannotBeSelf();
+    error ZeroAddressGuardian();
+    error ZeroWeight();
+    error GuardianAlreadyEnabled();
+    error SignersNotSorted();
+    error ThresholdExceedsTotalWeight();
 
     mapping(bytes32 id => mapping(address kernel => WeightedECDSASignerStorage)) public weightedStorage;
     mapping(address guardian => mapping(bytes32 id => mapping(address kernel => GuardianStorage))) public guardian;
@@ -53,22 +62,23 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel
 
         (address[] memory _guardians, uint24[] memory _weights, uint24 _threshold) =
             abi.decode(_data, (address[], uint24[], uint24));
-        require(_guardians.length == _weights.length, "Length mismatch");
-        require(_guardians.length > 0, "No guardians");
-        require(_threshold > 0, "Zero threshold");
+        require(_guardians.length == _weights.length, LengthMismatch());
+        require(_guardians.length > 0, EmptyGuardians());
+        require(_threshold > 0, ZeroThreshold());
 
         weightedStorage[id][msg.sender].firstGuardian = msg.sender;
         for (uint256 i = 0; i < _guardians.length; i++) {
-            require(_guardians[i] != msg.sender, "Guardian cannot be self");
-            require(_guardians[i] != address(0), "Guardian cannot be 0");
-            require(_weights[i] != 0, "Weight cannot be 0");
-            require(guardian[_guardians[i]][id][msg.sender].weight == 0, "Guardian already enabled");
+            require(_guardians[i] != msg.sender, GuardianCannotBeSelf());
+            require(_guardians[i] != address(0), ZeroAddressGuardian());
+            require(_weights[i] != 0, ZeroWeight());
+            require(guardian[_guardians[i]][id][msg.sender].weight == 0, GuardianAlreadyEnabled());
             guardian[_guardians[i]][id][msg.sender] =
                 GuardianStorage({weight: _weights[i], nextGuardian: weightedStorage[id][msg.sender].firstGuardian});
             weightedStorage[id][msg.sender].firstGuardian = _guardians[i];
             weightedStorage[id][msg.sender].totalWeight += _weights[i];
             emit GuardianAdded(_guardians[i], msg.sender, _weights[i]);
         }
+        require(_threshold <= weightedStorage[id][msg.sender].totalWeight, ThresholdExceedsTotalWeight());
         weightedStorage[id][msg.sender].threshold = _threshold;
     }
 
@@ -102,6 +112,12 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel
         return _validateUserOpSignature(id, userOp, userOpHash, userOp.signature, msg.sender);
     }
 
+    /// @notice Validate an ERC-1271 signature
+    /// @dev The `sender` parameter (requesting protocol) is intentionally unused.
+    ///      This signer authenticates the SIGNERS (guardians), not the requesting protocol.
+    ///      WARNING: Because sender is ignored, any protocol can request signature
+    ///      validation. If you need to restrict which protocols can request signatures,
+    ///      pair this signer with a CallerPolicy.
     function checkSignature(bytes32 id, address, bytes32 hash, bytes calldata sig)
         external
         view
@@ -138,6 +154,15 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel
     /**
      * @notice Internal function to validate user operation signatures
      * @dev Shared logic for both installed and stateless validator modes
+     *
+     *      SECURITY: Split Signature Scheme
+     *      The first N-1 signatures verify a proposalHash (EIP-712 typed data covering
+     *      account, id, callData, and nonce). The last signature MUST verify the full
+     *      userOpHash to bind the complete UserOp (including gas fields).
+     *      This prevents a scenario where guardians approve a proposal but an attacker
+     *      manipulates gas parameters in the final UserOp.
+     *      A double-counting check ensures a guardian who signed both the proposalHash
+     *      and userOpHash only has their weight counted once.
      */
     function _validateUserOpSignature(
         bytes32 id,
@@ -188,7 +213,7 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel
             signer = ECDSA.tryRecoverCalldata(proposalHash, sig[i * 65:(i + 1) * 65]);
 
             // Enforce sorted order to prevent signature reuse
-            require(signer > lastSigner, "Signers not sorted");
+            require(signer > lastSigner, SignersNotSorted());
             lastSigner = signer;
             proposalSigners[i] = signer;
 

src/signers/WeightedECDSASigner.sol

@@ -32,12 +32,13 @@ contract ECDSAValidator is IValidator, IHook, IStatelessValidator, IStatelessVal
 
     error InvalidDataLength();
     error ZeroAddressOwner();
+    error SenderNotOwner();
 
     function onInstall(bytes calldata _data) external payable override {
         if (_isInitialized(msg.sender)) revert AlreadyInitialized(msg.sender);
-        if (_data.length != 20) revert InvalidDataLength();
+        require(_data.length == 20, InvalidDataLength());
         address owner = address(bytes20(_data[0:20]));
-        if (owner == address(0)) revert ZeroAddressOwner();
+        require(owner != address(0), ZeroAddressOwner());
         ecdsaValidatorStorage[msg.sender].owner = owner;
         emit OwnerRegistered(msg.sender, owner);
     }
@@ -72,18 +73,28 @@ contract ECDSAValidator is IValidator, IHook, IStatelessValidator, IStatelessVal
         returns (uint256)
     {
         address owner = ecdsaValidatorStorage[msg.sender].owner;
+        // Fail if owner is not set (prevents matching with failed recovery returning address(0))
+        if (owner == address(0)) return SIG_VALIDATION_FAILED_UINT;
         return _verifySignature(userOpHash, userOp.signature, owner)
             ? SIG_VALIDATION_SUCCESS_UINT
             : SIG_VALIDATION_FAILED_UINT;
     }
 
+    /// @notice Validate an ERC-1271 signature
+    /// @dev The `sender` parameter (requesting protocol) is intentionally unused.
+    ///      This validator authenticates the SIGNER (owner), not the requesting protocol.
+    ///      WARNING: Because sender is ignored, any protocol can request signature
+    ///      validation. If you need to restrict which protocols can request signatures,
+    ///      pair this validator with a CallerPolicy.
     function isValidSignatureWithSender(address, bytes32 hash, bytes calldata sig)
         external
         view
         override
         returns (bytes4)
     {
         address owner = ecdsaValidatorStorage[msg.sender].owner;
+        // Fail if owner is not set (prevents matching with failed recovery returning address(0))
+        if (owner == address(0)) return ERC1271_INVALID;
         return _verifySignature(hash, sig, owner) ? ERC1271_MAGICVALUE : ERC1271_INVALID;
     }
 
@@ -106,7 +117,7 @@ contract ECDSAValidator is IValidator, IHook, IStatelessValidator, IStatelessVal
     }
 
     function preCheck(address msgSender, uint256, bytes calldata) external payable override returns (bytes memory) {
-        require(msgSender == ecdsaValidatorStorage[msg.sender].owner, "ECDSAValidator: sender is not owner");
+        require(msgSender == ecdsaValidatorStorage[msg.sender].owner, SenderNotOwner());
         return hex"";
     }