diff --git a/src/enforcers/ERC20PeriodTransferEnforcer.sol b/src/enforcers/ERC20PeriodTransferEnforcer.sol index 01615ed2..16012686 100644 --- a/src/enforcers/ERC20PeriodTransferEnforcer.sol +++ b/src/enforcers/ERC20PeriodTransferEnforcer.sol @@ -178,8 +178,9 @@ contract ERC20PeriodTransferEnforcer is CaveatEnforcer { ) private { - (address target_,, bytes calldata callData_) = _executionCallData.decodeSingle(); + (address target_, uint256 value_, bytes calldata callData_) = _executionCallData.decodeSingle(); + require(value_ == 0, "ERC20PeriodTransferEnforcer:invalid-value"); require(callData_.length == 68, "ERC20PeriodTransferEnforcer:invalid-execution-length"); (address token_, uint256 periodAmount_, uint256 periodDuration_, uint256 startDate_) = getTermsInfo(_terms); diff --git a/src/enforcers/ERC20TransferAmountEnforcer.sol b/src/enforcers/ERC20TransferAmountEnforcer.sol index 58d79748..716b4c13 100644 --- a/src/enforcers/ERC20TransferAmountEnforcer.sol +++ b/src/enforcers/ERC20TransferAmountEnforcer.sol @@ -82,8 +82,9 @@ contract ERC20TransferAmountEnforcer is CaveatEnforcer { internal returns (uint256 limit_, uint256 spent_) { - (address target_,, bytes calldata callData_) = _executionCallData.decodeSingle(); + (address target_, uint256 value_, bytes calldata callData_) = _executionCallData.decodeSingle(); + require(value_ == 0, "ERC20TransferAmountEnforcer:invalid-value"); require(callData_.length == 68, "ERC20TransferAmountEnforcer:invalid-execution-length"); address allowedContract_; diff --git a/src/enforcers/ERC721TransferEnforcer.sol b/src/enforcers/ERC721TransferEnforcer.sol index 1bd086e3..08be9630 100644 --- a/src/enforcers/ERC721TransferEnforcer.sol +++ b/src/enforcers/ERC721TransferEnforcer.sol @@ -37,7 +37,11 @@ contract ERC721TransferEnforcer is CaveatEnforcer { onlyDefaultExecutionMode(_mode) { (address permittedContract_, uint256 permittedTokenId_) = getTermsInfo(_terms); - (address target_,, bytes calldata callData_) = ExecutionLib.decodeSingle(_executionCallData); + (address target_, uint256 value_, bytes calldata callData_) = ExecutionLib.decodeSingle(_executionCallData); + + if (value_ != 0) { + revert("ERC721TransferEnforcer:invalid-value"); + } // Decode the remaining callData into NFT transfer parameters // The calldata should be at least 100 bytes (4 bytes for the selector + 96 bytes for the parameters) 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/ERC20PeriodTransferEnforcer.t.sol b/test/enforcers/ERC20PeriodTransferEnforcer.t.sol index e8990db9..de4a8464 100644 --- a/test/enforcers/ERC20PeriodTransferEnforcer.t.sol +++ b/test/enforcers/ERC20PeriodTransferEnforcer.t.sol @@ -96,6 +96,17 @@ contract ERC20PeriodTransferEnforcerTest is CaveatEnforcerBaseTest { ); } + /// @notice Reverts if a token transfer execution carries non-zero native value. + function testRevertOnNonZeroValue() public { + bytes memory terms_ = abi.encodePacked(address(basicERC20), periodAmount, periodDuration, startDate); + bytes memory callData_ = _encodeERC20Transfer(bob, 10 ether); + bytes memory execCallData_ = _encodeSingleExecution(address(basicERC20), 1, callData_); + vm.expectRevert("ERC20PeriodTransferEnforcer:invalid-value"); + erc20PeriodTransferEnforcer.beforeHook( + terms_, "", singleDefaultMode, execCallData_, dummyDelegationHash, address(0), redeemer + ); + } + /// @notice Reverts if the target contract in execution data does not match the token in terms_. function testInvalidContract() public { bytes memory terms_ = abi.encodePacked(address(basicERC20), periodAmount, periodDuration, startDate); diff --git a/test/enforcers/ERC20TransferAmountEnforcer.t.sol b/test/enforcers/ERC20TransferAmountEnforcer.t.sol index ad437049..be8bcbd5 100644 --- a/test/enforcers/ERC20TransferAmountEnforcer.t.sol +++ b/test/enforcers/ERC20TransferAmountEnforcer.t.sol @@ -200,6 +200,35 @@ contract ERC20TransferAmountEnforcerTest is CaveatEnforcerBaseTest { assertEq(erc20TransferAmountEnforcer.spentMap(address(delegationManager), delegationHash_), 0); } + // should FAIL with non-zero native value on a token transfer + function test_revertOnNonZeroValue() public { + uint256 spendingLimit_ = 1 ether; + Execution memory execution_ = Execution({ + target: address(basicERC20), + value: 1, + callData: abi.encodeWithSelector(IERC20.transfer.selector, address(users.bob.deleGator), spendingLimit_) + }); + bytes memory executionCallData_ = ExecutionLib.encodeSingle(execution_.target, execution_.value, execution_.callData); + bytes memory inputTerms_ = abi.encodePacked(address(basicERC20), spendingLimit_); + Caveat[] memory caveats_ = new Caveat[](1); + caveats_[0] = Caveat({ args: hex"", enforcer: address(erc20TransferAmountEnforcer), terms: inputTerms_ }); + Delegation memory delegation_ = Delegation({ + delegate: address(users.bob.deleGator), + delegator: address(users.alice.deleGator), + authority: ROOT_AUTHORITY, + caveats: caveats_, + salt: 0, + signature: hex"" + }); + bytes32 delegationHash_ = EncoderLib._getDelegationHash(delegation_); + vm.prank(address(delegationManager)); + vm.expectRevert("ERC20TransferAmountEnforcer:invalid-value"); + erc20TransferAmountEnforcer.beforeHook( + inputTerms_, hex"", singleDefaultMode, executionCallData_, delegationHash_, address(0), address(0) + ); + assertEq(erc20TransferAmountEnforcer.spentMap(address(delegationManager), delegationHash_), 0); + } + // should FAIL to INVOKE invalid method function test_methodFailsIfInvokesInvalidMethod() public { uint256 spendingLimit_ = 1 ether; diff --git a/test/enforcers/ERC721TransferEnforcer.t.sol b/test/enforcers/ERC721TransferEnforcer.t.sol index 387f4d25..7209739a 100644 --- a/test/enforcers/ERC721TransferEnforcer.t.sol +++ b/test/enforcers/ERC721TransferEnforcer.t.sol @@ -129,6 +129,28 @@ contract ERC721TransferEnforcerTest is CaveatEnforcerBaseTest { ); } + /// @notice Tests that a transfer reverts when the execution carries non-zero native value. + function test_revertOnNonZeroValue() public { + Execution memory execution_ = Execution({ + target: address(token), + value: 1, + callData: abi.encodeWithSelector(IERC721.transferFrom.selector, address(this), address(0xBEEF), TOKEN_ID) + }); + bytes memory executionCallData_ = ExecutionLib.encodeSingle(execution_.target, execution_.value, execution_.callData); + + vm.prank(address(delegationManager)); + vm.expectRevert("ERC721TransferEnforcer:invalid-value"); + erc721TransferEnforcer.beforeHook( + abi.encodePacked(address(token), TOKEN_ID), + hex"", + singleDefaultMode, + executionCallData_, + keccak256(""), + address(0), + address(0) + ); + } + /// @notice Tests that a transfer reverts when using an unauthorized function selector. function test_unauthorizedSelector_wrongMethod() public { bytes4 dummySelector_ = bytes4(keccak256("foo(address,address,uint256)")); diff --git a/test/enforcers/LogicalOrWrapperEnforcer.t.sol b/test/enforcers/LogicalOrWrapperEnforcer.t.sol index f0b93f4c..49fcc2f5 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 @@ -711,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 @@ -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)"); }