Merge branch 'dev' into feat/validator_benchmark

.github/workflows/test.yml

@@ -1,6 +1,12 @@
 name: test
 
-on: workflow_dispatch
+on:
+  workflow_dispatch:
+  pull_request:
+  push:
+    branches:
+      - "main"
+      - "dev"
 
 env:
   FOUNDRY_PROFILE: ci
@@ -32,3 +38,10 @@ jobs:
         run: |
           forge test -vvv
         id: test
+
+      - name: Run snapshot
+        run: NO_COLOR=1 forge snapshot --via-ir >> $GITHUB_STEP_SUMMARY
+        id: snapshot
+
+      - name: Run coverage
+        run: NO_COLOR=1 forge coverage >> $GITHUB_STEP_SUMMARY

src/Kernel.sol

@@ -28,6 +28,8 @@ import {ValidationData, ValidAfter, ValidUntil, parseValidationData, packValidat
 contract Kernel is EIP712, Compatibility, KernelStorage {
     /// @dev Selector of the `DisabledMode()` error, to be used in assembly, 'bytes4(keccak256(bytes("DisabledMode()")))', same as DisabledMode.selector()
     uint256 private constant _DISABLED_MODE_SELECTOR = 0xfc2f51c5;
+    bytes32 internal constant EIP712_DOMAIN_TYPEHASH =
+        0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f;
 
     /// @dev Current kernel name and version, todo: Need to expose getter for this variables?
     string public constant name = KERNEL_NAME;
@@ -288,8 +290,7 @@ contract Kernel is EIP712, Compatibility, KernelStorage {
         address proxyAddress = address(this);
 
         // Construct the domain separator with name, version, chainId, and proxy address.
-        bytes32 typeHash =
-            keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
+        bytes32 typeHash = EIP712_DOMAIN_TYPEHASH;
         return keccak256(abi.encode(typeHash, nameHash, versionHash, block.chainid, proxyAddress));
     }
 

src/utils/KernelTestBase.sol

@@ -15,10 +15,11 @@ import {IKernelValidator} from "../interfaces/IKernelValidator.sol";
 import {Call, ExecutionDetail} from "../common/Structs.sol";
 import {ValidationData, ValidUntil, ValidAfter} from "../common/Types.sol";
 import {KERNEL_VERSION, KERNEL_NAME} from "../common/Constants.sol";
+import {ECDSA} from "solady/utils/ECDSA.sol";
 
 import {ERC4337Utils} from "./ERC4337Utils.sol";
 import {Test} from "forge-std/Test.sol";
-import {console} from "forge-std/Console.sol";
+import {console} from "forge-std/console.sol";
 import {TestValidator} from "../mock/TestValidator.sol";
 import {TestExecutor} from "../mock/TestExecutor.sol";
 import {TestERC721} from "../mock/TestERC721.sol";
@@ -197,8 +198,13 @@ abstract contract KernelTestBase is Test {
 
     function test_validate_signature() external virtual {
         Kernel kernel2 = Kernel(payable(factory.createAccount(address(kernelImpl), getInitializeData(), 3)));
-        bytes32 hash = keccak256(abi.encodePacked("hello world"));
-        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", kernel.getDomainSeparator(), hash));
+        string memory message = "hello world";
+        bytes32 hash = ECDSA.toEthSignedMessageHash(bytes(message));
+        bytes32 digest = keccak256(
+            abi.encodePacked(
+                "\x19\x01", ERC4337Utils._buildDomainSeparator(KERNEL_NAME, KERNEL_VERSION, address(kernel)), hash
+            )
+        );
         bytes memory sig = signHash(digest);
         assertEq(kernel.isValidSignature(hash, sig), Kernel.isValidSignature.selector);
         assertEq(kernel2.isValidSignature(hash, sig), bytes4(0xffffffff));

src/validator/WeightedECDSAValidator.sol

@@ -51,34 +51,45 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
     event GuardianRemoved(address indexed guardian, address indexed kernel);
 
     function _domainNameAndVersion() internal pure override returns (string memory, string memory) {
-        return ("WeightedECDSAValidator", "0.0.2");
+        return ("WeightedECDSAValidator", "0.0.3");
     }
 
-    function enable(bytes calldata _data) external payable override {
-        (address[] memory _guardians, uint24[] memory _weights, uint24 _threshold, uint48 _delay) =
-            abi.decode(_data, (address[], uint24[], uint24, uint48));
+    function _addGuardians(address[] memory _guardians, uint24[] memory _weights, address _kernel) internal {
+        uint24 totalWeight = weightedStorage[_kernel].totalWeight;
         require(_guardians.length == _weights.length, "Length mismatch");
-        require(weightedStorage[msg.sender].totalWeight == 0, "Already enabled");
-        weightedStorage[msg.sender].firstGuardian = msg.sender;
+        uint160 prevGuardian = uint160(weightedStorage[_kernel].firstGuardian);
         for (uint256 i = 0; i < _guardians.length; i++) {
-            require(_guardians[i] != msg.sender, "Guardian cannot be self");
+            require(_guardians[i] != _kernel, "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]][msg.sender].weight == 0, "Guardian already enabled");
-            guardian[_guardians[i]][msg.sender] =
-                GuardianStorage({weight: _weights[i], nextGuardian: weightedStorage[msg.sender].firstGuardian});
-            weightedStorage[msg.sender].firstGuardian = _guardians[i];
-            weightedStorage[msg.sender].totalWeight += _weights[i];
-            emit GuardianAdded(_guardians[i], msg.sender, _weights[i]);
+            require(guardian[_guardians[i]][_kernel].weight == 0, "Guardian already enabled");
+            require(uint160(_guardians[i]) < prevGuardian, "Guardians not sorted");
+            guardian[_guardians[i]][_kernel] =
+                GuardianStorage({weight: _weights[i], nextGuardian: weightedStorage[_kernel].firstGuardian});
+            weightedStorage[_kernel].firstGuardian = _guardians[i];
+            totalWeight += _weights[i];
+            prevGuardian = uint160(_guardians[i]);
+            emit GuardianAdded(_guardians[i], _kernel, _weights[i]);
         }
+        weightedStorage[_kernel].totalWeight = totalWeight;
+    }
+
+    function enable(bytes calldata _data) external payable override {
+        (address[] memory _guardians, uint24[] memory _weights, uint24 _threshold, uint48 _delay) =
+            abi.decode(_data, (address[], uint24[], uint24, uint48));
+        require(_guardians.length == _weights.length, "Length mismatch");
+        require(weightedStorage[msg.sender].totalWeight == 0, "Already enabled");
+        weightedStorage[msg.sender].firstGuardian = address(uint160(type(uint160).max));
+        _addGuardians(_guardians, _weights, msg.sender);
         weightedStorage[msg.sender].delay = _delay;
         weightedStorage[msg.sender].threshold = _threshold;
+        require(_threshold <= weightedStorage[msg.sender].totalWeight, "Threshold too high");
     }
 
     function disable(bytes calldata) external payable override {
         require(weightedStorage[msg.sender].totalWeight != 0, "Not enabled");
         address currentGuardian = weightedStorage[msg.sender].firstGuardian;
-        while (currentGuardian != msg.sender) {
+        while (currentGuardian != address(uint160(type(uint160).max))) {
             address nextGuardian = guardian[currentGuardian][msg.sender].nextGuardian;
             emit GuardianRemoved(currentGuardian, msg.sender);
             delete guardian[currentGuardian][msg.sender];
@@ -101,18 +112,8 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
         }
         delete weightedStorage[msg.sender];
         require(_guardians.length == _weights.length, "Length mismatch");
-        weightedStorage[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]][msg.sender].weight == 0, "Guardian already enabled");
-            guardian[_guardians[i]][msg.sender] =
-                GuardianStorage({weight: _weights[i], nextGuardian: weightedStorage[msg.sender].firstGuardian});
-            weightedStorage[msg.sender].firstGuardian = _guardians[i];
-            weightedStorage[msg.sender].totalWeight += _weights[i];
-            emit GuardianAdded(_guardians[i], msg.sender, _weights[i]);
-        }
+        weightedStorage[msg.sender].firstGuardian = _guardians[0];
+        _addGuardians(_guardians, _weights, msg.sender);
         weightedStorage[msg.sender].delay = _delay;
         weightedStorage[msg.sender].threshold = _threshold;
     }
@@ -221,13 +222,14 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
                 return packValidationData(ValidAfter.wrap(0), ValidUntil.wrap(0));
             }
         } else if (proposal.status == ProposalStatus.Approved || passed) {
-            if (userOp.paymasterAndData.length == 0) {
+            if (userOp.paymasterAndData.length == 0 || address(bytes20(userOp.paymasterAndData[0:20])) == address(0)) {
                 address signer = ECDSA.recover(ECDSA.toEthSignedMessageHash(userOpHash), userOp.signature);
                 if (guardian[signer][msg.sender].weight != 0) {
                     proposal.status = ProposalStatus.Executed;
                     return packValidationData(proposal.validAfter, ValidUntil.wrap(0));
                 }
             } else {
+                proposal.status = ProposalStatus.Executed;
                 return packValidationData(proposal.validAfter, ValidUntil.wrap(0));
             }
         }
@@ -263,13 +265,17 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator {
             return SIG_VALIDATION_FAILED;
         }
         uint256 totalWeight = 0;
-        address signer;
+        address prevSigner = address(uint160(type(uint160).max));
         for (uint256 i = 0; i < sigCount; i++) {
-            signer = ECDSA.recover(hash, signature[i * 65:(i + 1) * 65]);
+            address signer = ECDSA.recover(hash, signature[i * 65:(i + 1) * 65]);
             totalWeight += guardian[signer][msg.sender].weight;
             if (totalWeight >= strg.threshold) {
                 return packValidationData(ValidAfter.wrap(0), ValidUntil.wrap(0));
             }
+            if (signer >= prevSigner) {
+                return SIG_VALIDATION_FAILED;
+            }
+            prevSigner = signer;
         }
         return SIG_VALIDATION_FAILED;
     }

test/foundry/validator/WeightedECDSAValidator.t.sol

@@ -29,6 +29,19 @@ contract KernelWeightedECDSATest is KernelTestBase {
         (owners[0], ownerKeys[0]) = makeAddrAndKey("owner0");
         (owners[1], ownerKeys[1]) = makeAddrAndKey("owner1");
         (owners[2], ownerKeys[2]) = makeAddrAndKey("owner2");
+        // sort owners and keys from largest to smallest owner address
+        for (uint256 i = 0; i < owners.length; i++) {
+            for (uint256 j = i + 1; j < owners.length; j++) {
+                if (owners[i] < owners[j]) {
+                    address tempAddr = owners[i];
+                    owners[i] = owners[j];
+                    owners[j] = tempAddr;
+                    uint256 tempKey = ownerKeys[i];
+                    ownerKeys[i] = ownerKeys[j];
+                    ownerKeys[j] = tempKey;
+                }
+            }
+        }
         weights = [uint24(1), uint24(2), uint24(3)];
         threshold = 3;
         delay = 0;
@@ -84,7 +97,7 @@ contract KernelWeightedECDSATest is KernelTestBase {
             abi.encode(
                 keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
                 keccak256("WeightedECDSAValidator"),
-                keccak256("0.0.2"),
+                keccak256("0.0.3"),
                 block.chainid,
                 address(defaultValidator)
             )
@@ -120,9 +133,9 @@ contract KernelWeightedECDSATest is KernelTestBase {
     }
 
     function getWrongSignature(bytes32 hash) internal view override returns (bytes memory) {
-        (uint8 v0, bytes32 r0, bytes32 s0) = vm.sign(ownerKeys[0], hash);
-        (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(ownerKeys[1] + 1, hash);
-        (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(ownerKeys[2] + 1, hash);
+        (uint8 v0, bytes32 r0, bytes32 s0) = vm.sign(ownerKeys[1], hash);
+        (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(ownerKeys[0], hash);
+        (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(ownerKeys[2], hash);
         return abi.encodePacked(r0, s0, v0, r1, s1, v1, r2, s2, v2);
     }
 
@@ -142,17 +155,15 @@ contract KernelWeightedECDSATest is KernelTestBase {
     }
 
     function test_default_validator_disable() external override {
-        //UserOperation memory op = buildUserOperation(
-        //    abi.encodeWithSelector(
-        //        IKernel.execute.selector,
-        //        address(defaultValidator),
-        //        0,
-        //        abi.encodeWithSelector(ECDSAValidator.disable.selector, ""),
-        //        Operation.Call
-        //    )
-        //);
-        //performUserOperationWithSig(op);
-        //(address owner) = ECDSAValidator(address(defaultValidator)).ecdsaValidatorStorage(address(kernel));
-        //assertEq(owner, address(0), "owner should be 0");
+        UserOperation memory op = buildUserOperation(
+            abi.encodeWithSelector(
+                IKernel.execute.selector,
+                address(defaultValidator),
+                0,
+                abi.encodeWithSelector(WeightedECDSAValidator.disable.selector, ""),
+                Operation.Call
+            )
+        );
+        performUserOperationWithSig(op);
     }
 }