From 4ebbc1dd5783b57367e3132122952d544069707f Mon Sep 17 00:00:00 2001 From: makemake Date: Mon, 3 Aug 2026 13:23:12 +0200 Subject: [PATCH 1/6] fix(safe): close assertion and guard audit gaps --- .../script/DeployCredibleSafeGuard.s.sol | 40 +- examples/safe/src/SafeConfigLockAssertion.sol | 96 +- examples/safe/src/SafeConfigLockHelpers.sol | 118 +-- examples/safe/src/SafeTxShapeAssertion.sol | 132 +-- examples/safe/src/SafeTxShapeHelpers.sol | 882 +----------------- src/protection/safe/README.md | 4 +- src/protection/safe/SafeTxShapeHelpers.sol | 77 +- .../safe/SafeTxShapeAssertion.t.sol | 33 +- .../CredibleSafeGuardScripts.t.sol | 51 + 9 files changed, 186 insertions(+), 1247 deletions(-) diff --git a/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol index d7c8149..e2da729 100644 --- a/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol +++ b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol @@ -15,6 +15,10 @@ contract DeployCredibleSafeGuard is Script { /// @notice Thrown when a required registry read (isCredibleBlock / lastCredibleBlock) does not /// return a single, well-formed 32-byte word. error RegistryReadFailed(address registry, string read); + /// @notice Thrown when the registry reports a block that cannot yet have been credible. + error RegistryLastCredibleBlockInFuture(address registry, uint256 reportedBlock, uint256 currentBlock); + + uint256 internal constant REGISTRY_READ_GAS_LIMIT = 50_000; function run() external returns (CredibleSafeGuard guard) { address registry = vm.envAddress("CREDIBLE_REGISTRY"); @@ -56,14 +60,38 @@ contract DeployCredibleSafeGuard is Script { // it passes the length check but the guard's runtime decode treats it as unreadable // (see CredibleSafeGuard._tryIsCredibleBlock's `value > 1` branch), which would otherwise // let a registry silently deploy a permanently-fail-open guard. - (bool credibleOk, bytes memory credibleData) = - registry.staticcall(abi.encodeCall(ICredibleRegistry.isCredibleBlock, (block.number))); - if (!credibleOk || credibleData.length != 32 || abi.decode(credibleData, (uint256)) > 1) { + (bool credibleOk, uint256 credibleWord) = + _boundedRegistryRead(registry, abi.encodeCall(ICredibleRegistry.isCredibleBlock, (block.number))); + if (!credibleOk || credibleWord > 1) { revert RegistryReadFailed(registry, "isCredibleBlock"); } - (bool lastOk, bytes memory lastData) = - registry.staticcall(abi.encodeCall(ICredibleRegistry.lastCredibleBlock, ())); - if (!lastOk || lastData.length != 32) revert RegistryReadFailed(registry, "lastCredibleBlock"); + (bool lastOk, uint256 lastCredibleBlock) = + _boundedRegistryRead(registry, abi.encodeCall(ICredibleRegistry.lastCredibleBlock, ())); + if (!lastOk) revert RegistryReadFailed(registry, "lastCredibleBlock"); + if (lastCredibleBlock > block.number) { + revert RegistryLastCredibleBlockInFuture(registry, lastCredibleBlock, block.number); + } + } + + /// @dev Mirrors the guard's runtime boundary: 50k gas, exactly one return word, and no + /// unbounded returndata allocation. Deployment rejects failures; runtime fails open. + function _boundedRegistryRead(address registry, bytes memory callData) + internal + view + returns (bool readable, uint256 value) + { + assembly ("memory-safe") { + readable := staticcall( + REGISTRY_READ_GAS_LIMIT, + registry, + add(callData, 0x20), + mload(callData), + 0x00, + 0x20 + ) + readable := and(readable, eq(returndatasize(), 0x20)) + value := mload(0x00) + } } } diff --git a/examples/safe/src/SafeConfigLockAssertion.sol b/examples/safe/src/SafeConfigLockAssertion.sol index ba04533..6e2966a 100644 --- a/examples/safe/src/SafeConfigLockAssertion.sol +++ b/examples/safe/src/SafeConfigLockAssertion.sol @@ -1,97 +1,5 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.13; -import {PhEvm} from "credible-std/PhEvm.sol"; -import {SafeConfigLockHelpers} from "./SafeConfigLockHelpers.sol"; - -/// @title SafeConfigLockAssertion -/// @author Phylax Systems -/// @notice Locks the critical configuration envelope for a Safe multisig. -/// @dev The assertion checks the Safe after each monitored transaction: -/// - threshold and owner count stay above configured minimums; -/// - owner and module sets match one of the approved set hashes; -/// - transaction guard, module guard, and fallback handler match expected addresses. -/// -/// Address set hashes are computed by sorting addresses ascending and then hashing -/// `abi.encode(sortedAddresses)`. For modules, `bytes32(0)` in the approved hash list -/// is a sentinel meaning "modules must be disabled". -contract SafeConfigLockAssertion is SafeConfigLockHelpers { - uint256 public immutable minThreshold; - uint256 public immutable minOwners; - address public immutable expectedGuard; - address public immutable expectedModuleGuard; - address public immutable expectedFallbackHandler; - - bytes32[] public approvedOwnerSetHashes; - bytes32[] public approvedModuleSetHashes; - - constructor( - uint256 minThreshold_, - uint256 minOwners_, - bytes32[] memory approvedOwnerSetHashes_, - bytes32[] memory approvedModuleSetHashes_, - address expectedGuard_, - address expectedModuleGuard_, - address expectedFallbackHandler_ - ) { - require(approvedOwnerSetHashes_.length != 0, "SafeConfigLock: owner hashes empty"); - require(approvedModuleSetHashes_.length != 0, "SafeConfigLock: module hashes empty"); - - minThreshold = minThreshold_; - minOwners = minOwners_; - expectedGuard = expectedGuard_; - expectedModuleGuard = expectedModuleGuard_; - expectedFallbackHandler = expectedFallbackHandler_; - - for (uint256 i; i < approvedOwnerSetHashes_.length; ++i) { - approvedOwnerSetHashes.push(approvedOwnerSetHashes_[i]); - } - - for (uint256 i; i < approvedModuleSetHashes_.length; ++i) { - approvedModuleSetHashes.push(approvedModuleSetHashes_[i]); - } - - _registerReshiramSpec(); - } - - function triggers() external view override { - registerStorageChangeTrigger(this.assertSafeConfiguration.selector); - } - - /// @notice Checks the Safe config after the triggering transaction has completed. - /// @dev Fails when a Safe transaction leaves owners, modules, guards, or fallback handling - /// outside the deployment-time policy. A zero module-set hash in the approved list - /// only approves the empty module set. - function assertSafeConfiguration() external view { - address safe = ph.getAssertionAdopter(); - PhEvm.ForkId memory post = _postTx(); - - address[] memory owners = _ownersAt(safe, post); - uint256 threshold = _thresholdAt(safe, post); - - require(threshold >= minThreshold, "SafeConfigLock: threshold below minimum"); - require(owners.length >= minOwners, "SafeConfigLock: owner count below minimum"); - require( - _isApprovedHash(hashAddressSet(owners), approvedOwnerSetHashes, false), - "SafeConfigLock: owner set not approved" - ); - - address[] memory modules = _modulesAt(safe, post); - require( - _isApprovedHash(hashAddressSet(modules), approvedModuleSetHashes, modules.length == 0), - "SafeConfigLock: module set not approved" - ); - - require(_guardAt(safe, post) == expectedGuard, "SafeConfigLock: guard mismatch"); - require(_moduleGuardAt(safe, post) == expectedModuleGuard, "SafeConfigLock: module guard mismatch"); - require(_fallbackHandlerAt(safe, post) == expectedFallbackHandler, "SafeConfigLock: fallback handler mismatch"); - } - - function approvedOwnerSetHashCount() external view returns (uint256) { - return approvedOwnerSetHashes.length; - } - - function approvedModuleSetHashCount() external view returns (uint256) { - return approvedModuleSetHashes.length; - } -} +// Re-export the single maintained implementation for existing example imports. +import {SafeConfigLockAssertion} from "credible-std/protection/safe/SafeConfigLockAssertion.sol"; diff --git a/examples/safe/src/SafeConfigLockHelpers.sol b/examples/safe/src/SafeConfigLockHelpers.sol index ba9333c..932780f 100644 --- a/examples/safe/src/SafeConfigLockHelpers.sol +++ b/examples/safe/src/SafeConfigLockHelpers.sol @@ -1,119 +1,5 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.13; -import {Assertion} from "credible-std/Assertion.sol"; -import {PhEvm} from "credible-std/PhEvm.sol"; -import {AssertionSpec} from "credible-std/SpecRecorder.sol"; - -interface ISafeConfigLockTarget { - function getThreshold() external view returns (uint256); - function getOwners() external view returns (address[] memory); - function getModulesPaginated(address start, uint256 pageSize) - external - view - returns (address[] memory array, address next); -} - -/// @title SafeConfigLockHelpers -/// @author Phylax Systems -/// @notice Shared constants and snapshot readers for Safe configuration assertions. -abstract contract SafeConfigLockHelpers is Assertion { - address internal constant SPEC_RECORDER = address(uint160(uint256(keccak256("SpecRecorder")))); - address internal constant SENTINEL_MODULES = address(0x1); - - uint256 internal constant MODULE_PAGE_SIZE = 256; - - bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = - 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5; - bytes32 internal constant GUARD_STORAGE_SLOT = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8; - bytes32 internal constant MODULE_GUARD_STORAGE_SLOT = - 0xb104e0b93118902c651344349b610029d694cfdec91c589c91ebafbcd0289947; - - /// @notice Computes the deterministic hash used by owner and module allow lists. - /// @dev Sorts the provided addresses in memory before hashing, so Safe linked-list order - /// does not affect the resulting set hash. - function hashAddressSet(address[] memory accounts) public pure returns (bytes32) { - _sortAddresses(accounts); - return keccak256(abi.encode(accounts)); - } - - function _ownersAt(address safe, PhEvm.ForkId memory fork) internal view returns (address[] memory owners) { - owners = abi.decode(_viewAt(safe, abi.encodeCall(ISafeConfigLockTarget.getOwners, ()), fork), (address[])); - } - - function _thresholdAt(address safe, PhEvm.ForkId memory fork) internal view returns (uint256) { - return _readUintAt(safe, abi.encodeCall(ISafeConfigLockTarget.getThreshold, ()), fork); - } - - function _modulesAt(address safe, PhEvm.ForkId memory fork) internal view returns (address[] memory modules) { - address next; - (modules, next) = abi.decode( - _viewAt( - safe, - abi.encodeCall(ISafeConfigLockTarget.getModulesPaginated, (SENTINEL_MODULES, MODULE_PAGE_SIZE)), - fork - ), - (address[], address) - ); - require(next == SENTINEL_MODULES, "SafeConfigLock: too many modules"); - } - - function _guardAt(address safe, PhEvm.ForkId memory fork) internal view returns (address) { - return _addressSlotAt(safe, GUARD_STORAGE_SLOT, fork); - } - - function _moduleGuardAt(address safe, PhEvm.ForkId memory fork) internal view returns (address) { - return _addressSlotAt(safe, MODULE_GUARD_STORAGE_SLOT, fork); - } - - function _fallbackHandlerAt(address safe, PhEvm.ForkId memory fork) internal view returns (address) { - return _addressSlotAt(safe, FALLBACK_HANDLER_STORAGE_SLOT, fork); - } - - function _addressSlotAt(address safe, bytes32 slot, PhEvm.ForkId memory fork) internal view returns (address) { - return address(uint160(uint256(ph.loadStateAt(safe, slot, fork)))); - } - - function _isApprovedHash(bytes32 actualHash, bytes32[] storage approvedHashes, bool emptySet) - internal - view - returns (bool) - { - for (uint256 i; i < approvedHashes.length; ++i) { - if (approvedHashes[i] == actualHash) { - return true; - } - - if (emptySet && approvedHashes[i] == bytes32(0)) { - return true; - } - } - - return false; - } - - function _sortAddresses(address[] memory accounts) internal pure { - for (uint256 i = 1; i < accounts.length; ++i) { - address current = accounts[i]; - uint256 j = i; - - while (j > 0 && uint160(accounts[j - 1]) > uint160(current)) { - accounts[j] = accounts[j - 1]; - --j; - } - - accounts[j] = current; - } - } - - function _viewFailureMessage() internal pure override returns (string memory) { - return "SafeConfigLock: safe view failed"; - } - - function _registerReshiramSpec() internal { - (bool ok,) = SPEC_RECORDER.call( - abi.encodeWithSelector(bytes4(keccak256("registerAssertionSpec(uint8)")), AssertionSpec.Reshiram) - ); - require(ok, "SafeConfigLock: spec registration failed"); - } -} +// Re-export the single maintained helper symbol for existing example imports. +import {SafeConfigLockHelpers} from "credible-std/protection/safe/SafeConfigLockHelpers.sol"; diff --git a/examples/safe/src/SafeTxShapeAssertion.sol b/examples/safe/src/SafeTxShapeAssertion.sol index 1565b2b..8ef9244 100644 --- a/examples/safe/src/SafeTxShapeAssertion.sol +++ b/examples/safe/src/SafeTxShapeAssertion.sol @@ -1,132 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.13; -import {SafeTxShapeHelpers} from "./SafeTxShapeHelpers.sol"; - -/// @title SafeTxShapeAssertion -/// @author Phylax Systems -/// @notice Enforces direct Safe action-shape policy for owner and module executions. -/// @dev Validates the Safe transaction tuple before settlement: known targets, exact -/// selectors, delegatecall restrictions, approved MultiSend batch contents, and -/// token approval spender/operator policy. -contract SafeTxShapeAssertion is SafeTxShapeHelpers { - constructor( - TargetPolicy[] memory targetPolicies_, - SelectorPolicy[] memory selectorPolicies_, - BatchExecutorPolicy[] memory batchExecutorPolicies_, - ApprovalPolicy[] memory approvalPolicies_, - bool moduleExecutionEnabled_, - address[] memory allowedModules_ - ) - SafeTxShapeHelpers( - targetPolicies_, - selectorPolicies_, - batchExecutorPolicies_, - approvalPolicies_, - moduleExecutionEnabled_, - allowedModules_ - ) - {} - - function triggers() external view override { - registerFnCallTrigger(this.assertSafeModulePolicy.selector, EXEC_TRANSACTION_SELECTOR); - registerFnCallTrigger(this.assertSafeModulePolicy.selector, EXEC_TRANSACTION_FROM_MODULE_SELECTOR); - registerFnCallTrigger(this.assertSafeModulePolicy.selector, EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR); - - registerFnCallTrigger(this.assertSafeDelegateCallPolicy.selector, EXEC_TRANSACTION_SELECTOR); - registerFnCallTrigger(this.assertSafeDelegateCallPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_SELECTOR); - registerFnCallTrigger( - this.assertSafeDelegateCallPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR - ); - - registerFnCallTrigger(this.assertSafeTargetSelectorPolicy.selector, EXEC_TRANSACTION_SELECTOR); - registerFnCallTrigger(this.assertSafeTargetSelectorPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_SELECTOR); - registerFnCallTrigger( - this.assertSafeTargetSelectorPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR - ); - - registerFnCallTrigger(this.assertSafeBatchPolicy.selector, EXEC_TRANSACTION_SELECTOR); - registerFnCallTrigger(this.assertSafeBatchPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_SELECTOR); - registerFnCallTrigger(this.assertSafeBatchPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR); - - registerFnCallTrigger(this.assertSafeApprovalPolicy.selector, EXEC_TRANSACTION_SELECTOR); - registerFnCallTrigger(this.assertSafeApprovalPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_SELECTOR); - registerFnCallTrigger(this.assertSafeApprovalPolicy.selector, EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR); - } - - /// @notice Ensures module executions are disabled or sent by an allowlisted module. - function assertSafeModulePolicy() external view { - Action memory action = _triggeredAction(); - if (action.fromModule) _validateModuleCaller(action.module); - } - - /// @notice Blocks direct, module, and inner delegatecalls except configured top-level MultiSend execution. - function assertSafeDelegateCallPolicy() external view { - Action memory action = _triggeredAction(); - if (action.operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(action.operation); - - (bool isBatchExecutor, uint256 batchIndex) = - _batchPolicyForAction(action.target, action.data, action.dataOffset, action.dataLength); - if (isBatchExecutor) { - BatchExecutorPolicy storage batchPolicy = batchExecutorPolicies[batchIndex]; - if (action.operation == OPERATION_DELEGATECALL && !batchPolicy.allowDelegateCall) { - revert SafeTxShapeBatchDelegateCallNotAllowed(action.target); - } - _validateMultiSendDelegateCallPolicy(action, batchPolicy); - return; - } - - if (action.operation == OPERATION_DELEGATECALL) revert SafeTxShapeDelegateCallBlocked(action.target); - } - - /// @notice Ensures every non-batch action uses a known target and allowed selector. - function assertSafeTargetSelectorPolicy() external view { - Action memory action = _triggeredAction(); - if (action.operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(action.operation); - - (bool isBatchExecutor, uint256 batchIndex) = - _batchPolicyForAction(action.target, action.data, action.dataOffset, action.dataLength); - if (isBatchExecutor) { - _validateMultiSendTargetSelectorPolicy(action, batchExecutorPolicies[batchIndex]); - return; - } - - if (action.operation == OPERATION_DELEGATECALL) return; - - _validateTargetAndSelector(action); - } - - /// @notice Strictly parses configured MultiSend batches and rejects malformed or nested batches. - function assertSafeBatchPolicy() external view { - Action memory action = _triggeredAction(); - if (action.operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(action.operation); - - (bool isBatchExecutor, uint256 batchIndex) = - _batchPolicyForAction(action.target, action.data, action.dataOffset, action.dataLength); - if (!isBatchExecutor) return; - - BatchExecutorPolicy storage batchPolicy = batchExecutorPolicies[batchIndex]; - if (action.operation == OPERATION_DELEGATECALL && !batchPolicy.allowDelegateCall) { - revert SafeTxShapeBatchDelegateCallNotAllowed(action.target); - } - _validateMultiSendBatchPolicy(action, batchPolicy); - } - - /// @notice Enforces spender/operator and amount limits for approval-like calls. - function assertSafeApprovalPolicy() external view { - Action memory action = _triggeredAction(); - if (action.operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(action.operation); - - (bool isBatchExecutor, uint256 batchIndex) = - _batchPolicyForAction(action.target, action.data, action.dataOffset, action.dataLength); - if (isBatchExecutor) { - _validateMultiSendApprovalPolicy(action, batchExecutorPolicies[batchIndex]); - return; - } - - if (action.operation == OPERATION_DELEGATECALL) return; - - if (action.dataLength < 4) return; - _validateApproval(action, _selectorAt(action.data, action.dataOffset)); - } -} +// The maintained implementation lives in src/. This source unit deliberately reuses that symbol +// so existing example imports resolve to the exact same contract and creation bytecode. +import {SafeTxShapeAssertion} from "credible-std/protection/safe/SafeTxShapeAssertion.sol"; diff --git a/examples/safe/src/SafeTxShapeHelpers.sol b/examples/safe/src/SafeTxShapeHelpers.sol index 9e9206d..fff7ccd 100644 --- a/examples/safe/src/SafeTxShapeHelpers.sol +++ b/examples/safe/src/SafeTxShapeHelpers.sol @@ -1,883 +1,5 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.13; -import {Assertion} from "credible-std/Assertion.sol"; -import {AssertionSpec} from "credible-std/SpecRecorder.sol"; -import {PhEvm} from "credible-std/PhEvm.sol"; - -/// @title SafeTxShapeHelpers -/// @author Phylax Systems -/// @notice Shared decoding and policy helpers for Safe transaction-shape assertions. -abstract contract SafeTxShapeHelpers is Assertion { - address internal constant SPEC_RECORDER = address(uint160(uint256(keccak256("SpecRecorder")))); - - uint8 internal constant OPERATION_CALL = 0; - uint8 internal constant OPERATION_DELEGATECALL = 1; - - uint8 public constant APPROVAL_KIND_ERC20_APPROVE = 1; - uint8 public constant APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE = 2; - uint8 public constant APPROVAL_KIND_ERC721_APPROVE = 3; - uint8 public constant APPROVAL_KIND_ERC721_SET_APPROVAL_FOR_ALL = 4; - uint8 public constant APPROVAL_KIND_ERC1155_SET_APPROVAL_FOR_ALL = 5; - - bytes4 public constant EXEC_TRANSACTION_SELECTOR = - bytes4(keccak256("execTransaction(address,uint256,bytes,uint8,uint256,uint256,uint256,address,address,bytes)")); - bytes4 public constant EXEC_TRANSACTION_FROM_MODULE_SELECTOR = - bytes4(keccak256("execTransactionFromModule(address,uint256,bytes,uint8)")); - bytes4 public constant EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR = - bytes4(keccak256("execTransactionFromModuleReturnData(address,uint256,bytes,uint8)")); - - bytes4 public constant MULTISEND_SELECTOR = bytes4(keccak256("multiSend(bytes)")); - bytes4 public constant APPROVE_SELECTOR = bytes4(keccak256("approve(address,uint256)")); - bytes4 public constant INCREASE_ALLOWANCE_SELECTOR = bytes4(keccak256("increaseAllowance(address,uint256)")); - bytes4 public constant SET_APPROVAL_FOR_ALL_SELECTOR = bytes4(keccak256("setApprovalForAll(address,bool)")); - - uint256 internal constant MULTISEND_HEADER_LENGTH = 85; - uint64 internal constant ALLOWANCE_READ_GAS = 500_000; - - struct TargetPolicy { - address target; - bool allowAnySelector; - bool allowEmptyCalldata; - bool allowFallbackCalldata; - bool allowNonzeroValue; - } - - struct SelectorPolicy { - address target; - bytes4 selector; - bool allowNonzeroValue; - } - - struct BatchExecutorPolicy { - address executor; - bytes4 selector; - bool allowDelegateCall; - uint256 maxActions; - bool allowNested; - } - - struct ApprovalPolicy { - address token; - address spender; - uint8 kind; - uint256 maxAmount; - bool allowUnlimited; - } - - struct TriggeredSafeCall { - bytes4 selector; - address caller; - bytes input; - uint256 callStart; - uint256 callEnd; - } - - struct OwnerTx { - address to; - uint256 value; - bytes data; - uint8 operation; - } - - struct ModuleTx { - address to; - uint256 value; - bytes data; - uint8 operation; - } - - struct Action { - address safe; - address module; - address target; - uint256 value; - bytes data; - uint256 dataOffset; - uint256 dataLength; - uint8 operation; - bool fromModule; - bool fromBatch; - } - - error SafeTxShapeDuplicateTarget(address target); - error SafeTxShapeDuplicateSelector(address target, bytes4 selector); - error SafeTxShapeDuplicateBatchExecutor(address executor, bytes4 selector); - error SafeTxShapeDuplicateApprovalPolicy(address token, address spender, uint8 kind); - error SafeTxShapeDuplicateModule(address module); - error SafeTxShapeInvalidPolicy(); - error SafeTxShapeTriggeredCallNotFound(bytes4 selector, uint256 callStart); - error SafeTxShapeUnsupportedEntrypoint(bytes4 selector); - error SafeTxShapeModuleExecutionDisabled(address module); - error SafeTxShapeModuleNotAllowed(address module); - error SafeTxShapeUnknownOperation(uint8 operation); - error SafeTxShapeDelegateCallBlocked(address target); - error SafeTxShapeInnerDelegateCallBlocked(address target); - error SafeTxShapeUnknownTarget(address target); - error SafeTxShapeSelectorNotAllowed(address target, bytes4 selector); - error SafeTxShapeCalldataTooShort(address target, uint256 length); - error SafeTxShapeEmptyCalldataBlocked(address target); - error SafeTxShapeFallbackCalldataBlocked(address target, uint256 length); - error SafeTxShapeNativeValueBlocked(address target, bytes4 selector, uint256 value); - error SafeTxShapeBatchDelegateCallNotAllowed(address executor); - error SafeTxShapeBatchPayloadMalformed(); - error SafeTxShapeBatchTooManyActions(uint256 maxActions); - error SafeTxShapeNestedBatchBlocked(address executor); - error SafeTxShapeApprovalMalformed(address token, bytes4 selector); - error SafeTxShapeApprovalTokenUnconfigured(address token, bytes4 selector); - error SafeTxShapeApprovalSpenderNotAllowed(address token, address spender, uint8 kind); - error SafeTxShapeApprovalUnlimitedBlocked(address token, address spender, uint8 kind); - error SafeTxShapeApprovalAmountAboveCap( - address token, address spender, uint8 kind, uint256 amount, uint256 maxAmount - ); - error SafeTxShapeGasRefundBlocked(uint256 gasPrice); - error SafeTxShapeAllowanceReadFailed(address token, address spender); - - TargetPolicy[] public targetPolicies; - SelectorPolicy[] public selectorPolicies; - BatchExecutorPolicy[] public batchExecutorPolicies; - ApprovalPolicy[] public approvalPolicies; - address[] public allowedModules; - - bool public immutable moduleExecutionEnabled; - - constructor( - TargetPolicy[] memory targetPolicies_, - SelectorPolicy[] memory selectorPolicies_, - BatchExecutorPolicy[] memory batchExecutorPolicies_, - ApprovalPolicy[] memory approvalPolicies_, - bool moduleExecutionEnabled_, - address[] memory allowedModules_ - ) { - if (targetPolicies_.length == 0) revert SafeTxShapeInvalidPolicy(); - if (moduleExecutionEnabled_ && allowedModules_.length == 0) revert SafeTxShapeInvalidPolicy(); - - moduleExecutionEnabled = moduleExecutionEnabled_; - - _storeTargetPolicies(targetPolicies_); - _storeSelectorPolicies(selectorPolicies_); - _storeBatchExecutorPolicies(batchExecutorPolicies_); - _storeApprovalPolicies(approvalPolicies_); - _storeAllowedModules(allowedModules_); - - _registerReshiramSpec(); - } - - function targetPolicyCount() external view returns (uint256) { - return targetPolicies.length; - } - - function selectorPolicyCount() external view returns (uint256) { - return selectorPolicies.length; - } - - function batchExecutorPolicyCount() external view returns (uint256) { - return batchExecutorPolicies.length; - } - - function approvalPolicyCount() external view returns (uint256) { - return approvalPolicies.length; - } - - function allowedModuleCount() external view returns (uint256) { - return allowedModules.length; - } - - function _triggeredAction() internal view returns (Action memory action) { - TriggeredSafeCall memory triggered = _resolveTriggeredSafeCall(); - address safe = ph.getAssertionAdopter(); - - if (triggered.selector == EXEC_TRANSACTION_SELECTOR) { - OwnerTx memory ownerTx = _decodeOwnerTx(triggered.input); - return Action({ - safe: safe, - module: address(0), - target: ownerTx.to, - value: ownerTx.value, - data: ownerTx.data, - dataOffset: 0, - dataLength: ownerTx.data.length, - operation: ownerTx.operation, - fromModule: false, - fromBatch: false - }); - } - - if ( - triggered.selector == EXEC_TRANSACTION_FROM_MODULE_SELECTOR - || triggered.selector == EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR - ) { - ModuleTx memory moduleTx = _decodeModuleTx(triggered.input); - return Action({ - safe: safe, - module: triggered.caller, - target: moduleTx.to, - value: moduleTx.value, - data: moduleTx.data, - dataOffset: 0, - dataLength: moduleTx.data.length, - operation: moduleTx.operation, - fromModule: true, - fromBatch: false - }); - } - - revert SafeTxShapeUnsupportedEntrypoint(triggered.selector); - } - - function _validateInnerDelegateCallPolicy(Action memory action) internal pure { - if (action.operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(action.operation); - if (action.operation == OPERATION_DELEGATECALL) revert SafeTxShapeInnerDelegateCallBlocked(action.target); - } - - function _validateInnerTargetSelectorPolicy(Action memory action) internal view { - if (action.operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(action.operation); - if (action.operation == OPERATION_DELEGATECALL) return; - - _validateTargetAndSelector(action); - } - - function _validateInnerApprovalPolicy(Action memory action) internal view { - if (action.operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(action.operation); - if (action.operation == OPERATION_DELEGATECALL) return; - - if (action.dataLength < 4) return; - _validateApproval(action, _selectorAt(action.data, action.dataOffset)); - } - - function _validateMultiSendDelegateCallPolicy(Action memory action, BatchExecutorPolicy storage batchPolicy) - internal - view - { - (uint256 transactionsOffset, uint256 transactionsLength) = _multiSendTransactions(action, batchPolicy); - uint256 offset; - while (offset < transactionsLength) { - (Action memory innerAction, uint256 nextOffset) = - _readMultiSendAction(action, transactionsOffset, transactionsLength, offset); - _validateInnerDelegateCallPolicy(innerAction); - offset = nextOffset; - } - } - - function _validateMultiSendTargetSelectorPolicy(Action memory action, BatchExecutorPolicy storage batchPolicy) - internal - view - { - (uint256 transactionsOffset, uint256 transactionsLength) = _multiSendTransactions(action, batchPolicy); - uint256 offset; - while (offset < transactionsLength) { - (Action memory innerAction, uint256 nextOffset) = - _readMultiSendAction(action, transactionsOffset, transactionsLength, offset); - _validateInnerTargetSelectorPolicy(innerAction); - offset = nextOffset; - } - } - - function _validateMultiSendBatchPolicy(Action memory action, BatchExecutorPolicy storage batchPolicy) - internal - view - { - (uint256 transactionsOffset, uint256 transactionsLength) = _multiSendTransactions(action, batchPolicy); - uint256 offset; - uint256 actionCount; - - while (offset < transactionsLength) { - (Action memory innerAction, uint256 nextOffset) = - _readMultiSendAction(action, transactionsOffset, transactionsLength, offset); - - ++actionCount; - if (actionCount > batchPolicy.maxActions) revert SafeTxShapeBatchTooManyActions(batchPolicy.maxActions); - if (innerAction.operation == OPERATION_DELEGATECALL) { - revert SafeTxShapeInnerDelegateCallBlocked(innerAction.target); - } - if (_isConfiguredBatchCall( - innerAction.target, innerAction.data, innerAction.dataOffset, innerAction.dataLength - )) { - revert SafeTxShapeNestedBatchBlocked(innerAction.target); - } - - offset = nextOffset; - } - } - - function _validateMultiSendApprovalPolicy(Action memory action, BatchExecutorPolicy storage batchPolicy) - internal - view - { - (uint256 transactionsOffset, uint256 transactionsLength) = _multiSendTransactions(action, batchPolicy); - uint256 offset; - while (offset < transactionsLength) { - (Action memory innerAction, uint256 nextOffset) = - _readMultiSendAction(action, transactionsOffset, transactionsLength, offset); - _validateInnerApprovalPolicy(innerAction); - offset = nextOffset; - } - } - - function _multiSendTransactions(Action memory action, BatchExecutorPolicy storage batchPolicy) - internal - view - returns (uint256 transactionsOffset, uint256 transactionsLength) - { - if (action.operation == OPERATION_DELEGATECALL && !batchPolicy.allowDelegateCall) { - revert SafeTxShapeBatchDelegateCallNotAllowed(action.target); - } - - return _decodeSingleBytesArgument(action.data, action.dataOffset, action.dataLength, batchPolicy.selector); - } - - function _readMultiSendAction( - Action memory parent, - uint256 transactionsOffset, - uint256 transactionsLength, - uint256 offset - ) internal pure returns (Action memory innerAction, uint256 nextOffset) { - if (offset > transactionsLength || transactionsLength - offset < MULTISEND_HEADER_LENGTH) { - revert SafeTxShapeBatchPayloadMalformed(); - } - - uint256 entryOffset = transactionsOffset + offset; - uint8 operation = uint8(parent.data[entryOffset]); - if (operation > OPERATION_DELEGATECALL) revert SafeTxShapeUnknownOperation(operation); - - address target = _readPackedAddress(parent.data, entryOffset + 1); - - uint256 value = _readUint256(parent.data, entryOffset + 21); - uint256 dataLength = _readUint256(parent.data, entryOffset + 53); - uint256 dataOffset = entryOffset + MULTISEND_HEADER_LENGTH; - uint256 transactionsEnd = transactionsOffset + transactionsLength; - if (dataOffset > transactionsEnd || dataLength > transactionsEnd - dataOffset) { - revert SafeTxShapeBatchPayloadMalformed(); - } - - innerAction = Action({ - safe: parent.safe, - module: parent.module, - target: target, - value: value, - data: parent.data, - dataOffset: dataOffset, - dataLength: dataLength, - operation: operation, - fromModule: parent.fromModule, - fromBatch: true - }); - nextOffset = offset + MULTISEND_HEADER_LENGTH + dataLength; - } - - function _validateTargetAndSelector(Action memory action) internal view returns (bytes4 selector) { - if (action.target == address(0)) revert SafeTxShapeUnknownTarget(action.target); - - (bool knownTarget, uint256 targetIndex) = _targetPolicyIndex(action.target); - if (!knownTarget) revert SafeTxShapeUnknownTarget(action.target); - - TargetPolicy storage targetPolicy = targetPolicies[targetIndex]; - - if (action.dataLength == 0) { - if (!targetPolicy.allowEmptyCalldata) revert SafeTxShapeEmptyCalldataBlocked(action.target); - if (action.value != 0 && !targetPolicy.allowNonzeroValue) { - revert SafeTxShapeNativeValueBlocked(action.target, bytes4(0), action.value); - } - return bytes4(0); - } - - if (action.dataLength < 4) { - if (!targetPolicy.allowFallbackCalldata) { - revert SafeTxShapeFallbackCalldataBlocked(action.target, action.dataLength); - } - if (action.value != 0 && !targetPolicy.allowNonzeroValue) { - revert SafeTxShapeNativeValueBlocked(action.target, bytes4(0), action.value); - } - return bytes4(0); - } - - selector = _selectorAt(action.data, action.dataOffset); - - if (targetPolicy.allowAnySelector) { - if (action.value != 0 && !targetPolicy.allowNonzeroValue) { - revert SafeTxShapeNativeValueBlocked(action.target, selector, action.value); - } - return selector; - } - - (bool selectorAllowed, uint256 selectorIndex) = _selectorPolicyIndex(action.target, selector); - if (!selectorAllowed) revert SafeTxShapeSelectorNotAllowed(action.target, selector); - - if (action.value != 0 && !selectorPolicies[selectorIndex].allowNonzeroValue) { - revert SafeTxShapeNativeValueBlocked(action.target, selector, action.value); - } - } - - function _validateApproval(Action memory action, bytes4 selector) internal view { - if (selector == APPROVE_SELECTOR) { - if (action.dataLength != 68) revert SafeTxShapeApprovalMalformed(action.target, selector); - - address spender = _readAbiAddress(action.data, action.dataOffset + 4); - uint256 amountOrTokenId = _readUint256(action.data, action.dataOffset + 36); - bool erc20 = _tokenHasApprovalKind(action.target, APPROVAL_KIND_ERC20_APPROVE); - bool erc721 = _tokenHasApprovalKind(action.target, APPROVAL_KIND_ERC721_APPROVE); - - if (erc20) { - if (amountOrTokenId == 0) return; - _validateNumericApproval(action.target, spender, APPROVAL_KIND_ERC20_APPROVE, amountOrTokenId); - return; - } - - if (erc721) { - if (spender == address(0)) return; - _validateOperatorApproval(action.target, spender, APPROVAL_KIND_ERC721_APPROVE); - return; - } - - revert SafeTxShapeApprovalTokenUnconfigured(action.target, selector); - } - - if (selector == INCREASE_ALLOWANCE_SELECTOR) { - if (action.dataLength != 68) revert SafeTxShapeApprovalMalformed(action.target, selector); - if (!_tokenHasApprovalKind(action.target, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE)) { - revert SafeTxShapeApprovalTokenUnconfigured(action.target, selector); - } - - address spender = _readAbiAddress(action.data, action.dataOffset + 4); - uint256 addedValue = _readUint256(action.data, action.dataOffset + 36); - if (addedValue == 0) return; - - _validateIncreaseAllowanceFinalState(action.safe, action.target, spender); - return; - } - - if (selector == SET_APPROVAL_FOR_ALL_SELECTOR) { - if (action.dataLength != 68) revert SafeTxShapeApprovalMalformed(action.target, selector); - - bool erc721 = _tokenHasApprovalKind(action.target, APPROVAL_KIND_ERC721_SET_APPROVAL_FOR_ALL); - bool erc1155 = _tokenHasApprovalKind(action.target, APPROVAL_KIND_ERC1155_SET_APPROVAL_FOR_ALL); - if (!erc721 && !erc1155) revert SafeTxShapeApprovalTokenUnconfigured(action.target, selector); - - address operator = _readAbiAddress(action.data, action.dataOffset + 4); - bool approved = _readAbiBool(action.data, action.dataOffset + 36); - if (!approved) return; - - if (erc721 && _operatorApprovalAllowed(action.target, operator, APPROVAL_KIND_ERC721_SET_APPROVAL_FOR_ALL)) - { - return; - } - if ( - erc1155 && _operatorApprovalAllowed(action.target, operator, APPROVAL_KIND_ERC1155_SET_APPROVAL_FOR_ALL) - ) { - return; - } - - revert SafeTxShapeApprovalSpenderNotAllowed( - action.target, - operator, - erc721 ? APPROVAL_KIND_ERC721_SET_APPROVAL_FOR_ALL : APPROVAL_KIND_ERC1155_SET_APPROVAL_FOR_ALL - ); - } - } - - function _validateNumericApproval(address token, address spender, uint8 kind, uint256 amount) internal view { - for (uint256 i; i < approvalPolicies.length; ++i) { - ApprovalPolicy storage policy = approvalPolicies[i]; - if (policy.token == token && policy.spender == spender && policy.kind == kind) { - if (amount == type(uint256).max) { - if (!policy.allowUnlimited) revert SafeTxShapeApprovalUnlimitedBlocked(token, spender, kind); - return; - } - - if (amount > policy.maxAmount) { - revert SafeTxShapeApprovalAmountAboveCap(token, spender, kind, amount, policy.maxAmount); - } - return; - } - } - - revert SafeTxShapeApprovalSpenderNotAllowed(token, spender, kind); - } - - function _validateIncreaseAllowanceFinalState(address safe, address token, address spender) internal view { - ApprovalPolicy storage policy = _approvalPolicy(token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE); - - PhEvm.TriggerContext memory triggerCtx = ph.context(); - PhEvm.StaticCallResult memory result = ph.staticcallAt( - token, - abi.encodeWithSignature("allowance(address,address)", safe, spender), - ALLOWANCE_READ_GAS, - _postCall(triggerCtx.callEnd) - ); - if (!result.ok || result.data.length < 32) { - revert SafeTxShapeAllowanceReadFailed(token, spender); - } - - uint256 finalAllowance = abi.decode(result.data, (uint256)); - if (finalAllowance == type(uint256).max) { - if (!policy.allowUnlimited) { - revert SafeTxShapeApprovalUnlimitedBlocked(token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE); - } - return; - } - if (finalAllowance > policy.maxAmount) { - revert SafeTxShapeApprovalAmountAboveCap( - token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE, finalAllowance, policy.maxAmount - ); - } - } - - function _approvalPolicy(address token, address spender, uint8 kind) - internal - view - returns (ApprovalPolicy storage policy) - { - for (uint256 i; i < approvalPolicies.length; ++i) { - policy = approvalPolicies[i]; - if (policy.token == token && policy.spender == spender && policy.kind == kind) return policy; - } - revert SafeTxShapeApprovalSpenderNotAllowed(token, spender, kind); - } - - function _validateOperatorApproval(address token, address operator, uint8 kind) internal view { - if (!_operatorApprovalAllowed(token, operator, kind)) { - revert SafeTxShapeApprovalSpenderNotAllowed(token, operator, kind); - } - } - - function _operatorApprovalAllowed(address token, address operator, uint8 kind) internal view returns (bool) { - if (operator == address(0)) return false; - - for (uint256 i; i < approvalPolicies.length; ++i) { - ApprovalPolicy storage policy = approvalPolicies[i]; - if (policy.token == token && policy.spender == operator && policy.kind == kind) { - return true; - } - } - - return false; - } - - function _validateModuleCaller(address module) internal view { - if (!moduleExecutionEnabled) revert SafeTxShapeModuleExecutionDisabled(module); - - for (uint256 i; i < allowedModules.length; ++i) { - if (allowedModules[i] == module) return; - } - - revert SafeTxShapeModuleNotAllowed(module); - } - - function _resolveTriggeredSafeCall() internal view returns (TriggeredSafeCall memory triggered) { - address safe = ph.getAssertionAdopter(); - PhEvm.TriggerContext memory context = ph.context(); - PhEvm.CallInputs[] memory calls = ph.getAllCallInputs(safe, context.selector); - - for (uint256 i; i < calls.length; ++i) { - if (calls[i].id == context.callStart) { - return TriggeredSafeCall({ - selector: context.selector, - caller: calls[i].caller, - input: ph.callinputAt(context.callStart), - callStart: context.callStart, - callEnd: context.callEnd - }); - } - } - - revert SafeTxShapeTriggeredCallNotFound(context.selector, context.callStart); - } - - function _decodeOwnerTx(bytes memory input) internal pure returns (OwnerTx memory ownerTx) { - if (input.length < 324 || _selector(input) != EXEC_TRANSACTION_SELECTOR) { - revert SafeTxShapeBatchPayloadMalformed(); - } - - uint256 gasPrice = _readUint256(input, 196); - if (gasPrice != 0) revert SafeTxShapeGasRefundBlocked(gasPrice); - - ownerTx.to = _readAbiAddress(input, 4); - ownerTx.value = _readUint256(input, 36); - ownerTx.data = _readDynamicBytes(input, 4, _readUint256(input, 68)); - ownerTx.operation = _readAbiUint8(input, 100); - } - - function _decodeModuleTx(bytes memory input) internal pure returns (ModuleTx memory moduleTx) { - if ( - input.length < 132 - || (_selector(input) != EXEC_TRANSACTION_FROM_MODULE_SELECTOR - && _selector(input) != EXEC_TRANSACTION_FROM_MODULE_RETURN_DATA_SELECTOR) - ) { - revert SafeTxShapeBatchPayloadMalformed(); - } - - moduleTx.to = _readAbiAddress(input, 4); - moduleTx.value = _readUint256(input, 36); - moduleTx.data = _readDynamicBytes(input, 4, _readUint256(input, 68)); - moduleTx.operation = _readAbiUint8(input, 100); - } - - function _decodeSingleBytesArgument( - bytes memory input, - uint256 inputOffset, - uint256 inputLength, - bytes4 expectedSelector - ) internal pure returns (uint256 argumentOffset, uint256 argumentLength) { - if (inputOffset > input.length || inputLength > input.length - inputOffset || inputLength < 68) { - revert SafeTxShapeBatchPayloadMalformed(); - } - if (_selectorAt(input, inputOffset) != expectedSelector) revert SafeTxShapeBatchPayloadMalformed(); - - uint256 offset = _readUint256(input, inputOffset + 4); - if (offset != 32) revert SafeTxShapeBatchPayloadMalformed(); - - uint256 inputEnd = inputOffset + inputLength; - uint256 lengthOffset = inputOffset + 4 + offset; - argumentLength = _readUint256(input, lengthOffset); - argumentOffset = lengthOffset + 32; - if (argumentOffset > inputEnd) revert SafeTxShapeBatchPayloadMalformed(); - if (argumentLength > inputEnd - argumentOffset) revert SafeTxShapeBatchPayloadMalformed(); - - uint256 paddedLength = argumentLength; - uint256 remainder = argumentLength % 32; - if (remainder != 0) paddedLength += 32 - remainder; - - uint256 paddedEnd = argumentOffset + paddedLength; - if (paddedEnd != inputEnd) revert SafeTxShapeBatchPayloadMalformed(); - for (uint256 i = argumentLength; i < paddedLength; ++i) { - if (input[argumentOffset + i] != 0) revert SafeTxShapeBatchPayloadMalformed(); - } - } - - function _batchPolicyForAction(address target, bytes memory data, uint256 dataOffset, uint256 dataLength) - internal - view - returns (bool found, uint256 index) - { - if (dataLength < 4) return (false, 0); - return _batchPolicyIndex(target, _selectorAt(data, dataOffset)); - } - - function _isConfiguredBatchCall(address target, bytes memory data, uint256 dataOffset, uint256 dataLength) - internal - view - returns (bool) - { - if (dataLength < 4) return false; - (bool found,) = _batchPolicyIndex(target, _selectorAt(data, dataOffset)); - return found; - } - - function _targetPolicyIndex(address target) internal view returns (bool found, uint256 index) { - for (uint256 i; i < targetPolicies.length; ++i) { - if (targetPolicies[i].target == target) return (true, i); - } - return (false, 0); - } - - function _selectorPolicyIndex(address target, bytes4 selector) internal view returns (bool found, uint256 index) { - for (uint256 i; i < selectorPolicies.length; ++i) { - if (selectorPolicies[i].target == target && selectorPolicies[i].selector == selector) return (true, i); - } - return (false, 0); - } - - function _batchPolicyIndex(address executor, bytes4 selector) internal view returns (bool found, uint256 index) { - for (uint256 i; i < batchExecutorPolicies.length; ++i) { - if (batchExecutorPolicies[i].executor == executor && batchExecutorPolicies[i].selector == selector) { - return (true, i); - } - } - return (false, 0); - } - - function _tokenHasApprovalKind(address token, uint8 kind) internal view returns (bool) { - for (uint256 i; i < approvalPolicies.length; ++i) { - if (approvalPolicies[i].token == token && approvalPolicies[i].kind == kind) return true; - } - return false; - } - - function _storeTargetPolicies(TargetPolicy[] memory policies) private { - for (uint256 i; i < policies.length; ++i) { - if (policies[i].target == address(0)) revert SafeTxShapeInvalidPolicy(); - - for (uint256 j; j < i; ++j) { - if (policies[j].target == policies[i].target) revert SafeTxShapeDuplicateTarget(policies[i].target); - } - - targetPolicies.push(policies[i]); - } - } - - function _storeSelectorPolicies(SelectorPolicy[] memory policies) private { - for (uint256 i; i < policies.length; ++i) { - if (policies[i].target == address(0) || policies[i].selector == bytes4(0)) { - revert SafeTxShapeInvalidPolicy(); - } - if (!_targetPolicyExistsInMemory(policies[i].target)) revert SafeTxShapeInvalidPolicy(); - - for (uint256 j; j < i; ++j) { - if (policies[j].target == policies[i].target && policies[j].selector == policies[i].selector) { - revert SafeTxShapeDuplicateSelector(policies[i].target, policies[i].selector); - } - } - - selectorPolicies.push(policies[i]); - } - } - - function _storeBatchExecutorPolicies(BatchExecutorPolicy[] memory policies) private { - for (uint256 i; i < policies.length; ++i) { - if ( - policies[i].executor == address(0) || policies[i].selector == bytes4(0) || policies[i].maxActions == 0 - || policies[i].allowNested - ) { - revert SafeTxShapeInvalidPolicy(); - } - - for (uint256 j; j < i; ++j) { - if (policies[j].executor == policies[i].executor && policies[j].selector == policies[i].selector) { - revert SafeTxShapeDuplicateBatchExecutor(policies[i].executor, policies[i].selector); - } - } - - batchExecutorPolicies.push(policies[i]); - } - } - - function _storeApprovalPolicies(ApprovalPolicy[] memory policies) private { - for (uint256 i; i < policies.length; ++i) { - if ( - policies[i].token == address(0) || policies[i].spender == address(0) - || !_isSupportedApprovalKind(policies[i].kind) - ) { - revert SafeTxShapeInvalidPolicy(); - } - - for (uint256 j; j < i; ++j) { - if ( - policies[j].token == policies[i].token && policies[j].spender == policies[i].spender - && policies[j].kind == policies[i].kind - ) { - revert SafeTxShapeDuplicateApprovalPolicy(policies[i].token, policies[i].spender, policies[i].kind); - } - - if ( - policies[j].token == policies[i].token - && ((policies[j].kind == APPROVAL_KIND_ERC20_APPROVE - && policies[i].kind == APPROVAL_KIND_ERC721_APPROVE) - || (policies[j].kind == APPROVAL_KIND_ERC721_APPROVE - && policies[i].kind == APPROVAL_KIND_ERC20_APPROVE)) - ) { - revert SafeTxShapeInvalidPolicy(); - } - } - - approvalPolicies.push(policies[i]); - } - } - - function _storeAllowedModules(address[] memory modules) private { - for (uint256 i; i < modules.length; ++i) { - if (modules[i] == address(0)) revert SafeTxShapeInvalidPolicy(); - - for (uint256 j; j < i; ++j) { - if (modules[j] == modules[i]) revert SafeTxShapeDuplicateModule(modules[i]); - } - - allowedModules.push(modules[i]); - } - } - - function _targetPolicyExistsInMemory(address target) private view returns (bool) { - for (uint256 i; i < targetPolicies.length; ++i) { - if (targetPolicies[i].target == target) return true; - } - return false; - } - - function _isSupportedApprovalKind(uint8 kind) private pure returns (bool) { - return kind == APPROVAL_KIND_ERC20_APPROVE || kind == APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE - || kind == APPROVAL_KIND_ERC721_APPROVE || kind == APPROVAL_KIND_ERC721_SET_APPROVAL_FOR_ALL - || kind == APPROVAL_KIND_ERC1155_SET_APPROVAL_FOR_ALL; - } - - function _registerReshiramSpec() internal { - (bool ok,) = SPEC_RECORDER.call( - abi.encodeWithSelector(bytes4(keccak256("registerAssertionSpec(uint8)")), AssertionSpec.Reshiram) - ); - if (!ok) revert SafeTxShapeInvalidPolicy(); - } - - function _stripSelector(bytes memory input) internal pure returns (bytes memory args) { - if (input.length < 4) revert SafeTxShapeCalldataTooShort(address(0), input.length); - args = _slice(input, 4, input.length - 4); - } - - function _selector(bytes memory input) internal pure returns (bytes4 selector) { - if (input.length < 4) revert SafeTxShapeCalldataTooShort(address(0), input.length); - selector = _selectorAt(input, 0); - } - - function _selectorAt(bytes memory input, uint256 offset) internal pure returns (bytes4 selector) { - if (offset > input.length || input.length - offset < 4) { - revert SafeTxShapeCalldataTooShort(address(0), input.length); - } - selector = bytes4( - (uint32(uint8(input[offset])) << 24) | (uint32(uint8(input[offset + 1])) << 16) - | (uint32(uint8(input[offset + 2])) << 8) | uint32(uint8(input[offset + 3])) - ); - } - - function _readAbiAddress(bytes memory data, uint256 offset) internal pure returns (address value) { - value = address(uint160(_readUint256(data, offset))); - } - - function _readAbiUint8(bytes memory data, uint256 offset) internal pure returns (uint8 value) { - uint256 raw = _readUint256(data, offset); - if (raw > type(uint8).max) revert SafeTxShapeBatchPayloadMalformed(); - value = uint8(raw); - } - - function _readAbiBool(bytes memory data, uint256 offset) internal pure returns (bool value) { - uint256 raw = _readUint256(data, offset); - if (raw > 1) revert SafeTxShapeApprovalMalformed(address(0), bytes4(0)); - value = raw == 1; - } - - function _readDynamicBytes(bytes memory data, uint256 headStart, uint256 relativeOffset) - internal - pure - returns (bytes memory value) - { - if (headStart > data.length || relativeOffset > data.length - headStart) { - revert SafeTxShapeBatchPayloadMalformed(); - } - uint256 lengthOffset = headStart + relativeOffset; - uint256 valueLength = _readUint256(data, lengthOffset); - uint256 valueOffset = lengthOffset + 32; - if (valueOffset > data.length) revert SafeTxShapeBatchPayloadMalformed(); - if (valueLength > data.length - valueOffset) revert SafeTxShapeBatchPayloadMalformed(); - value = _slice(data, valueOffset, valueLength); - } - - function _readPackedAddress(bytes memory data, uint256 offset) internal pure returns (address value) { - if (offset > data.length || data.length - offset < 20) revert SafeTxShapeBatchPayloadMalformed(); - uint160 result; - for (uint256 i; i < 20; ++i) { - result = (result << 8) | uint160(uint8(data[offset + i])); - } - value = address(result); - } - - function _readUint256(bytes memory data, uint256 offset) internal pure returns (uint256 value) { - if (offset > data.length || data.length - offset < 32) revert SafeTxShapeBatchPayloadMalformed(); - for (uint256 i; i < 32; ++i) { - value = (value << 8) | uint256(uint8(data[offset + i])); - } - } - - function _slice(bytes memory data, uint256 offset, uint256 length) internal pure returns (bytes memory out) { - if (offset > data.length || length > data.length - offset) revert SafeTxShapeBatchPayloadMalformed(); - out = new bytes(length); - for (uint256 i; i < length; ++i) { - out[i] = data[offset + i]; - } - } -} +// Re-export the single maintained helper symbol for existing example imports. +import {SafeTxShapeHelpers} from "credible-std/protection/safe/SafeTxShapeHelpers.sol"; diff --git a/src/protection/safe/README.md b/src/protection/safe/README.md index 9b413bb..dd2fde4 100644 --- a/src/protection/safe/README.md +++ b/src/protection/safe/README.md @@ -179,7 +179,7 @@ Approval policy: Approval resets and revocations are allowed by default when the token is configured for that approval kind: ERC-20 `approve(spender, 0)`, ERC-721 `approve(address(0), tokenId)`, and `setApprovalForAll(operator, false)` reduce approval risk. Risk-increasing approvals to untrusted spenders/operators, ERC-20 unlimited approvals without explicit permission, and ERC-20 amounts above cap are blocked. -For ERC-20 `approve(spender, amount)` the cap binds `amount` directly. For ERC-20 `increaseAllowance(spender, addedValue)` the cap binds the post-state `allowance(safe, spender)` so two consecutive `increaseAllowance` calls inside a `MultiSend` cannot stack above the cap. +For ERC-20 `approve(spender, amount)` the cap binds `amount` directly. For ERC-20 `increaseAllowance(spender, addedValue)` the cap binds the pre-execution allowance plus all positive grants requested for that owner/token/spender in the batch. This is intentionally stricter than checking final state: consuming or reducing an oversized transient allowance later in the transaction does not make the batch valid. For CALL-based executors, the executor is the token owner; for DELEGATECALL-based executors, the Safe is the owner. ### Material Effect @@ -224,7 +224,7 @@ Owner and module set hashes are computed by sorting addresses ascending and hash For modules, `bytes32(0)` in `approvedModuleSetHashes` means modules must be disabled. This is useful when the safest policy is that only owner-approved Safe transactions may execute. -Module-set checks paginate Safe modules in pages of 256. Very large module sets may exceed the assertion gas limit while reading and hashing the full set; keep protected Safes below that practical cap or split module-heavy operational surfaces behind a smaller approved module set. +Module-set checks paginate Safe modules in pages of 256. The complete set is always read and hashed; very large sets can exceed the assertion execution budget, so deployments should benchmark their configured set size and prefer a smaller operational module surface. The regression suite covers a 32-module set within the current assertion gas limit; this is a tested bound, not a protocol-enforced maximum. ## Material Effect diff --git a/src/protection/safe/SafeTxShapeHelpers.sol b/src/protection/safe/SafeTxShapeHelpers.sol index 47ce158..31612f8 100644 --- a/src/protection/safe/SafeTxShapeHelpers.sol +++ b/src/protection/safe/SafeTxShapeHelpers.sol @@ -293,11 +293,52 @@ abstract contract SafeTxShapeHelpers is Assertion { while (offset < transactionsLength) { (Action memory innerAction, uint256 nextOffset) = _readMultiSendAction(action, transactionsOffset, transactionsLength, offset); - _validateInnerApprovalPolicy(innerAction); + if ( + innerAction.operation == OPERATION_CALL && innerAction.dataLength == 68 + && _selectorAt(innerAction.data, innerAction.dataOffset) == INCREASE_ALLOWANCE_SELECTOR + ) { + address spender = _readAbiAddress(innerAction.data, innerAction.dataOffset + 4); + uint256 cumulativeIncrease = _cumulativeIncreaseAllowance( + action, transactionsOffset, transactionsLength, nextOffset, innerAction.target, spender + ); + _validateIncreaseAllowanceFromPreState( + innerAction.caller, innerAction.target, spender, cumulativeIncrease + ); + } else { + _validateInnerApprovalPolicy(innerAction); + } offset = nextOffset; } } + /// @dev Sums every positive grant to the same allowance up to the current batch entry. This is + /// intentionally conservative: later `transferFrom` calls or allowance reductions do not + /// erase a transient grant that exceeded policy during execution. + function _cumulativeIncreaseAllowance( + Action memory parent, + uint256 transactionsOffset, + uint256 transactionsLength, + uint256 throughOffset, + address token, + address spender + ) internal pure returns (uint256 total) { + uint256 cursor; + while (cursor < throughOffset) { + (Action memory candidate, uint256 nextCursor) = + _readMultiSendAction(parent, transactionsOffset, transactionsLength, cursor); + if ( + candidate.operation == OPERATION_CALL && candidate.target == token && candidate.dataLength == 68 + && _selectorAt(candidate.data, candidate.dataOffset) == INCREASE_ALLOWANCE_SELECTOR + && _readAbiAddress(candidate.data, candidate.dataOffset + 4) == spender + ) { + uint256 increase = _readUint256(candidate.data, candidate.dataOffset + 36); + if (increase > type(uint256).max - total) return type(uint256).max; + total += increase; + } + cursor = nextCursor; + } + } + function _multiSendTransactions(Action memory action, BatchExecutorPolicy storage batchPolicy) internal view @@ -430,10 +471,7 @@ abstract contract SafeTxShapeHelpers is Assertion { uint256 addedValue = _readUint256(action.data, action.dataOffset + 36); if (addedValue == 0) return; - // `increaseAllowance(spender, addedValue)` adds to the current allowance; treating `addedValue` - // as the final amount would let two inner calls land above `maxAmount`. Verify the post-state - // allowance instead so the cap binds the actual final allowance after the transaction. - _validateIncreaseAllowanceFinalState(action.caller, action.target, spender); + _validateIncreaseAllowanceFromPreState(action.caller, action.target, spender, addedValue); return; } @@ -482,33 +520,34 @@ abstract contract SafeTxShapeHelpers is Assertion { } } - function _validateIncreaseAllowanceFinalState(address owner, address token, address spender) internal view { + function _validateIncreaseAllowanceFromPreState( + address owner, + address token, + address spender, + uint256 cumulativeIncrease + ) internal view { if (!_approvalPolicyExists[token][spender][APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE]) { revert SafeTxShapeApprovalSpenderNotAllowed(token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE); } PhEvm.TriggerContext memory triggerCtx = ph.context(); - PhEvm.ForkId memory postFork = _postCall(triggerCtx.callEnd); + PhEvm.ForkId memory preFork = _preCall(triggerCtx.callStart); PhEvm.StaticCallResult memory result = ph.staticcallAt( - token, abi.encodeWithSignature("allowance(address,address)", owner, spender), ALLOWANCE_READ_GAS, postFork + token, abi.encodeWithSignature("allowance(address,address)", owner, spender), ALLOWANCE_READ_GAS, preFork ); - if (!result.ok || result.data.length < 32) { + if (!result.ok || result.data.length != 32) { revert SafeTxShapeAllowanceReadFailed(token, spender); } - uint256 finalAllowance = abi.decode(result.data, (uint256)); + uint256 initialAllowance = abi.decode(result.data, (uint256)); ApprovalPolicy storage policy = _approvalPolicyByKey[token][spender][APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE]; - - if (finalAllowance == type(uint256).max) { - if (!policy.allowUnlimited) { - revert SafeTxShapeApprovalUnlimitedBlocked(token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE); - } - return; - } - if (finalAllowance > policy.maxAmount) { + if (cumulativeIncrease > policy.maxAmount || initialAllowance > policy.maxAmount - cumulativeIncrease) { + uint256 peakAllowance = cumulativeIncrease > type(uint256).max - initialAllowance + ? type(uint256).max + : initialAllowance + cumulativeIncrease; revert SafeTxShapeApprovalAmountAboveCap( - token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE, finalAllowance, policy.maxAmount + token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE, peakAllowance, policy.maxAmount ); } } diff --git a/test/protection/safe/SafeTxShapeAssertion.t.sol b/test/protection/safe/SafeTxShapeAssertion.t.sol index 479e581..49209a2 100644 --- a/test/protection/safe/SafeTxShapeAssertion.t.sol +++ b/test/protection/safe/SafeTxShapeAssertion.t.sol @@ -565,13 +565,44 @@ contract SafeTxShapeAssertionTest is Test, CredibleTest { address(erc20Token), TRUSTED_SPENDER, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE, - uint256(101), + uint256(102), uint256(100) ) ); _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_CALL); } + function testBlocksTransientAllowanceAcrossMultipleBatchGrants() public { + _armBaselinePolicyFor(false, SafeTxShapeAssertion.assertSafeApprovalPolicy.selector); + + bytes memory txs = bytes.concat( + _packMultiSendTx( + OP_CALL, + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, uint256(60)) + ), + _packMultiSendTx( + OP_CALL, + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, uint256(60)) + ) + ); + + vm.expectRevert( + abi.encodeWithSelector( + SafeTxShapeHelpers.SafeTxShapeApprovalAmountAboveCap.selector, + address(erc20Token), + TRUSTED_SPENDER, + APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE, + uint256(120), + uint256(100) + ) + ); + _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); + } + function testBlocksDuplicatePolicyEntries() public { SafeTxShapeHelpers.TargetPolicy[] memory targets = _baselineTargets(); targets[1].target = targets[0].target; diff --git a/test/protection/safe/integration/CredibleSafeGuardScripts.t.sol b/test/protection/safe/integration/CredibleSafeGuardScripts.t.sol index dde06dd..bf22ae4 100644 --- a/test/protection/safe/integration/CredibleSafeGuardScripts.t.sol +++ b/test/protection/safe/integration/CredibleSafeGuardScripts.t.sol @@ -34,6 +34,24 @@ contract NonCanonicalBoolRegistry { } } +contract SlowRegistry { + function isCredibleBlock(uint256) external pure returns (bool) { + uint256 value; + while (true) value++; + return value == 0; + } +} + +contract OversizedRegistryResponse { + fallback() external { + assembly { + mstore(0x00, 0) + mstore(0x20, 0) + return(0x00, 0x40) + } + } +} + contract CredibleSafeGuardScriptsTest is Test { bytes32 internal constant GUARD_STORAGE_SLOT = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8; bytes32 internal constant REFERENCE_CHECKSUM = 0x8994ee462d748c24ecd7804083007dd231e36ff84da4b272921c30d1ae7f0df0; @@ -123,6 +141,39 @@ contract CredibleSafeGuardScriptsTest is Test { deployer.validateRegistry(address(badRegistry)); } + function test_validateRegistry_rejectsSlowRegistryWithinRuntimeGasBound() public { + SlowRegistry slowRegistry = new SlowRegistry(); + vm.expectRevert( + abi.encodeWithSelector( + DeployCredibleSafeGuard.RegistryReadFailed.selector, address(slowRegistry), "isCredibleBlock" + ) + ); + deployer.validateRegistry(address(slowRegistry)); + } + + function test_validateRegistry_rejectsOversizedReturnData() public { + OversizedRegistryResponse oversized = new OversizedRegistryResponse(); + vm.expectRevert( + abi.encodeWithSelector( + DeployCredibleSafeGuard.RegistryReadFailed.selector, address(oversized), "isCredibleBlock" + ) + ); + deployer.validateRegistry(address(oversized)); + } + + function test_validateRegistry_rejectsFutureLastCredibleBlock() public { + registry.setLastCredibleBlock(block.number + 1); + vm.expectRevert( + abi.encodeWithSelector( + DeployCredibleSafeGuard.RegistryLastCredibleBlockInFuture.selector, + address(registry), + block.number + 1, + block.number + ) + ); + deployer.validateRegistry(address(registry)); + } + function test_installBatch_matchesSafeTransactionBuilderSchema() public { CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD, PROTOCOL_MANAGER); string memory json = generator.buildInstallBatch(address(safe), address(guard), block.chainid, CREATED_AT); From de064dfceb6ab8f4714635eb4e528f755fc6394b Mon Sep 17 00:00:00 2001 From: makemake Date: Mon, 3 Aug 2026 14:47:39 +0200 Subject: [PATCH 2/6] fix(safe): preserve unlimited allowance policy --- src/protection/safe/SafeTxShapeHelpers.sol | 1 + .../safe/SafeTxShapeAssertion.t.sol | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/protection/safe/SafeTxShapeHelpers.sol b/src/protection/safe/SafeTxShapeHelpers.sol index 31612f8..e640fcf 100644 --- a/src/protection/safe/SafeTxShapeHelpers.sol +++ b/src/protection/safe/SafeTxShapeHelpers.sol @@ -546,6 +546,7 @@ abstract contract SafeTxShapeHelpers is Assertion { uint256 peakAllowance = cumulativeIncrease > type(uint256).max - initialAllowance ? type(uint256).max : initialAllowance + cumulativeIncrease; + if (peakAllowance == type(uint256).max && policy.allowUnlimited) return; revert SafeTxShapeApprovalAmountAboveCap( token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE, peakAllowance, policy.maxAmount ); diff --git a/test/protection/safe/SafeTxShapeAssertion.t.sol b/test/protection/safe/SafeTxShapeAssertion.t.sol index 49209a2..5094c2b 100644 --- a/test/protection/safe/SafeTxShapeAssertion.t.sol +++ b/test/protection/safe/SafeTxShapeAssertion.t.sol @@ -603,6 +603,28 @@ contract SafeTxShapeAssertionTest is Test, CredibleTest { _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); } + function testIncreaseAllowanceCanReachUnlimitedWhenPolicyAllowsIt() public { + SafeTxShapeHelpers.ApprovalPolicy[] memory approvals = _approvalPolicies(false); + approvals[1].allowUnlimited = true; + + _armPolicyFor( + _baselineTargets(), + _baselineSelectors(), + _baselineBatchPolicies(4), + approvals, + false, + _noModules(), + SafeTxShapeAssertion.assertSafeApprovalPolicy.selector + ); + + _execOwner( + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, type(uint256).max), + OP_CALL + ); + } + function testBlocksDuplicatePolicyEntries() public { SafeTxShapeHelpers.TargetPolicy[] memory targets = _baselineTargets(); targets[1].target = targets[0].target; From 69a56c02391d57056bf1212a940ad48ebb7e0bd4 Mon Sep 17 00:00:00 2001 From: makemake Date: Tue, 4 Aug 2026 14:28:21 +0200 Subject: [PATCH 3/6] fix(safe): bound batch approval validation cost --- src/protection/safe/README.md | 4 +- src/protection/safe/SafeTxShapeHelpers.sol | 94 +++++++++++-------- .../safe/SafeTxShapeAssertion.t.sol | 69 ++++++++++++++ 3 files changed, 125 insertions(+), 42 deletions(-) diff --git a/src/protection/safe/README.md b/src/protection/safe/README.md index dd2fde4..f1ed8dd 100644 --- a/src/protection/safe/README.md +++ b/src/protection/safe/README.md @@ -160,7 +160,7 @@ Batch executor policy: - approved executor address; - approved batch selector, normally `multiSend(bytes)`; - whether top-level delegatecall to that executor is allowed; -- maximum inner action count; +- maximum inner action count, capped globally at four based on the PCL assertion-gas regression; - nested batching flag, reserved for future support and rejected in this MVP. Module policy: @@ -179,7 +179,7 @@ Approval policy: Approval resets and revocations are allowed by default when the token is configured for that approval kind: ERC-20 `approve(spender, 0)`, ERC-721 `approve(address(0), tokenId)`, and `setApprovalForAll(operator, false)` reduce approval risk. Risk-increasing approvals to untrusted spenders/operators, ERC-20 unlimited approvals without explicit permission, and ERC-20 amounts above cap are blocked. -For ERC-20 `approve(spender, amount)` the cap binds `amount` directly. For ERC-20 `increaseAllowance(spender, addedValue)` the cap binds the pre-execution allowance plus all positive grants requested for that owner/token/spender in the batch. This is intentionally stricter than checking final state: consuming or reducing an oversized transient allowance later in the transaction does not make the batch valid. For CALL-based executors, the executor is the token owner; for DELEGATECALL-based executors, the Safe is the owner. +For ERC-20 `approve(spender, amount)` the cap binds `amount` directly. For ERC-20 `increaseAllowance(spender, addedValue)` the cap binds the pre-execution allowance plus all positive grants requested for that owner/token/spender in the batch. This is intentionally stricter than checking final state: consuming or reducing an oversized transient allowance later in the transaction does not make the batch valid. The approval assertion decodes the batch once, accumulates each configured allowance by policy index, and reads each initial allowance once, so multiple grants do not trigger repeated prefix rescans. Batch policies cannot configure more than four actions; the four-grant regression executes below PCL's 300,000-gas local assertion ceiling, while the former unbounded configuration could exhaust the budget before reaching a policy decision. For CALL-based executors, the executor is the token owner; for DELEGATECALL-based executors, the Safe is the owner. ### Material Effect diff --git a/src/protection/safe/SafeTxShapeHelpers.sol b/src/protection/safe/SafeTxShapeHelpers.sol index e640fcf..39d2255 100644 --- a/src/protection/safe/SafeTxShapeHelpers.sol +++ b/src/protection/safe/SafeTxShapeHelpers.sol @@ -32,6 +32,7 @@ abstract contract SafeTxShapeHelpers is Assertion { uint256 internal constant MULTISEND_HEADER_LENGTH = 85; uint64 internal constant ALLOWANCE_READ_GAS = 500_000; + uint256 internal constant MAX_BATCH_ACTIONS = 4; struct TargetPolicy { address target; @@ -144,6 +145,8 @@ abstract contract SafeTxShapeHelpers is Assertion { mapping(address token => mapping(address spender => mapping(uint8 kind => ApprovalPolicy))) internal _approvalPolicyByKey; mapping(address token => mapping(address spender => mapping(uint8 kind => bool))) internal _approvalPolicyExists; + mapping(address token => mapping(address spender => mapping(uint8 kind => uint256 indexPlusOne))) internal + _approvalPolicyIndexPlusOne; mapping(address token => mapping(uint8 kind => bool)) internal _tokenApprovalKindRegistered; mapping(address module => bool allowed) internal _allowedModule; @@ -289,21 +292,49 @@ abstract contract SafeTxShapeHelpers is Assertion { view { (uint256 transactionsOffset, uint256 transactionsLength) = _multiSendTransactions(action, batchPolicy); + uint256[] memory cumulativeIncreases = new uint256[](approvalPolicies.length); + uint256[] memory initialAllowances = new uint256[](approvalPolicies.length); + bool[] memory allowanceLoaded = new bool[](approvalPolicies.length); uint256 offset; + uint256 actionCount; while (offset < transactionsLength) { (Action memory innerAction, uint256 nextOffset) = _readMultiSendAction(action, transactionsOffset, transactionsLength, offset); + ++actionCount; + if (actionCount > batchPolicy.maxActions) revert SafeTxShapeBatchTooManyActions(batchPolicy.maxActions); if ( innerAction.operation == OPERATION_CALL && innerAction.dataLength == 68 && _selectorAt(innerAction.data, innerAction.dataOffset) == INCREASE_ALLOWANCE_SELECTOR ) { address spender = _readAbiAddress(innerAction.data, innerAction.dataOffset + 4); - uint256 cumulativeIncrease = _cumulativeIncreaseAllowance( - action, transactionsOffset, transactionsLength, nextOffset, innerAction.target, spender - ); - _validateIncreaseAllowanceFromPreState( - innerAction.caller, innerAction.target, spender, cumulativeIncrease - ); + uint256 addedValue = _readUint256(innerAction.data, innerAction.dataOffset + 36); + if (addedValue == 0) { + _validateInnerApprovalPolicy(innerAction); + offset = nextOffset; + continue; + } + + uint256 policyIndexPlusOne = + _approvalPolicyIndexPlusOne[innerAction.target][spender][APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE]; + if (policyIndexPlusOne == 0) { + revert SafeTxShapeApprovalSpenderNotAllowed( + innerAction.target, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE + ); + } + + uint256 policyIndex = policyIndexPlusOne - 1; + if (!allowanceLoaded[policyIndex]) { + initialAllowances[policyIndex] = + _readAllowanceFromPreState(innerAction.caller, innerAction.target, spender); + allowanceLoaded[policyIndex] = true; + } + + uint256 cumulativeIncrease = cumulativeIncreases[policyIndex]; + cumulativeIncrease = addedValue > type(uint256).max - cumulativeIncrease + ? type(uint256).max + : cumulativeIncrease + addedValue; + cumulativeIncreases[policyIndex] = cumulativeIncrease; + _validateAllowancePeak(innerAction.target, spender, initialAllowances[policyIndex], cumulativeIncrease); } else { _validateInnerApprovalPolicy(innerAction); } @@ -311,34 +342,6 @@ abstract contract SafeTxShapeHelpers is Assertion { } } - /// @dev Sums every positive grant to the same allowance up to the current batch entry. This is - /// intentionally conservative: later `transferFrom` calls or allowance reductions do not - /// erase a transient grant that exceeded policy during execution. - function _cumulativeIncreaseAllowance( - Action memory parent, - uint256 transactionsOffset, - uint256 transactionsLength, - uint256 throughOffset, - address token, - address spender - ) internal pure returns (uint256 total) { - uint256 cursor; - while (cursor < throughOffset) { - (Action memory candidate, uint256 nextCursor) = - _readMultiSendAction(parent, transactionsOffset, transactionsLength, cursor); - if ( - candidate.operation == OPERATION_CALL && candidate.target == token && candidate.dataLength == 68 - && _selectorAt(candidate.data, candidate.dataOffset) == INCREASE_ALLOWANCE_SELECTOR - && _readAbiAddress(candidate.data, candidate.dataOffset + 4) == spender - ) { - uint256 increase = _readUint256(candidate.data, candidate.dataOffset + 36); - if (increase > type(uint256).max - total) return type(uint256).max; - total += increase; - } - cursor = nextCursor; - } - } - function _multiSendTransactions(Action memory action, BatchExecutorPolicy storage batchPolicy) internal view @@ -530,17 +533,26 @@ abstract contract SafeTxShapeHelpers is Assertion { revert SafeTxShapeApprovalSpenderNotAllowed(token, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE); } + uint256 initialAllowance = _readAllowanceFromPreState(owner, token, spender); + _validateAllowancePeak(token, spender, initialAllowance, cumulativeIncrease); + } + + function _readAllowanceFromPreState(address owner, address token, address spender) internal view returns (uint256) { PhEvm.TriggerContext memory triggerCtx = ph.context(); PhEvm.ForkId memory preFork = _preCall(triggerCtx.callStart); - PhEvm.StaticCallResult memory result = ph.staticcallAt( token, abi.encodeWithSignature("allowance(address,address)", owner, spender), ALLOWANCE_READ_GAS, preFork ); - if (!result.ok || result.data.length != 32) { - revert SafeTxShapeAllowanceReadFailed(token, spender); - } + if (!result.ok || result.data.length != 32) revert SafeTxShapeAllowanceReadFailed(token, spender); + return abi.decode(result.data, (uint256)); + } - uint256 initialAllowance = abi.decode(result.data, (uint256)); + function _validateAllowancePeak( + address token, + address spender, + uint256 initialAllowance, + uint256 cumulativeIncrease + ) internal view { ApprovalPolicy storage policy = _approvalPolicyByKey[token][spender][APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE]; if (cumulativeIncrease > policy.maxAmount || initialAllowance > policy.maxAmount - cumulativeIncrease) { uint256 peakAllowance = cumulativeIncrease > type(uint256).max - initialAllowance @@ -794,7 +806,7 @@ abstract contract SafeTxShapeHelpers is Assertion { for (uint256 i; i < policies.length; ++i) { if ( policies[i].executor == address(0) || policies[i].selector == bytes4(0) || policies[i].maxActions == 0 - || policies[i].allowNested + || policies[i].maxActions > MAX_BATCH_ACTIONS || policies[i].allowNested ) { revert SafeTxShapeInvalidPolicy(); } @@ -838,6 +850,8 @@ abstract contract SafeTxShapeHelpers is Assertion { approvalPolicies.push(policies[i]); _approvalPolicyByKey[policies[i].token][policies[i].spender][policies[i].kind] = policies[i]; _approvalPolicyExists[policies[i].token][policies[i].spender][policies[i].kind] = true; + _approvalPolicyIndexPlusOne[policies[i].token][policies[i].spender][policies[i].kind] = + approvalPolicies.length; _tokenApprovalKindRegistered[policies[i].token][policies[i].kind] = true; } } diff --git a/test/protection/safe/SafeTxShapeAssertion.t.sol b/test/protection/safe/SafeTxShapeAssertion.t.sol index 5094c2b..2e18f8b 100644 --- a/test/protection/safe/SafeTxShapeAssertion.t.sol +++ b/test/protection/safe/SafeTxShapeAssertion.t.sol @@ -470,6 +470,18 @@ contract SafeTxShapeAssertionTest is Test, CredibleTest { _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); } + function testRejectsBatchPolicyAboveMeasuredGlobalLimit() public { + vm.expectRevert(SafeTxShapeHelpers.SafeTxShapeInvalidPolicy.selector); + new SafeTxShapeAssertion( + _baselineTargets(), + _baselineSelectors(), + _baselineBatchPolicies(5), + _approvalPolicies(false), + false, + _noModules() + ); + } + function testBlocksErc20ApprovalToUntrustedSpender() public { _armBaselinePolicyFor(false, SafeTxShapeAssertion.assertSafeApprovalPolicy.selector); @@ -603,6 +615,63 @@ contract SafeTxShapeAssertionTest is Test, CredibleTest { _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); } + function testManyBatchAllowanceGrantsAccumulateInOnePass() public { + _armPolicyFor( + _baselineTargets(), + _baselineSelectors(), + _baselineBatchPolicies(4), + _approvalPolicies(false), + false, + _noModules(), + SafeTxShapeAssertion.assertSafeApprovalPolicy.selector + ); + + bytes memory txs; + for (uint256 i; i < 4; ++i) { + txs = bytes.concat( + txs, + _packMultiSendTx( + OP_CALL, + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, uint256(24)) + ) + ); + } + + _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); + } + + function testApprovalPolicyEnforcesConfiguredBatchLimitIndependently() public { + _armPolicyFor( + _baselineTargets(), + _baselineSelectors(), + _baselineBatchPolicies(1), + _approvalPolicies(false), + false, + _noModules(), + SafeTxShapeAssertion.assertSafeApprovalPolicy.selector + ); + + bytes memory txs = bytes.concat( + _packMultiSendTx( + OP_CALL, + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, uint256(1)) + ), + _packMultiSendTx( + OP_CALL, + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, uint256(1)) + ) + ); + + vm.expectRevert(abi.encodeWithSelector(SafeTxShapeHelpers.SafeTxShapeBatchTooManyActions.selector, 1)); + _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); + } + function testIncreaseAllowanceCanReachUnlimitedWhenPolicyAllowsIt() public { SafeTxShapeHelpers.ApprovalPolicy[] memory approvals = _approvalPolicies(false); approvals[1].allowUnlimited = true; From e6ade7b5516dda718469dc2d208cf53eebfa34d5 Mon Sep 17 00:00:00 2001 From: makemake Date: Wed, 5 Aug 2026 15:26:36 +0200 Subject: [PATCH 4/6] fix(safe): reject mixed batch allowance grants --- src/protection/safe/SafeTxShapeHelpers.sol | 18 ++++++++- .../safe/SafeTxShapeAssertion.t.sol | 38 +++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/protection/safe/SafeTxShapeHelpers.sol b/src/protection/safe/SafeTxShapeHelpers.sol index 39d2255..23b8783 100644 --- a/src/protection/safe/SafeTxShapeHelpers.sol +++ b/src/protection/safe/SafeTxShapeHelpers.sol @@ -130,6 +130,7 @@ abstract contract SafeTxShapeHelpers is Assertion { error SafeTxShapeApprovalAmountAboveCap( address token, address spender, uint8 kind, uint256 amount, uint256 maxAmount ); + error SafeTxShapeMixedApprovalMethodsBlocked(); error SafeTxShapeAllowanceReadFailed(address token, address spender); error SafeTxShapeGasRefundBlocked(uint256 gasPrice); @@ -295,6 +296,8 @@ abstract contract SafeTxShapeHelpers is Assertion { uint256[] memory cumulativeIncreases = new uint256[](approvalPolicies.length); uint256[] memory initialAllowances = new uint256[](approvalPolicies.length); bool[] memory allowanceLoaded = new bool[](approvalPolicies.length); + bool approveSeen; + bool increaseAllowanceSeen; uint256 offset; uint256 actionCount; while (offset < transactionsLength) { @@ -302,9 +305,18 @@ abstract contract SafeTxShapeHelpers is Assertion { _readMultiSendAction(action, transactionsOffset, transactionsLength, offset); ++actionCount; if (actionCount > batchPolicy.maxActions) revert SafeTxShapeBatchTooManyActions(batchPolicy.maxActions); - if ( + bytes4 selector = + innerAction.dataLength >= 4 ? _selectorAt(innerAction.data, innerAction.dataOffset) : bytes4(0); + if (innerAction.operation == OPERATION_CALL && innerAction.dataLength == 68 && selector == APPROVE_SELECTOR) + { + _validateInnerApprovalPolicy(innerAction); + if (_tokenHasApprovalKind(innerAction.target, APPROVAL_KIND_ERC20_APPROVE)) { + if (increaseAllowanceSeen) revert SafeTxShapeMixedApprovalMethodsBlocked(); + approveSeen = true; + } + } else if ( innerAction.operation == OPERATION_CALL && innerAction.dataLength == 68 - && _selectorAt(innerAction.data, innerAction.dataOffset) == INCREASE_ALLOWANCE_SELECTOR + && selector == INCREASE_ALLOWANCE_SELECTOR ) { address spender = _readAbiAddress(innerAction.data, innerAction.dataOffset + 4); uint256 addedValue = _readUint256(innerAction.data, innerAction.dataOffset + 36); @@ -323,6 +335,8 @@ abstract contract SafeTxShapeHelpers is Assertion { } uint256 policyIndex = policyIndexPlusOne - 1; + if (approveSeen) revert SafeTxShapeMixedApprovalMethodsBlocked(); + increaseAllowanceSeen = true; if (!allowanceLoaded[policyIndex]) { initialAllowances[policyIndex] = _readAllowanceFromPreState(innerAction.caller, innerAction.target, spender); diff --git a/test/protection/safe/SafeTxShapeAssertion.t.sol b/test/protection/safe/SafeTxShapeAssertion.t.sol index 2e18f8b..34fc622 100644 --- a/test/protection/safe/SafeTxShapeAssertion.t.sol +++ b/test/protection/safe/SafeTxShapeAssertion.t.sol @@ -615,6 +615,44 @@ contract SafeTxShapeAssertionTest is Test, CredibleTest { _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); } + function testBlocksTransientAllowanceAcrossMixedBatchGrantMethods() public { + _armBaselinePolicyFor(false, SafeTxShapeAssertion.assertSafeApprovalPolicy.selector); + + bytes memory txs = bytes.concat( + _packMultiSendTx( + OP_CALL, address(erc20Token), 0, abi.encodeCall(MockApprovalTarget.approve, (TRUSTED_SPENDER, 80)) + ), + _packMultiSendTx( + OP_CALL, + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, uint256(30)) + ) + ); + + vm.expectRevert(SafeTxShapeHelpers.SafeTxShapeMixedApprovalMethodsBlocked.selector); + _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); + } + + function testBlocksTransientAllowanceAcrossReversedMixedBatchGrantMethods() public { + _armBaselinePolicyFor(false, SafeTxShapeAssertion.assertSafeApprovalPolicy.selector); + + bytes memory txs = bytes.concat( + _packMultiSendTx( + OP_CALL, + address(erc20Token), + 0, + abi.encodeWithSelector(INCREASE_ALLOWANCE_SELECTOR, TRUSTED_SPENDER, uint256(30)) + ), + _packMultiSendTx( + OP_CALL, address(erc20Token), 0, abi.encodeCall(MockApprovalTarget.approve, (TRUSTED_SPENDER, 80)) + ) + ); + + vm.expectRevert(SafeTxShapeHelpers.SafeTxShapeMixedApprovalMethodsBlocked.selector); + _execOwner(address(multiSend), 0, abi.encodeWithSelector(MULTISEND_SELECTOR, txs), OP_DELEGATECALL); + } + function testManyBatchAllowanceGrantsAccumulateInOnePass() public { _armPolicyFor( _baselineTargets(), From 711df59dd04a9892b73f3b11e240991e07993e5a Mon Sep 17 00:00:00 2001 From: makemake Date: Wed, 5 Aug 2026 16:14:35 +0200 Subject: [PATCH 5/6] docs(safe): clarify batch-wide grant restriction --- src/protection/safe/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/protection/safe/README.md b/src/protection/safe/README.md index f1ed8dd..1f251c9 100644 --- a/src/protection/safe/README.md +++ b/src/protection/safe/README.md @@ -181,6 +181,8 @@ Approval resets and revocations are allowed by default when the token is configu For ERC-20 `approve(spender, amount)` the cap binds `amount` directly. For ERC-20 `increaseAllowance(spender, addedValue)` the cap binds the pre-execution allowance plus all positive grants requested for that owner/token/spender in the batch. This is intentionally stricter than checking final state: consuming or reducing an oversized transient allowance later in the transaction does not make the batch valid. The approval assertion decodes the batch once, accumulates each configured allowance by policy index, and reads each initial allowance once, so multiple grants do not trigger repeated prefix rescans. Batch policies cannot configure more than four actions; the four-grant regression executes below PCL's 300,000-gas local assertion ceiling, while the former unbounded configuration could exhaust the budget before reaching a policy decision. For CALL-based executors, the executor is the token owner; for DELEGATECALL-based executors, the Safe is the owner. +A batch cannot mix ERC-20 `approve` with a positive `increaseAllowance` anywhere in the batch, even when the calls concern different tokens, owners, or spenders. This conservative restriction is stricter than Safe itself and prevents transient grants from bypassing independently configured caps. + ### Material Effect - A compromised UI cannot redirect signers to an arbitrary contract with arbitrary calldata. From 2d3ee45f80bc9f0eb0a1d1a4c7767f7bd93618d7 Mon Sep 17 00:00:00 2001 From: makemake Date: Wed, 5 Aug 2026 16:20:43 +0200 Subject: [PATCH 6/6] fix(safe): keep allowance accumulation stack bounded --- src/protection/safe/SafeTxShapeHelpers.sol | 54 ++++++++++++---------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/protection/safe/SafeTxShapeHelpers.sol b/src/protection/safe/SafeTxShapeHelpers.sol index 23b8783..25a9d36 100644 --- a/src/protection/safe/SafeTxShapeHelpers.sol +++ b/src/protection/safe/SafeTxShapeHelpers.sol @@ -318,37 +318,14 @@ abstract contract SafeTxShapeHelpers is Assertion { innerAction.operation == OPERATION_CALL && innerAction.dataLength == 68 && selector == INCREASE_ALLOWANCE_SELECTOR ) { - address spender = _readAbiAddress(innerAction.data, innerAction.dataOffset + 4); - uint256 addedValue = _readUint256(innerAction.data, innerAction.dataOffset + 36); - if (addedValue == 0) { + if (_readUint256(innerAction.data, innerAction.dataOffset + 36) == 0) { _validateInnerApprovalPolicy(innerAction); offset = nextOffset; continue; } - - uint256 policyIndexPlusOne = - _approvalPolicyIndexPlusOne[innerAction.target][spender][APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE]; - if (policyIndexPlusOne == 0) { - revert SafeTxShapeApprovalSpenderNotAllowed( - innerAction.target, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE - ); - } - - uint256 policyIndex = policyIndexPlusOne - 1; if (approveSeen) revert SafeTxShapeMixedApprovalMethodsBlocked(); increaseAllowanceSeen = true; - if (!allowanceLoaded[policyIndex]) { - initialAllowances[policyIndex] = - _readAllowanceFromPreState(innerAction.caller, innerAction.target, spender); - allowanceLoaded[policyIndex] = true; - } - - uint256 cumulativeIncrease = cumulativeIncreases[policyIndex]; - cumulativeIncrease = addedValue > type(uint256).max - cumulativeIncrease - ? type(uint256).max - : cumulativeIncrease + addedValue; - cumulativeIncreases[policyIndex] = cumulativeIncrease; - _validateAllowancePeak(innerAction.target, spender, initialAllowances[policyIndex], cumulativeIncrease); + _accumulateIncreaseAllowance(innerAction, cumulativeIncreases, initialAllowances, allowanceLoaded); } else { _validateInnerApprovalPolicy(innerAction); } @@ -356,6 +333,33 @@ abstract contract SafeTxShapeHelpers is Assertion { } } + function _accumulateIncreaseAllowance( + Action memory action, + uint256[] memory cumulativeIncreases, + uint256[] memory initialAllowances, + bool[] memory allowanceLoaded + ) internal view { + address spender = _readAbiAddress(action.data, action.dataOffset + 4); + uint256 addedValue = _readUint256(action.data, action.dataOffset + 36); + uint256 policyIndexPlusOne = + _approvalPolicyIndexPlusOne[action.target][spender][APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE]; + if (policyIndexPlusOne == 0) { + revert SafeTxShapeApprovalSpenderNotAllowed(action.target, spender, APPROVAL_KIND_ERC20_INCREASE_ALLOWANCE); + } + + uint256 policyIndex = policyIndexPlusOne - 1; + if (!allowanceLoaded[policyIndex]) { + initialAllowances[policyIndex] = _readAllowanceFromPreState(action.caller, action.target, spender); + allowanceLoaded[policyIndex] = true; + } + + uint256 cumulativeIncrease = cumulativeIncreases[policyIndex]; + cumulativeIncrease = + addedValue > type(uint256).max - cumulativeIncrease ? type(uint256).max : cumulativeIncrease + addedValue; + cumulativeIncreases[policyIndex] = cumulativeIncrease; + _validateAllowancePeak(action.target, spender, initialAllowances[policyIndex], cumulativeIncrease); + } + function _multiSendTransactions(Action memory action, BatchExecutorPolicy storage batchPolicy) internal view