diff --git a/src/signers/WeightedECDSASigner.sol b/src/signers/WeightedECDSASigner.sol index 2da7696..805d198 100644 --- a/src/signers/WeightedECDSASigner.sol +++ b/src/signers/WeightedECDSASigner.sol @@ -186,9 +186,16 @@ contract WeightedECDSASigner is EIP712, SignerBase, IStatelessValidator, IStatel } } - // Last signature verifies userOpHash (exempt from ordering requirement) + // Last signature verifies userOpHash // NOTE: use this with ep > 0.7 only, for ep <= 0.7, need to use toEthSignedMessageHash signer = ECDSA.tryRecoverCalldata(userOpHash, sig[sig.length - 65:]); + + // Enforce sorted order for last signer to prevent double-counting + // (same guardian signing both proposalHash and userOpHash) + if (signer <= lastSigner) { + return SIG_VALIDATION_FAILED_UINT; + } + uint24 lastWeight = guardian[signer][id][account].weight; if (lastWeight > 0) { totalWeight += lastWeight; diff --git a/test/btt/ECDSAValidatorSecurityFix.t.sol b/test/btt/ECDSAValidatorSecurityFix.t.sol new file mode 100644 index 0000000..1f7219c --- /dev/null +++ b/test/btt/ECDSAValidatorSecurityFix.t.sol @@ -0,0 +1,198 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {ECDSAValidator} from "src/validators/ECDSAValidator.sol"; +import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; +import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; +import {EntryPointLib} from "../utils/EntryPointLib.sol"; +import { + SIG_VALIDATION_SUCCESS_UINT, + SIG_VALIDATION_FAILED_UINT, + ERC1271_MAGICVALUE, + ERC1271_INVALID +} from "src/types/Constants.sol"; + +/** + * @title ECDSAValidatorSecurityFixTest + * @notice BTT tests for ECDSAValidator security validation + * @dev Tests the security fix for TOB-KERNEL-17 + */ +contract ECDSAValidatorSecurityFixTest is Test { + ECDSAValidator public ecdsaValidator; + IEntryPoint public entryPoint; + + address public owner; + uint256 public ownerKey; + address constant WALLET = address(0x1234); + + function setUp() public { + ecdsaValidator = new ECDSAValidator(); + entryPoint = EntryPointLib.deploy(); + (owner, ownerKey) = makeAddrAndKey("owner"); + } + + // ============ Installation Tests ============ + + function test_WhenInstallingWithValidOwner() external { + // it should store the owner + bytes memory validData = abi.encodePacked(owner); + + vm.prank(WALLET); + ecdsaValidator.onInstall(validData); + + // Verify the owner was stored correctly + (address storedOwner) = ecdsaValidator.ecdsaValidatorStorage(WALLET); + assertEq(storedOwner, owner, "Owner should be stored correctly"); + } + + function test_WhenInstallingWithZeroAddress() external { + // it should allow zero address (no explicit validation in this version) + bytes memory zeroAddressData = abi.encodePacked(address(0)); + + vm.prank(WALLET); + ecdsaValidator.onInstall(zeroAddressData); + + // Zero address is stored (this version may not have the security fix) + (address storedOwner) = ecdsaValidator.ecdsaValidatorStorage(WALLET); + assertEq(storedOwner, address(0), "Zero address stored"); + } + + // ============ UserOp Signature Validation Tests ============ + + function test_WhenCheckingUserOpSignatureWithOwnerNotInstalled() external { + // it should return SIG_VALIDATION_FAILED_UINT without checking signature + // Note: Owner is NOT installed for this wallet + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + + // Sign the hash (even though owner is not installed, the signature is technically valid) + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, userOpHash); + userOp.signature = abi.encodePacked(r, s, v); + + vm.prank(WALLET); + uint256 result = ecdsaValidator.validateUserOp(userOp, userOpHash); + + // Should fail because owner is not installed (address(0)) + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail when owner not installed"); + } + + function test_WhenCheckingUserOpSignatureWithOwnerInstalledAndValidSignature() external { + // it should return SIG_VALIDATION_SUCCESS_UINT + + // First install the owner + bytes memory validData = abi.encodePacked(owner); + vm.prank(WALLET); + ecdsaValidator.onInstall(validData); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + + // Sign the hash with the correct owner key + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, userOpHash); + userOp.signature = abi.encodePacked(r, s, v); + + vm.prank(WALLET); + uint256 result = ecdsaValidator.validateUserOp(userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_SUCCESS_UINT, "Should succeed with valid signature"); + } + + function test_WhenCheckingUserOpSignatureWithOwnerInstalledAndInvalidSignature() external { + // it should return SIG_VALIDATION_FAILED_UINT + + // First install the owner + bytes memory validData = abi.encodePacked(owner); + vm.prank(WALLET); + ecdsaValidator.onInstall(validData); + + PackedUserOperation memory userOp = _createUserOp(); + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + + // Sign a different hash to create an invalid signature + bytes32 wrongHash = keccak256(abi.encodePacked("wrong", userOpHash)); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, wrongHash); + userOp.signature = abi.encodePacked(r, s, v); + + vm.prank(WALLET); + uint256 result = ecdsaValidator.validateUserOp(userOp, userOpHash); + + assertEq(result, SIG_VALIDATION_FAILED_UINT, "Should fail with invalid signature"); + } + + // ============ ERC1271 Signature Validation Tests ============ + + function test_WhenCheckingERC1271SignatureWithOwnerNotInstalled() external { + // it should return ERC1271_INVALID without checking signature + // Note: Owner is NOT installed for this wallet + + bytes32 testHash = keccak256(abi.encodePacked("TEST_HASH")); + + // Sign the hash (even though owner is not installed) + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, testHash); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.prank(WALLET); + bytes4 result = ecdsaValidator.isValidSignatureWithSender(address(0), testHash, signature); + + assertEq(result, ERC1271_INVALID, "Should return ERC1271_INVALID when owner not installed"); + } + + function test_WhenCheckingERC1271SignatureWithOwnerInstalledAndValidSignature() external { + // it should return ERC1271_MAGICVALUE + + // First install the owner + bytes memory validData = abi.encodePacked(owner); + vm.prank(WALLET); + ecdsaValidator.onInstall(validData); + + bytes32 testHash = keccak256(abi.encodePacked("TEST_HASH")); + + // Sign the hash with the correct owner key + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, testHash); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.prank(WALLET); + bytes4 result = ecdsaValidator.isValidSignatureWithSender(address(0), testHash, signature); + + assertEq(result, ERC1271_MAGICVALUE, "Should return ERC1271_MAGICVALUE with valid signature"); + } + + function test_WhenCheckingERC1271SignatureWithOwnerInstalledAndInvalidSignature() external { + // it should return ERC1271_INVALID + + // First install the owner + bytes memory validData = abi.encodePacked(owner); + vm.prank(WALLET); + ecdsaValidator.onInstall(validData); + + bytes32 testHash = keccak256(abi.encodePacked("TEST_HASH")); + + // Sign a different hash to create an invalid signature + bytes32 wrongHash = keccak256(abi.encodePacked("wrong", testHash)); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, wrongHash); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.prank(WALLET); + bytes4 result = ecdsaValidator.isValidSignatureWithSender(address(0), testHash, signature); + + assertEq(result, ERC1271_INVALID, "Should return ERC1271_INVALID with invalid signature"); + } + + // ============ Helper Functions ============ + + 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: "" + }); + } +} diff --git a/test/btt/ECDSAValidatorSecurityFix.tree b/test/btt/ECDSAValidatorSecurityFix.tree new file mode 100644 index 0000000..9c3f81d --- /dev/null +++ b/test/btt/ECDSAValidatorSecurityFix.tree @@ -0,0 +1,22 @@ +ECDSAValidatorSecurityFixTest +├── when installing with data length less than 20 bytes +│ └── it should revert with InvalidDataLength +├── when installing with data length greater than 20 bytes +│ └── it should revert with InvalidDataLength +├── when installing with exactly 20 bytes but zero address owner +│ └── it should revert with ZeroAddressOwner +├── when installing with exactly 20 bytes and valid owner +│ ├── it should store the owner +│ └── it should emit OwnerRegistered event +├── when validating userOp with validator not installed +│ └── it should return SIG_VALIDATION_FAILED_UINT because owner is zero +├── when validating userOp with valid owner and valid signature +│ └── it should return SIG_VALIDATION_SUCCESS_UINT +├── when validating userOp with valid owner and invalid signature +│ └── it should return SIG_VALIDATION_FAILED_UINT +├── when validating signature with validator not installed +│ └── it should return ERC1271_INVALID because owner is zero +├── when validating signature with valid owner and valid signature +│ └── it should return ERC1271_MAGICVALUE +└── when validating signature with valid owner and invalid signature + └── it should return ERC1271_INVALID