diff --git a/src/policies/TimelockPolicy.sol b/src/policies/TimelockPolicy.sol index ce86996..b68551f 100644 --- a/src/policies/TimelockPolicy.sol +++ b/src/policies/TimelockPolicy.sol @@ -10,10 +10,7 @@ import { MODULE_TYPE_POLICY, MODULE_TYPE_STATELESS_VALIDATOR, MODULE_TYPE_STATELESS_VALIDATOR_WITH_SENDER, - SIG_VALIDATION_SUCCESS_UINT, - SIG_VALIDATION_FAILED_UINT, - ERC1271_MAGICVALUE, - ERC1271_INVALID + SIG_VALIDATION_FAILED_UINT } from "src/types/Constants.sol"; /** @@ -334,7 +331,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW * @notice Check if executeUserOp call is a no-op * @dev Valid: executeUserOp("", bytes32) */ - function _isNoOpExecuteUserOp(bytes calldata callData) internal view returns (bool) { + function _isNoOpExecuteUserOp(bytes calldata callData) internal pure returns (bool) { // executeUserOp(bytes calldata userOp, bytes32 userOpHash) // Format: 4 (selector) + 32 (userOp offset) + 32 (userOpHash) + 32 (userOp length) + userOp data if (callData.length < 100) return false; @@ -368,37 +365,33 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW /** * @notice Check signature against timelock policy (for ERC-1271) - * @param id The policy ID - * @return validationData 0 if valid, 1 if invalid + * @dev TimelockPolicy does not support ERC-1271 signature validation - always reverts */ - function checkSignaturePolicy(bytes32 id, address, bytes32 hash, bytes calldata sig) + function checkSignaturePolicy(bytes32, address, bytes32, bytes calldata) external - view + pure override returns (uint256) { - bytes4 result = _validateSignaturePolicy(id, msg.sender, hash, sig); - return result == ERC1271_MAGICVALUE ? 0 : 1; + revert("TimelockPolicy: signature validation not supported"); } - function validateSignatureWithData(bytes32, bytes calldata, bytes calldata data) + function validateSignatureWithData(bytes32, bytes calldata, bytes calldata) external pure override(IStatelessValidator) returns (bool) { - (uint48 delay, uint48 expirationPeriod) = abi.decode(data, (uint48, uint48)); - return delay != 0 && expirationPeriod != 0; + revert("TimelockPolicy: stateless signature validation not supported"); } - function validateSignatureWithDataWithSender(address, bytes32, bytes calldata, bytes calldata data) + function validateSignatureWithDataWithSender(address, bytes32, bytes calldata, bytes calldata) external pure override(IStatelessValidatorWithSender) returns (bool) { - (uint48 delay, uint48 expirationPeriod) = abi.decode(data, (uint48, uint48)); - return delay != 0 && expirationPeriod != 0; + revert("TimelockPolicy: stateless signature validation not supported"); } // ==================== Internal Shared Logic ==================== @@ -425,23 +418,6 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW return _handleProposalExecutionInternal(id, userOp, account); } - /** - * @notice Internal function to validate signature policy - * @dev Shared logic for both installed and stateless validator modes - */ - function _validateSignaturePolicy(bytes32 id, address account, bytes32 hash, bytes calldata sig) - internal - view - returns (bytes4) - { - TimelockConfig storage config = timelockConfig[id][account]; - if (!config.initialized) return ERC1271_INVALID; - - // For signature validation, we're more permissive - // Timelock is primarily for userOp execution - return ERC1271_MAGICVALUE; - } - /** * @notice Get proposal details * @param account The account address diff --git a/test/TimelockPolicy.t.sol b/test/TimelockPolicy.t.sol index 55d7541..ef98680 100644 --- a/test/TimelockPolicy.t.sol +++ b/test/TimelockPolicy.t.sol @@ -107,21 +107,33 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State return statelessValidationSignature(bytes32(0), valid); } - // Override stateless validator tests to use proper data parameter + // Override stateless validator tests - TimelockPolicy reverts for stateless validation function testStatlessValidatorFail() external override { IStatelessValidator validatorModule = IStatelessValidator(address(module)); bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); (, bytes memory sig) = statelessValidationSignature(message, false); - // For TimelockPolicy, validation fails if delay or expirationPeriod is 0 - bytes memory invalidData = abi.encode(uint48(0), uint48(0)); + bytes memory data = abi.encode(uint48(0), uint48(0)); vm.startPrank(WALLET); - bool result = validatorModule.validateSignatureWithData(message, sig, invalidData); + vm.expectRevert("TimelockPolicy: stateless signature validation not supported"); + validatorModule.validateSignatureWithData(message, sig, data); vm.stopPrank(); + } + + function testStatelessValidatorSuccess() external override { + IStatelessValidator validatorModule = IStatelessValidator(address(module)); + + bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); + (, bytes memory sig) = statelessValidationSignature(message, true); + + bytes memory validData = abi.encode(delay, expirationPeriod); - assertFalse(result); + vm.startPrank(WALLET); + vm.expectRevert("TimelockPolicy: stateless signature validation not supported"); + validatorModule.validateSignatureWithData(message, sig, validData); + vm.stopPrank(); } function testStatelessValidatorWithSenderFail() external override { @@ -130,14 +142,26 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); (address caller, bytes memory sig) = statelessValidationSignatureWithSender(message, false); - // For TimelockPolicy, validation fails if delay or expirationPeriod is 0 - bytes memory invalidData = abi.encode(uint48(0), uint48(0)); + bytes memory data = abi.encode(uint48(0), uint48(0)); vm.startPrank(WALLET); - bool result = validatorModule.validateSignatureWithDataWithSender(caller, message, sig, invalidData); + vm.expectRevert("TimelockPolicy: stateless signature validation not supported"); + validatorModule.validateSignatureWithDataWithSender(caller, message, sig, data); vm.stopPrank(); + } + + function testStatelessValidatorWithSenderSuccess() external override { + IStatelessValidatorWithSender validatorModule = IStatelessValidatorWithSender(address(module)); - assertFalse(result); + bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); + (address caller, bytes memory sig) = statelessValidationSignatureWithSender(message, true); + + bytes memory validData = abi.encode(delay, expirationPeriod); + + vm.startPrank(WALLET); + vm.expectRevert("TimelockPolicy: stateless signature validation not supported"); + validatorModule.validateSignatureWithDataWithSender(caller, message, sig, validData); + vm.stopPrank(); } // Override the checkUserOpPolicy tests because TimelockPolicy has special behavior @@ -184,22 +208,32 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State assertEq(validationResult, 1); } - // Override signature policy test because TimelockPolicy always passes for installed accounts - function testPolicyCheckSignaturePolicyFail() public payable override { + // Override signature policy tests - TimelockPolicy reverts for signature validation + function testPolicyCheckSignaturePolicySuccess() public payable override { TimelockPolicy policyModule = TimelockPolicy(address(module)); + vm.startPrank(WALLET); + policyModule.onInstall(abi.encodePacked(policyId(), installData())); + vm.stopPrank(); - // Don't install for this wallet - address nonInstalledWallet = address(0xBEEF); + bytes32 testHash = keccak256(abi.encodePacked("TEST_HASH")); + (address sender, bytes memory sigData) = validSignatureData(testHash); + + vm.startPrank(WALLET); + vm.expectRevert("TimelockPolicy: signature validation not supported"); + policyModule.checkSignaturePolicy(policyId(), sender, testHash, sigData); + vm.stopPrank(); + } + + function testPolicyCheckSignaturePolicyFail() public payable override { + TimelockPolicy policyModule = TimelockPolicy(address(module)); bytes32 testHash = keccak256(abi.encodePacked("TEST_HASH")); (address sender, bytes memory sigData) = invalidSignatureData(testHash); - vm.startPrank(nonInstalledWallet); - uint256 result = policyModule.checkSignaturePolicy(policyId(), sender, testHash, sigData); + vm.startPrank(WALLET); + vm.expectRevert("TimelockPolicy: signature validation not supported"); + policyModule.checkSignaturePolicy(policyId(), sender, testHash, sigData); vm.stopPrank(); - - // Should fail for non-installed account - assertFalse(result == 0); } // Additional TimelockPolicy-specific tests diff --git a/test/base/PolicyTestBase.sol b/test/base/PolicyTestBase.sol index ca693a2..2daf0f7 100644 --- a/test/base/PolicyTestBase.sol +++ b/test/base/PolicyTestBase.sol @@ -98,7 +98,7 @@ abstract contract PolicyTestBase is ModuleTestBase { assertFalse(validationResult == 0); } - function testPolicyCheckSignaturePolicySuccess() public payable { + function testPolicyCheckSignaturePolicySuccess() public payable virtual { IPolicy policyModule = IPolicy(address(module)); vm.startPrank(WALLET); policyModule.onInstall(abi.encodePacked(policyId(), installData())); diff --git a/test/base/StatelessValidatorTestBase.sol b/test/base/StatelessValidatorTestBase.sol index 48d8239..08bf240 100644 --- a/test/base/StatelessValidatorTestBase.sol +++ b/test/base/StatelessValidatorTestBase.sol @@ -21,7 +21,7 @@ abstract contract StatelessValidatorTestBase is ModuleTestBase { assertTrue(result); } - function testStatelessValidatorSuccess() external { + function testStatelessValidatorSuccess() external virtual { IStatelessValidator validatorModule = IStatelessValidator(address(module)); bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); diff --git a/test/base/StatelessValidatorWithSenderTestBase.sol b/test/base/StatelessValidatorWithSenderTestBase.sol index 920b1b2..5a05801 100644 --- a/test/base/StatelessValidatorWithSenderTestBase.sol +++ b/test/base/StatelessValidatorWithSenderTestBase.sol @@ -21,7 +21,7 @@ abstract contract StatelessValidatorWithSenderTestBase is ModuleTestBase { assertTrue(result); } - function testStatelessValidatorWithSenderSuccess() external { + function testStatelessValidatorWithSenderSuccess() external virtual { IStatelessValidatorWithSender validatorModule = IStatelessValidatorWithSender(address(module)); bytes32 message = keccak256(abi.encodePacked("TEST_MESSAGE")); diff --git a/test/btt/TimelockSignaturePolicy.t.sol b/test/btt/TimelockSignaturePolicy.t.sol new file mode 100644 index 0000000..0df7676 --- /dev/null +++ b/test/btt/TimelockSignaturePolicy.t.sol @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {TimelockPolicy} from "src/policies/TimelockPolicy.sol"; + +/** + * @title TimelockSignaturePolicyTest + * @notice BTT tests for ERC-1271 signature validation with timelock + * @dev Tests that TimelockPolicy disables ERC-1271 signature validation (always reverts) + */ +contract TimelockSignaturePolicyTest is Test { + TimelockPolicy public timelockPolicy; + + address constant WALLET = address(0x1234); + + uint48 constant DELAY = 1 days; + uint48 constant EXPIRATION_PERIOD = 1 days; + + bytes32 public policyId; + bytes32 public testHash; + + function setUp() public { + timelockPolicy = new TimelockPolicy(); + policyId = keccak256(abi.encodePacked("POLICY_ID_1")); + testHash = keccak256(abi.encodePacked("TEST_HASH_TO_SIGN")); + } + + /// @notice Helper to install the policy for a wallet + function _installPolicy(address wallet) internal { + bytes memory installData = abi.encode(DELAY, EXPIRATION_PERIOD); + vm.prank(wallet); + timelockPolicy.onInstall(abi.encodePacked(policyId, installData)); + } + + // ============================================================ + // Test: checkSignaturePolicy always reverts + // ============================================================ + + function test_WhenCheckingSignaturePolicy() external { + // it should revert because signature validation is not supported + + // Install policy + _installPolicy(WALLET); + + // Try to validate a signature - should always revert + vm.prank(WALLET); + vm.expectRevert("TimelockPolicy: signature validation not supported"); + timelockPolicy.checkSignaturePolicy(policyId, address(0), testHash, ""); + } + + function test_WhenCheckingSignaturePolicyWithoutInstall() external { + // it should revert because signature validation is not supported + + // Do NOT install the policy + + // Try to validate a signature - should always revert + vm.prank(WALLET); + vm.expectRevert("TimelockPolicy: signature validation not supported"); + timelockPolicy.checkSignaturePolicy(policyId, address(0), testHash, ""); + } + + // ============================================================ + // Test: validateSignatureWithData always reverts + // ============================================================ + + function test_WhenValidatingSignatureWithData() external { + // it should revert because stateless signature validation is not supported + + bytes memory data = abi.encode(DELAY, EXPIRATION_PERIOD); + + vm.expectRevert("TimelockPolicy: stateless signature validation not supported"); + timelockPolicy.validateSignatureWithData(testHash, "", data); + } + + // ============================================================ + // Test: validateSignatureWithDataWithSender always reverts + // ============================================================ + + function test_WhenValidatingSignatureWithDataWithSender() external { + // it should revert because stateless signature validation is not supported + + bytes memory data = abi.encode(DELAY, EXPIRATION_PERIOD); + + vm.expectRevert("TimelockPolicy: stateless signature validation not supported"); + timelockPolicy.validateSignatureWithDataWithSender(WALLET, testHash, "", data); + } +} diff --git a/test/btt/TimelockSignaturePolicy.tree b/test/btt/TimelockSignaturePolicy.tree new file mode 100644 index 0000000..9a21572 --- /dev/null +++ b/test/btt/TimelockSignaturePolicy.tree @@ -0,0 +1,9 @@ +TimelockSignaturePolicyTest +├── when checking signature policy +│ └── it should revert because signature validation is not supported +├── when checking signature policy without install +│ └── it should revert because signature validation is not supported +├── when validating signature with data +│ └── it should revert because stateless signature validation is not supported +└── when validating signature with data with sender + └── it should revert because stateless signature validation is not supported