From edf621a53815e32db2b7d6809c1084599459cf11 Mon Sep 17 00:00:00 2001 From: SashaMIT Date: Thu, 6 Aug 2026 02:25:49 +0700 Subject: [PATCH 1/2] fix(enforcers): isolate stateful sub-enforcer state per caveat group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LogicalOrWrapperEnforcer forwarded the raw delegation hash to every caveat in the selected group. Stateful enforcers key state by delegation hash (not terms), so all groups referencing the same stateful enforcer shared one state: the first-used group initialised it (period start, call counter, spent map) for every other group — invisibly to the delegator, and beyond what the header security notice covers. The forwarded hash is now namespaced by the selected group index (keccak256(delegationHash, groupIndex)); each group's budgets/periods are independent, and a shared budget remains expressible by placing caveats in a single group. Includes an isolation regression test (LimitedCallsEnforcer, limit 1 in two groups) that fails without the fix. Signed-off-by: SashaMIT Co-authored-by: Cursor --- src/enforcers/LogicalOrWrapperEnforcer.sol | 20 ++++- test/enforcers/LogicalOrWrapperEnforcer.t.sol | 78 ++++++++++++++++--- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/src/enforcers/LogicalOrWrapperEnforcer.sol b/src/enforcers/LogicalOrWrapperEnforcer.sol index f0b771dc..9ce1b5cd 100644 --- a/src/enforcers/LogicalOrWrapperEnforcer.sol +++ b/src/enforcers/LogicalOrWrapperEnforcer.sol @@ -47,6 +47,9 @@ import { ModeCode, Caveat } from "../utils/Types.sol"; * - Never assume the redeemer will select the most restrictive group. * - Design caveat groups with the understanding that the redeemer will choose the path of least * resistance. + * - Stateful caveat enforcers keep isolated state per group (the delegation hash forwarded + * to sub-enforcers is namespaced by the selected group index), so budgets/periods do not + * leak across groups. To share a budget across alternative paths, use a single group. * * Use this enforcer at your own risk and ensure it aligns with your intended security model. */ @@ -266,11 +269,26 @@ contract LogicalOrWrapperEnforcer is CaveatEnforcer { selectedGroup_.caveatArgs[i], _params.mode, _params.executionCallData, - _params.delegationHash, + _getGroupDelegationHash(_params.delegationHash, selectedGroup_.groupIndex), _params.delegator, _params.redeemer ) ); } } + + /** + * @notice Namespaces the delegation hash by the selected group. + * @dev Stateful caveat enforcers key their state by delegation hash (not by terms), + * so without namespacing every group that references the same stateful enforcer + * shares one state: the first-used group initialises it (e.g. pinning a period + * start or filling a call counter) for all other groups, invisibly to the + * delegator. With namespacing each group's state is isolated; a shared budget + * can still be expressed by placing the caveats in a single group. + * @param _delegationHash The hash of the delegation being redeemed. + * @param _groupIndex The index of the caveat group selected for this redemption. + */ + function _getGroupDelegationHash(bytes32 _delegationHash, uint256 _groupIndex) internal pure returns (bytes32) { + return keccak256(abi.encode(_delegationHash, _groupIndex)); + } } diff --git a/test/enforcers/LogicalOrWrapperEnforcer.t.sol b/test/enforcers/LogicalOrWrapperEnforcer.t.sol index f0b93f4c..e6d945a7 100644 --- a/test/enforcers/LogicalOrWrapperEnforcer.t.sol +++ b/test/enforcers/LogicalOrWrapperEnforcer.t.sol @@ -14,6 +14,7 @@ import { LogicalOrWrapperEnforcer } from "../../src/enforcers/LogicalOrWrapperEn import { ICaveatEnforcer } from "../../src/interfaces/ICaveatEnforcer.sol"; import { AllowedMethodsEnforcer } from "../../src/enforcers/AllowedMethodsEnforcer.sol"; import { AllowedTargetsEnforcer } from "../../src/enforcers/AllowedTargetsEnforcer.sol"; +import { LimitedCallsEnforcer } from "../../src/enforcers/LimitedCallsEnforcer.sol"; import { NativeTokenTransferAmountEnforcer } from "../../src/enforcers/NativeTokenTransferAmountEnforcer.sol"; import { TimestampEnforcer } from "../../src/enforcers/TimestampEnforcer.sol"; import { ArgsEqualityCheckEnforcer } from "../../src/enforcers/ArgsEqualityCheckEnforcer.sol"; @@ -139,6 +140,12 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { ////////////////////// Helper Functions ////////////////////// + /// @dev The namespaced delegation hash the wrapper forwards to sub-enforcers for a + /// given group — mirrors LogicalOrWrapperEnforcer._getGroupDelegationHash + function _groupDelegationHash(bytes32 delegationHash_, uint256 groupIndex_) internal pure returns (bytes32) { + return keccak256(abi.encode(delegationHash_, groupIndex_)); + } + function _createCaveatGroup( address[] memory _enforcers, bytes[] memory _terms @@ -306,6 +313,54 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { ); } + /// @notice Tests that stateful enforcers keep per-group isolated state when the same + /// stateful enforcer appears in multiple groups: each group's allowance is independent. + function test_statefulEnforcerStateIsIsolatedPerGroup() public { + LimitedCallsEnforcer limitedCallsEnforcer_ = new LimitedCallsEnforcer(); + + // Two groups, both referencing the same stateful enforcer with limit = 1 call each + LogicalOrWrapperEnforcer.CaveatGroup[] memory groups_ = new LogicalOrWrapperEnforcer.CaveatGroup[](2); + for (uint256 g_ = 0; g_ < 2; g_++) { + address[] memory enforcers_ = new address[](1); + enforcers_[0] = address(limitedCallsEnforcer_); + bytes[] memory terms_ = new bytes[](1); + terms_[0] = abi.encode(uint256(1)); + groups_[g_] = _createCaveatGroup(enforcers_, terms_); + } + + Execution memory execution_ = Execution({ target: address(aliceDeleGatorCounter), value: 0, callData: hex"" }); + bytes memory executionCallData_ = ExecutionLib.encodeSingle(execution_.target, execution_.value, execution_.callData); + bytes32 delegationHash_ = keccak256("some-delegation"); + + bytes[] memory caveatArgs_ = new bytes[](1); + caveatArgs_[0] = hex""; + + // Group 0: first call consumes its allowance + vm.prank(address(delegationManager)); + logicalOrWrapperEnforcer.beforeHook( + abi.encode(groups_), abi.encode(_createSelectedGroup(0, caveatArgs_)), singleDefaultMode, executionCallData_, delegationHash_, address(0), address(0) + ); + // Group 0: second call exceeds its own limit + vm.prank(address(delegationManager)); + vm.expectRevert("LimitedCallsEnforcer:limit-exceeded"); + logicalOrWrapperEnforcer.beforeHook( + abi.encode(groups_), abi.encode(_createSelectedGroup(0, caveatArgs_)), singleDefaultMode, executionCallData_, delegationHash_, address(0), address(0) + ); + + // Group 1: independent allowance — without per-group namespacing this would revert + // because the shared counter was already exhausted by group 0 + vm.prank(address(delegationManager)); + logicalOrWrapperEnforcer.beforeHook( + abi.encode(groups_), abi.encode(_createSelectedGroup(1, caveatArgs_)), singleDefaultMode, executionCallData_, delegationHash_, address(0), address(0) + ); + // Group 1: second call also exceeds its own limit + vm.prank(address(delegationManager)); + vm.expectRevert("LimitedCallsEnforcer:limit-exceeded"); + logicalOrWrapperEnforcer.beforeHook( + abi.encode(groups_), abi.encode(_createSelectedGroup(1, caveatArgs_)), singleDefaultMode, executionCallData_, delegationHash_, address(0), address(0) + ); + } + /// @notice Tests that the ArgsEqualityCheckEnforcer works correctly when terms match args through the LogicalOrWrapperEnforcer function test_argsEqualityCheckEnforcerSuccess() public { // Create a group with a single caveat (args equality check) @@ -684,7 +739,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { bytes32 hashKey_ = erc20BalanceChangeEnforcer.getHashKey( address(logicalOrWrapperEnforcer), // LogicalOrWrapperEnforcer is the caller address(mockToken), - keccak256("") + _groupDelegationHash(keccak256(""), 0) ); assertTrue(erc20BalanceChangeEnforcer.isLocked(hashKey_), "Balance cache should be locked during execution_"); // Verify the cached balance is stored @@ -812,7 +867,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate state: spentMap should track the transfer amount uint256 spentAmount_ = erc20TransferAmountEnforcer.spentMap( address(logicalOrWrapperEnforcer), // LogicalOrWrapperEnforcer is the delegationManager - keccak256("") + _groupDelegationHash(keccak256(""), 0) ); assertEq(spentAmount_, 50 ether, "SpentMap should track 50 ether spent"); @@ -859,7 +914,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { ); // Validate state after first transfer - uint256 spentAfterFirst_ = erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), keccak256("")); + uint256 spentAfterFirst_ = erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), _groupDelegationHash(keccak256(""), 0)); assertEq(spentAfterFirst_, 50 ether, "SpentMap should show 50 ether spent after first transfer"); // Simulate the transfer (would be done by the actual execution_ in prod) @@ -1415,7 +1470,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate state after first call assertEq( - erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), delegationHash_), + erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), _groupDelegationHash(delegationHash_, 0)), 30 ether, "Should have 30 ether spent after first call" ); @@ -1437,7 +1492,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate final state assertEq( - erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), delegationHash_), + erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), _groupDelegationHash(delegationHash_, 0)), 80 ether, "Should have 80 ether spent after second call" ); @@ -1489,7 +1544,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate state: streaming allowance should be initialized and track spent amount (,,,, uint256 spent_) = erc20StreamingEnforcer.streamingAllowances( address(logicalOrWrapperEnforcer), // LogicalOrWrapperEnforcer is the delegationManager - keccak256("") + _groupDelegationHash(keccak256(""), 0) ); assertEq(spent_, 5 ether, "Spent amount should be 5 ether"); } @@ -1584,7 +1639,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate state: periodic allowance should be initialized and track transferred amount (,,, uint256 lastTransferPeriod_, uint256 transferredInCurrentPeriod_) = erc20PeriodTransferEnforcer.periodicAllowances( address(logicalOrWrapperEnforcer), // LogicalOrWrapperEnforcer is the delegationManager - keccak256("") + _groupDelegationHash(keccak256(""), 0) ); assertEq(lastTransferPeriod_, 1, "Last transfer period should be 1 (current period)"); assertEq(transferredInCurrentPeriod_, 50 ether, "Transferred in current period should be 50 ether"); @@ -1826,7 +1881,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate state: Check spent amount assertEq( - erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), keccak256("")), + erc20TransferAmountEnforcer.spentMap(address(logicalOrWrapperEnforcer), _groupDelegationHash(keccak256(""), 0)), 75 ether, "ERC20 transfer enforcer should track 75 ether spent" ); @@ -2274,7 +2329,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate enforcer state (LogicalOrWrapperEnforcer acts as delegationManager for wrapped enforcer) bytes32 delegationHash_ = EncoderLib._getDelegationHash(delegation); (,,, uint256 lastTransferPeriod, uint256 transferredInCurrentPeriod) = - erc20PeriodTransferEnforcer.periodicAllowances(address(logicalOrWrapperEnforcer), delegationHash_); + erc20PeriodTransferEnforcer.periodicAllowances(address(logicalOrWrapperEnforcer), _groupDelegationHash(delegationHash_, 0)); assertEq(lastTransferPeriod, 1, "Should be in first period"); assertEq(transferredInCurrentPeriod, 30 ether, "Should track 30 ether transferred"); } @@ -2301,7 +2356,8 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { // Validate enforcer state (LogicalOrWrapperEnforcer acts as delegationManager for wrapped enforcer) bytes32 delegationHash_ = EncoderLib._getDelegationHash(delegation); - (,,,, uint256 spent) = erc20StreamingEnforcer.streamingAllowances(address(logicalOrWrapperEnforcer), delegationHash_); + (,,,, uint256 spent) = + erc20StreamingEnforcer.streamingAllowances(address(logicalOrWrapperEnforcer), _groupDelegationHash(delegationHash_, 0)); assertEq(spent, 5 ether, "Should track 5 ether spent"); } @@ -2330,7 +2386,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { bytes memory terms_ = abi.encodePacked(address(mockToken), uint256(50 ether), uint256(1 days), block.timestamp); bytes memory args = abi.encode(uint256(0)); (uint256 available,,) = - multiTokenPeriodEnforcer.getAvailableAmount(delegationHash_, address(logicalOrWrapperEnforcer), terms_, args); + multiTokenPeriodEnforcer.getAvailableAmount(_groupDelegationHash(delegationHash_, 0), address(logicalOrWrapperEnforcer), terms_, args); assertEq(available, 30 ether, "Should have 30 ether remaining (50 - 20)"); } From b70d03fb528483e3d4a53b3cd6bfe85f3c47df9c Mon Sep 17 00:00:00 2001 From: SashaMIT Date: Thu, 6 Aug 2026 02:37:52 +0700 Subject: [PATCH 2/2] test: namespace the post-afterHook lock check too (bugbot) The afterHook assertion still read the raw delegation hash key, which was never locked, so it always passed. Point it at the namespaced key like the before-hook check. Signed-off-by: SashaMIT Co-authored-by: Cursor --- test/enforcers/LogicalOrWrapperEnforcer.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/enforcers/LogicalOrWrapperEnforcer.t.sol b/test/enforcers/LogicalOrWrapperEnforcer.t.sol index e6d945a7..49fcc2f5 100644 --- a/test/enforcers/LogicalOrWrapperEnforcer.t.sol +++ b/test/enforcers/LogicalOrWrapperEnforcer.t.sol @@ -766,7 +766,7 @@ contract LogicalOrWrapperEnforcerTest is CaveatEnforcerBaseTest { bytes32 afterHashKey_ = erc20BalanceChangeEnforcer.getHashKey( address(logicalOrWrapperEnforcer), // LogicalOrWrapperEnforcer is the caller address(mockToken), - keccak256("") + _groupDelegationHash(keccak256(""), 0) ); assertFalse(erc20BalanceChangeEnforcer.isLocked(afterHashKey_), "Balance cache should be unlocked after afterHook"); // Note: balanceCache is not cleared, but isLocked is false, so it can be reused