From cea9e643fb870d17342869f06ac51c722a8a0985 Mon Sep 17 00:00:00 2001 From: leekt Date: Mon, 5 Feb 2024 23:36:27 +0900 Subject: [PATCH 1/2] audit fixes --- src/validator/WeightedECDSAValidator.sol | 61 ++++++++++--------- .../validator/WeightedECDSAValidator.t.sol | 37 +++++++---- 2 files changed, 56 insertions(+), 42 deletions(-) diff --git a/src/validator/WeightedECDSAValidator.sol b/src/validator/WeightedECDSAValidator.sol index c3a0552e..f98b622c 100644 --- a/src/validator/WeightedECDSAValidator.sol +++ b/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,14 @@ 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)); } + require(signer < prevSigner, "signature not in descending order"); } return SIG_VALIDATION_FAILED; } diff --git a/test/foundry/validator/WeightedECDSAValidator.t.sol b/test/foundry/validator/WeightedECDSAValidator.t.sol index 46797c4d..77be383c 100644 --- a/test/foundry/validator/WeightedECDSAValidator.t.sol +++ b/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) ) @@ -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); } } From 9bc9cc62960e2354907830ed304fbeef8dd5406e Mon Sep 17 00:00:00 2001 From: leekt Date: Tue, 6 Feb 2024 12:22:43 +0900 Subject: [PATCH 2/2] fix: update prevSigner to signer on validateSignature and added invariant testing for this --- src/validator/WeightedECDSAValidator.sol | 5 ++++- test/foundry/validator/WeightedECDSAValidator.t.sol | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/validator/WeightedECDSAValidator.sol b/src/validator/WeightedECDSAValidator.sol index f98b622c..345eed2c 100644 --- a/src/validator/WeightedECDSAValidator.sol +++ b/src/validator/WeightedECDSAValidator.sol @@ -272,7 +272,10 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator { if (totalWeight >= strg.threshold) { return packValidationData(ValidAfter.wrap(0), ValidUntil.wrap(0)); } - require(signer < prevSigner, "signature not in descending order"); + if (signer >= prevSigner) { + return SIG_VALIDATION_FAILED; + } + prevSigner = signer; } return SIG_VALIDATION_FAILED; } diff --git a/test/foundry/validator/WeightedECDSAValidator.t.sol b/test/foundry/validator/WeightedECDSAValidator.t.sol index 77be383c..b6e96853 100644 --- a/test/foundry/validator/WeightedECDSAValidator.t.sol +++ b/test/foundry/validator/WeightedECDSAValidator.t.sol @@ -133,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); }