From 17f78f7874ed650b575e952c6fe5634aab668e2d Mon Sep 17 00:00:00 2001 From: taek Date: Mon, 9 Feb 2026 13:18:03 +0900 Subject: [PATCH 1/5] fix(TimelockPolicy): make permissionless proposals inert until session key approval --- src/policies/TimelockPolicy.sol | 78 +++++---- test/TimelockPolicy.t.sol | 57 +++++-- test/btt/Timelock.t.sol | 226 ++++++++++++++++++++----- test/btt/TimelockEpochValidation.t.sol | 66 ++++++-- 4 files changed, 329 insertions(+), 98 deletions(-) diff --git a/src/policies/TimelockPolicy.sol b/src/policies/TimelockPolicy.sol index f1340ca..7fdf64d 100644 --- a/src/policies/TimelockPolicy.sol +++ b/src/policies/TimelockPolicy.sol @@ -21,7 +21,8 @@ import { contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorWithSender { enum ProposalStatus { None, // Proposal doesn't exist - Pending, // Proposal created, waiting for timelock + Proposed, // Proposal created but not yet approved (inert, no clock) + Pending, // Proposal approved, clock started, waiting for timelock Executed, // Proposal executed Cancelled // Proposal cancelled } @@ -53,6 +54,10 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash, uint256 validAfter, uint256 validUntil ); + event ProposalApproved( + address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash, uint256 validAfter, uint256 validUntil + ); + event ProposalExecuted(address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash); event ProposalCancelled(address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash); @@ -119,7 +124,9 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW /** * @notice Create a proposal for time-delayed execution - * @dev Anyone can create a proposal - the timelock delay provides the security + * @dev Anyone can create a proposal. The proposal is inert until approved by the + * session key holder via a no-op UserOp. The timelock clock does not start + * until approval, so spam proposals can be safely ignored. * @param id The policy ID * @param account The account address * @param callData The calldata for the future operation @@ -129,10 +136,6 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW TimelockConfig storage config = timelockConfig[id][account]; if (!config.initialized) revert IModule.NotInitialized(account); - // Calculate proposal timing - uint48 validAfter = uint48(block.timestamp) + config.delay; - uint48 validUntil = validAfter + config.expirationPeriod; - // Create userOp key for storage lookup bytes32 userOpKey = keccak256(abi.encode(account, keccak256(callData), nonce)); @@ -141,11 +144,11 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW revert ProposalAlreadyExists(); } - // Create proposal (stored by userOpKey) with current epoch + // Create INERT proposal — clock does NOT start until approved via UserOp proposals[userOpKey][id][account] = - Proposal({status: ProposalStatus.Pending, validAfter: validAfter, validUntil: validUntil, epoch: currentEpoch[id][account]}); + Proposal({status: ProposalStatus.Proposed, validAfter: 0, validUntil: 0, epoch: currentEpoch[id][account]}); - emit ProposalCreated(account, id, userOpKey, validAfter, validUntil); + emit ProposalCreated(account, id, userOpKey, 0, 0); } /** @@ -167,7 +170,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW bytes32 userOpKey = keccak256(abi.encode(account, keccak256(callData), nonce)); Proposal storage proposal = proposals[userOpKey][id][account]; - if (proposal.status != ProposalStatus.Pending) { + if (proposal.status != ProposalStatus.Pending && proposal.status != ProposalStatus.Proposed) { revert ProposalNotPending(); } @@ -197,10 +200,13 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW } /** - * @notice Handle proposal creation from userOp - * @dev Signature format: [callDataLength(32)][callData][nonce(32)][remaining sig data] + * @notice Handle proposal approval (or create+approve) from userOp + * @dev Called when the session key holder submits a no-op UserOp. + * If a matching inert proposal exists (Proposed status), approves it and starts the clock. + * If no proposal exists, creates and approves in one step. + * Signature format: [callDataLength(32)][callData][nonce(32)][remaining sig data] */ - function _handleProposalCreationInternal( + function _handleProposalApprovalInternal( bytes32 id, PackedUserOperation calldata userOp, TimelockConfig storage config, @@ -217,27 +223,40 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW bytes calldata proposalCallData = sig[32:32 + callDataLength]; uint256 proposalNonce = uint256(bytes32(sig[32 + callDataLength:64 + callDataLength])); - // Calculate proposal timing + // Calculate proposal timing (clock starts NOW) uint48 validAfter = uint48(block.timestamp) + config.delay; uint48 validUntil = validAfter + config.expirationPeriod; // Create userOp key for storage lookup (using PROPOSAL calldata and nonce, not current userOp) bytes32 userOpKey = keccak256(abi.encode(userOp.sender, keccak256(proposalCallData), proposalNonce)); - // Check proposal doesn't already exist - if (proposals[userOpKey][id][account].status != ProposalStatus.None) { - return SIG_VALIDATION_FAILED_UINT; // Proposal already exists - } - - // Create proposal with current epoch - proposals[userOpKey][id][account] = - Proposal({status: ProposalStatus.Pending, validAfter: validAfter, validUntil: validUntil, epoch: currentEpoch[id][account]}); - - emit ProposalCreated(account, id, userOpKey, validAfter, validUntil); + Proposal storage proposal = proposals[userOpKey][id][account]; - // Return success (validationData = 0) to allow the proposal creation to persist - // EntryPoint treats validationData == 0 as valid (no time range check) - return _packValidationData(0, 0); + if (proposal.status == ProposalStatus.Proposed) { + // Approve existing inert proposal — start the clock + if (proposal.epoch != currentEpoch[id][account]) return SIG_VALIDATION_FAILED_UINT; + + proposal.status = ProposalStatus.Pending; + proposal.validAfter = validAfter; + proposal.validUntil = validUntil; + + emit ProposalApproved(account, id, userOpKey, validAfter, validUntil); + return _packValidationData(0, 0); + } else if (proposal.status == ProposalStatus.None) { + // Create + approve in one step (session key holder creating directly) + proposals[userOpKey][id][account] = Proposal({ + status: ProposalStatus.Pending, + validAfter: validAfter, + validUntil: validUntil, + epoch: currentEpoch[id][account] + }); + + emit ProposalCreated(account, id, userOpKey, validAfter, validUntil); + return _packValidationData(0, 0); + } else { + // Proposal exists in wrong state (Pending, Executed, Cancelled) + return SIG_VALIDATION_FAILED_UINT; + } } /** @@ -414,11 +433,10 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW TimelockConfig storage config = timelockConfig[id][account]; if (!config.initialized) return SIG_VALIDATION_FAILED_UINT; - // Check if this is a proposal creation request + // Check if this is a proposal approval (or create+approve) request // Criteria: calldata is a no-op AND signature has proposal data (length >= 65) if (_isNoOpCalldata(userOp.callData) && sig.length >= 65) { - // This is a proposal creation request - return _handleProposalCreationInternal(id, userOp, config, sig, account); + return _handleProposalApprovalInternal(id, userOp, config, sig, account); } // Otherwise, this is a proposal execution request diff --git a/test/TimelockPolicy.t.sol b/test/TimelockPolicy.t.sol index 2df90c2..71423c5 100644 --- a/test/TimelockPolicy.t.sol +++ b/test/TimelockPolicy.t.sol @@ -173,11 +173,30 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State PackedUserOperation memory userOp = validUserOp(); - // First create a proposal + // First create a proposal (inert) vm.startPrank(WALLET); policyModule.createProposal(policyId(), WALLET, userOp.callData, userOp.nonce); vm.stopPrank(); + // Approve the proposal via no-op UserOp (starts the clock) + bytes memory sig = abi.encodePacked( + bytes32(userOp.callData.length), userOp.callData, bytes32(userOp.nonce), bytes1(0x00) + ); + PackedUserOperation memory approveOp = 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: sig + }); + vm.startPrank(WALLET); + policyModule.checkUserOpPolicy(policyId(), approveOp); + vm.stopPrank(); + // Fast forward past the delay vm.warp(block.timestamp + delay + 1); @@ -251,13 +270,13 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State policyModule.createProposal(policyId(), WALLET, callData, nonce); vm.stopPrank(); - // Verify proposal was created + // Verify proposal was created as inert (Proposed) (TimelockPolicy.ProposalStatus status, uint256 validAfter, uint256 validUntil) = policyModule.getProposal(WALLET, callData, nonce, policyId(), WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending)); - assertEq(validAfter, block.timestamp + delay); - assertEq(validUntil, block.timestamp + delay + expirationPeriod); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed)); + assertEq(validAfter, 0); + assertEq(validUntil, 0); } function testCancelProposal() public { @@ -291,7 +310,7 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State policyModule.onInstall(abi.encodePacked(policyId(), installData())); vm.stopPrank(); - // Create a proposal via checkUserOpPolicy with no-op calldata + // Create+approve a proposal via checkUserOpPolicy with no-op calldata bytes memory proposalCallData = hex"1234"; uint256 proposalNonce = 1; @@ -318,11 +337,10 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State uint256 result = policyModule.checkUserOpPolicy(policyId(), userOp); vm.stopPrank(); - // Returns success (validationData = 0) - valid indefinitely per ERC-4337 - // This allows proposal creation via UserOp without external caller + // Returns success (validationData = 0) for state persistence assertEq(result, 0); - // Verify proposal was created + // Verify proposal was created+approved (Pending with timing) (TimelockPolicy.ProposalStatus status,,) = policyModule.getProposal(WALLET, proposalCallData, proposalNonce, policyId(), WALLET); @@ -338,11 +356,30 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State PackedUserOperation memory userOp = validUserOp(); - // Create a proposal + // Create and approve a proposal vm.startPrank(WALLET); policyModule.createProposal(policyId(), WALLET, userOp.callData, userOp.nonce); vm.stopPrank(); + // Approve via no-op UserOp + bytes memory sig = abi.encodePacked( + bytes32(userOp.callData.length), userOp.callData, bytes32(userOp.nonce), bytes1(0x00) + ); + PackedUserOperation memory approveOp = 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: sig + }); + vm.startPrank(WALLET); + policyModule.checkUserOpPolicy(policyId(), approveOp); + vm.stopPrank(); + // Fast forward past delay vm.warp(block.timestamp + delay + 1); diff --git a/test/btt/Timelock.t.sol b/test/btt/Timelock.t.sol index 04ec804..f48f842 100644 --- a/test/btt/Timelock.t.sol +++ b/test/btt/Timelock.t.sol @@ -199,28 +199,25 @@ contract TimelockTest is Test { } function test_GivenConfigIsInitializedAndProposalDoesNotExist() external whenCallingCreateProposal { - // it should store the proposal with pending status - // it should set correct validAfter and validUntil - // it should emit ProposalCreated + // it should store the proposal with Proposed (inert) status + // it should NOT set timing (validAfter = 0, validUntil = 0) + // it should emit ProposalCreated with zero timing bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 100; - uint256 createTime = block.timestamp; bytes32 expectedKey = keccak256(abi.encode(WALLET, keccak256(callData), nonce)); vm.expectEmit(true, true, true, true); - emit TimelockPolicy.ProposalCreated( - WALLET, POLICY_ID, expectedKey, uint48(createTime) + DELAY, uint48(createTime) + DELAY + EXPIRATION - ); + emit TimelockPolicy.ProposalCreated(WALLET, POLICY_ID, expectedKey, 0, 0); timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); (TimelockPolicy.ProposalStatus status, uint256 validAfter, uint256 validUntil) = timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Status should be Pending"); - assertEq(validAfter, createTime + DELAY, "validAfter should be timestamp + delay"); - assertEq(validUntil, createTime + DELAY + EXPIRATION, "validUntil should be validAfter + expiration"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Status should be Proposed"); + assertEq(validAfter, 0, "validAfter should be 0 (inert)"); + assertEq(validUntil, 0, "validUntil should be 0 (inert)"); } function test_GivenNotInitialized_WhenCallingCreateProposal() external whenCallingCreateProposal { @@ -249,8 +246,8 @@ contract TimelockTest is Test { _; } - function test_GivenCallerIsAccountAndProposalIsPending() external whenCallingCancelProposal { - // it should set status to cancelled + function test_GivenCallerIsAccountAndProposalIsProposed() external whenCallingCancelProposal { + // it should set status to cancelled (cancelling an inert Proposed proposal) // it should emit ProposalCancelled bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 300; @@ -270,6 +267,31 @@ contract TimelockTest is Test { assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Cancelled), "Status should be Cancelled"); } + function test_GivenCallerIsAccountAndProposalIsPending() external whenCallingCancelProposal { + // it should set status to cancelled (cancelling an approved Pending proposal) + // it should emit ProposalCancelled + bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); + uint256 nonce = 301; + + // Create and approve via UserOp to get Pending status + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory userOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, userOp); + + bytes32 expectedKey = keccak256(abi.encode(WALLET, keccak256(callData), nonce)); + + vm.expectEmit(true, true, true, true); + emit TimelockPolicy.ProposalCancelled(WALLET, POLICY_ID, expectedKey); + + vm.prank(WALLET); + timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); + + (TimelockPolicy.ProposalStatus status,,) = + timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Cancelled), "Status should be Cancelled"); + } + function test_GivenCallerIsNotAccount() external whenCallingCancelProposal { // it should revert with OnlyAccount bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); @@ -324,9 +346,9 @@ contract TimelockTest is Test { } function test_GivenNoopCalldataAndValidSignature() external whenCallingCheckUserOpPolicyToCreateProposal { - // it should create the proposal + // it should create+approve the proposal in one step (no prior external proposal) // it should return zero for state persistence - // it should emit ProposalCreated + // it should emit ProposalCreated with timing bytes memory proposalCallData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 proposalNonce = 700; bytes memory sig = _createProposalSignature(proposalCallData, proposalNonce); @@ -352,7 +374,7 @@ contract TimelockTest is Test { (TimelockPolicy.ProposalStatus status,,) = timelockPolicy.getProposal(WALLET, proposalCallData, proposalNonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should be created"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should be Pending (created+approved)"); } function test_GivenNoopCalldataAndSignatureShorterThan65Bytes() @@ -388,23 +410,23 @@ contract TimelockTest is Test { assertEq(result, SIG_VALIDATION_FAILED, "Should fail when signature claims more data than available"); } - function test_GivenNoopCalldataAndProposalAlreadyExists() external whenCallingCheckUserOpPolicyToCreateProposal { - // it should return SIG_VALIDATION_FAILED + function test_GivenNoopCalldataAndProposalAlreadyPending() external whenCallingCheckUserOpPolicyToCreateProposal { + // it should return SIG_VALIDATION_FAILED when proposal is already Pending bytes memory proposalCallData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 proposalNonce = 800; bytes memory sig = _createProposalSignature(proposalCallData, proposalNonce); PackedUserOperation memory userOp = _createNoopUserOp(WALLET, sig); - // First creation should succeed + // First call: create+approve → Pending vm.prank(WALLET); timelockPolicy.checkUserOpPolicy(POLICY_ID, userOp); - // Second creation should fail + // Second call: already Pending → should fail vm.prank(WALLET); uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, userOp); - assertEq(result, SIG_VALIDATION_FAILED, "Should fail for duplicate proposal"); + assertEq(result, SIG_VALIDATION_FAILED, "Should fail for already Pending proposal"); } // ============ checkUserOpPolicy - Proposal Execution Tests ============ @@ -420,10 +442,16 @@ contract TimelockTest is Test { bytes memory proposalCallData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "action"); uint256 proposalNonce = 900; - uint256 createTime = block.timestamp; + // Create inert proposal timelockPolicy.createProposal(POLICY_ID, WALLET, proposalCallData, proposalNonce); - // Get the actual stored proposal values + // Approve the proposal via no-op UserOp (starts the clock) + bytes memory sig = _createProposalSignature(proposalCallData, proposalNonce); + PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + + // Get the actual stored proposal values (now has timing) (, uint256 storedValidAfter, uint256 storedValidUntil) = timelockPolicy.getProposal(WALLET, proposalCallData, proposalNonce, POLICY_ID, WALLET); @@ -484,7 +512,11 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1100; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create and approve via UserOp + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); vm.warp(block.timestamp + DELAY + 1); @@ -616,15 +648,14 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1200; - uint256 createTime = block.timestamp; timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); (TimelockPolicy.ProposalStatus status, uint256 validAfter, uint256 validUntil) = timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Status should be Pending"); - assertEq(validAfter, createTime + DELAY, "validAfter should be correct"); - assertEq(validUntil, createTime + DELAY + EXPIRATION, "validUntil should be correct"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Status should be Proposed"); + assertEq(validAfter, 0, "validAfter should be 0 (inert)"); + assertEq(validUntil, 0, "validUntil should be 0 (inert)"); } function test_GivenProposalDoesNotExist_WhenCallingGetProposal() external whenCallingGetProposal { @@ -923,11 +954,15 @@ contract TimelockTest is Test { function test_WhenPackingValidationData() external { // it should correctly pack validAfter and validUntil - // Test the packing by creating a proposal and checking the returned validation data + // Test the packing by creating+approving a proposal and checking the returned validation data bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1400; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create and approve via UserOp + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); // Get the actual stored proposal values (, uint256 storedValidAfter, uint256 storedValidUntil) = @@ -964,24 +999,20 @@ contract TimelockTest is Test { assertEq(result, SIG_VALIDATION_FAILED, "Attack without proposal should fail"); } - function test_GivenAttackerTriesToExecuteImmediatelyAfterCreation() external whenTestingSecurityScenarios { - // it should return packed data with future validAfter + function test_GivenAttackerTriesToExecuteUnapprovedProposal() external whenTestingSecurityScenarios { + // it should return SIG_VALIDATION_FAILED because proposal is inert (Proposed, not Pending) bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "action"); uint256 nonce = 1500; - uint256 createTime = block.timestamp; timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - // Try to execute immediately (before timelock) + // Try to execute without approval PackedUserOperation memory executeOp = _createUserOpWithCalldata(WALLET, callData, nonce, ""); vm.prank(WALLET); uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, executeOp); - // Extract validAfter - it should be in the future - uint48 validAfter = uint48(result >> 208); - assertGt(validAfter, block.timestamp, "validAfter should be in the future"); - assertEq(validAfter, uint48(createTime) + DELAY, "validAfter should be createTime + DELAY"); + assertEq(result, SIG_VALIDATION_FAILED, "Unapproved proposal should fail execution"); } function test_GivenAttackerTriesToReexecuteAUsedProposal() external whenTestingSecurityScenarios { @@ -989,7 +1020,11 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "action"); uint256 nonce = 1600; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create and approve via UserOp + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); vm.warp(block.timestamp + DELAY + 1); @@ -1034,6 +1069,117 @@ contract TimelockTest is Test { assertGt(validUntil, validAfter, "validUntil should be after validAfter"); } + // ============ Proposal Approval Tests ============ + + modifier whenCallingCheckUserOpPolicyToApproveProposal() { + _; + } + + function test_ApproveExistingProposal() external whenCallingCheckUserOpPolicyToApproveProposal { + // it should transition Proposed → Pending and start the clock + bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "approve_test"); + uint256 nonce = 2000; + + // Create inert proposal externally + timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + + // Verify it's Proposed with no timing + (TimelockPolicy.ProposalStatus statusBefore, uint256 vaBefore, uint256 vuBefore) = + timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); + assertEq(uint256(statusBefore), uint256(TimelockPolicy.ProposalStatus.Proposed), "Should be Proposed"); + assertEq(vaBefore, 0, "validAfter should be 0 before approval"); + assertEq(vuBefore, 0, "validUntil should be 0 before approval"); + + // Approve via no-op UserOp + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + + bytes32 expectedKey = keccak256(abi.encode(WALLET, keccak256(callData), nonce)); + uint48 expectedValidAfter = uint48(block.timestamp) + DELAY; + uint48 expectedValidUntil = expectedValidAfter + EXPIRATION; + + vm.expectEmit(true, true, true, true); + emit TimelockPolicy.ProposalApproved(WALLET, POLICY_ID, expectedKey, expectedValidAfter, expectedValidUntil); + + vm.prank(WALLET); + uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + + assertEq(result, 0, "Approval should return 0 for state persistence"); + + // Verify it's now Pending with timing set + (TimelockPolicy.ProposalStatus statusAfter, uint256 vaAfter, uint256 vuAfter) = + timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); + assertEq(uint256(statusAfter), uint256(TimelockPolicy.ProposalStatus.Pending), "Should be Pending after approval"); + assertEq(vaAfter, expectedValidAfter, "validAfter should be set"); + assertEq(vuAfter, expectedValidUntil, "validUntil should be set"); + } + + function test_CannotApproveAlreadyPendingProposal() external whenCallingCheckUserOpPolicyToApproveProposal { + // it should return SIG_VALIDATION_FAILED + bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); + uint256 nonce = 2100; + + // Create and approve in one step + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + + // Try to approve again + vm.prank(WALLET); + uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + + assertEq(result, SIG_VALIDATION_FAILED, "Should fail for already Pending proposal"); + } + + function test_CannotApproveCancelledProposal() external whenCallingCheckUserOpPolicyToApproveProposal { + // it should return SIG_VALIDATION_FAILED + bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); + uint256 nonce = 2200; + + // Create and cancel + timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + vm.prank(WALLET); + timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); + + // Try to approve + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + + assertEq(result, SIG_VALIDATION_FAILED, "Should fail for cancelled proposal"); + } + + function test_SpamProposalsAreInert() external whenTestingSecurityScenarios { + // it should demonstrate that spam proposals cannot be executed without approval + bytes memory callData1 = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "approve_usdc"); + bytes memory callData2 = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "approve_weth"); + bytes memory callData3 = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "transfer_all"); + + // Attacker creates many proposals + vm.startPrank(ATTACKER); + timelockPolicy.createProposal(POLICY_ID, WALLET, callData1, 0); + timelockPolicy.createProposal(POLICY_ID, WALLET, callData2, 0); + timelockPolicy.createProposal(POLICY_ID, WALLET, callData3, 0); + vm.stopPrank(); + + // Wait past timelock + vm.warp(block.timestamp + DELAY + 1); + + // None can be executed because they're all in Proposed (inert) status + PackedUserOperation memory op1 = _createUserOpWithCalldata(WALLET, callData1, 0, ""); + PackedUserOperation memory op2 = _createUserOpWithCalldata(WALLET, callData2, 0, ""); + PackedUserOperation memory op3 = _createUserOpWithCalldata(WALLET, callData3, 0, ""); + + vm.prank(WALLET); + assertEq(timelockPolicy.checkUserOpPolicy(POLICY_ID, op1), SIG_VALIDATION_FAILED, "Spam proposal 1 should fail"); + vm.prank(WALLET); + assertEq(timelockPolicy.checkUserOpPolicy(POLICY_ID, op2), SIG_VALIDATION_FAILED, "Spam proposal 2 should fail"); + vm.prank(WALLET); + assertEq(timelockPolicy.checkUserOpPolicy(POLICY_ID, op3), SIG_VALIDATION_FAILED, "Spam proposal 3 should fail"); + } + function test_GivenAttackerTriesToCancelAnotherAccountsProposal() external whenTestingSecurityScenarios { // it should revert with OnlyAccount bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); @@ -1045,9 +1191,9 @@ contract TimelockTest is Test { vm.expectRevert(TimelockPolicy.OnlyAccount.selector); timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); - // Verify proposal is still pending + // Verify proposal is still Proposed (inert) (TimelockPolicy.ProposalStatus status,,) = timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should still be pending"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Proposal should still be Proposed"); } } diff --git a/test/btt/TimelockEpochValidation.t.sol b/test/btt/TimelockEpochValidation.t.sol index 6abe651..9e28484 100644 --- a/test/btt/TimelockEpochValidation.t.sol +++ b/test/btt/TimelockEpochValidation.t.sol @@ -66,6 +66,23 @@ contract TimelockEpochValidationTest is Test { }); } + function _approveProposal(address wallet, bytes32 policyId, bytes memory callData, uint256 nonce) internal { + bytes memory sig = abi.encodePacked(bytes32(callData.length), callData, bytes32(nonce), bytes1(0x00)); + PackedUserOperation memory noopOp = 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: sig + }); + vm.prank(wallet); + timelockPolicy.checkUserOpPolicy(policyId, noopOp); + } + // ==================== Installing the Policy ==================== modifier whenInstallingThePolicy() { @@ -150,7 +167,7 @@ contract TimelockEpochValidationTest is Test { _createProposal(WALLET, POLICY_ID_1, callData, nonce); // it should store the current epoch in the proposal - // it should use the epoch from currentEpoch mapping + // it should be in Proposed (inert) status with no timing bytes32 userOpKey = timelockPolicy.computeUserOpKey(WALLET, callData, nonce); ( TimelockPolicy.ProposalStatus status, @@ -159,10 +176,10 @@ contract TimelockEpochValidationTest is Test { uint256 proposalEpoch ) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should be pending"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Proposal should be Proposed (inert)"); assertEq(proposalEpoch, 1, "Proposal epoch should match current epoch (1)"); - assertEq(validAfter, block.timestamp + DELAY, "validAfter should be correct"); - assertEq(validUntil, block.timestamp + DELAY + EXPIRATION_PERIOD, "validUntil should be correct"); + assertEq(validAfter, 0, "validAfter should be 0 (inert)"); + assertEq(validUntil, 0, "validUntil should be 0 (inert)"); } function test_GivenCreatingViaCreateProposalFunction() external whenCreatingAProposal { @@ -171,15 +188,16 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"5678"; uint256 nonce = 42; - uint256 currentEpoch = timelockPolicy.currentEpoch(POLICY_ID_1, WALLET); + uint256 epoch = timelockPolicy.currentEpoch(POLICY_ID_1, WALLET); _createProposal(WALLET, POLICY_ID_1, callData, nonce); - // it should record the epoch at creation time + // it should record the epoch at creation time (proposal is inert) bytes32 userOpKey = timelockPolicy.computeUserOpKey(WALLET, callData, nonce); - (,,, uint256 proposalEpoch) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); + (TimelockPolicy.ProposalStatus status,,, uint256 proposalEpoch) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); - assertEq(proposalEpoch, currentEpoch, "Proposal epoch should equal current epoch at creation"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Should be Proposed"); + assertEq(proposalEpoch, epoch, "Proposal epoch should equal current epoch at creation"); } // ==================== Executing a Proposal ==================== @@ -204,6 +222,9 @@ contract TimelockEpochValidationTest is Test { _createProposal(WALLET, POLICY_ID_1, callData, nonce); + // Approve the proposal (starts the clock) + _approveProposal(WALLET, POLICY_ID_1, callData, nonce); + // Warp past the timelock delay vm.warp(block.timestamp + DELAY + 1); @@ -227,9 +248,12 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"1234"; uint256 nonce = 1; - // Create proposal in epoch 1 + // Create proposal in epoch 1 (inert) _createProposal(WALLET, POLICY_ID_1, callData, nonce); + // Approve the proposal (starts the clock, epoch 1) + _approveProposal(WALLET, POLICY_ID_1, callData, nonce); + // Warp past the timelock delay vm.warp(block.timestamp + DELAY + 1); @@ -246,10 +270,10 @@ contract TimelockEpochValidationTest is Test { // it should return SIG_VALIDATION_FAILED assertEq(validationResult, SIG_VALIDATION_FAILED_UINT, "Stale proposal should fail validation"); - // it should not mark proposal as executed + // it should not mark proposal as executed (still Pending from epoch 1) bytes32 userOpKey = timelockPolicy.computeUserOpKey(WALLET, callData, nonce); (TimelockPolicy.ProposalStatus status,,,) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should still be pending"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should still be Pending"); } function test_GivenTheProposalEpochDoesNotMatchCurrentEpoch() external whenExecutingAProposal { @@ -258,8 +282,9 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"abcd"; uint256 nonce = 5; - // Create proposal in epoch 1 + // Create and approve proposal in epoch 1 _createProposal(WALLET, POLICY_ID_1, callData, nonce); + _approveProposal(WALLET, POLICY_ID_1, callData, nonce); bytes32 userOpKey = timelockPolicy.computeUserOpKey(WALLET, callData, nonce); (,,, uint256 proposalEpoch) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); @@ -288,7 +313,7 @@ contract TimelockEpochValidationTest is Test { assertEq( uint256(statusAfter), uint256(TimelockPolicy.ProposalStatus.Pending), - "Proposal status should remain pending" + "Proposal status should remain Pending" ); } @@ -338,7 +363,9 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"1234"; uint256 nonce = 1; + // Create and approve in epoch 1 _createProposal(WALLET, POLICY_ID_1, callData, nonce); + _approveProposal(WALLET, POLICY_ID_1, callData, nonce); uint256 epochBeforeUninstall = timelockPolicy.currentEpoch(POLICY_ID_1, WALLET); @@ -359,7 +386,7 @@ contract TimelockEpochValidationTest is Test { uint256 validationResult = timelockPolicy.checkUserOpPolicy(POLICY_ID_1, userOp); assertEq(validationResult, SIG_VALIDATION_FAILED_UINT, "Old proposal should be invalid"); - // it should allow new proposals with new epoch + // it should allow new proposals with new epoch (Proposed status) bytes memory newCallData = hex"5678"; uint256 newNonce = 2; @@ -369,7 +396,7 @@ contract TimelockEpochValidationTest is Test { (TimelockPolicy.ProposalStatus status,,, uint256 newProposalEpoch) = timelockPolicy.proposals(newUserOpKey, POLICY_ID_1, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "New proposal should be created"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "New proposal should be Proposed"); assertEq(newProposalEpoch, epochAfterReinstall, "New proposal should have current epoch"); } @@ -468,10 +495,11 @@ contract TimelockEpochValidationTest is Test { _uninstallPolicy(WALLET, POLICY_ID_1); _installPolicy(WALLET, POLICY_ID_1); - // User creates legitimate proposal with new epoch + // User creates and approves legitimate proposal with new epoch bytes memory callData = hex"abcd"; uint256 userNonce = 200; _createProposal(WALLET, POLICY_ID_1, callData, userNonce); + _approveProposal(WALLET, POLICY_ID_1, callData, userNonce); vm.warp(block.timestamp + DELAY + 1); @@ -489,12 +517,14 @@ contract TimelockEpochValidationTest is Test { _installPolicy(WALLET, POLICY_ID_1); _installPolicy(WALLET2, POLICY_ID_1); - // Both users create proposals + // Both users create and approve proposals bytes memory callData1 = hex"1111"; bytes memory callData2 = hex"2222"; _createProposal(WALLET, POLICY_ID_1, callData1, 1); + _approveProposal(WALLET, POLICY_ID_1, callData1, 1); _createProposal(WALLET2, POLICY_ID_1, callData2, 1); + _approveProposal(WALLET2, POLICY_ID_1, callData2, 1); vm.warp(block.timestamp + DELAY + 1); @@ -506,7 +536,7 @@ contract TimelockEpochValidationTest is Test { assertEq(timelockPolicy.currentEpoch(POLICY_ID_1, WALLET), 2, "WALLET should be epoch 2"); assertEq(timelockPolicy.currentEpoch(POLICY_ID_1, WALLET2), 1, "WALLET2 should still be epoch 1"); - // WALLET's old proposal should fail + // WALLET's old proposal should fail (epoch mismatch) PackedUserOperation memory userOp1 = _createUserOp(WALLET, callData1, 1); vm.prank(WALLET); uint256 result1 = timelockPolicy.checkUserOpPolicy(POLICY_ID_1, userOp1); From 4b66879afc6f79b1c7c7c84934895dd2e737415a Mon Sep 17 00:00:00 2001 From: taek Date: Mon, 9 Feb 2026 13:19:49 +0900 Subject: [PATCH 2/5] feat(TimelockPolicy): add proposer to ProposalCreated event --- src/policies/TimelockPolicy.sol | 6 +++--- test/btt/Timelock.t.sol | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/policies/TimelockPolicy.sol b/src/policies/TimelockPolicy.sol index 7fdf64d..d261120 100644 --- a/src/policies/TimelockPolicy.sol +++ b/src/policies/TimelockPolicy.sol @@ -51,7 +51,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW mapping(bytes32 => mapping(bytes32 => mapping(address => Proposal))) public proposals; event ProposalCreated( - address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash, uint256 validAfter, uint256 validUntil + address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash, address proposer, uint256 validAfter, uint256 validUntil ); event ProposalApproved( @@ -148,7 +148,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW proposals[userOpKey][id][account] = Proposal({status: ProposalStatus.Proposed, validAfter: 0, validUntil: 0, epoch: currentEpoch[id][account]}); - emit ProposalCreated(account, id, userOpKey, 0, 0); + emit ProposalCreated(account, id, userOpKey, msg.sender, 0, 0); } /** @@ -251,7 +251,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW epoch: currentEpoch[id][account] }); - emit ProposalCreated(account, id, userOpKey, validAfter, validUntil); + emit ProposalCreated(account, id, userOpKey, account, validAfter, validUntil); return _packValidationData(0, 0); } else { // Proposal exists in wrong state (Pending, Executed, Cancelled) diff --git a/test/btt/Timelock.t.sol b/test/btt/Timelock.t.sol index f48f842..e52bcd1 100644 --- a/test/btt/Timelock.t.sol +++ b/test/btt/Timelock.t.sol @@ -208,7 +208,7 @@ contract TimelockTest is Test { bytes32 expectedKey = keccak256(abi.encode(WALLET, keccak256(callData), nonce)); vm.expectEmit(true, true, true, true); - emit TimelockPolicy.ProposalCreated(WALLET, POLICY_ID, expectedKey, 0, 0); + emit TimelockPolicy.ProposalCreated(WALLET, POLICY_ID, expectedKey, address(this), 0, 0); timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); @@ -362,6 +362,7 @@ contract TimelockTest is Test { WALLET, POLICY_ID, expectedKey, + WALLET, uint48(block.timestamp) + DELAY, uint48(block.timestamp) + DELAY + EXPIRATION ); From 17d950e221653a743bcf586a7cde5b145178b00b Mon Sep 17 00:00:00 2001 From: taek Date: Mon, 9 Feb 2026 23:24:57 +0900 Subject: [PATCH 3/5] fix(TimelockPolicy): remove createProposal, proposals only via no-op UserOp --- src/policies/TimelockPolicy.sol | 99 ++------- test/TimelockPolicy.t.sol | 79 ++++--- test/btt/Timelock.t.sol | 282 +++++-------------------- test/btt/TimelockEpochValidation.t.sol | 113 ++++------ 4 files changed, 167 insertions(+), 406 deletions(-) diff --git a/src/policies/TimelockPolicy.sol b/src/policies/TimelockPolicy.sol index d261120..4ef4b86 100644 --- a/src/policies/TimelockPolicy.sol +++ b/src/policies/TimelockPolicy.sol @@ -21,8 +21,7 @@ import { contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorWithSender { enum ProposalStatus { None, // Proposal doesn't exist - Proposed, // Proposal created but not yet approved (inert, no clock) - Pending, // Proposal approved, clock started, waiting for timelock + Pending, // Clock started, waiting for timelock Executed, // Proposal executed Cancelled // Proposal cancelled } @@ -51,10 +50,6 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW mapping(bytes32 => mapping(bytes32 => mapping(address => Proposal))) public proposals; event ProposalCreated( - address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash, address proposer, uint256 validAfter, uint256 validUntil - ); - - event ProposalApproved( address indexed wallet, bytes32 indexed id, bytes32 indexed proposalHash, uint256 validAfter, uint256 validUntil ); @@ -67,7 +62,6 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW error InvalidDelay(); error InvalidExpirationPeriod(); error ProposalNotFound(); - error ProposalAlreadyExists(); error TimelockNotExpired(uint256 validAfter, uint256 currentTime); error ProposalExpired(uint256 validUntil, uint256 currentTime); error ProposalNotPending(); @@ -122,35 +116,6 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW || moduleTypeId == MODULE_TYPE_STATELESS_VALIDATOR_WITH_SENDER; } - /** - * @notice Create a proposal for time-delayed execution - * @dev Anyone can create a proposal. The proposal is inert until approved by the - * session key holder via a no-op UserOp. The timelock clock does not start - * until approval, so spam proposals can be safely ignored. - * @param id The policy ID - * @param account The account address - * @param callData The calldata for the future operation - * @param nonce The nonce for the future operation - */ - function createProposal(bytes32 id, address account, bytes calldata callData, uint256 nonce) external { - TimelockConfig storage config = timelockConfig[id][account]; - if (!config.initialized) revert IModule.NotInitialized(account); - - // Create userOp key for storage lookup - bytes32 userOpKey = keccak256(abi.encode(account, keccak256(callData), nonce)); - - // Check proposal doesn't already exist - if (proposals[userOpKey][id][account].status != ProposalStatus.None) { - revert ProposalAlreadyExists(); - } - - // Create INERT proposal — clock does NOT start until approved via UserOp - proposals[userOpKey][id][account] = - Proposal({status: ProposalStatus.Proposed, validAfter: 0, validUntil: 0, epoch: currentEpoch[id][account]}); - - emit ProposalCreated(account, id, userOpKey, msg.sender, 0, 0); - } - /** * @notice Cancel a pending proposal * @dev Only the account itself can cancel proposals to prevent griefing @@ -170,7 +135,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW bytes32 userOpKey = keccak256(abi.encode(account, keccak256(callData), nonce)); Proposal storage proposal = proposals[userOpKey][id][account]; - if (proposal.status != ProposalStatus.Pending && proposal.status != ProposalStatus.Proposed) { + if (proposal.status != ProposalStatus.Pending) { revert ProposalNotPending(); } @@ -200,13 +165,12 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW } /** - * @notice Handle proposal approval (or create+approve) from userOp - * @dev Called when the session key holder submits a no-op UserOp. - * If a matching inert proposal exists (Proposed status), approves it and starts the clock. - * If no proposal exists, creates and approves in one step. + * @notice Handle proposal creation from a no-op UserOp + * @dev Called when the session key holder submits a no-op UserOp with proposal data in the signature. + * Creates a new Pending proposal with the timelock clock started. * Signature format: [callDataLength(32)][callData][nonce(32)][remaining sig data] */ - function _handleProposalApprovalInternal( + function _handleProposalCreationInternal( bytes32 id, PackedUserOperation calldata userOp, TimelockConfig storage config, @@ -223,40 +187,28 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW bytes calldata proposalCallData = sig[32:32 + callDataLength]; uint256 proposalNonce = uint256(bytes32(sig[32 + callDataLength:64 + callDataLength])); - // Calculate proposal timing (clock starts NOW) - uint48 validAfter = uint48(block.timestamp) + config.delay; - uint48 validUntil = validAfter + config.expirationPeriod; - // Create userOp key for storage lookup (using PROPOSAL calldata and nonce, not current userOp) bytes32 userOpKey = keccak256(abi.encode(userOp.sender, keccak256(proposalCallData), proposalNonce)); Proposal storage proposal = proposals[userOpKey][id][account]; - if (proposal.status == ProposalStatus.Proposed) { - // Approve existing inert proposal — start the clock - if (proposal.epoch != currentEpoch[id][account]) return SIG_VALIDATION_FAILED_UINT; - - proposal.status = ProposalStatus.Pending; - proposal.validAfter = validAfter; - proposal.validUntil = validUntil; - - emit ProposalApproved(account, id, userOpKey, validAfter, validUntil); - return _packValidationData(0, 0); - } else if (proposal.status == ProposalStatus.None) { - // Create + approve in one step (session key holder creating directly) - proposals[userOpKey][id][account] = Proposal({ - status: ProposalStatus.Pending, - validAfter: validAfter, - validUntil: validUntil, - epoch: currentEpoch[id][account] - }); - - emit ProposalCreated(account, id, userOpKey, account, validAfter, validUntil); - return _packValidationData(0, 0); - } else { - // Proposal exists in wrong state (Pending, Executed, Cancelled) + if (proposal.status != ProposalStatus.None) { return SIG_VALIDATION_FAILED_UINT; } + + // Calculate proposal timing (clock starts NOW) + uint48 validAfter = uint48(block.timestamp) + config.delay; + uint48 validUntil = validAfter + config.expirationPeriod; + + proposals[userOpKey][id][account] = Proposal({ + status: ProposalStatus.Pending, + validAfter: validAfter, + validUntil: validUntil, + epoch: currentEpoch[id][account] + }); + + emit ProposalCreated(account, id, userOpKey, validAfter, validUntil); + return _packValidationData(0, 0); } /** @@ -393,12 +345,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW * @notice Check signature against timelock policy (for ERC-1271) * @dev TimelockPolicy does not support ERC-1271 signature validation - always reverts */ - function checkSignaturePolicy(bytes32, address, bytes32, bytes calldata) - external - pure - override - returns (uint256) - { + function checkSignaturePolicy(bytes32, address, bytes32, bytes calldata) external pure override returns (uint256) { revert("TimelockPolicy: signature validation not supported"); } @@ -436,7 +383,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW // Check if this is a proposal approval (or create+approve) request // Criteria: calldata is a no-op AND signature has proposal data (length >= 65) if (_isNoOpCalldata(userOp.callData) && sig.length >= 65) { - return _handleProposalApprovalInternal(id, userOp, config, sig, account); + return _handleProposalCreationInternal(id, userOp, config, sig, account); } // Otherwise, this is a proposal execution request diff --git a/test/TimelockPolicy.t.sol b/test/TimelockPolicy.t.sol index 71423c5..951f6b9 100644 --- a/test/TimelockPolicy.t.sol +++ b/test/TimelockPolicy.t.sol @@ -173,16 +173,10 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State PackedUserOperation memory userOp = validUserOp(); - // First create a proposal (inert) - vm.startPrank(WALLET); - policyModule.createProposal(policyId(), WALLET, userOp.callData, userOp.nonce); - vm.stopPrank(); - - // Approve the proposal via no-op UserOp (starts the clock) - bytes memory sig = abi.encodePacked( - bytes32(userOp.callData.length), userOp.callData, bytes32(userOp.nonce), bytes1(0x00) - ); - PackedUserOperation memory approveOp = PackedUserOperation({ + // Create proposal via no-op UserOp (creates Pending directly with clock started) + bytes memory sig = + abi.encodePacked(bytes32(userOp.callData.length), userOp.callData, bytes32(userOp.nonce), bytes1(0x00)); + PackedUserOperation memory noopOp = PackedUserOperation({ sender: WALLET, nonce: 0, initCode: "", @@ -194,7 +188,7 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State signature: sig }); vm.startPrank(WALLET); - policyModule.checkUserOpPolicy(policyId(), approveOp); + policyModule.checkUserOpPolicy(policyId(), noopOp); vm.stopPrank(); // Fast forward past the delay @@ -266,17 +260,33 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State bytes memory callData = hex"1234"; uint256 nonce = 1; + // Create proposal via no-op UserOp + bytes memory sig = abi.encodePacked(bytes32(callData.length), callData, bytes32(nonce), bytes1(0x00)); + PackedUserOperation memory noopOp = 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: sig + }); + vm.startPrank(WALLET); - policyModule.createProposal(policyId(), WALLET, callData, nonce); + uint256 result = policyModule.checkUserOpPolicy(policyId(), noopOp); vm.stopPrank(); - // Verify proposal was created as inert (Proposed) + assertEq(result, 0); + + // Verify proposal was created as Pending with timing (TimelockPolicy.ProposalStatus status, uint256 validAfter, uint256 validUntil) = policyModule.getProposal(WALLET, callData, nonce, policyId(), WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed)); - assertEq(validAfter, 0); - assertEq(validUntil, 0); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending)); + assertGt(validAfter, 0); + assertGt(validUntil, validAfter); } function testCancelProposal() public { @@ -288,9 +298,22 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State bytes memory callData = hex"1234"; uint256 nonce = 1; - // Create proposal + // Create proposal via no-op UserOp + bytes memory sig = abi.encodePacked(bytes32(callData.length), callData, bytes32(nonce), bytes1(0x00)); + PackedUserOperation memory noopOp = 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: sig + }); + vm.startPrank(WALLET); - policyModule.createProposal(policyId(), WALLET, callData, nonce); + policyModule.checkUserOpPolicy(policyId(), noopOp); vm.stopPrank(); // Cancel proposal @@ -310,7 +333,7 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State policyModule.onInstall(abi.encodePacked(policyId(), installData())); vm.stopPrank(); - // Create+approve a proposal via checkUserOpPolicy with no-op calldata + // Create a proposal via checkUserOpPolicy with no-op calldata bytes memory proposalCallData = hex"1234"; uint256 proposalNonce = 1; @@ -340,7 +363,7 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State // Returns success (validationData = 0) for state persistence assertEq(result, 0); - // Verify proposal was created+approved (Pending with timing) + // Verify proposal was created as Pending with timing (TimelockPolicy.ProposalStatus status,,) = policyModule.getProposal(WALLET, proposalCallData, proposalNonce, policyId(), WALLET); @@ -356,16 +379,10 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State PackedUserOperation memory userOp = validUserOp(); - // Create and approve a proposal - vm.startPrank(WALLET); - policyModule.createProposal(policyId(), WALLET, userOp.callData, userOp.nonce); - vm.stopPrank(); - - // Approve via no-op UserOp - bytes memory sig = abi.encodePacked( - bytes32(userOp.callData.length), userOp.callData, bytes32(userOp.nonce), bytes1(0x00) - ); - PackedUserOperation memory approveOp = PackedUserOperation({ + // Create proposal via no-op UserOp (creates Pending directly) + bytes memory sig = + abi.encodePacked(bytes32(userOp.callData.length), userOp.callData, bytes32(userOp.nonce), bytes1(0x00)); + PackedUserOperation memory noopOp = PackedUserOperation({ sender: WALLET, nonce: 0, initCode: "", @@ -377,7 +394,7 @@ contract TimelockPolicyTest is PolicyTestBase, StatelessValidatorTestBase, State signature: sig }); vm.startPrank(WALLET); - policyModule.checkUserOpPolicy(policyId(), approveOp); + policyModule.checkUserOpPolicy(policyId(), noopOp); vm.stopPrank(); // Fast forward past delay diff --git a/test/btt/Timelock.t.sol b/test/btt/Timelock.t.sol index e52bcd1..739ab31 100644 --- a/test/btt/Timelock.t.sol +++ b/test/btt/Timelock.t.sol @@ -192,88 +192,19 @@ contract TimelockTest is Test { assertFalse(timelockPolicy.isModuleType(999), "Should not support invalid type"); } - // ============ createProposal Tests ============ - - modifier whenCallingCreateProposal() { - _; - } - - function test_GivenConfigIsInitializedAndProposalDoesNotExist() external whenCallingCreateProposal { - // it should store the proposal with Proposed (inert) status - // it should NOT set timing (validAfter = 0, validUntil = 0) - // it should emit ProposalCreated with zero timing - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); - uint256 nonce = 100; - - bytes32 expectedKey = keccak256(abi.encode(WALLET, keccak256(callData), nonce)); - - vm.expectEmit(true, true, true, true); - emit TimelockPolicy.ProposalCreated(WALLET, POLICY_ID, expectedKey, address(this), 0, 0); - - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - - (TimelockPolicy.ProposalStatus status, uint256 validAfter, uint256 validUntil) = - timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Status should be Proposed"); - assertEq(validAfter, 0, "validAfter should be 0 (inert)"); - assertEq(validUntil, 0, "validUntil should be 0 (inert)"); - } - - function test_GivenNotInitialized_WhenCallingCreateProposal() external whenCallingCreateProposal { - // it should revert with NotInitialized - address uninitWallet = address(0x9999); - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); - - vm.expectRevert(abi.encodeWithSelector(IModule.NotInitialized.selector, uninitWallet)); - timelockPolicy.createProposal(POLICY_ID, uninitWallet, callData, 0); - } - - function test_GivenProposalAlreadyExists() external whenCallingCreateProposal { - // it should revert with ProposalAlreadyExists - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); - uint256 nonce = 200; - - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - - vm.expectRevert(TimelockPolicy.ProposalAlreadyExists.selector); - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - } - // ============ cancelProposal Tests ============ modifier whenCallingCancelProposal() { _; } - function test_GivenCallerIsAccountAndProposalIsProposed() external whenCallingCancelProposal { - // it should set status to cancelled (cancelling an inert Proposed proposal) - // it should emit ProposalCancelled - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); - uint256 nonce = 300; - - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - - bytes32 expectedKey = keccak256(abi.encode(WALLET, keccak256(callData), nonce)); - - vm.expectEmit(true, true, true, true); - emit TimelockPolicy.ProposalCancelled(WALLET, POLICY_ID, expectedKey); - - vm.prank(WALLET); - timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); - - (TimelockPolicy.ProposalStatus status,,) = - timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Cancelled), "Status should be Cancelled"); - } - function test_GivenCallerIsAccountAndProposalIsPending() external whenCallingCancelProposal { - // it should set status to cancelled (cancelling an approved Pending proposal) + // it should set status to cancelled (cancelling a Pending proposal) // it should emit ProposalCancelled bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 301; - // Create and approve via UserOp to get Pending status + // Create proposal via no-op UserOp to get Pending status bytes memory sig = _createProposalSignature(callData, nonce); PackedUserOperation memory userOp = _createNoopUserOp(WALLET, sig); vm.prank(WALLET); @@ -297,7 +228,11 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 400; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create proposal via no-op UserOp + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); vm.prank(ATTACKER); vm.expectRevert(TimelockPolicy.OnlyAccount.selector); @@ -329,7 +264,11 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 600; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create proposal via no-op UserOp + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); vm.prank(WALLET); timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); @@ -346,7 +285,7 @@ contract TimelockTest is Test { } function test_GivenNoopCalldataAndValidSignature() external whenCallingCheckUserOpPolicyToCreateProposal { - // it should create+approve the proposal in one step (no prior external proposal) + // it should create the proposal as Pending with clock started // it should return zero for state persistence // it should emit ProposalCreated with timing bytes memory proposalCallData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); @@ -362,7 +301,6 @@ contract TimelockTest is Test { WALLET, POLICY_ID, expectedKey, - WALLET, uint48(block.timestamp) + DELAY, uint48(block.timestamp) + DELAY + EXPIRATION ); @@ -375,7 +313,7 @@ contract TimelockTest is Test { (TimelockPolicy.ProposalStatus status,,) = timelockPolicy.getProposal(WALLET, proposalCallData, proposalNonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should be Pending (created+approved)"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should be Pending"); } function test_GivenNoopCalldataAndSignatureShorterThan65Bytes() @@ -419,11 +357,11 @@ contract TimelockTest is Test { PackedUserOperation memory userOp = _createNoopUserOp(WALLET, sig); - // First call: create+approve → Pending + // First call: create -> Pending vm.prank(WALLET); timelockPolicy.checkUserOpPolicy(POLICY_ID, userOp); - // Second call: already Pending → should fail + // Second call: already Pending -> should fail vm.prank(WALLET); uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, userOp); @@ -443,14 +381,11 @@ contract TimelockTest is Test { bytes memory proposalCallData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "action"); uint256 proposalNonce = 900; - // Create inert proposal - timelockPolicy.createProposal(POLICY_ID, WALLET, proposalCallData, proposalNonce); - - // Approve the proposal via no-op UserOp (starts the clock) + // Create proposal via no-op UserOp (creates Pending directly) bytes memory sig = _createProposalSignature(proposalCallData, proposalNonce); - PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); vm.prank(WALLET); - timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); // Get the actual stored proposal values (now has timing) (, uint256 storedValidAfter, uint256 storedValidUntil) = @@ -495,7 +430,11 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1000; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create proposal via no-op UserOp + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); vm.prank(WALLET); timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); @@ -513,11 +452,11 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1100; - // Create and approve via UserOp + // Create proposal via no-op UserOp bytes memory sig = _createProposalSignature(callData, nonce); - PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); vm.prank(WALLET); - timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); vm.warp(block.timestamp + DELAY + 1); @@ -649,14 +588,22 @@ contract TimelockTest is Test { bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1200; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create proposal via no-op UserOp (creates Pending with timing) + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); (TimelockPolicy.ProposalStatus status, uint256 validAfter, uint256 validUntil) = timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Status should be Proposed"); - assertEq(validAfter, 0, "validAfter should be 0 (inert)"); - assertEq(validUntil, 0, "validUntil should be 0 (inert)"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Status should be Pending"); + assertEq(validAfter, uint256(uint48(block.timestamp) + DELAY), "validAfter should be block.timestamp + DELAY"); + assertEq( + validUntil, + uint256(uint48(block.timestamp) + DELAY + EXPIRATION), + "validUntil should be validAfter + EXPIRATION" + ); } function test_GivenProposalDoesNotExist_WhenCallingGetProposal() external whenCallingGetProposal { @@ -955,15 +902,15 @@ contract TimelockTest is Test { function test_WhenPackingValidationData() external { // it should correctly pack validAfter and validUntil - // Test the packing by creating+approving a proposal and checking the returned validation data + // Test the packing by creating a proposal and checking the returned validation data bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1400; - // Create and approve via UserOp + // Create proposal via no-op UserOp bytes memory sig = _createProposalSignature(callData, nonce); - PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); vm.prank(WALLET); - timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); // Get the actual stored proposal values (, uint256 storedValidAfter, uint256 storedValidUntil) = @@ -1000,32 +947,16 @@ contract TimelockTest is Test { assertEq(result, SIG_VALIDATION_FAILED, "Attack without proposal should fail"); } - function test_GivenAttackerTriesToExecuteUnapprovedProposal() external whenTestingSecurityScenarios { - // it should return SIG_VALIDATION_FAILED because proposal is inert (Proposed, not Pending) - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "action"); - uint256 nonce = 1500; - - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - - // Try to execute without approval - PackedUserOperation memory executeOp = _createUserOpWithCalldata(WALLET, callData, nonce, ""); - - vm.prank(WALLET); - uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, executeOp); - - assertEq(result, SIG_VALIDATION_FAILED, "Unapproved proposal should fail execution"); - } - function test_GivenAttackerTriesToReexecuteAUsedProposal() external whenTestingSecurityScenarios { // it should return SIG_VALIDATION_FAILED bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "action"); uint256 nonce = 1600; - // Create and approve via UserOp + // Create proposal via no-op UserOp bytes memory sig = _createProposalSignature(callData, nonce); - PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); vm.prank(WALLET); - timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); vm.warp(block.timestamp + DELAY + 1); @@ -1070,131 +1001,24 @@ contract TimelockTest is Test { assertGt(validUntil, validAfter, "validUntil should be after validAfter"); } - // ============ Proposal Approval Tests ============ - - modifier whenCallingCheckUserOpPolicyToApproveProposal() { - _; - } - - function test_ApproveExistingProposal() external whenCallingCheckUserOpPolicyToApproveProposal { - // it should transition Proposed → Pending and start the clock - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "approve_test"); - uint256 nonce = 2000; - - // Create inert proposal externally - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - - // Verify it's Proposed with no timing - (TimelockPolicy.ProposalStatus statusBefore, uint256 vaBefore, uint256 vuBefore) = - timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(statusBefore), uint256(TimelockPolicy.ProposalStatus.Proposed), "Should be Proposed"); - assertEq(vaBefore, 0, "validAfter should be 0 before approval"); - assertEq(vuBefore, 0, "validUntil should be 0 before approval"); - - // Approve via no-op UserOp - bytes memory sig = _createProposalSignature(callData, nonce); - PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); - - bytes32 expectedKey = keccak256(abi.encode(WALLET, keccak256(callData), nonce)); - uint48 expectedValidAfter = uint48(block.timestamp) + DELAY; - uint48 expectedValidUntil = expectedValidAfter + EXPIRATION; - - vm.expectEmit(true, true, true, true); - emit TimelockPolicy.ProposalApproved(WALLET, POLICY_ID, expectedKey, expectedValidAfter, expectedValidUntil); - - vm.prank(WALLET); - uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); - - assertEq(result, 0, "Approval should return 0 for state persistence"); - - // Verify it's now Pending with timing set - (TimelockPolicy.ProposalStatus statusAfter, uint256 vaAfter, uint256 vuAfter) = - timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(statusAfter), uint256(TimelockPolicy.ProposalStatus.Pending), "Should be Pending after approval"); - assertEq(vaAfter, expectedValidAfter, "validAfter should be set"); - assertEq(vuAfter, expectedValidUntil, "validUntil should be set"); - } - - function test_CannotApproveAlreadyPendingProposal() external whenCallingCheckUserOpPolicyToApproveProposal { - // it should return SIG_VALIDATION_FAILED - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); - uint256 nonce = 2100; - - // Create and approve in one step - bytes memory sig = _createProposalSignature(callData, nonce); - PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); - vm.prank(WALLET); - timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); - - // Try to approve again - vm.prank(WALLET); - uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); - - assertEq(result, SIG_VALIDATION_FAILED, "Should fail for already Pending proposal"); - } - - function test_CannotApproveCancelledProposal() external whenCallingCheckUserOpPolicyToApproveProposal { - // it should return SIG_VALIDATION_FAILED - bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); - uint256 nonce = 2200; - - // Create and cancel - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); - vm.prank(WALLET); - timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); - - // Try to approve - bytes memory sig = _createProposalSignature(callData, nonce); - PackedUserOperation memory approveOp = _createNoopUserOp(WALLET, sig); - vm.prank(WALLET); - uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, approveOp); - - assertEq(result, SIG_VALIDATION_FAILED, "Should fail for cancelled proposal"); - } - - function test_SpamProposalsAreInert() external whenTestingSecurityScenarios { - // it should demonstrate that spam proposals cannot be executed without approval - bytes memory callData1 = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "approve_usdc"); - bytes memory callData2 = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "approve_weth"); - bytes memory callData3 = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "transfer_all"); - - // Attacker creates many proposals - vm.startPrank(ATTACKER); - timelockPolicy.createProposal(POLICY_ID, WALLET, callData1, 0); - timelockPolicy.createProposal(POLICY_ID, WALLET, callData2, 0); - timelockPolicy.createProposal(POLICY_ID, WALLET, callData3, 0); - vm.stopPrank(); - - // Wait past timelock - vm.warp(block.timestamp + DELAY + 1); - - // None can be executed because they're all in Proposed (inert) status - PackedUserOperation memory op1 = _createUserOpWithCalldata(WALLET, callData1, 0, ""); - PackedUserOperation memory op2 = _createUserOpWithCalldata(WALLET, callData2, 0, ""); - PackedUserOperation memory op3 = _createUserOpWithCalldata(WALLET, callData3, 0, ""); - - vm.prank(WALLET); - assertEq(timelockPolicy.checkUserOpPolicy(POLICY_ID, op1), SIG_VALIDATION_FAILED, "Spam proposal 1 should fail"); - vm.prank(WALLET); - assertEq(timelockPolicy.checkUserOpPolicy(POLICY_ID, op2), SIG_VALIDATION_FAILED, "Spam proposal 2 should fail"); - vm.prank(WALLET); - assertEq(timelockPolicy.checkUserOpPolicy(POLICY_ID, op3), SIG_VALIDATION_FAILED, "Spam proposal 3 should fail"); - } - function test_GivenAttackerTriesToCancelAnotherAccountsProposal() external whenTestingSecurityScenarios { // it should revert with OnlyAccount bytes memory callData = abi.encodeWithSelector(IERC7579Execution.execute.selector, bytes32(0), "test"); uint256 nonce = 1800; - timelockPolicy.createProposal(POLICY_ID, WALLET, callData, nonce); + // Create proposal via no-op UserOp from WALLET + bytes memory sig = _createProposalSignature(callData, nonce); + PackedUserOperation memory noopOp = _createNoopUserOp(WALLET, sig); + vm.prank(WALLET); + timelockPolicy.checkUserOpPolicy(POLICY_ID, noopOp); vm.prank(ATTACKER); vm.expectRevert(TimelockPolicy.OnlyAccount.selector); timelockPolicy.cancelProposal(POLICY_ID, WALLET, callData, nonce); - // Verify proposal is still Proposed (inert) + // Verify proposal is still Pending (TimelockPolicy.ProposalStatus status,,) = timelockPolicy.getProposal(WALLET, callData, nonce, POLICY_ID, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Proposal should still be Proposed"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should still be Pending"); } } diff --git a/test/btt/TimelockEpochValidation.t.sol b/test/btt/TimelockEpochValidation.t.sol index 9e28484..92a049a 100644 --- a/test/btt/TimelockEpochValidation.t.sol +++ b/test/btt/TimelockEpochValidation.t.sol @@ -44,8 +44,21 @@ contract TimelockEpochValidationTest is Test { } function _createProposal(address wallet, bytes32 policyId, bytes memory callData, uint256 nonce) internal { + // Create proposal via no-op UserOp (creates Pending directly with clock started) + bytes memory sig = abi.encodePacked(bytes32(callData.length), callData, bytes32(nonce), bytes1(0x00)); + PackedUserOperation memory noopOp = 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: sig + }); vm.prank(wallet); - timelockPolicy.createProposal(policyId, wallet, callData, nonce); + timelockPolicy.checkUserOpPolicy(policyId, noopOp); } function _createUserOp(address sender, bytes memory callData, uint256 nonce) @@ -66,23 +79,6 @@ contract TimelockEpochValidationTest is Test { }); } - function _approveProposal(address wallet, bytes32 policyId, bytes memory callData, uint256 nonce) internal { - bytes memory sig = abi.encodePacked(bytes32(callData.length), callData, bytes32(nonce), bytes1(0x00)); - PackedUserOperation memory noopOp = 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: sig - }); - vm.prank(wallet); - timelockPolicy.checkUserOpPolicy(policyId, noopOp); - } - // ==================== Installing the Policy ==================== modifier whenInstallingThePolicy() { @@ -102,8 +98,7 @@ contract TimelockEpochValidationTest is Test { assertEq(epochAfter, 1, "Epoch should be 1 after first install"); // it should initialize the policy config - (uint48 delay, uint48 expirationPeriod, bool initialized) = - timelockPolicy.timelockConfig(POLICY_ID_1, WALLET); + (uint48 delay, uint48 expirationPeriod, bool initialized) = timelockPolicy.timelockConfig(POLICY_ID_1, WALLET); assertTrue(initialized, "Policy should be initialized"); assertEq(delay, DELAY, "Delay should match"); assertEq(expirationPeriod, EXPIRATION_PERIOD, "Expiration period should match"); @@ -167,22 +162,18 @@ contract TimelockEpochValidationTest is Test { _createProposal(WALLET, POLICY_ID_1, callData, nonce); // it should store the current epoch in the proposal - // it should be in Proposed (inert) status with no timing + // it should be in Pending status with timing set bytes32 userOpKey = timelockPolicy.computeUserOpKey(WALLET, callData, nonce); - ( - TimelockPolicy.ProposalStatus status, - uint48 validAfter, - uint48 validUntil, - uint256 proposalEpoch - ) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); - - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Proposal should be Proposed (inert)"); + (TimelockPolicy.ProposalStatus status, uint48 validAfter, uint48 validUntil, uint256 proposalEpoch) = + timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); + + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Proposal should be Pending"); assertEq(proposalEpoch, 1, "Proposal epoch should match current epoch (1)"); - assertEq(validAfter, 0, "validAfter should be 0 (inert)"); - assertEq(validUntil, 0, "validUntil should be 0 (inert)"); + assertGt(validAfter, 0, "validAfter should be set"); + assertGt(validUntil, validAfter, "validUntil should be after validAfter"); } - function test_GivenCreatingViaCreateProposalFunction() external whenCreatingAProposal { + function test_GivenCreatingViaNoOpUserOp() external whenCreatingAProposal { _installPolicy(WALLET, POLICY_ID_1); bytes memory callData = hex"5678"; @@ -192,11 +183,12 @@ contract TimelockEpochValidationTest is Test { _createProposal(WALLET, POLICY_ID_1, callData, nonce); - // it should record the epoch at creation time (proposal is inert) + // it should record the epoch at creation time (proposal is Pending) bytes32 userOpKey = timelockPolicy.computeUserOpKey(WALLET, callData, nonce); - (TimelockPolicy.ProposalStatus status,,, uint256 proposalEpoch) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); + (TimelockPolicy.ProposalStatus status,,, uint256 proposalEpoch) = + timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "Should be Proposed"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "Should be Pending"); assertEq(proposalEpoch, epoch, "Proposal epoch should equal current epoch at creation"); } @@ -220,11 +212,9 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"1234"; uint256 nonce = 1; + // Create proposal via no-op UserOp (creates Pending directly with clock started) _createProposal(WALLET, POLICY_ID_1, callData, nonce); - // Approve the proposal (starts the clock) - _approveProposal(WALLET, POLICY_ID_1, callData, nonce); - // Warp past the timelock delay vm.warp(block.timestamp + DELAY + 1); @@ -248,12 +238,9 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"1234"; uint256 nonce = 1; - // Create proposal in epoch 1 (inert) + // Create proposal via no-op UserOp (Pending with clock started, epoch 1) _createProposal(WALLET, POLICY_ID_1, callData, nonce); - // Approve the proposal (starts the clock, epoch 1) - _approveProposal(WALLET, POLICY_ID_1, callData, nonce); - // Warp past the timelock delay vm.warp(block.timestamp + DELAY + 1); @@ -282,9 +269,8 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"abcd"; uint256 nonce = 5; - // Create and approve proposal in epoch 1 + // Create proposal via no-op UserOp in epoch 1 (Pending with clock started) _createProposal(WALLET, POLICY_ID_1, callData, nonce); - _approveProposal(WALLET, POLICY_ID_1, callData, nonce); bytes32 userOpKey = timelockPolicy.computeUserOpKey(WALLET, callData, nonce); (,,, uint256 proposalEpoch) = timelockPolicy.proposals(userOpKey, POLICY_ID_1, WALLET); @@ -363,9 +349,8 @@ contract TimelockEpochValidationTest is Test { bytes memory callData = hex"1234"; uint256 nonce = 1; - // Create and approve in epoch 1 + // Create proposal via no-op UserOp in epoch 1 (Pending with clock started) _createProposal(WALLET, POLICY_ID_1, callData, nonce); - _approveProposal(WALLET, POLICY_ID_1, callData, nonce); uint256 epochBeforeUninstall = timelockPolicy.currentEpoch(POLICY_ID_1, WALLET); @@ -386,7 +371,7 @@ contract TimelockEpochValidationTest is Test { uint256 validationResult = timelockPolicy.checkUserOpPolicy(POLICY_ID_1, userOp); assertEq(validationResult, SIG_VALIDATION_FAILED_UINT, "Old proposal should be invalid"); - // it should allow new proposals with new epoch (Proposed status) + // it should allow new proposals with new epoch (Pending status) bytes memory newCallData = hex"5678"; uint256 newNonce = 2; @@ -396,7 +381,7 @@ contract TimelockEpochValidationTest is Test { (TimelockPolicy.ProposalStatus status,,, uint256 newProposalEpoch) = timelockPolicy.proposals(newUserOpKey, POLICY_ID_1, WALLET); - assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Proposed), "New proposal should be Proposed"); + assertEq(uint256(status), uint256(TimelockPolicy.ProposalStatus.Pending), "New proposal should be Pending"); assertEq(newProposalEpoch, epochAfterReinstall, "New proposal should have current epoch"); } @@ -456,35 +441,26 @@ contract TimelockEpochValidationTest is Test { _; } - function test_WhenAttackerTriesToExecuteOldProposal() external whenVerifyingFullAttackScenario { + function test_WhenAttackerCannotCreateProposalsDirectly() external whenVerifyingFullAttackScenario { // User installs policy _installPolicy(WALLET, POLICY_ID_1); assertEq(timelockPolicy.currentEpoch(POLICY_ID_1, WALLET), 1, "Should start at epoch 1"); - // Attacker creates malicious proposal + // Without createProposal, an attacker cannot create proposals for WALLET. + // The only way to create a proposal is via checkUserOpPolicy which requires + // msg.sender == WALLET (the account itself). So there is no external attack vector. + + // Verify that trying to execute a non-existent proposal fails bytes memory maliciousCallData = hex"deadbeef"; uint256 maliciousNonce = 666; - vm.prank(ATTACKER); - timelockPolicy.createProposal(POLICY_ID_1, WALLET, maliciousCallData, maliciousNonce); - - // Time passes, timelock expires - vm.warp(block.timestamp + DELAY + 1); - - // User decides to uninstall and reinstall - _uninstallPolicy(WALLET, POLICY_ID_1); - _installPolicy(WALLET, POLICY_ID_1); - - assertEq(timelockPolicy.currentEpoch(POLICY_ID_1, WALLET), 2, "Should be at epoch 2"); - - // Attacker tries to execute the old proposal PackedUserOperation memory maliciousUserOp = _createUserOp(WALLET, maliciousCallData, maliciousNonce); vm.prank(WALLET); uint256 validationResult = timelockPolicy.checkUserOpPolicy(POLICY_ID_1, maliciousUserOp); - // it should fail due to epoch mismatch - assertEq(validationResult, SIG_VALIDATION_FAILED_UINT, "Attack should be thwarted by epoch check"); + // it should fail because no proposal exists + assertEq(validationResult, SIG_VALIDATION_FAILED_UINT, "Non-existent proposal should fail"); } function test_WhenUserCreatesNewProposalAfterReinstall() external whenVerifyingFullAttackScenario { @@ -495,11 +471,10 @@ contract TimelockEpochValidationTest is Test { _uninstallPolicy(WALLET, POLICY_ID_1); _installPolicy(WALLET, POLICY_ID_1); - // User creates and approves legitimate proposal with new epoch + // User creates legitimate proposal via no-op UserOp with new epoch bytes memory callData = hex"abcd"; uint256 userNonce = 200; _createProposal(WALLET, POLICY_ID_1, callData, userNonce); - _approveProposal(WALLET, POLICY_ID_1, callData, userNonce); vm.warp(block.timestamp + DELAY + 1); @@ -517,14 +492,12 @@ contract TimelockEpochValidationTest is Test { _installPolicy(WALLET, POLICY_ID_1); _installPolicy(WALLET2, POLICY_ID_1); - // Both users create and approve proposals + // Both users create proposals via no-op UserOp bytes memory callData1 = hex"1111"; bytes memory callData2 = hex"2222"; _createProposal(WALLET, POLICY_ID_1, callData1, 1); - _approveProposal(WALLET, POLICY_ID_1, callData1, 1); _createProposal(WALLET2, POLICY_ID_1, callData2, 1); - _approveProposal(WALLET2, POLICY_ID_1, callData2, 1); vm.warp(block.timestamp + DELAY + 1); From 28ee229897e667aaabc629b3817fa8c130e33e01 Mon Sep 17 00:00:00 2001 From: taek Date: Mon, 9 Feb 2026 23:36:16 +0900 Subject: [PATCH 4/5] fix(TimelockPolicy): remove dead code and add callDataLength overflow guard --- src/policies/TimelockPolicy.sol | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/policies/TimelockPolicy.sol b/src/policies/TimelockPolicy.sol index 4ef4b86..56d330e 100644 --- a/src/policies/TimelockPolicy.sol +++ b/src/policies/TimelockPolicy.sol @@ -61,12 +61,8 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW error InvalidDelay(); error InvalidExpirationPeriod(); - error ProposalNotFound(); - error TimelockNotExpired(uint256 validAfter, uint256 currentTime); - error ProposalExpired(uint256 validUntil, uint256 currentTime); error ProposalNotPending(); error OnlyAccount(); - error ProposalFromPreviousEpoch(); error ParametersTooLarge(); /** @@ -82,7 +78,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW if (delay == 0) revert InvalidDelay(); if (expirationPeriod == 0) revert InvalidExpirationPeriod(); - // Prevent uint48 overflow in createProposal: uint48(block.timestamp) + delay + expirationPeriod + // Prevent uint48 overflow: uint48(block.timestamp) + delay + expirationPeriod if (uint256(delay) + uint256(expirationPeriod) > type(uint48).max - block.timestamp) { revert ParametersTooLarge(); } @@ -181,8 +177,8 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW // Format: [callDataLength(32 bytes)][callData][nonce(32 bytes)][...] uint256 callDataLength = uint256(bytes32(sig[0:32])); - // Validate signature has enough data - if (sig.length < 64 + callDataLength) return SIG_VALIDATION_FAILED_UINT; + // Validate signature has enough data (check callDataLength first to prevent overflow) + if (callDataLength > sig.length || sig.length < 64 + callDataLength) return SIG_VALIDATION_FAILED_UINT; bytes calldata proposalCallData = sig[32:32 + callDataLength]; uint256 proposalNonce = uint256(bytes32(sig[32 + callDataLength:64 + callDataLength])); @@ -380,7 +376,7 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW TimelockConfig storage config = timelockConfig[id][account]; if (!config.initialized) return SIG_VALIDATION_FAILED_UINT; - // Check if this is a proposal approval (or create+approve) request + // Check if this is a proposal creation request // Criteria: calldata is a no-op AND signature has proposal data (length >= 65) if (_isNoOpCalldata(userOp.callData) && sig.length >= 65) { return _handleProposalCreationInternal(id, userOp, config, sig, account); From 4a83f3f66f8893019123e253ac7dae4f71023a80 Mon Sep 17 00:00:00 2001 From: taek Date: Tue, 10 Feb 2026 00:13:58 +0900 Subject: [PATCH 5/5] fix(TimelockPolicy): add mode check and fix executeUserOp offset in no-op detection --- src/policies/TimelockPolicy.sol | 10 ++++--- test/btt/Timelock.t.sol | 46 ++++++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/policies/TimelockPolicy.sol b/src/policies/TimelockPolicy.sol index 88bfcdb..be4103e 100644 --- a/src/policies/TimelockPolicy.sol +++ b/src/policies/TimelockPolicy.sol @@ -281,6 +281,10 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW // ABI layout: 4 (selector) + 32 (mode) + 32 (offset) + 32 (length) + data if (callData.length < 100) return false; + // Only accept single call mode (callType = first byte of mode must be 0x00). + // Prevents delegatecall (0xFE) or batch (0x01) payloads from being treated as no-ops. + if (callData[4] != 0x00) return false; + // Offset to executionCalldata: 2 head slots (mode + offset) = 64 uint256 offset = uint256(bytes32(callData[36:68])); if (offset != 64) return false; @@ -314,12 +318,12 @@ contract TimelockPolicy is PolicyBase, IStatelessValidator, IStatelessValidatorW */ 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 + // Format: 4 (selector) + 32 (userOp offset=64) + 32 (userOpHash) + 32 (userOp length) + userOp data if (callData.length < 100) return false; - // Decode offset to userOp data (should be 32) + // Decode offset to userOp data (should be 64: past 2 head slots) uint256 offset = uint256(bytes32(callData[4:36])); - if (offset != 32) return false; + if (offset != 64) return false; // userOpHash is at bytes 36-68 (we don't validate it) diff --git a/test/btt/Timelock.t.sol b/test/btt/Timelock.t.sol index a52af49..ec254a8 100644 --- a/test/btt/Timelock.t.sol +++ b/test/btt/Timelock.t.sol @@ -825,6 +825,40 @@ contract TimelockTest is Test { assertEq(result, SIG_VALIDATION_FAILED, "Non-empty inner calldata should not be noop"); } + function test_GivenModeIsDelegatecall() external whenDetectingERC7579ExecuteNoop { + // it should not be detected as noop (delegatecall mode 0xFE) + // Mode with callType=0xFE (delegatecall) should be rejected + bytes32 delegatecallMode = bytes32(uint256(0xFE) << 248); + bytes memory executionCalldata = abi.encodePacked(bytes20(WALLET), uint256(0)); + + bytes memory callData = + abi.encodeWithSelector(IERC7579Execution.execute.selector, delegatecallMode, executionCalldata); + + bytes memory sig = _createProposalSignature("proposal", 3); + PackedUserOperation memory userOp = _createUserOpWithCalldata(WALLET, callData, 0, sig); + + vm.prank(WALLET); + uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, userOp); + + assertEq(result, SIG_VALIDATION_FAILED, "Delegatecall mode should not be noop"); + } + + function test_GivenModeIsBatch() external whenDetectingERC7579ExecuteNoop { + // it should not be detected as noop (batch mode 0x01) + bytes32 batchMode = bytes32(uint256(0x01) << 248); + bytes memory executionCalldata = abi.encodePacked(bytes20(WALLET), uint256(0)); + + bytes memory callData = + abi.encodeWithSelector(IERC7579Execution.execute.selector, batchMode, executionCalldata); + + PackedUserOperation memory userOp = _createUserOpWithCalldata(WALLET, callData, 0, ""); + + vm.prank(WALLET); + uint256 result = timelockPolicy.checkUserOpPolicy(POLICY_ID, userOp); + + assertEq(result, SIG_VALIDATION_FAILED, "Batch mode should not be noop"); + } + // ============ _isNoOpExecuteUserOp Tests ============ modifier whenDetectingExecuteUserOpNoop() { @@ -833,11 +867,11 @@ contract TimelockTest is Test { function test_GivenUserOpDataIsEmpty() external whenDetectingExecuteUserOpNoop { // it should be detected as noop - bytes memory callData = abi.encodePacked( + // Use encodeWithSelector to guarantee proper ABI encoding + bytes memory callData = abi.encodeWithSelector( IAccountExecute.executeUserOp.selector, - bytes32(uint256(32)), // offset to userOp - bytes32(0), // userOpHash - bytes32(uint256(0)) // userOp length = 0 + "", // empty bytes userOp + bytes32(0) // userOpHash ); bytes memory sig = _createProposalSignature("proposal", 2); @@ -864,11 +898,11 @@ contract TimelockTest is Test { assertEq(result, SIG_VALIDATION_FAILED, "Too short executeUserOp should not be noop"); } - function test_GivenOffsetIsNot32_WhenDetectingExecuteUserOpNoop() external whenDetectingExecuteUserOpNoop { + function test_GivenOffsetIsNot64_WhenDetectingExecuteUserOpNoop() external whenDetectingExecuteUserOpNoop { // it should not be detected as noop bytes memory callData = abi.encodePacked( IAccountExecute.executeUserOp.selector, - bytes32(uint256(64)), // wrong offset + bytes32(uint256(32)), // wrong offset (should be 64) bytes32(0), bytes32(uint256(0)) );