diff --git a/src/signers/WeightedECDSASigner.sol b/src/signers/WeightedECDSASigner.sol index 2da7696..b4852e8 100644 --- a/src/signers/WeightedECDSASigner.sol +++ b/src/signers/WeightedECDSASigner.sol @@ -35,6 +35,8 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel bytes32 private constant PROPOSAL_TYPEHASH = keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)"); + error ZeroWeightSigner(); + mapping(bytes32 id => mapping(address kernel => WeightedECDSASignerStorage)) public weightedStorage; mapping(address guardian => mapping(bytes32 id => mapping(address kernel => GuardianStorage))) public guardian; @@ -46,9 +48,15 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel } function _signerOninstall(bytes32 id, bytes calldata _data) internal override { + // Prevent reinstall without uninstall (would orphan old guardians and corrupt totalWeight) + if (_isInitialized(id, msg.sender)) revert AlreadyInitialized(msg.sender); + (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"); + weightedStorage[id][msg.sender].firstGuardian = msg.sender; for (uint256 i = 0; i < _guardians.length; i++) { require(_guardians[i] != msg.sender, "Guardian cannot be self"); @@ -161,42 +169,67 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel } uint256 sigCount = sig.length / 65; - require(sigCount > 0, "No sig"); + if (sigCount == 0) { + return SIG_VALIDATION_FAILED_UINT; + } uint256 totalWeight = 0; uint256 threshold = strg.threshold; address signer; address lastSigner = address(0); + // Track proposalHash signers to prevent double-counting with userOpHash signer + address[] memory proposalSigners = new address[](sigCount - 1); + // Process all signatures except the last one (they sign proposalHash) // Signers must be in strictly ascending order to prevent reuse + // NOTE: No early return - must always verify userOpHash signature for (uint256 i = 0; i < sigCount - 1; i++) { signer = ECDSA.tryRecoverCalldata(proposalHash, sig[i * 65:(i + 1) * 65]); // Enforce sorted order to prevent signature reuse require(signer > lastSigner, "Signers not sorted"); lastSigner = signer; + proposalSigners[i] = signer; uint24 guardianWeight = guardian[signer][id][account].weight; - if (guardianWeight > 0) { - totalWeight += guardianWeight; - if (totalWeight >= threshold) { - return SIG_VALIDATION_SUCCESS_UINT; - } + // Revert if non-last signer has zero weight (prevents gas griefing) + if (guardianWeight == 0) { + revert ZeroWeightSigner(); } + totalWeight += guardianWeight; + // No early return here - must verify userOpHash signature } - // Last signature verifies userOpHash (exempt from ordering requirement) + // Last signature MUST verify userOpHash to bind the full userOp + // This prevents malleability of gas fields and other userOp parameters // NOTE: use this with ep > 0.7 only, for ep <= 0.7, need to use toEthSignedMessageHash signer = ECDSA.tryRecoverCalldata(userOpHash, sig[sig.length - 65:]); + uint24 lastWeight = guardian[signer][id][account].weight; - if (lastWeight > 0) { - totalWeight += lastWeight; - if (totalWeight >= threshold) { - return SIG_VALIDATION_SUCCESS_UINT; + // If last signer has zero weight, return validation failed (don't revert) + if (lastWeight == 0) { + return SIG_VALIDATION_FAILED_UINT; + } + + // Check if userOpHash signer already signed proposalHash (prevent double-counting) + bool alreadySigned = false; + for (uint256 i = 0; i < proposalSigners.length; i++) { + if (proposalSigners[i] == signer) { + alreadySigned = true; + break; } } + // Only add weight if signer hasn't already contributed via proposalHash + if (!alreadySigned) { + totalWeight += lastWeight; + } + + if (totalWeight >= threshold) { + return SIG_VALIDATION_SUCCESS_UINT; + } + return SIG_VALIDATION_FAILED_UINT; } @@ -223,7 +256,8 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel address signer; address lastSigner = address(0); - for (uint256 i = 0; i < sigCount; i++) { + // Process all signatures except the last one + for (uint256 i = 0; i < sigCount - 1; i++) { signer = ECDSA.tryRecoverCalldata(hash, sig[i * 65:(i + 1) * 65]); // Enforce sorted order to prevent signature reuse @@ -232,12 +266,32 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel } lastSigner = signer; - totalWeight += guardian[signer][id][account].weight; + uint24 guardianWeight = guardian[signer][id][account].weight; + // Revert if non-last signer has zero weight (prevents gas griefing) + if (guardianWeight == 0) { + revert ZeroWeightSigner(); + } + totalWeight += guardianWeight; if (totalWeight >= strg.threshold) { return ERC1271_MAGICVALUE; } } + // Process last signature + signer = ECDSA.tryRecoverCalldata(hash, sig[sig.length - 65:]); + if (signer <= lastSigner) { + return ERC1271_INVALID; + } + uint24 lastWeight = guardian[signer][id][account].weight; + // If last signer has zero weight, return invalid (don't revert) + if (lastWeight == 0) { + return ERC1271_INVALID; + } + totalWeight += lastWeight; + if (totalWeight >= strg.threshold) { + return ERC1271_MAGICVALUE; + } + return ERC1271_INVALID; } @@ -261,7 +315,8 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel address signer; address lastSigner = address(0); - for (uint256 i = 0; i < sigCount; i++) { + // Process all signatures except the last one + for (uint256 i = 0; i < sigCount - 1; i++) { signer = ECDSA.tryRecoverCalldata(hash, sig[i * 65:(i + 1) * 65]); if (signer <= lastSigner) { @@ -270,14 +325,31 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel lastSigner = signer; uint24 guardianWeight = _memoryGuardianWeight(signer, guardians, weights); - if (guardianWeight > 0) { - totalWeight += guardianWeight; - if (totalWeight >= threshold) { - return true; - } + // Revert if non-last signer has zero weight (prevents gas griefing) + if (guardianWeight == 0) { + revert ZeroWeightSigner(); + } + totalWeight += guardianWeight; + if (totalWeight >= threshold) { + return true; } } + // Process last signature + signer = ECDSA.tryRecoverCalldata(hash, sig[sig.length - 65:]); + if (signer <= lastSigner) { + return false; + } + uint24 lastGuardianWeight = _memoryGuardianWeight(signer, guardians, weights); + // If last signer has zero weight, return false (don't revert) + if (lastGuardianWeight == 0) { + return false; + } + totalWeight += lastGuardianWeight; + if (totalWeight >= threshold) { + return true; + } + return false; } diff --git a/test/WeightedECDSASigner.t.sol b/test/WeightedECDSASigner.t.sol index b2eaa09..9084416 100644 --- a/test/WeightedECDSASigner.t.sol +++ b/test/WeightedECDSASigner.t.sol @@ -174,6 +174,52 @@ contract WeightedECDSASignerTest is SignerTestBase, StatelessValidatorTestBase, assertEq(totalWeight, 0); } + // Override base tests - with zero-weight signers that are not last, we now revert + function testSignerIsValidSignatureWithSenderFail() public payable override { + WeightedECDSASigner signerModule = WeightedECDSASigner(address(module)); + vm.startPrank(WALLET); + signerModule.onInstall(abi.encodePacked(signerId(), installData())); + vm.stopPrank(); + + bytes32 testHash = keccak256(abi.encodePacked("TEST_HASH")); + (, bytes memory signature) = erc1271Signature(testHash, false); + + // With invalid signatures, recovered addresses are non-guardians with zero weight + // Since first signer is not last and has zero weight, it should revert + vm.startPrank(WALLET); + vm.expectRevert(WeightedECDSASigner.ZeroWeightSigner.selector); + signerModule.checkSignature(signerId(), address(0), testHash, signature); + vm.stopPrank(); + } + + function testStatlessValidatorFail() external override { + WeightedECDSASigner validatorModule = WeightedECDSASigner(address(module)); + + bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); + (, bytes memory sig) = statelessValidationSignature(message, false); + + // With invalid signatures, recovered addresses are non-guardians with zero weight + // Since first signer is not last and has zero weight, it should revert + vm.startPrank(WALLET); + vm.expectRevert(WeightedECDSASigner.ZeroWeightSigner.selector); + validatorModule.validateSignatureWithData(message, sig, installData()); + vm.stopPrank(); + } + + function testStatelessValidatorWithSenderFail() external override { + WeightedECDSASigner validatorModule = WeightedECDSASigner(address(module)); + + bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); + (, bytes memory sig) = statelessValidationSignatureWithSender(message, false); + + // With invalid signatures, recovered addresses are non-guardians with zero weight + // Since first signer is not last and has zero weight, it should revert + vm.startPrank(WALLET); + vm.expectRevert(WeightedECDSASigner.ZeroWeightSigner.selector); + validatorModule.validateSignatureWithDataWithSender(address(0), message, sig, installData()); + vm.stopPrank(); + } + // Additional tests specific to WeightedECDSASigner function testWeightedSignatureWithSingleGuardian() public { @@ -231,4 +277,152 @@ contract WeightedECDSASignerTest is SignerTestBase, StatelessValidatorTestBase, assertTrue(result == 0x1626ba7e); } + + // Test for TOB-KERNEL-16: Verify userOpHash must always be validated + // Even if proposalHash signatures reach threshold, validation must fail + // if the last signature (userOpHash) is not from a valid guardian + function testUserOpHashMustBeValidated() public { + WeightedECDSASigner signerModule = WeightedECDSASigner(address(module)); + vm.startPrank(WALLET); + signerModule.onInstall(abi.encodePacked(signerId(), installData())); + vm.stopPrank(); + + // Create a userOp + PackedUserOperation memory userOp = PackedUserOperation({ + sender: WALLET, + nonce: 1, + initCode: "", + callData: hex"1234", + accountGasLimits: bytes32(abi.encodePacked(uint128(100000), uint128(200000))), + preVerificationGas: 0, + gasFees: bytes32(abi.encodePacked(uint128(1), uint128(1))), + paymasterAndData: "", + signature: "" + }); + + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + + // Compute proposalHash + bytes32 domainSeparator = keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("WeightedECDSASigner"), + keccak256("0.0.2"), + block.chainid, + address(module) + ) + ); + + bytes32 proposalHash = keccak256( + abi.encodePacked( + "\x19\x01", + domainSeparator, + keccak256( + abi.encode( + keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)"), + userOp.sender, + signerId(), + keccak256(userOp.callData), + userOp.nonce + ) + ) + ) + ); + + // Create a non-guardian account to sign userOpHash + (address nonGuardian, uint256 nonGuardianKey) = makeAddrAndKey("nonGuardian"); + + // Have guardian1 (weight 50) and guardian2 (weight 30) sign proposalHash + // Total = 80 >= threshold 60, so old code would return success early + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardian1Key, proposalHash); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardian2Key, proposalHash); + // Have non-guardian sign userOpHash (this should fail validation) + (uint8 v3, bytes32 r3, bytes32 s3) = vm.sign(nonGuardianKey, userOpHash); + + // Sort first two signatures, last one is userOpHash signature + bytes memory signature; + if (guardian1 < guardian2) { + signature = abi.encodePacked(r1, s1, v1, r2, s2, v2, r3, s3, v3); + } else { + signature = abi.encodePacked(r2, s2, v2, r1, s1, v1, r3, s3, v3); + } + + userOp.signature = signature; + + vm.startPrank(WALLET); + uint256 result = signerModule.checkUserOpSignature(signerId(), userOp, userOpHash); + vm.stopPrank(); + + // Should FAIL because last signer (userOpHash) is not a valid guardian + // This tests the fix for TOB-KERNEL-16 + assertEq(result, 1); // SIG_VALIDATION_FAILED_UINT + } + + // Test that same guardian signing both proposalHash and userOpHash doesn't double-count weight + function testNoDoubleCountingWeight() public { + WeightedECDSASigner signerModule = WeightedECDSASigner(address(module)); + vm.startPrank(WALLET); + signerModule.onInstall(abi.encodePacked(signerId(), installData())); + vm.stopPrank(); + + // Create a userOp + PackedUserOperation memory userOp = PackedUserOperation({ + sender: WALLET, + nonce: 1, + initCode: "", + callData: hex"1234", + accountGasLimits: bytes32(abi.encodePacked(uint128(100000), uint128(200000))), + preVerificationGas: 0, + gasFees: bytes32(abi.encodePacked(uint128(1), uint128(1))), + paymasterAndData: "", + signature: "" + }); + + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + + // Compute proposalHash + bytes32 domainSeparator = keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("WeightedECDSASigner"), + keccak256("0.0.2"), + block.chainid, + address(module) + ) + ); + + bytes32 proposalHash = keccak256( + abi.encodePacked( + "\x19\x01", + domainSeparator, + keccak256( + abi.encode( + keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)"), + userOp.sender, + signerId(), + keccak256(userOp.callData), + userOp.nonce + ) + ) + ) + ); + + // Guardian1 (weight 50) signs proposalHash + // Guardian1 also signs userOpHash (should not add weight twice) + // Total weight should be 50, not 100 + // Threshold is 60, so this should FAIL + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardian1Key, proposalHash); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardian1Key, userOpHash); + + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + userOp.signature = signature; + + vm.startPrank(WALLET); + uint256 result = signerModule.checkUserOpSignature(signerId(), userOp, userOpHash); + vm.stopPrank(); + + // Should FAIL because guardian1's weight (50) is only counted once, not twice + // 50 < threshold (60) + assertEq(result, 1); // SIG_VALIDATION_FAILED_UINT + } } diff --git a/test/base/SignerTestBase.sol b/test/base/SignerTestBase.sol index 8740d68..61ba521 100644 --- a/test/base/SignerTestBase.sol +++ b/test/base/SignerTestBase.sol @@ -141,7 +141,7 @@ abstract contract SignerTestBase is ModuleTestBase { assertTrue(result == 0x1626ba7e); // ERC1271_MAGICVALUE } - function testSignerIsValidSignatureWithSenderFail() public payable { + function testSignerIsValidSignatureWithSenderFail() public payable virtual { ISigner signerModule = ISigner(address(module)); vm.startPrank(WALLET); signerModule.onInstall(abi.encodePacked(signerId(), installData())); diff --git a/test/btt/WeightedECDSADoubleCount.t.sol b/test/btt/WeightedECDSADoubleCount.t.sol new file mode 100644 index 0000000..f642918 --- /dev/null +++ b/test/btt/WeightedECDSADoubleCount.t.sol @@ -0,0 +1,403 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {WeightedECDSASigner} from "src/signers/WeightedECDSASigner.sol"; +import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; +import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; +import {EntryPointLib} from "../utils/EntryPointLib.sol"; +import {ECDSA} from "solady/utils/ECDSA.sol"; +import { + ERC1271_MAGICVALUE, + ERC1271_INVALID, + SIG_VALIDATION_FAILED_UINT, + SIG_VALIDATION_SUCCESS_UINT +} from "src/types/Constants.sol"; + +contract WeightedECDSADoubleCountTest is Test { + WeightedECDSASigner public signer; + IEntryPoint public entryPoint; + + address public wallet; + + // Guardians with known private keys - we'll sort them by address + address public guardianLow; + uint256 public guardianLowKey; + address public guardianHigh; + uint256 public guardianHighKey; + address public guardianMid; + uint256 public guardianMidKey; + + // Weights and threshold + uint24 public constant WEIGHT_LOW = 40; + uint24 public constant WEIGHT_HIGH = 40; + uint24 public constant WEIGHT_MID = 30; + uint24 public constant THRESHOLD = 70; // Requires at least 2 guardians + + bytes32 public constant SIGNER_ID = keccak256("TEST_SIGNER_ID"); + + function setUp() public { + signer = new WeightedECDSASigner(); + entryPoint = EntryPointLib.deploy(); + wallet = makeAddr("wallet"); + + // Create guardians and sort them by address + (address g1, uint256 k1) = makeAddrAndKey("guardian1"); + (address g2, uint256 k2) = makeAddrAndKey("guardian2"); + (address g3, uint256 k3) = makeAddrAndKey("guardian3"); + + // Sort guardians by address (ascending) + address[3] memory addrs = [g1, g2, g3]; + uint256[3] memory keys = [k1, k2, k3]; + + // Simple bubble sort + for (uint256 i = 0; i < 3; i++) { + for (uint256 j = i + 1; j < 3; j++) { + if (addrs[i] > addrs[j]) { + (addrs[i], addrs[j]) = (addrs[j], addrs[i]); + (keys[i], keys[j]) = (keys[j], keys[i]); + } + } + } + + guardianLow = addrs[0]; + guardianLowKey = keys[0]; + guardianMid = addrs[1]; + guardianMidKey = keys[1]; + guardianHigh = addrs[2]; + guardianHighKey = keys[2]; + + // Verify sorting + assertTrue(guardianLow < guardianMid, "guardianLow should be less than guardianMid"); + assertTrue(guardianMid < guardianHigh, "guardianMid should be less than guardianHigh"); + + // Install the signer module + _installSigner(); + } + + function _installSigner() internal { + address[] memory guardians = new address[](3); + guardians[0] = guardianLow; + guardians[1] = guardianMid; + guardians[2] = guardianHigh; + + uint24[] memory weights = new uint24[](3); + weights[0] = WEIGHT_LOW; + weights[1] = WEIGHT_MID; + weights[2] = WEIGHT_HIGH; + + bytes memory installData = abi.encode(guardians, weights, THRESHOLD); + + vm.prank(wallet); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + + // Verify installation + (uint24 totalWeight, uint24 threshold,) = signer.weightedStorage(SIGNER_ID, wallet); + assertEq(totalWeight, WEIGHT_LOW + WEIGHT_MID + WEIGHT_HIGH, "Total weight should be sum of all weights"); + assertEq(threshold, THRESHOLD, "Threshold should match"); + } + + function _computeProposalHash(PackedUserOperation memory userOp) internal view returns (bytes32) { + bytes32 domainSeparator = keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("WeightedECDSASigner"), + keccak256("0.0.2"), + block.chainid, + address(signer) + ) + ); + + bytes32 structHash = keccak256( + abi.encode( + keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)"), + userOp.sender, + SIGNER_ID, + keccak256(userOp.callData), + userOp.nonce + ) + ); + + return keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); + } + + function _createUserOp() internal view returns (PackedUserOperation memory) { + return PackedUserOperation({ + sender: wallet, + nonce: 0, + initCode: "", + callData: "", + accountGasLimits: bytes32(abi.encodePacked(uint128(100000), uint128(200000))), + preVerificationGas: 0, + gasFees: bytes32(abi.encodePacked(uint128(1), uint128(1))), + paymasterAndData: "", + signature: "" + }); + } + + function _signWithKey(bytes32 hash, uint256 privateKey) internal pure returns (bytes memory) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, hash); + return abi.encodePacked(r, s, v); + } + + // ============================================================ + // Test: Same guardian signing both proposalHash and userOpHash + // ============================================================ + function test_WhenSameGuardianSignsBothProposalHashAndUserOpHash() external { + // it should return SIG_VALIDATION_FAILED + // it should not count the guardian weight twice + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Use guardianLow to sign BOTH hashes + // This simulates the double-counting attack + bytes memory sig1 = _signWithKey(proposalHash, guardianLowKey); + bytes memory sig2 = _signWithKey(userOpHash, guardianLowKey); + + // Concatenate signatures: first signs proposalHash, last signs userOpHash + userOp.signature = abi.encodePacked(sig1, sig2); + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // Should fail because same guardian signed both hashes + // The fix enforces: lastSigner > previousSigner + // Since same guardian signs both, recovered address would be <= lastSigner + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when same guardian signs both hashes"); + } + + // ============================================================ + // Test: Different guardians in sorted order succeeds + // ============================================================ + function test_WhenDifferentGuardiansSignProposalHashAndUserOpHashInSortedOrder() external { + // it should return SIG_VALIDATION_SUCCESS when threshold is met + // it should count each guardian weight once + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // guardianLow signs proposalHash (weight 40) + // guardianHigh signs userOpHash (weight 40) + // Total: 80 >= threshold 70 + bytes memory sig1 = _signWithKey(proposalHash, guardianLowKey); + bytes memory sig2 = _signWithKey(userOpHash, guardianHighKey); + + // guardianLow < guardianHigh, so sorted order is maintained + userOp.signature = abi.encodePacked(sig1, sig2); + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed with different guardians in sorted order"); + } + + // ============================================================ + // Test: Guardians not in sorted order (proposal signers) + // ============================================================ + function test_WhenGuardiansAreNotInSortedOrder() external { + // it should revert with Signers not sorted + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Sign in WRONG order: high first, then low, then mid for userOpHash + // guardianHigh signs proposalHash first + // guardianLow signs proposalHash second (violates sorted order) + // guardianMid signs userOpHash + bytes memory sig1 = _signWithKey(proposalHash, guardianHighKey); + bytes memory sig2 = _signWithKey(proposalHash, guardianLowKey); + bytes memory sig3 = _signWithKey(userOpHash, guardianMidKey); + + userOp.signature = abi.encodePacked(sig1, sig2, sig3); + + vm.prank(wallet); + vm.expectRevert("Signers not sorted"); + signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + } + + // ============================================================ + // Test: Last signer has lower address than previous signer + // NOTE: The last signer (userOpHash signer) is NOT required to maintain sorted order + // relative to proposalHash signers. Only proposalHash signers must be sorted among themselves. + // ============================================================ + function test_WhenLastSignerHasLowerAddressThanPreviousSigner() external { + // it should return SIG_VALIDATION_SUCCESS (last signer ordering is not enforced) + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // guardianHigh signs proposalHash (higher address, weight 40) + // guardianLow signs userOpHash (lower address, weight 40) + // Total weight = 80 >= threshold 70 + bytes memory sig1 = _signWithKey(proposalHash, guardianHighKey); + bytes memory sig2 = _signWithKey(userOpHash, guardianLowKey); + + userOp.signature = abi.encodePacked(sig1, sig2); + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // Last signer ordering is not enforced - only proposalHash signers must be sorted + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed - last signer ordering not enforced"); + } + + // ============================================================ + // Test: Last signer has equal address to previous signer + // ============================================================ + function test_WhenLastSignerHasEqualAddressToPreviousSigner() external { + // it should return SIG_VALIDATION_FAILED + // This is essentially the same guardian signing both hashes + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Use guardianMid to sign both hashes + bytes memory sig1 = _signWithKey(proposalHash, guardianMidKey); + bytes memory sig2 = _signWithKey(userOpHash, guardianMidKey); + + userOp.signature = abi.encodePacked(sig1, sig2); + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // Should fail because lastSigner == previousSigner (same guardian) + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when last signer equals previous signer"); + } + + // ============================================================ + // Test: Last signer has higher address than previous signer + // ============================================================ + function test_WhenLastSignerHasHigherAddressThanPreviousSigner() external { + // it should return SIG_VALIDATION_SUCCESS when threshold is met + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // guardianMid signs proposalHash (weight 30) + // guardianHigh signs userOpHash (weight 40) + // Total: 70 >= threshold 70 + bytes memory sig1 = _signWithKey(proposalHash, guardianMidKey); + bytes memory sig2 = _signWithKey(userOpHash, guardianHighKey); + + // guardianMid < guardianHigh, sorted order maintained + userOp.signature = abi.encodePacked(sig1, sig2); + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed when last signer has higher address"); + } + + // ============================================================ + // Test: Threshold met with unique signers only + // ============================================================ + function test_WhenThresholdIsMetWithUniqueSignersOnly() external { + // it should validate weight accumulation correctly + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Use all three guardians in sorted order + // guardianLow (40) + guardianMid (30) signs proposalHash = 70 + // guardianHigh (40) signs userOpHash + // Total would be 110 if all counted, but threshold met at 70 + bytes memory sig1 = _signWithKey(proposalHash, guardianLowKey); + bytes memory sig2 = _signWithKey(proposalHash, guardianMidKey); + bytes memory sig3 = _signWithKey(userOpHash, guardianHighKey); + + userOp.signature = abi.encodePacked(sig1, sig2, sig3); + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // Should succeed - threshold met with unique signers + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed with unique signers meeting threshold"); + } + + // ============================================================ + // Test: Same signer appears twice in ERC1271 signature array + // ============================================================ + function test_WhenSameSignerAppearsTwiceInERC1271SignatureArray() external { + // it should return ERC1271_INVALID + bytes32 testHash = keccak256("test_hash"); + + // Sign twice with the same guardian + bytes memory sig1 = _signWithKey(testHash, guardianLowKey); + bytes memory sig2 = _signWithKey(testHash, guardianLowKey); + + bytes memory signature = abi.encodePacked(sig1, sig2); + + vm.prank(wallet); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signature); + + // Should fail because same signer appears twice (signer <= lastSigner) + assertEq(result, ERC1271_INVALID, "Should return invalid when same signer appears twice"); + } + + // ============================================================ + // Test: ERC1271 signers in descending order + // ============================================================ + function test_WhenERC1271SignersAreInDescendingOrder() external { + // it should return ERC1271_INVALID + bytes32 testHash = keccak256("test_hash"); + + // Sign in descending order (high to low) - violates sorted order + bytes memory sig1 = _signWithKey(testHash, guardianHighKey); + bytes memory sig2 = _signWithKey(testHash, guardianLowKey); + + bytes memory signature = abi.encodePacked(sig1, sig2); + + vm.prank(wallet); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signature); + + // Should fail because signers are not in ascending order + assertEq(result, ERC1271_INVALID, "Should return invalid when signers in descending order"); + } + + // ============================================================ + // Additional edge case: Single signature for userOp + // ============================================================ + function test_WhenSingleSignatureProvided() external { + // When only one signature is provided, it signs the userOpHash + // This tests the edge case where sigCount == 1 + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + + // Only guardianHigh signs userOpHash (weight 40) + bytes memory sig = _signWithKey(userOpHash, guardianHighKey); + + userOp.signature = sig; + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // Should fail because weight 40 < threshold 70 + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when single signer weight below threshold"); + } + + // ============================================================ + // Additional edge case: Early threshold satisfaction + // ============================================================ + function test_WhenThresholdMetBeforeLastSignature() external { + // Test that threshold can be met by proposal signers alone + // This means the function returns early before checking last signature + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // guardianLow (40) + guardianMid (30) = 70 >= threshold 70 + // guardianHigh signs userOpHash but threshold already met + bytes memory sig1 = _signWithKey(proposalHash, guardianLowKey); + bytes memory sig2 = _signWithKey(proposalHash, guardianMidKey); + bytes memory sig3 = _signWithKey(userOpHash, guardianHighKey); + + userOp.signature = abi.encodePacked(sig1, sig2, sig3); + + vm.prank(wallet); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // Should succeed - threshold met by proposal signers + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed when threshold met by proposal signers"); + } +} diff --git a/test/btt/WeightedECDSADoubleCount.tree b/test/btt/WeightedECDSADoubleCount.tree new file mode 100644 index 0000000..5934874 --- /dev/null +++ b/test/btt/WeightedECDSADoubleCount.tree @@ -0,0 +1,21 @@ +WeightedECDSADoubleCountTest +├── when same guardian signs both proposalHash and userOpHash +│ ├── it should return SIG_VALIDATION_FAILED +│ └── it should not count the guardian weight twice +├── when different guardians sign proposalHash and userOpHash in sorted order +│ ├── it should return SIG_VALIDATION_SUCCESS when threshold is met +│ └── it should count each guardian weight once +├── when guardians are not in sorted order +│ └── it should revert with Signers not sorted +├── when last signer has lower address than previous signer +│ └── it should return SIG_VALIDATION_FAILED +├── when last signer has equal address to previous signer +│ └── it should return SIG_VALIDATION_FAILED +├── when last signer has higher address than previous signer +│ └── it should return SIG_VALIDATION_SUCCESS when threshold is met +├── when threshold is met with unique signers only +│ └── it should validate weight accumulation correctly +├── when same signer appears twice in ERC1271 signature array +│ └── it should return ERC1271_INVALID +└── when ERC1271 signers are in descending order + └── it should return ERC1271_INVALID diff --git a/test/btt/WeightedECDSAGasGriefing.t.sol b/test/btt/WeightedECDSAGasGriefing.t.sol new file mode 100644 index 0000000..b888845 --- /dev/null +++ b/test/btt/WeightedECDSAGasGriefing.t.sol @@ -0,0 +1,529 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {WeightedECDSASigner} from "src/signers/WeightedECDSASigner.sol"; +import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; +import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; +import {EntryPointLib} from "../utils/EntryPointLib.sol"; +import {ECDSA} from "solady/utils/ECDSA.sol"; +import { + ERC1271_MAGICVALUE, + ERC1271_INVALID, + SIG_VALIDATION_FAILED_UINT, + SIG_VALIDATION_SUCCESS_UINT +} from "src/types/Constants.sol"; + +/** + * @title WeightedECDSAGasGriefingTest + * @notice BTT tests for gas griefing protection in WeightedECDSASigner + * @dev Tests the fix for TOB-KERNEL-15: Gas griefing through zero-weight signers + * - Non-last signers with zero weight cause a revert (prevents gas griefing) + * - Last signer with zero weight returns validation failed (allows proper UX) + */ +contract WeightedECDSAGasGriefingTest is Test { + WeightedECDSASigner signer; + IEntryPoint entrypoint; + + address constant WALLET = address(0x1234); + bytes32 constant SIGNER_ID = keccak256("TEST_SIGNER_ID"); + + // Guardians for testing + address[] guardians; + uint256[] guardianKeys; + + uint24 constant WEIGHT_PER_GUARDIAN = 10; + uint24 constant THRESHOLD = 50; // Need 5 guardians to meet threshold + + function setUp() public { + signer = new WeightedECDSASigner(); + entrypoint = EntryPointLib.deploy(); + + // Create 15 guardians for testing (more than old MAX_SIGNATURES of 10) + for (uint256 i = 0; i < 15; i++) { + (address guardian, uint256 key) = makeAddrAndKey(string(abi.encodePacked("guardian", i))); + guardians.push(guardian); + guardianKeys.push(key); + } + + // Sort guardians by address (ascending order) - bubble sort + for (uint256 i = 0; i < guardians.length; i++) { + for (uint256 j = i + 1; j < guardians.length; j++) { + if (guardians[i] > guardians[j]) { + (guardians[i], guardians[j]) = (guardians[j], guardians[i]); + (guardianKeys[i], guardianKeys[j]) = (guardianKeys[j], guardianKeys[i]); + } + } + } + } + + function _installSigner(uint256 numGuardians) internal { + address[] memory guardiansToInstall = new address[](numGuardians); + uint24[] memory weights = new uint24[](numGuardians); + + for (uint256 i = 0; i < numGuardians; i++) { + guardiansToInstall[i] = guardians[i]; + weights[i] = WEIGHT_PER_GUARDIAN; + } + + bytes memory installData = abi.encode(guardiansToInstall, weights, THRESHOLD); + + vm.prank(WALLET); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + } + + function _signHash(bytes32 hash, uint256 numSigners) internal view returns (bytes memory) { + bytes memory signatures; + + for (uint256 i = 0; i < numSigners; i++) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(guardianKeys[i], hash); + signatures = abi.encodePacked(signatures, r, s, v); + } + + return signatures; + } + + function _computeProposalHash(PackedUserOperation memory userOp) internal view returns (bytes32) { + bytes32 domainSeparator = keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("WeightedECDSASigner"), + keccak256("0.0.2"), + block.chainid, + address(signer) + ) + ); + + return keccak256( + abi.encodePacked( + "\x19\x01", + domainSeparator, + keccak256( + abi.encode( + keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)"), + userOp.sender, + SIGNER_ID, + keccak256(userOp.callData), + userOp.nonce + ) + ) + ) + ); + } + + function _createUserOp() internal pure returns (PackedUserOperation memory) { + return PackedUserOperation({ + sender: WALLET, + nonce: 0, + initCode: "", + callData: abi.encodeWithSignature("execute()"), + accountGasLimits: bytes32(0), + preVerificationGas: 0, + gasFees: bytes32(0), + paymasterAndData: "", + signature: "" + }); + } + + function _signUserOp(PackedUserOperation memory userOp, uint256 numSigners) + internal + view + returns (bytes memory) + { + bytes32 proposalHash = _computeProposalHash(userOp); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + bytes memory signatures; + + // Sign proposalHash for all except last signer + for (uint256 i = 0; i < numSigners - 1; i++) { + (uint8 vi, bytes32 ri, bytes32 si) = vm.sign(guardianKeys[i], proposalHash); + signatures = abi.encodePacked(signatures, ri, si, vi); + } + + // Last signer signs userOpHash + (uint8 vLast, bytes32 rLast, bytes32 sLast) = vm.sign(guardianKeys[numSigners - 1], userOpHash); + signatures = abi.encodePacked(signatures, rLast, sLast, vLast); + + return signatures; + } + + // ============ ERC1271 Signature Validation Tests ============ + + modifier whenValidatingERC1271Signature() { + _; + } + + function test_WhenNon_lastSignerHasZeroWeight() external whenValidatingERC1271Signature { + _installSigner(5); + + bytes32 testHash = keccak256("test"); + + // Create a non-guardian with a specific private key that gives a low address + // We need an address lower than guardians[4] (the highest guardian we use) + uint256 nonGuardianKey = 0x1234567890abcdef; + address nonGuardian = vm.addr(nonGuardianKey); + + // If nonGuardian happens to be higher than all guardians, keep trying different keys + while (nonGuardian > guardians[4]) { + nonGuardianKey += 1; + nonGuardian = vm.addr(nonGuardianKey); + } + + address[] memory mixedSigners = new address[](5); + uint256[] memory mixedKeys = new uint256[](5); + + mixedSigners[0] = nonGuardian; + mixedKeys[0] = nonGuardianKey; + for (uint256 i = 0; i < 4; i++) { + mixedSigners[i + 1] = guardians[i]; + mixedKeys[i + 1] = guardianKeys[i]; + } + + // Sort + for (uint256 i = 0; i < 5; i++) { + for (uint256 j = i + 1; j < 5; j++) { + if (mixedSigners[i] > mixedSigners[j]) { + (mixedSigners[i], mixedSigners[j]) = (mixedSigners[j], mixedSigners[i]); + (mixedKeys[i], mixedKeys[j]) = (mixedKeys[j], mixedKeys[i]); + } + } + } + + // Ensure nonGuardian is not last + require(mixedSigners[4] != nonGuardian, "Test setup: nonGuardian should not be last"); + + bytes memory signatures; + for (uint256 i = 0; i < 5; i++) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(mixedKeys[i], testHash); + signatures = abi.encodePacked(signatures, r, s, v); + } + + // it should revert with ZeroWeightSigner + vm.prank(WALLET); + vm.expectRevert(WeightedECDSASigner.ZeroWeightSigner.selector); + signer.checkSignature(SIGNER_ID, address(0), testHash, signatures); + } + + function test_WhenLastSignerHasZeroWeight() external whenValidatingERC1271Signature { + _installSigner(5); + + bytes32 testHash = keccak256("test"); + + // Create a non-guardian that will be last after sorting (high address) + (address nonGuardian, uint256 nonGuardianKey) = makeAddrAndKey("zzz_lastNonGuardian"); + + address[] memory mixedSigners = new address[](5); + uint256[] memory mixedKeys = new uint256[](5); + + for (uint256 i = 0; i < 4; i++) { + mixedSigners[i] = guardians[i]; + mixedKeys[i] = guardianKeys[i]; + } + mixedSigners[4] = nonGuardian; + mixedKeys[4] = nonGuardianKey; + + // Sort + for (uint256 i = 0; i < 5; i++) { + for (uint256 j = i + 1; j < 5; j++) { + if (mixedSigners[i] > mixedSigners[j]) { + (mixedSigners[i], mixedSigners[j]) = (mixedSigners[j], mixedSigners[i]); + (mixedKeys[i], mixedKeys[j]) = (mixedKeys[j], mixedKeys[i]); + } + } + } + + require(mixedSigners[4] == nonGuardian, "Test setup: nonGuardian should be last"); + + bytes memory signatures; + for (uint256 i = 0; i < 5; i++) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(mixedKeys[i], testHash); + signatures = abi.encodePacked(signatures, r, s, v); + } + + // it should return ERC1271_INVALID + vm.prank(WALLET); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signatures); + assertEq(result, ERC1271_INVALID); + } + + function test_WhenAllSignersAreValidGuardians() external whenValidatingERC1271Signature { + _installSigner(5); + + bytes32 testHash = keccak256("test"); + bytes memory signatures = _signHash(testHash, 5); + + vm.prank(WALLET); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signatures); + + // it should return ERC1271_MAGICVALUE + assertEq(result, ERC1271_MAGICVALUE); + } + + function test_WhenSignatureCountIsZero() external whenValidatingERC1271Signature { + _installSigner(5); + + bytes32 testHash = keccak256("test"); + bytes memory signatures = ""; + + vm.prank(WALLET); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signatures); + + // it should return ERC1271_INVALID + assertEq(result, ERC1271_INVALID); + } + + function test_WhenSignatureLengthIsNotAMultipleOf65() external whenValidatingERC1271Signature { + _installSigner(5); + + bytes32 testHash = keccak256("test"); + bytes memory signatures = new bytes(100); // Not a multiple of 65 + + vm.prank(WALLET); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signatures); + + // it should return ERC1271_INVALID + assertEq(result, ERC1271_INVALID); + } + + function test_WhenMoreThan10ValidGuardiansSign() external whenValidatingERC1271Signature { + _installSigner(12); + + bytes32 testHash = keccak256("test"); + bytes memory signatures = _signHash(testHash, 12); + + vm.prank(WALLET); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signatures); + + // it should succeed with no arbitrary limit + assertEq(result, ERC1271_MAGICVALUE); + } + + function test_WhenNon_lastSignersAreNotInSortedOrder() external whenValidatingERC1271Signature { + _installSigner(5); + + bytes32 testHash = keccak256("test"); + + // Sign in wrong order: higher address before lower address + bytes memory signatures; + // Sign with guardian at index 1 first (higher address than index 0) + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardianKeys[1], testHash); + signatures = abi.encodePacked(signatures, r1, s1, v1); + // Sign with guardian at index 0 second (lower address - wrong order) + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardianKeys[0], testHash); + signatures = abi.encodePacked(signatures, r2, s2, v2); + // Sign with guardian at index 2 last + (uint8 v3, bytes32 r3, bytes32 s3) = vm.sign(guardianKeys[2], testHash); + signatures = abi.encodePacked(signatures, r3, s3, v3); + + // it should return ERC1271_INVALID + vm.prank(WALLET); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signatures); + assertEq(result, ERC1271_INVALID); + } + + // ============ ERC4337 UserOp Validation Tests ============ + + modifier whenValidatingERC4337UserOp() { + _; + } + + function test_WhenNon_lastSignerHasZeroWeight_WhenValidatingERC4337UserOp() external whenValidatingERC4337UserOp { + _installSigner(5); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 proposalHash = _computeProposalHash(userOp); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + // Create a non-guardian with a specific private key that gives a low address + uint256 nonGuardianKey = 0x1234567890abcdef; + address nonGuardian = vm.addr(nonGuardianKey); + + // If nonGuardian happens to be higher than all guardians, keep trying different keys + while (nonGuardian > guardians[4]) { + nonGuardianKey += 1; + nonGuardian = vm.addr(nonGuardianKey); + } + + address[] memory mixedSigners = new address[](5); + uint256[] memory mixedKeys = new uint256[](5); + + mixedSigners[0] = nonGuardian; + mixedKeys[0] = nonGuardianKey; + for (uint256 i = 0; i < 4; i++) { + mixedSigners[i + 1] = guardians[i]; + mixedKeys[i + 1] = guardianKeys[i]; + } + + // Sort + for (uint256 i = 0; i < 5; i++) { + for (uint256 j = i + 1; j < 5; j++) { + if (mixedSigners[i] > mixedSigners[j]) { + (mixedSigners[i], mixedSigners[j]) = (mixedSigners[j], mixedSigners[i]); + (mixedKeys[i], mixedKeys[j]) = (mixedKeys[j], mixedKeys[i]); + } + } + } + + require(mixedSigners[4] != nonGuardian, "Test setup: nonGuardian should not be last"); + + // Sign: first 4 sign proposalHash, last signs userOpHash + bytes memory signatures; + for (uint256 i = 0; i < 4; i++) { + (uint8 vi, bytes32 ri, bytes32 si) = vm.sign(mixedKeys[i], proposalHash); + signatures = abi.encodePacked(signatures, ri, si, vi); + } + (uint8 v, bytes32 r, bytes32 s) = vm.sign(mixedKeys[4], userOpHash); + signatures = abi.encodePacked(signatures, r, s, v); + + userOp.signature = signatures; + + // it should revert with ZeroWeightSigner + vm.prank(WALLET); + vm.expectRevert(WeightedECDSASigner.ZeroWeightSigner.selector); + signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + } + + function test_WhenLastSignerHasZeroWeight_WhenValidatingERC4337UserOp() external whenValidatingERC4337UserOp { + _installSigner(5); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 proposalHash = _computeProposalHash(userOp); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + // Create a non-guardian that will be last after sorting + (address nonGuardian, uint256 nonGuardianKey) = makeAddrAndKey("zzz_lastNonGuardian"); + + // Use 4 guardians + nonGuardian, ensure nonGuardian is last + address[] memory mixedSigners = new address[](5); + uint256[] memory mixedKeys = new uint256[](5); + + for (uint256 i = 0; i < 4; i++) { + mixedSigners[i] = guardians[i]; + mixedKeys[i] = guardianKeys[i]; + } + mixedSigners[4] = nonGuardian; + mixedKeys[4] = nonGuardianKey; + + // Sort + for (uint256 i = 0; i < 5; i++) { + for (uint256 j = i + 1; j < 5; j++) { + if (mixedSigners[i] > mixedSigners[j]) { + (mixedSigners[i], mixedSigners[j]) = (mixedSigners[j], mixedSigners[i]); + (mixedKeys[i], mixedKeys[j]) = (mixedKeys[j], mixedKeys[i]); + } + } + } + + require(mixedSigners[4] == nonGuardian, "Test setup: nonGuardian should be last"); + + // Sign: first 4 sign proposalHash, last (nonGuardian) signs userOpHash + bytes memory signatures; + for (uint256 i = 0; i < 4; i++) { + (uint8 vi, bytes32 ri, bytes32 si) = vm.sign(mixedKeys[i], proposalHash); + signatures = abi.encodePacked(signatures, ri, si, vi); + } + (uint8 vLast, bytes32 rLast, bytes32 sLast) = vm.sign(nonGuardianKey, userOpHash); + signatures = abi.encodePacked(signatures, rLast, sLast, vLast); + + userOp.signature = signatures; + + // it should return SIG_VALIDATION_FAILED + vm.prank(WALLET); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + assertEq(result, SIG_VALIDATION_FAILED_UINT); + } + + function test_WhenAllSignersAreValidGuardians_WhenValidatingERC4337UserOp() external whenValidatingERC4337UserOp { + _installSigner(5); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + userOp.signature = _signUserOp(userOp, 5); + + vm.prank(WALLET); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // it should return SIG_VALIDATION_SUCCESS + assertEq(result, SIG_VALIDATION_SUCCESS_UINT); + } + + function test_WhenSignatureCountIsZero_WhenValidatingERC4337UserOp() external whenValidatingERC4337UserOp { + _installSigner(5); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + userOp.signature = ""; + + vm.prank(WALLET); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // it should return SIG_VALIDATION_FAILED + assertEq(result, SIG_VALIDATION_FAILED_UINT); + } + + function test_WhenSignatureLengthIsNotAMultipleOf65_WhenValidatingERC4337UserOp() + external + whenValidatingERC4337UserOp + { + _installSigner(5); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + userOp.signature = new bytes(100); // Not a multiple of 65 + + vm.prank(WALLET); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // it should return SIG_VALIDATION_FAILED + assertEq(result, SIG_VALIDATION_FAILED_UINT); + } + + function test_WhenMoreThan10ValidGuardiansSign_WhenValidatingERC4337UserOp() external whenValidatingERC4337UserOp { + _installSigner(12); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + userOp.signature = _signUserOp(userOp, 12); + + vm.prank(WALLET); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + // it should succeed with no arbitrary limit + assertEq(result, SIG_VALIDATION_SUCCESS_UINT); + } + + function test_RevertWhen_Non_lastSignersAreNotInSortedOrder() + external + whenValidatingERC4337UserOp + { + _installSigner(5); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 proposalHash = _computeProposalHash(userOp); + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + // Sign in wrong order: higher address before lower address for proposalHash + bytes memory signatures; + // Sign with guardian at index 1 first (higher address than index 0) + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardianKeys[1], proposalHash); + signatures = abi.encodePacked(signatures, r1, s1, v1); + // Sign with guardian at index 0 second (lower address - wrong order) + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardianKeys[0], proposalHash); + signatures = abi.encodePacked(signatures, r2, s2, v2); + // Sign userOpHash with guardian at index 2 last + (uint8 v3, bytes32 r3, bytes32 s3) = vm.sign(guardianKeys[2], userOpHash); + signatures = abi.encodePacked(signatures, r3, s3, v3); + + userOp.signature = signatures; + + // it should revert with "Signers not sorted" + vm.prank(WALLET); + vm.expectRevert("Signers not sorted"); + signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + } +} diff --git a/test/btt/WeightedECDSAGasGriefing.tree b/test/btt/WeightedECDSAGasGriefing.tree new file mode 100644 index 0000000..759d7b9 --- /dev/null +++ b/test/btt/WeightedECDSAGasGriefing.tree @@ -0,0 +1,31 @@ +WeightedECDSAGasGriefingTest +├── when validating ERC1271 signature +│ ├── when non-last signer has zero weight +│ │ └── it should revert with ZeroWeightSigner +│ ├── when last signer has zero weight +│ │ └── it should return ERC1271_INVALID +│ ├── when all signers are valid guardians +│ │ └── it should return ERC1271_MAGICVALUE +│ ├── when signature count is zero +│ │ └── it should return ERC1271_INVALID +│ ├── when signature length is not a multiple of 65 +│ │ └── it should return ERC1271_INVALID +│ ├── when more than 10 valid guardians sign +│ │ └── it should succeed with no arbitrary limit +│ └── when non-last signers are not in sorted order +│ └── it should return ERC1271_INVALID +└── when validating ERC4337 userOp + ├── when non-last signer has zero weight + │ └── it should revert with ZeroWeightSigner + ├── when last signer has zero weight + │ └── it should return SIG_VALIDATION_FAILED + ├── when all signers are valid guardians + │ └── it should return SIG_VALIDATION_SUCCESS + ├── when signature count is zero + │ └── it should return SIG_VALIDATION_FAILED + ├── when signature length is not a multiple of 65 + │ └── it should return SIG_VALIDATION_FAILED + ├── when more than 10 valid guardians sign + │ └── it should succeed with no arbitrary limit + └── when non-last signers are not in sorted order + └── it should revert diff --git a/test/btt/WeightedECDSAInstall.t.sol b/test/btt/WeightedECDSAInstall.t.sol new file mode 100644 index 0000000..b9a69cd --- /dev/null +++ b/test/btt/WeightedECDSAInstall.t.sol @@ -0,0 +1,323 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {WeightedECDSASigner} from "src/signers/WeightedECDSASigner.sol"; +import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; +import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; +import {EntryPointLib} from "../utils/EntryPointLib.sol"; +import {IModule} from "src/interfaces/IERC7579Modules.sol"; +import { + ERC1271_MAGICVALUE, + ERC1271_INVALID, + SIG_VALIDATION_FAILED_UINT, + SIG_VALIDATION_SUCCESS_UINT +} from "src/types/Constants.sol"; + +/** + * @title WeightedECDSAInstallTest + * @notice BTT tests for installation, uninstallation, and threshold zero checks + */ +contract WeightedECDSAInstallTest is Test { + WeightedECDSASigner signer; + IEntryPoint entrypoint; + + address constant WALLET = address(0x1234); + bytes32 constant SIGNER_ID = keccak256("TEST_SIGNER_ID"); + + address guardian1; + uint256 guardian1Key; + address guardian2; + uint256 guardian2Key; + + function setUp() public { + signer = new WeightedECDSASigner(); + entrypoint = EntryPointLib.deploy(); + + (guardian1, guardian1Key) = makeAddrAndKey("guardian1"); + (guardian2, guardian2Key) = makeAddrAndKey("guardian2"); + } + + // ============ Installation Tests ============ + + modifier whenInstallingSigner() { + _; + } + + function test_RevertWhen_GuardiansAndWeightsLengthMismatch() external whenInstallingSigner { + address[] memory guardians = new address[](2); + guardians[0] = guardian1; + guardians[1] = guardian2; + + uint24[] memory weights = new uint24[](1); // Mismatch: 1 weight for 2 guardians + weights[0] = 50; + + bytes memory installData = abi.encode(guardians, weights, uint24(50)); + + // it should revert + vm.prank(WALLET); + vm.expectRevert("Length mismatch"); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + } + + function test_RevertWhen_GuardianIsSelf() external whenInstallingSigner { + address[] memory guardians = new address[](1); + guardians[0] = WALLET; // Guardian is msg.sender + + uint24[] memory weights = new uint24[](1); + weights[0] = 50; + + bytes memory installData = abi.encode(guardians, weights, uint24(50)); + + // it should revert + vm.prank(WALLET); + vm.expectRevert("Guardian cannot be self"); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + } + + function test_RevertWhen_GuardianIsAddressZero() external whenInstallingSigner { + address[] memory guardians = new address[](1); + guardians[0] = address(0); + + uint24[] memory weights = new uint24[](1); + weights[0] = 50; + + bytes memory installData = abi.encode(guardians, weights, uint24(50)); + + // it should revert + vm.prank(WALLET); + vm.expectRevert("Guardian cannot be 0"); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + } + + function test_RevertWhen_WeightIsZero() external whenInstallingSigner { + address[] memory guardians = new address[](1); + guardians[0] = guardian1; + + uint24[] memory weights = new uint24[](1); + weights[0] = 0; // Zero weight + + bytes memory installData = abi.encode(guardians, weights, uint24(50)); + + // it should revert + vm.prank(WALLET); + vm.expectRevert("Weight cannot be 0"); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + } + + function test_RevertWhen_GuardianIsAlreadyEnabled() external whenInstallingSigner { + address[] memory guardians = new address[](2); + guardians[0] = guardian1; + guardians[1] = guardian1; // Duplicate guardian + + uint24[] memory weights = new uint24[](2); + weights[0] = 50; + weights[1] = 50; + + bytes memory installData = abi.encode(guardians, weights, uint24(50)); + + // it should revert + vm.prank(WALLET); + vm.expectRevert("Guardian already enabled"); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + } + + function test_WhenAllParametersAreValid() external whenInstallingSigner { + address[] memory guardians = new address[](2); + guardians[0] = guardian1; + guardians[1] = guardian2; + + uint24[] memory weights = new uint24[](2); + weights[0] = 50; + weights[1] = 50; + + bytes memory installData = abi.encode(guardians, weights, uint24(60)); + + // it should install successfully + vm.prank(WALLET); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + + (uint24 totalWeight, uint24 threshold,) = signer.weightedStorage(SIGNER_ID, WALLET); + assertEq(totalWeight, 100); + assertEq(threshold, 60); + } + + // ============ Uninstallation Tests ============ + + modifier whenUninstallingSigner() { + _; + } + + function test_WhenSignerIsNotInitialized() external whenUninstallingSigner { + // it should revert with NotInitialized + vm.prank(WALLET); + vm.expectRevert(abi.encodeWithSelector(IModule.NotInitialized.selector, WALLET)); + signer.onUninstall(abi.encodePacked(SIGNER_ID)); + } + + function test_WhenSignerIsInitialized() external whenUninstallingSigner { + // First install + address[] memory guardians = new address[](1); + guardians[0] = guardian1; + + uint24[] memory weights = new uint24[](1); + weights[0] = 50; + + bytes memory installData = abi.encode(guardians, weights, uint24(50)); + + vm.prank(WALLET); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + + // Verify installed + (uint24 totalWeight,,) = signer.weightedStorage(SIGNER_ID, WALLET); + assertEq(totalWeight, 50); + + // it should uninstall successfully + vm.prank(WALLET); + signer.onUninstall(abi.encodePacked(SIGNER_ID)); + + // Verify uninstalled + (totalWeight,,) = signer.weightedStorage(SIGNER_ID, WALLET); + assertEq(totalWeight, 0); + } + + function test_WhenUninstallingWithMultipleGuardians() external whenUninstallingSigner { + // Install with 3 guardians + address guardian3; + (guardian3,) = makeAddrAndKey("guardian3"); + + address[] memory guardians = new address[](3); + guardians[0] = guardian1; + guardians[1] = guardian2; + guardians[2] = guardian3; + + uint24[] memory weights = new uint24[](3); + weights[0] = 30; + weights[1] = 30; + weights[2] = 40; + + bytes memory installData = abi.encode(guardians, weights, uint24(60)); + + vm.prank(WALLET); + signer.onInstall(abi.encodePacked(SIGNER_ID, installData)); + + // Verify installed + (uint24 totalWeight,,) = signer.weightedStorage(SIGNER_ID, WALLET); + assertEq(totalWeight, 100); + + // Uninstall + vm.prank(WALLET); + signer.onUninstall(abi.encodePacked(SIGNER_ID)); + + // Verify uninstalled + (totalWeight,,) = signer.weightedStorage(SIGNER_ID, WALLET); + assertEq(totalWeight, 0); + + // Verify guardians are cleared + (uint24 g1Weight,) = signer.guardian(guardian1, SIGNER_ID, WALLET); + (uint24 g2Weight,) = signer.guardian(guardian2, SIGNER_ID, WALLET); + (uint24 g3Weight,) = signer.guardian(guardian3, SIGNER_ID, WALLET); + assertEq(g1Weight, 0); + assertEq(g2Weight, 0); + assertEq(g3Weight, 0); + } + + // ============ Threshold Zero Tests ============ + + modifier whenCheckingThresholdZero() { + _; + } + + function test_WhenValidatingUserOpWithThresholdZero() external whenCheckingThresholdZero { + // Don't install the signer - threshold will be 0 + + PackedUserOperation memory userOp = PackedUserOperation({ + sender: WALLET, + nonce: 0, + initCode: "", + callData: hex"1234", + accountGasLimits: bytes32(0), + preVerificationGas: 0, + gasFees: bytes32(0), + paymasterAndData: "", + signature: hex"00" // Some signature data + }); + + bytes32 userOpHash = entrypoint.getUserOpHash(userOp); + + // it should return SIG_VALIDATION_FAILED + vm.prank(WALLET); + uint256 result = signer.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + assertEq(result, SIG_VALIDATION_FAILED_UINT); + } + + function test_WhenValidatingERC1271WithThresholdZero() external whenCheckingThresholdZero { + // Don't install the signer - threshold will be 0 + + bytes32 testHash = keccak256("test"); + bytes memory signature = new bytes(65); + + // it should return ERC1271_INVALID + vm.prank(WALLET); + bytes4 result = signer.checkSignature(SIGNER_ID, address(0), testHash, signature); + assertEq(result, ERC1271_INVALID); + } + + // ============ Module Type Tests ============ + + modifier whenCheckingModuleType() { + _; + } + + function test_WhenTypeIsSigner() external whenCheckingModuleType { + assertTrue(signer.isModuleType(6)); // MODULE_TYPE_SIGNER + } + + function test_WhenTypeIsStatelessValidator() external whenCheckingModuleType { + assertTrue(signer.isModuleType(7)); // MODULE_TYPE_STATELESS_VALIDATOR + } + + function test_WhenTypeIsStatelessValidatorWithSender() external whenCheckingModuleType { + assertTrue(signer.isModuleType(10)); // MODULE_TYPE_STATELESS_VALIDATOR_WITH_SENDER + } + + function test_WhenTypeIsValidator() external whenCheckingModuleType { + assertFalse(signer.isModuleType(1)); // MODULE_TYPE_VALIDATOR - not supported + } + + // ============ Stateless Validator With Sender Tests ============ + + function test_WhenValidatingSignatureWithDataWithSender() external { + bytes32 hash = keccak256("test"); + + // Create sorted guardians + address[] memory guardians = new address[](2); + uint256[] memory keys = new uint256[](2); + guardians[0] = guardian1; + guardians[1] = guardian2; + keys[0] = guardian1Key; + keys[1] = guardian2Key; + + // Sort + if (guardian1 > guardian2) { + (guardians[0], guardians[1]) = (guardians[1], guardians[0]); + (keys[0], keys[1]) = (keys[1], keys[0]); + } + + uint24[] memory weights = new uint24[](2); + weights[0] = 50; + weights[1] = 50; + + bytes memory data = abi.encode(guardians, weights, uint24(50)); + + // Sign with both guardians + bytes memory signatures; + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(keys[0], hash); + signatures = abi.encodePacked(signatures, r1, s1, v1); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(keys[1], hash); + signatures = abi.encodePacked(signatures, r2, s2, v2); + + bool result = signer.validateSignatureWithDataWithSender(address(0), hash, signatures, data); + assertTrue(result); + } +} diff --git a/test/btt/WeightedECDSAInstall.tree b/test/btt/WeightedECDSAInstall.tree new file mode 100644 index 0000000..7600621 --- /dev/null +++ b/test/btt/WeightedECDSAInstall.tree @@ -0,0 +1,37 @@ +WeightedECDSAInstallTest +├── when installing signer +│ ├── when guardians and weights length mismatch +│ │ └── it should revert +│ ├── when guardian is self +│ │ └── it should revert +│ ├── when guardian is address zero +│ │ └── it should revert +│ ├── when weight is zero +│ │ └── it should revert +│ ├── when guardian is already enabled +│ │ └── it should revert +│ └── when all parameters are valid +│ └── it should install successfully +├── when uninstalling signer +│ ├── when signer is not initialized +│ │ └── it should revert with NotInitialized +│ ├── when signer is initialized +│ │ └── it should uninstall successfully +│ └── when uninstalling with multiple guardians +│ └── it should clear all guardian data +├── when checking threshold zero +│ ├── when validating userOp with threshold zero +│ │ └── it should return SIG_VALIDATION_FAILED +│ └── when validating ERC1271 with threshold zero +│ └── it should return ERC1271_INVALID +├── when checking module type +│ ├── when type is signer +│ │ └── it should return true +│ ├── when type is stateless validator +│ │ └── it should return true +│ ├── when type is stateless validator with sender +│ │ └── it should return true +│ └── when type is validator +│ └── it should return false +└── when validating signature with data with sender + └── it should validate correctly diff --git a/test/btt/WeightedECDSAStateless.t.sol b/test/btt/WeightedECDSAStateless.t.sol new file mode 100644 index 0000000..a298cfa --- /dev/null +++ b/test/btt/WeightedECDSAStateless.t.sol @@ -0,0 +1,312 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {WeightedECDSASigner} from "src/signers/WeightedECDSASigner.sol"; +import {ECDSA} from "solady/utils/ECDSA.sol"; + +/** + * @title WeightedECDSAStatelessTest + * @notice BTT tests for stateless signature validation + */ +contract WeightedECDSAStatelessTest is Test { + WeightedECDSASigner signer; + + address guardian1; + uint256 guardian1Key; + address guardian2; + uint256 guardian2Key; + address guardian3; + uint256 guardian3Key; + + // Sorted guardians + address[] sortedGuardians; + uint256[] sortedKeys; + + function setUp() public { + signer = new WeightedECDSASigner(); + + (guardian1, guardian1Key) = makeAddrAndKey("guardian1"); + (guardian2, guardian2Key) = makeAddrAndKey("guardian2"); + (guardian3, guardian3Key) = makeAddrAndKey("guardian3"); + + // Sort guardians by address + sortedGuardians = new address[](3); + sortedKeys = new uint256[](3); + + sortedGuardians[0] = guardian1; + sortedGuardians[1] = guardian2; + sortedGuardians[2] = guardian3; + sortedKeys[0] = guardian1Key; + sortedKeys[1] = guardian2Key; + sortedKeys[2] = guardian3Key; + + // Bubble sort + for (uint256 i = 0; i < 3; i++) { + for (uint256 j = i + 1; j < 3; j++) { + if (sortedGuardians[i] > sortedGuardians[j]) { + (sortedGuardians[i], sortedGuardians[j]) = (sortedGuardians[j], sortedGuardians[i]); + (sortedKeys[i], sortedKeys[j]) = (sortedKeys[j], sortedKeys[i]); + } + } + } + } + + function _createData(uint24 threshold) internal view returns (bytes memory) { + uint24[] memory weights = new uint24[](3); + weights[0] = 40; + weights[1] = 30; + weights[2] = 30; + return abi.encode(sortedGuardians, weights, threshold); + } + + function _signHash(bytes32 hash, uint256[] memory keys) internal view returns (bytes memory) { + bytes memory signatures; + for (uint256 i = 0; i < keys.length; i++) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(keys[i], hash); + signatures = abi.encodePacked(signatures, r, s, v); + } + return signatures; + } + + // ============ Test Cases ============ + + function test_WhenThresholdIsZero() external { + bytes32 hash = keccak256("test"); + + uint256[] memory keys = new uint256[](1); + keys[0] = sortedKeys[0]; + bytes memory sig = _signHash(hash, keys); + + // Create data with threshold 0 + uint24[] memory weights = new uint24[](3); + weights[0] = 40; + weights[1] = 30; + weights[2] = 30; + bytes memory data = abi.encode(sortedGuardians, weights, uint24(0)); + + // it should return false + bool result = signer.validateSignatureWithData(hash, sig, data); + assertFalse(result); + } + + function test_WhenGuardiansAndWeightsLengthMismatch() external { + bytes32 hash = keccak256("test"); + + uint256[] memory keys = new uint256[](1); + keys[0] = sortedKeys[0]; + bytes memory sig = _signHash(hash, keys); + + // Create data with mismatched lengths + uint24[] memory weights = new uint24[](2); // Only 2 weights for 3 guardians + weights[0] = 40; + weights[1] = 30; + bytes memory data = abi.encode(sortedGuardians, weights, uint24(50)); + + // it should return false + bool result = signer.validateSignatureWithData(hash, sig, data); + assertFalse(result); + } + + function test_WhenSignatureCountIsZero() external { + bytes32 hash = keccak256("test"); + bytes memory sig = ""; // Empty signature + + bytes memory data = _createData(50); + + // it should return false + bool result = signer.validateSignatureWithData(hash, sig, data); + assertFalse(result); + } + + function test_WhenNon_lastSignerIsNotInSortedOrder() external { + bytes32 hash = keccak256("test"); + + // Sign in wrong order: second guardian before first (for non-last signatures) + bytes memory signatures; + // Sign with higher address first (wrong order) + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(sortedKeys[1], hash); + signatures = abi.encodePacked(signatures, r1, s1, v1); + // Sign with lower address second (wrong order) + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(sortedKeys[0], hash); + signatures = abi.encodePacked(signatures, r2, s2, v2); + // Last signature + (uint8 v3, bytes32 r3, bytes32 s3) = vm.sign(sortedKeys[2], hash); + signatures = abi.encodePacked(signatures, r3, s3, v3); + + bytes memory data = _createData(50); + + // it should return false + bool result = signer.validateSignatureWithData(hash, signatures, data); + assertFalse(result); + } + + function test_WhenThresholdIsMetBeforeLastSignature() external { + bytes32 hash = keccak256("test"); + + // Sign with first two guardians (40 + 30 = 70 >= threshold 50) + uint256[] memory keys = new uint256[](3); + keys[0] = sortedKeys[0]; + keys[1] = sortedKeys[1]; + keys[2] = sortedKeys[2]; + bytes memory sig = _signHash(hash, keys); + + // Threshold 50, first two guardians have 70 weight combined + bytes memory data = _createData(50); + + // it should return true early + bool result = signer.validateSignatureWithData(hash, sig, data); + assertTrue(result); + } + + function test_WhenLastSignerIsNotInSortedOrder() external { + bytes32 hash = keccak256("test"); + + // Sign with first guardian, then last signer has lower address + // This requires creating a scenario where last signer address < previous signer address + // We'll use only the highest address guardian first, then a lower one last + bytes memory signatures; + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(sortedKeys[2], hash); // Highest address + signatures = abi.encodePacked(signatures, r1, s1, v1); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(sortedKeys[0], hash); // Lowest address (wrong order) + signatures = abi.encodePacked(signatures, r2, s2, v2); + + bytes memory data = _createData(50); + + // it should return false + bool result = signer.validateSignatureWithData(hash, signatures, data); + assertFalse(result); + } + + function test_WhenLastSignerHasZeroWeight() external { + bytes32 hash = keccak256("test"); + + // Create a non-guardian that will be last + (address nonGuardian, uint256 nonGuardianKey) = makeAddrAndKey("zzz_nonGuardian"); + + // Sign with first guardian, then non-guardian (zero weight) as last + bytes memory signatures; + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(sortedKeys[0], hash); + signatures = abi.encodePacked(signatures, r1, s1, v1); + + // Make sure nonGuardian is higher than sortedGuardians[0] + require(nonGuardian > sortedGuardians[0], "Test setup: nonGuardian must be higher"); + + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(nonGuardianKey, hash); + signatures = abi.encodePacked(signatures, r2, s2, v2); + + bytes memory data = _createData(50); + + // it should return false + bool result = signer.validateSignatureWithData(hash, signatures, data); + assertFalse(result); + } + + function test_WhenNon_lastSignerHasZeroWeight() external { + bytes32 hash = keccak256("test"); + + // We need a non-guardian that is NOT the last signer + // We'll use 2 signers: nonGuardian first, then a valid guardian last + // Sign nonGuardian first, then highest guardian last + uint256 nonGuardianKey = 0x1234567890abcdef; + address nonGuardian = vm.addr(nonGuardianKey); + + // Find a key that gives an address lower than the highest guardian + while (nonGuardian >= sortedGuardians[2]) { + nonGuardianKey += 1; + nonGuardian = vm.addr(nonGuardianKey); + } + + // Sign in order: nonGuardian (low address, zero weight), then highest guardian (valid) + bytes memory signatures; + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(nonGuardianKey, hash); + signatures = abi.encodePacked(signatures, r1, s1, v1); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(sortedKeys[2], hash); + signatures = abi.encodePacked(signatures, r2, s2, v2); + + bytes memory data = _createData(50); + + // it should revert with ZeroWeightSigner (nonGuardian is not last) + vm.expectRevert(WeightedECDSASigner.ZeroWeightSigner.selector); + signer.validateSignatureWithData(hash, signatures, data); + } + + function test_WhenAllSignersAreValidAndThresholdIsMet() external { + bytes32 hash = keccak256("test"); + + // Sign with all three guardians in sorted order + uint256[] memory keys = new uint256[](3); + keys[0] = sortedKeys[0]; + keys[1] = sortedKeys[1]; + keys[2] = sortedKeys[2]; + bytes memory sig = _signHash(hash, keys); + + bytes memory data = _createData(100); // Threshold 100, total weight is 100 + + // it should return true + bool result = signer.validateSignatureWithData(hash, sig, data); + assertTrue(result); + } + + function test_WhenSingleSignatureMeetsThreshold() external { + bytes32 hash = keccak256("test"); + + // Sign with only first guardian + uint256[] memory keys = new uint256[](1); + keys[0] = sortedKeys[0]; + bytes memory sig = _signHash(hash, keys); + + // First guardian has weight 40, set threshold to 40 + uint24[] memory weights = new uint24[](3); + weights[0] = 40; + weights[1] = 30; + weights[2] = 30; + bytes memory data = abi.encode(sortedGuardians, weights, uint24(40)); + + // it should return true with single signature + bool result = signer.validateSignatureWithData(hash, sig, data); + assertTrue(result); + } + + function test_WhenSingleSignatureDoesNotMeetThreshold() external { + bytes32 hash = keccak256("test"); + + // Sign with only first guardian + uint256[] memory keys = new uint256[](1); + keys[0] = sortedKeys[0]; + bytes memory sig = _signHash(hash, keys); + + // First guardian has weight 40, threshold 50 - should fail + bytes memory data = _createData(50); + + // it should return false + bool result = signer.validateSignatureWithData(hash, sig, data); + assertFalse(result); + } + + function test_WhenValidatingSignatureWithDataWithSenderSucceeds() external { + bytes32 hash = keccak256("test"); + + uint256[] memory keys = new uint256[](2); + keys[0] = sortedKeys[0]; + keys[1] = sortedKeys[1]; + bytes memory sig = _signHash(hash, keys); + + bytes memory data = _createData(50); + + // it should return true via validateSignatureWithDataWithSender + bool result = signer.validateSignatureWithDataWithSender(address(0x5678), hash, sig, data); + assertTrue(result); + } + + function test_WhenValidatingSignatureWithDataWithSenderFails() external { + bytes32 hash = keccak256("test"); + bytes memory sig = ""; // Empty signature + + bytes memory data = _createData(50); + + // it should return false via validateSignatureWithDataWithSender + bool result = signer.validateSignatureWithDataWithSender(address(0x5678), hash, sig, data); + assertFalse(result); + } +} diff --git a/test/btt/WeightedECDSAStateless.tree b/test/btt/WeightedECDSAStateless.tree new file mode 100644 index 0000000..6c47302 --- /dev/null +++ b/test/btt/WeightedECDSAStateless.tree @@ -0,0 +1,27 @@ +WeightedECDSAStatelessTest +├── when threshold is zero +│ └── it should return false +├── when guardians and weights length mismatch +│ └── it should return false +├── when signature count is zero +│ └── it should return false +├── when non-last signer is not in sorted order +│ └── it should return false +├── when threshold is met before last signature +│ └── it should return true early +├── when last signer is not in sorted order +│ └── it should return false +├── when last signer has zero weight +│ └── it should return false +├── when non-last signer has zero weight +│ └── it should revert with ZeroWeightSigner +├── when all signers are valid and threshold is met +│ └── it should return true +├── when single signature meets threshold +│ └── it should return true +├── when single signature does not meet threshold +│ └── it should return false +├── when validating signature with data with sender succeeds +│ └── it should return true +└── when validating signature with data with sender fails + └── it should return false diff --git a/test/btt/WeightedECDSAUserOpHash.t.sol b/test/btt/WeightedECDSAUserOpHash.t.sol new file mode 100644 index 0000000..f53b6bd --- /dev/null +++ b/test/btt/WeightedECDSAUserOpHash.t.sol @@ -0,0 +1,474 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {WeightedECDSASigner} from "src/signers/WeightedECDSASigner.sol"; +import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; +import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; +import {EntryPointLib} from "../utils/EntryPointLib.sol"; +import {ECDSA} from "solady/utils/ECDSA.sol"; +import { + SIG_VALIDATION_FAILED_UINT, + SIG_VALIDATION_SUCCESS_UINT +} from "src/types/Constants.sol"; + +/// @title WeightedECDSAUserOpHashTest +/// @notice BTT tests for the security fix on branch fix/tob-kernel-16 +/// @dev Tests the fix for: Weighted validator can skip userOpHash, allowing mutable user operations +contract WeightedECDSAUserOpHashTest is Test { + WeightedECDSASigner public signerModule; + IEntryPoint public ENTRYPOINT; + + address constant WALLET = address(0x1234); + bytes32 constant SIGNER_ID = keccak256(abi.encodePacked("BTT_SIGNER_ID")); + + // Guardian keys - will be sorted by address after generation + address public guardian1; + uint256 public guardian1Key; + address public guardian2; + uint256 public guardian2Key; + address public guardian3; + uint256 public guardian3Key; + + // Weights and threshold + uint24 public weight1 = 50; + uint24 public weight2 = 30; + uint24 public weight3 = 20; + uint24 public threshold = 60; // Need guardian1 + guardian2 (50 + 30 = 80) to meet threshold + + function setUp() public { + signerModule = new WeightedECDSASigner(); + ENTRYPOINT = EntryPointLib.deploy(); + + // Generate guardian keys + (guardian1, guardian1Key) = makeAddrAndKey("guardian1"); + (guardian2, guardian2Key) = makeAddrAndKey("guardian2"); + (guardian3, guardian3Key) = makeAddrAndKey("guardian3"); + + // Install the signer module with guardians + _installModule(); + } + + function _installModule() internal { + address[] memory guardians = new address[](3); + guardians[0] = guardian1; + guardians[1] = guardian2; + guardians[2] = guardian3; + + uint24[] memory weights = new uint24[](3); + weights[0] = weight1; + weights[1] = weight2; + weights[2] = weight3; + + bytes memory installData = abi.encode(guardians, weights, threshold); + + vm.prank(WALLET); + signerModule.onInstall(abi.encodePacked(SIGNER_ID, installData)); + } + + function _createUserOp() internal view returns (PackedUserOperation memory) { + return PackedUserOperation({ + sender: WALLET, + nonce: 0, + initCode: "", + callData: abi.encodeWithSignature("execute(address,uint256,bytes)", address(0x5678), 0, ""), + accountGasLimits: bytes32(abi.encodePacked(uint128(100000), uint128(200000))), + preVerificationGas: 0, + gasFees: bytes32(abi.encodePacked(uint128(1), uint128(1))), + paymasterAndData: "", + signature: "" + }); + } + + function _computeProposalHash(PackedUserOperation memory userOp) internal view returns (bytes32) { + bytes32 domainSeparator = keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("WeightedECDSASigner"), + keccak256("0.0.2"), + block.chainid, + address(signerModule) + ) + ); + + return keccak256( + abi.encodePacked( + "\x19\x01", + domainSeparator, + keccak256( + abi.encode( + keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)"), + userOp.sender, + SIGNER_ID, + keccak256(userOp.callData), + userOp.nonce + ) + ) + ) + ); + } + + /// @dev Sort two signers and return signatures in sorted order + function _sortAndSignProposalHash( + bytes32 proposalHash, + address signerA, + uint256 keyA, + address signerB, + uint256 keyB + ) internal pure returns (bytes memory) { + (uint8 vA, bytes32 rA, bytes32 sA) = vm.sign(keyA, proposalHash); + (uint8 vB, bytes32 rB, bytes32 sB) = vm.sign(keyB, proposalHash); + + if (signerA < signerB) { + return abi.encodePacked(rA, sA, vA, rB, sB, vB); + } else { + return abi.encodePacked(rB, sB, vB, rA, sA, vA); + } + } + + /// @dev Sign userOpHash (last signature, no sorting required) + function _signUserOpHash(bytes32 userOpHash, uint256 key) internal pure returns (bytes memory) { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(key, userOpHash); + return abi.encodePacked(r, s, v); + } + + // ==================== Test Cases ==================== + + function test_WhenUserOpHashSignerIsNotAValidGuardian() external { + // it should return SIG_VALIDATION_FAILED_UINT + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Create a non-guardian signer + (address invalidSigner, uint256 invalidKey) = makeAddrAndKey("invalidSigner"); + + // Guardian1 signs proposalHash (valid) + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardian1Key, proposalHash); + + // Invalid signer signs userOpHash + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(invalidKey, userOpHash); + + // Build signature: guardian1 (proposalHash) + invalidSigner (userOpHash) + bytes memory signature; + if (guardian1 < invalidSigner) { + signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + } else { + // Need to swap order for proposalHash signers, but last sig is always userOpHash + signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + } + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = signerModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when userOpHash signer is not a valid guardian"); + } + + modifier whenSameGuardianSignsBothProposalHashAndUserOpHash() { + _; + } + + function test_GivenTheGuardianWeightAloneDoesNotMeetThreshold() + external + whenSameGuardianSignsBothProposalHashAndUserOpHash + { + // it should return SIG_VALIDATION_FAILED_UINT because weight is not double counted + + // Setup: Use guardian3 (weight 20) who alone doesn't meet threshold (60) + // Even if they sign both proposalHash and userOpHash, weight should not be double-counted + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Guardian3 signs proposalHash + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardian3Key, proposalHash); + + // Guardian3 also signs userOpHash + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardian3Key, userOpHash); + + // Build signature: guardian3 (proposalHash) + guardian3 (userOpHash) + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = signerModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when same guardian's weight is not double-counted"); + } + + function test_GivenTheGuardianWeightAloneMeetsThreshold() + external + whenSameGuardianSignsBothProposalHashAndUserOpHash + { + // it should return SIG_VALIDATION_SUCCESS_UINT without double counting + + // For this test, we need to set up a scenario where a single guardian's weight meets threshold + // We'll create a new module installation with guardian1 having weight >= threshold + + // Deploy new module for this specific test + WeightedECDSASigner testModule = new WeightedECDSASigner(); + + address[] memory guardians = new address[](1); + guardians[0] = guardian1; + + uint24[] memory weights = new uint24[](1); + weights[0] = 100; // Weight meets threshold of 60 + + uint24 testThreshold = 60; + + bytes memory installData = abi.encode(guardians, weights, testThreshold); + + vm.prank(WALLET); + testModule.onInstall(abi.encodePacked(SIGNER_ID, installData)); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + + // Compute proposal hash for the test module + bytes32 domainSeparator = keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("WeightedECDSASigner"), + keccak256("0.0.2"), + block.chainid, + address(testModule) + ) + ); + + bytes32 proposalHash = keccak256( + abi.encodePacked( + "\x19\x01", + domainSeparator, + keccak256( + abi.encode( + keccak256("Proposal(address account,bytes32 id,bytes callData,uint256 nonce)"), + userOp.sender, + SIGNER_ID, + keccak256(userOp.callData), + userOp.nonce + ) + ) + ) + ); + + // Guardian1 signs proposalHash + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardian1Key, proposalHash); + + // Guardian1 also signs userOpHash + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardian1Key, userOpHash); + + // Build signature: guardian1 (proposalHash) + guardian1 (userOpHash) + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = testModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed when guardian weight meets threshold even without double counting"); + } + + function test_WhenProposalHashSignaturesReachThresholdButUserOpHashIsMissing() external { + // it should return SIG_VALIDATION_FAILED_UINT because userOpHash is always required + + // This test verifies that even if proposalHash signatures reach threshold, + // validation fails if the last signature (userOpHash) is not from a valid guardian + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Guardian1 (50) + Guardian2 (30) = 80 >= threshold (60) + // Both sign proposalHash, but the "userOpHash" slot will have an invalid signature + + // Sort guardian1 and guardian2 + address lowerAddr; + address higherAddr; + uint256 lowerKey; + uint256 higherKey; + + if (guardian1 < guardian2) { + lowerAddr = guardian1; + lowerKey = guardian1Key; + higherAddr = guardian2; + higherKey = guardian2Key; + } else { + lowerAddr = guardian2; + lowerKey = guardian2Key; + higherAddr = guardian1; + higherKey = guardian1Key; + } + + // Sign proposalHash with sorted order + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(lowerKey, proposalHash); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(higherKey, proposalHash); + + // Create an invalid last signature (wrong hash - not userOpHash) + bytes32 wrongHash = keccak256("wrong"); + (address invalidSigner, uint256 invalidKey) = makeAddrAndKey("invalid"); + (uint8 v3, bytes32 r3, bytes32 s3) = vm.sign(invalidKey, wrongHash); + + // Build signature: two proposalHash sigs + invalid userOpHash sig + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2, r3, s3, v3); + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = signerModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when userOpHash signature is missing/invalid"); + } + + function test_WhenProposalHashSignaturesReachThresholdAndUserOpHashSignerIsInvalid() external { + // it should return SIG_VALIDATION_FAILED_UINT + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Sort guardian1 and guardian2 for proposalHash signatures + address lowerAddr; + address higherAddr; + uint256 lowerKey; + uint256 higherKey; + + if (guardian1 < guardian2) { + lowerAddr = guardian1; + lowerKey = guardian1Key; + higherAddr = guardian2; + higherKey = guardian2Key; + } else { + lowerAddr = guardian2; + lowerKey = guardian2Key; + higherAddr = guardian1; + higherKey = guardian1Key; + } + + // Sign proposalHash with sorted order + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(lowerKey, proposalHash); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(higherKey, proposalHash); + + // Sign userOpHash with non-guardian + (address invalidSigner, uint256 invalidKey) = makeAddrAndKey("invalidGuardian"); + (uint8 v3, bytes32 r3, bytes32 s3) = vm.sign(invalidKey, userOpHash); + + // Build signature + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2, r3, s3, v3); + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = signerModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when userOpHash signer is not a guardian"); + } + + modifier whenValidMulti_sigWithDifferentGuardiansForProposalHashAndUserOpHash() { + _; + } + + function test_GivenCombinedWeightMeetsThreshold() + external + whenValidMulti_sigWithDifferentGuardiansForProposalHashAndUserOpHash + { + // it should return SIG_VALIDATION_SUCCESS_UINT + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Guardian1 (50) signs proposalHash + // Guardian2 (30) signs userOpHash + // Total weight = 80 >= threshold (60) + + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardian1Key, proposalHash); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardian2Key, userOpHash); + + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = signerModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed when combined weight meets threshold"); + } + + function test_GivenCombinedWeightDoesNotMeetThreshold() + external + whenValidMulti_sigWithDifferentGuardiansForProposalHashAndUserOpHash + { + // it should return SIG_VALIDATION_FAILED_UINT + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Guardian3 (20) signs proposalHash + // Guardian2 (30) signs userOpHash (assuming they are different signers) + // Total weight = 50 < threshold (60) + + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(guardian3Key, proposalHash); + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(guardian2Key, userOpHash); + + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = signerModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when combined weight does not meet threshold"); + } + + function test_WhenGuardianWithLowerAddressSignsUserOpHash() external { + // it should return SIG_VALIDATION_SUCCESS_UINT because no sorted order required for last signer + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOp); + bytes32 proposalHash = _computeProposalHash(userOp); + + // Determine which guardian has a higher address to sign proposalHash + // and which has a lower address to sign userOpHash + // This tests that the last signer (userOpHash) doesn't need to follow sorted order + + address higherAddr; + uint256 higherKey; + address lowerAddr; + uint256 lowerKey; + + if (guardian1 > guardian2) { + higherAddr = guardian1; + higherKey = guardian1Key; + lowerAddr = guardian2; + lowerKey = guardian2Key; + } else { + higherAddr = guardian2; + higherKey = guardian2Key; + lowerAddr = guardian1; + lowerKey = guardian1Key; + } + + // Higher address guardian signs proposalHash + (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(higherKey, proposalHash); + + // Lower address guardian signs userOpHash (this tests that last signer has no sorted requirement) + (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(lowerKey, userOpHash); + + // Build signature: higher (proposalHash) + lower (userOpHash) + // The last signature is for userOpHash and doesn't need to follow sorted order + bytes memory signature = abi.encodePacked(r1, s1, v1, r2, s2, v2); + + userOp.signature = signature; + + vm.prank(WALLET); + uint256 result = signerModule.checkUserOpSignature(SIGNER_ID, userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed when lower address guardian signs userOpHash (no sorted order required for last signer)"); + } +} diff --git a/test/btt/WeightedECDSAUserOpHash.tree b/test/btt/WeightedECDSAUserOpHash.tree new file mode 100644 index 0000000..3561a12 --- /dev/null +++ b/test/btt/WeightedECDSAUserOpHash.tree @@ -0,0 +1,19 @@ +WeightedECDSAUserOpHashTest +├── when userOpHash signer is not a valid guardian +│ └── it should return SIG_VALIDATION_FAILED_UINT +├── when same guardian signs both proposalHash and userOpHash +│ ├── given the guardian weight alone does not meet threshold +│ │ └── it should return SIG_VALIDATION_FAILED_UINT because weight is not double counted +│ └── given the guardian weight alone meets threshold +│ └── it should return SIG_VALIDATION_SUCCESS_UINT without double counting +├── when proposalHash signatures reach threshold but userOpHash is missing +│ └── it should return SIG_VALIDATION_FAILED_UINT because userOpHash is always required +├── when proposalHash signatures reach threshold and userOpHash signer is invalid +│ └── it should return SIG_VALIDATION_FAILED_UINT +├── when valid multi-sig with different guardians for proposalHash and userOpHash +│ ├── given combined weight meets threshold +│ │ └── it should return SIG_VALIDATION_SUCCESS_UINT +│ └── given combined weight does not meet threshold +│ └── it should return SIG_VALIDATION_FAILED_UINT +└── when guardian with lower address signs userOpHash + └── it should return SIG_VALIDATION_SUCCESS_UINT because no sorted order required for last signer