From ad71fc995e812f6ba242811adb5dd8616b632b7d Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Wed, 4 Nov 2026 11:53:43 +0000 Subject: [PATCH 1/3] Validate MultiSend overrides and add Safe regression tests --- README.md | 66 ++++++-- src/Safe.sol | 9 ++ test/Safe.t.sol | 3 + test/SafeDirectExecValidation.t.sol | 223 ++++++++++++++++++++++++++++ test/SafeMultiSendOverride.t.sol | 94 ++++++++++++ test/SafeValidationExtra.t.sol | 218 +++++++++++++++++++++++++++ 6 files changed, 600 insertions(+), 13 deletions(-) create mode 100644 test/SafeDirectExecValidation.t.sol create mode 100644 test/SafeMultiSendOverride.t.sol create mode 100644 test/SafeValidationExtra.t.sol diff --git a/README.md b/README.md index 4cea8c1..b2af8ec 100644 --- a/README.md +++ b/README.md @@ -86,38 +86,62 @@ Batches delegatecall `MultiSendCallOnly`. On a chain the Safe transaction servic safe.withMultiSendCallOnly(0x...); ``` +The override applies to every chain queried through that client. Clear it with `safe.withMultiSendCallOnly(address(0))` when it no longer applies. On the current chain, an override with no code reverts with `MultiSendCallOnlyHasNoCode` before a batch is built. Queries for another chain cannot check that chain's code. + +Use only a trusted `MultiSendCallOnly` deployment. The batch delegatecalls this address in the Safe's context. Checking that code exists does not verify what the code does; the same limitation applies to automatic canonical-address fallback. + #### Direct execution with several owners (no Transaction Service) On a chain without the Safe Transaction Service, owners can approve a transaction and anyone can execute it with `execTransaction`. Approvals come from offline signatures, on-chain `approveHash` calls, or a mix of both. +For a single call, collect signatures over its hash and pass them in any order: + ```solidity -bytes32 safeTxHash = safe.getSafeTxHash(target, 0, data, Enum.Operation.Call, nonce); +bytes32 singleHash = safe.getSafeTxHash(target, 0, data, Enum.Operation.Call, nonce); -// Path 1: offline signatures over safeTxHash, e.g. from safe.sign(...) or `cast wallet sign`, in any order. -// eth_sign signatures must have v + 4 (31/32), as safe.sign produces for Trezor. +// Both owners sign singleHash, for example with safe.sign(...). bytes[] memory signatures = new bytes[](2); signatures[0] = signatureFromOwnerA; signatures[1] = signatureFromOwnerB; +bytes memory execData = safe.getExecTransactionDataWithSignatures(target, data, signatures, nonce); +``` -// Path 2: an owner sends approveHash as an ordinary transaction, which works with --ledger or --trezor -// without the ffi signing step. Send this calldata to the Safe address: -bytes memory approveData = safe.getApproveHashData(safeTxHash); -// Then collect v = 1 signatures for every owner that approved, plus the executor if it is an owner: -bytes[] memory approved = safe.getApprovedHashSignatures(safeTxHash, executor); +For a batch, collect new signatures over the MultiSend payload with `DelegateCall`: -// Build execTransaction calldata: signatures are checked against the owners and the threshold, then sorted -bytes memory execData = safe.getExecTransactionDataWithSignatures(target, data, signatures, nonce); -bytes memory batchExecData = safe.getExecTransactionsDataWithSignatures(targets, datas, signatures, nonce); +```solidity +(address batchTo, bytes memory batchData) = safe.getProposeTransactionsTargetAndData(targets, datas); +bytes32 batchHash = safe.getSafeTxHash(batchTo, 0, batchData, Enum.Operation.DelegateCall, nonce); + +// Both owners sign batchHash. Signatures over singleHash cannot authorize this batch. +bytes[] memory batchSignatures = new bytes[](2); +batchSignatures[0] = batchSignatureFromOwnerA; +batchSignatures[1] = batchSignatureFromOwnerB; +bytes memory batchExecData = safe.getExecTransactionsDataWithSignatures(targets, datas, batchSignatures, nonce); +``` + +For on-chain approvals, an owner sends `approveHash` calldata to the Safe as an ordinary transaction. This works with `--ledger` or `--trezor` without the FFI signing step: + +```solidity +bytes32 approvalHash = safe.getSafeTxHash(target, 0, data, Enum.Operation.Call, nonce); +bytes memory approveData = safe.getApproveHashData(approvalHash); + +// After enough owners have approved on-chain, collect their entries and build execution calldata. +bytes[] memory approvals = safe.getApprovedHashSignatures(approvalHash, executor); +bytes memory approvedExecData = safe.getExecTransactionDataWithSignatures(target, data, approvals, nonce); ``` -Both paths can be mixed in one `bytes[]`. Batches must be signed with `Enum.Operation.DelegateCall`. `combineSignatures(safeTxHash, signatures)` returns the combined `signatures` bytes on their own. +`getApprovedHashSignatures` also includes `executor` when it is an owner. That account must actually call `execTransaction` unless it has separately approved the hash on-chain. Pass `address(0)` when execution will come from a non-owner. + +Offline signatures and approval entries can be combined in one `bytes[]`, with each owner appearing once. `combineSignatures(safeTxHash, signatures)` checks owner identities, duplicates and the threshold, then sorts the entries. For approval entries (`v = 1`), it does not verify consent: the Safe checks the recorded approval or the actual executor during execution. Building calldata successfully does not guarantee it can execute. + +ECDSA signatures use `v = 27/28`. `eth_sign` signatures need `v + 4` (`31/32`), as `safe.sign` produces for Trezor. Contract signatures (`v = 0`) are not supported by these helpers. The execution helpers use zero ETH value and zero gas/refund fields. #### Validate a proposal before signing A hardware wallet shows only hashes. Before co-signing, check that a proposal on the Transaction Service matches the transaction your script builds: ```solidity -// Single transaction: reverts with ProposedTransactionMismatch(field, expected, actual) on the first difference +// Single transaction: compares the proposal with these locally supplied expectations safe.validateProposedTransaction(safeTxHash, to, value, data, Enum.Operation.Call, nonce); // Batch: rebuilds the MultiSend payload from targets and datas, as proposeTransactions does @@ -133,6 +157,10 @@ Safe.logSafeTxHashes(hashes); `validateProposedTransactionJson` checks a response you already fetched, for example in tests. +Hash computation and proposal validation support Safe v1.3.0 and later. Older Safes use a different EIP-712 domain. These helpers require zero `safeTxGas`, `baseGas`, `gasPrice`, `gasToken` and `refundReceiver`, matching transactions proposed by this library. They compare calldata byte for byte without decoding it. + +A differing field reverts with `ProposedTransactionMismatch(field, expected, actual)`. Malformed responses or missing required fields may instead produce a JSON parser error. Supply expected values from your own script; copying them from the response would defeat the comparison. + #### Simulation (no hardware wallet required) Simulate transactions against a local fork before broadcasting. No signing device is needed — the library writes directly to the Safe's `approvedHashes` storage slot. @@ -213,6 +241,18 @@ ffi = true - All `Recon-Fuzz/solidity-http` dependencies +### Tests + +Run `npm ci` and `forge test` from the repository root. Some tests read Base through its public RPC. Signature validation and the additional hash tests also deploy Safes locally, so those checks do not require an RPC connection. + +The two tests that submit proposals to the live Transaction Service are skipped by default. To run them explicitly against the repository's test Safe: + +```bash +SAFE_UTILS_LIVE_API_TESTS=true forge test --match-test 'test_Safe_propose' +``` + +This command writes proposals to the service. It does not broadcast on-chain transactions. + ### Third-party integrations The following blockchains are integrated via third-party APIs and not the official `safe.global` tx service: diff --git a/src/Safe.sol b/src/Safe.sol index 31fed4f..9fd504b 100644 --- a/src/Safe.sol +++ b/src/Safe.sol @@ -37,6 +37,7 @@ library Safe { error ApiKitUrlNotFound(uint256 chainId); error MultiSendCallOnlyNotFound(uint256 chainId); + error MultiSendCallOnlyHasNoCode(address multiSendCallOnly); error ArrayLengthsMismatch(uint256 a, uint256 b); error ProposeTransactionFailed(uint256 statusCode, string response); error InvalidSignatureLength(uint256 index, uint256 length); @@ -114,6 +115,8 @@ library Safe { /// @notice Use `multiSendCallOnly` for batches instead of the address resolved from the chain id /// @dev For chains whose MultiSendCallOnly is not at a canonical address. address(0) clears the override. + /// The override must be trusted: batches delegatecall it in the Safe's context. Code presence is + /// checked when resolving it on the current chain; this does not verify the implementation. /// /// @param self The Safe client /// @param multiSendCallOnly The MultiSendCallOnly contract to delegatecall for batches @@ -127,8 +130,12 @@ library Safe { /// @dev In order: the client override set with {withMultiSendCallOnly}; the deployment safe-deployments /// lists for a chain the transaction service serves; and, for the current chain only, the canonical /// v1.4.1 then v1.3.0 address if it has code. Reverts with MultiSendCallOnlyNotFound otherwise. + /// An override with no code on the current chain reverts with MultiSendCallOnlyHasNoCode. function getMultiSendCallOnly(Client storage self, uint256 chainId) internal view returns (MultiSendCallOnly) { if (self.multiSendCallOnly != address(0)) { + if (chainId == block.chainid && self.multiSendCallOnly.code.length == 0) { + revert MultiSendCallOnlyHasNoCode(self.multiSendCallOnly); + } return MultiSendCallOnly(self.multiSendCallOnly); } if (chainId == 324) { @@ -1045,6 +1052,8 @@ library Safe { /// Contract signatures (v = 0) are not supported. Each entry must belong to a current owner, owners /// must not repeat, and at least `threshold` entries are required. The output is sorted by owner /// address ascending, as Safe's checkNSignatures requires, so the input order does not matter. + /// For v = 1, this checks owner identity only. The Safe verifies that the owner approved this hash + /// on-chain or is the actual executor. A successful combination does not prove those approvals exist. /// /// @param self The Safe client /// @param safeTxHash The Safe transaction hash the owners signed or approved diff --git a/test/Safe.t.sol b/test/Safe.t.sol index 9fc4474..650dc2f 100644 --- a/test/Safe.t.sol +++ b/test/Safe.t.sol @@ -28,6 +28,7 @@ contract SafeTest is Test { } function test_Safe_proposeTransaction() public { + vm.skip(!vm.envOr("SAFE_UTILS_LIVE_API_TESTS", false), "requires live Transaction Service writes"); address weth = 0x4200000000000000000000000000000000000006; vm.rememberKey(uint256(foundrySigner1PrivateKey)); safe.proposeTransaction(weth, abi.encodeCall(IWETH.withdraw, (0)), foundrySigner1); @@ -41,6 +42,7 @@ contract SafeTest is Test { } function test_Safe_proposeTransactionsWithSignature() public { + vm.skip(!vm.envOr("SAFE_UTILS_LIVE_API_TESTS", false), "requires live Transaction Service writes"); address weth = 0x4200000000000000000000000000000000000006; // Create batch of transactions @@ -189,6 +191,7 @@ contract SafeMultiSendFallbackTest is Test { function test_Safe_withMultiSendCallOnly_overridesResolution() public { address custom = address(0xCAFE); + vm.etch(custom, v141Code); vm.etch(MULTI_SEND_CALL_ONLY_ADDRESS_V141_CANONICAL, v141Code); safe.withMultiSendCallOnly(custom); diff --git a/test/SafeDirectExecValidation.t.sol b/test/SafeDirectExecValidation.t.sol new file mode 100644 index 0000000..16be3ed --- /dev/null +++ b/test/SafeDirectExecValidation.t.sol @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {Test} from "forge-std/Test.sol"; +import {Safe} from "../src/Safe.sol"; +import {Safe as SafeAccount} from "safe-smart-account/Safe.sol"; +import {SafeProxy} from "safe-smart-account/proxies/SafeProxy.sol"; +import {Enum} from "safe-smart-account/common/Enum.sol"; + +contract SignatureValidationReceiver { + uint256 public calls; + address public sender; + + function ping() external { + calls++; + sender = msg.sender; + } +} + +/// @notice Signature and approval checks against a locally deployed canonical Safe. +contract SafeDirectExecValidationTest is Test { + using Safe for *; + + uint256 constant CURVE_ORDER = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141; + + Safe.Client safe; + SafeAccount account; + SignatureValidationReceiver receiver; + address[] owners; + uint256[] ownerKeys; + address nonOwnerExecutor = makeAddr("executor"); + + function setUp() public { + SafeAccount singleton = new SafeAccount(); + account = SafeAccount(payable(address(new SafeProxy(address(singleton))))); + for (uint256 i; i < 3; i++) { + ownerKeys.push(i + 1); + owners.push(vm.addr(i + 1)); + } + account.setup(owners, 2, address(0), "", address(0), address(0), 0, payable(address(0))); + safe.initialize(address(account)); + receiver = new SignatureValidationReceiver(); + } + + function test_Safe_execWithSignatures_forgedApprovalEntries_rejectedBySafe() public { + bytes[] memory signatures = new bytes[](2); + signatures[0] = _approvalSignature(owners[0]); + signatures[1] = _approvalSignature(owners[1]); + + // The helper recognizes the owners, but the Safe must verify their actual approvals. + bytes memory execData = _execData(signatures, 0); + _assertMissingApproval(nonOwnerExecutor, execData); + assertEq(account.nonce(), 0); + assertEq(receiver.calls(), 0); + } + + function test_Safe_execWithSignatures_implicitApproval_requiresMatchingExecutor() public { + _approve(owners[0], _hash(0)); + bytes[] memory signatures = safe.getApprovedHashSignatures(_hash(0), owners[1]); + assertEq(signatures.length, 2); + bytes memory execData = _execData(signatures, 0); + + _assertMissingApproval(nonOwnerExecutor, execData); + assertEq(account.nonce(), 0); + assertEq(receiver.calls(), 0); + + _exec(owners[1], execData); + assertEq(account.nonce(), 1); + assertEq(receiver.calls(), 1); + assertEq(receiver.sender(), address(account)); + } + + function test_Safe_execWithSignatures_approvals_boundToTransactionHash() public { + _approve(owners[0], _hash(0)); + _approve(owners[1], _hash(0)); + bytes[] memory signatures = safe.getApprovedHashSignatures(_hash(0), nonOwnerExecutor); + assertEq(signatures.length, 2); + + SignatureValidationReceiver otherReceiver = new SignatureValidationReceiver(); + bytes memory execData = safe.getExecTransactionDataWithSignatures( + address(otherReceiver), abi.encodeCall(otherReceiver.ping, ()), signatures, 0 + ); + _assertMissingApproval(nonOwnerExecutor, execData); + assertEq(account.nonce(), 0); + assertEq(otherReceiver.calls(), 0); + + _exec(nonOwnerExecutor, _execData(signatures, 0)); + assertEq(account.nonce(), 1); + assertEq(receiver.calls(), 1); + } + + function test_Safe_execWithSignatures_approvals_boundToCurrentNonce() public { + _approve(owners[0], _hash(1)); + _approve(owners[1], _hash(1)); + bytes[] memory signatures = safe.getApprovedHashSignatures(_hash(1), nonOwnerExecutor); + assertEq(signatures.length, 2); + assertEq(safe.getApprovedHashSignatures(_hash(0), nonOwnerExecutor).length, 0); + bytes memory execData = _execData(signatures, 1); + + // execTransaction uses the Safe's current nonce, regardless of the helper's explicit nonce. + _assertMissingApproval(nonOwnerExecutor, execData); + assertEq(account.nonce(), 0); + assertEq(receiver.calls(), 0); + + bytes[] memory currentSignatures = new bytes[](2); + currentSignatures[0] = _sign(0, 0, false); + currentSignatures[1] = _sign(1, 0, false); + _exec(nonOwnerExecutor, _execData(currentSignatures, 0)); + _exec(nonOwnerExecutor, execData); + assertEq(account.nonce(), 2); + assertEq(receiver.calls(), 2); + } + + function test_Safe_combineSignatures_revertsWithMalformedLength() public { + bytes[] memory signatures = new bytes[](2); + signatures[0] = new bytes(64); + signatures[1] = _sign(1, 0, false); + + vm.expectRevert(abi.encodeWithSelector(Safe.InvalidSignatureLength.selector, 0, 64)); + this.exposedCombineSignatures(signatures); + } + + function test_Safe_combineSignatures_revertsWithContractSignature() public { + bytes[] memory signatures = new bytes[](2); + signatures[0] = abi.encodePacked(bytes32(uint256(uint160(owners[0]))), bytes32(0), uint8(0)); + signatures[1] = _sign(1, 0, false); + + vm.expectRevert(abi.encodeWithSelector(Safe.UnsupportedSignatureType.selector, 0, 0)); + this.exposedCombineSignatures(signatures); + } + + function test_Safe_combineSignatures_malleatedSignature_cannotCountOwnerTwice() public { + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKeys[0], _hash(0)); + bytes[] memory signatures = new bytes[](2); + signatures[0] = abi.encodePacked(r, s, v); + signatures[1] = abi.encodePacked(r, bytes32(CURVE_ORDER - uint256(s)), v == 27 ? uint8(28) : uint8(27)); + + vm.expectRevert(abi.encodeWithSelector(Safe.DuplicateSigner.selector, owners[0])); + this.exposedCombineSignatures(signatures); + } + + function test_Safe_execWithSignatures_allOwnersApproved_nonOwnerExecutes() public { + for (uint256 i; i < owners.length; i++) { + _approve(owners[i], _hash(0)); + } + bytes[] memory signatures = safe.getApprovedHashSignatures(_hash(0), nonOwnerExecutor); + assertEq(signatures.length, 3); + + _exec(nonOwnerExecutor, _execData(signatures, 0)); + assertEq(account.nonce(), 1); + assertEq(receiver.calls(), 1); + assertEq(receiver.sender(), address(account)); + } + + function testFuzz_Safe_execWithSignatures_mixedFormatsAndOwnerOrder_executes(uint8 permutation, uint8 modes) + public + { + permutation = uint8(bound(permutation, 0, 5)); + uint256[2] memory order; + order[0] = permutation / 2; + order[1] = (order[0] + (permutation % 2 == 0 ? 1 : 2)) % 3; + bytes[] memory signatures = new bytes[](2); + for (uint256 i; i < signatures.length; i++) { + uint256 ownerIndex = order[i]; + uint8 mode = (modes >> (i * 2)) & 3; + if (mode == 0) { + _approve(owners[ownerIndex], _hash(0)); + signatures[i] = _approvalSignature(owners[ownerIndex]); + } else { + signatures[i] = _sign(ownerIndex, 0, mode == 1); + } + } + + _exec(nonOwnerExecutor, _execData(signatures, 0)); + assertEq(account.nonce(), 1); + assertEq(receiver.calls(), 1); + assertEq(receiver.sender(), address(account)); + } + + function exposedCombineSignatures(bytes[] memory signatures) external view returns (bytes memory) { + return safe.combineSignatures(_hash(0), signatures); + } + + function _hash(uint256 nonce) private view returns (bytes32) { + return safe.getSafeTxHash(address(receiver), 0, abi.encodeCall(receiver.ping, ()), Enum.Operation.Call, nonce); + } + + function _sign(uint256 ownerIndex, uint256 nonce, bool ethSign) private view returns (bytes memory) { + bytes32 digest = _hash(nonce); + if (ethSign) digest = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", digest)); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKeys[ownerIndex], digest); + return abi.encodePacked(r, s, ethSign ? v + 4 : v); + } + + function _approvalSignature(address owner) private pure returns (bytes memory) { + return abi.encodePacked(bytes32(uint256(uint160(owner))), bytes32(0), uint8(1)); + } + + function _execData(bytes[] memory signatures, uint256 nonce) private view returns (bytes memory) { + return safe.getExecTransactionDataWithSignatures( + address(receiver), abi.encodeCall(receiver.ping, ()), signatures, nonce + ); + } + + function _approve(address owner, bytes32 digest) private { + vm.prank(owner); + account.approveHash(digest); + } + + function _assertMissingApproval(address executor, bytes memory execData) private { + vm.prank(executor); + (bool ok, bytes memory ret) = address(account).call(execData); + assertFalse(ok, "Safe accepted an unapproved transaction"); + assertEq(ret, abi.encodeWithSignature("Error(string)", "GS025")); + } + + function _exec(address executor, bytes memory execData) private { + vm.prank(executor); + (bool ok, bytes memory ret) = address(account).call(execData); + assertTrue(ok, "call to Safe reverted"); + assertTrue(abi.decode(ret, (bool)), "execTransaction returned false"); + } +} diff --git a/test/SafeMultiSendOverride.t.sol b/test/SafeMultiSendOverride.t.sol new file mode 100644 index 0000000..42778bc --- /dev/null +++ b/test/SafeMultiSendOverride.t.sol @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {Test} from "forge-std/Test.sol"; +import {Safe} from "../src/Safe.sol"; +import {Safe as SafeAccount} from "safe-smart-account/Safe.sol"; +import {SafeProxy} from "safe-smart-account/proxies/SafeProxy.sol"; +import {MultiSendCallOnly} from "safe-smart-account/libraries/MultiSendCallOnly.sol"; +import {Enum} from "safe-smart-account/common/Enum.sol"; + +contract MultiSendOverrideReceiver { + uint256 public calls; + address public sender; + + function ping() external { + calls++; + sender = msg.sender; + } +} + +contract SafeMultiSendOverrideTest is Test { + using Safe for *; + + Safe.Client safe; + SafeAccount account; + MultiSendCallOnly multiSend; + MultiSendOverrideReceiver receiver; + + function setUp() public { + SafeAccount singleton = new SafeAccount(); + account = SafeAccount(payable(address(new SafeProxy(address(singleton))))); + address[] memory owners = new address[](1); + owners[0] = address(this); + account.setup(owners, 1, address(0), "", address(0), address(0), 0, payable(address(0))); + safe.initialize(address(account)); + multiSend = new MultiSendCallOnly(); + receiver = new MultiSendOverrideReceiver(); + } + + function test_Safe_multiSendOverride_withoutCode_rejectsBatch() public { + address emptyTarget = makeAddr("empty multisend"); + safe.withMultiSendCallOnly(emptyTarget); + + vm.expectRevert(abi.encodeWithSelector(Safe.MultiSendCallOnlyHasNoCode.selector, emptyTarget)); + this.exposedBatch(); + assertEq(account.nonce(), 0); + assertEq(receiver.calls(), 0); + } + + function test_Safe_multiSendOverride_checksCodeWhenUsed() public { + safe.withMultiSendCallOnly(address(multiSend)); + vm.etch(address(multiSend), ""); + + vm.expectRevert(abi.encodeWithSelector(Safe.MultiSendCallOnlyHasNoCode.selector, address(multiSend))); + this.exposedBatch(); + } + + function test_Safe_multiSendOverride_otherChain_doesNotInspectLocalCode() public { + address remoteTarget = makeAddr("remote multisend"); + safe.withMultiSendCallOnly(remoteTarget); + assertEq(address(safe.getMultiSendCallOnly(block.chainid + 1)), remoteTarget); + } + + function test_Safe_multiSendOverride_withCode_executesBatch() public { + safe.withMultiSendCallOnly(address(multiSend)); + (address[] memory targets, bytes[] memory datas) = _batch(); + (address to, bytes memory data) = safe.getProposeTransactionsTargetAndData(targets, datas); + assertEq(to, address(multiSend)); + bytes32 hash = safe.getSafeTxHash(to, 0, data, Enum.Operation.DelegateCall, 0); + bytes[] memory signatures = safe.getApprovedHashSignatures(hash, address(this)); + bytes memory execData = safe.getExecTransactionsDataWithSignatures(targets, datas, signatures, 0); + + (bool ok, bytes memory result) = address(account).call(execData); + assertTrue(ok); + assertTrue(abi.decode(result, (bool))); + assertEq(account.nonce(), 1); + assertEq(receiver.calls(), 2); + assertEq(receiver.sender(), address(account)); + } + + function exposedBatch() external view returns (address, bytes memory) { + (address[] memory targets, bytes[] memory datas) = _batch(); + return safe.getProposeTransactionsTargetAndData(targets, datas); + } + + function _batch() private view returns (address[] memory targets, bytes[] memory datas) { + targets = new address[](2); + datas = new bytes[](2); + targets[0] = address(receiver); + targets[1] = address(receiver); + datas[0] = abi.encodeCall(receiver.ping, ()); + datas[1] = abi.encodeCall(receiver.ping, ()); + } +} diff --git a/test/SafeValidationExtra.t.sol b/test/SafeValidationExtra.t.sol new file mode 100644 index 0000000..89a4d4f --- /dev/null +++ b/test/SafeValidationExtra.t.sol @@ -0,0 +1,218 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {Safe} from "../src/Safe.sol"; +import {Safe as SafeSmartAccount} from "safe-smart-account/Safe.sol"; +import {Enum} from "safe-smart-account/common/Enum.sol"; +import {BaseProposalFixture} from "./SafeValidation.t.sol"; + +/// @notice Differential hash coverage against a local Safe, without an RPC connection. +contract SafeHashesLocalTest is BaseProposalFixture { + struct Transaction { + address to; + uint256 value; + bytes data; + uint256 nonce; + } + + function testFuzz_Safe_getSafeTxHashes_matchesLocalSafe( + uint64 chainId, + Transaction memory transaction, + bool delegateCall + ) public { + vm.chainId(chainId); + SafeSmartAccount account = new SafeSmartAccount(); + Enum.Operation operation = delegateCall ? Enum.Operation.DelegateCall : Enum.Operation.Call; + Safe.SafeTxHashes memory hashes = Safe.getSafeTxHashes( + address(account), chainId, transaction.to, transaction.value, transaction.data, operation, transaction.nonce + ); + + assertEq(hashes.domainSeparator, account.domainSeparator()); + assertEq(hashes.safeTxHash, _accountHash(account, transaction, operation)); + assertEq( + abi.encodePacked(hex"1901", hashes.domainSeparator, hashes.messageHash), + _accountTransactionData(account, transaction, operation) + ); + } + + function _accountHash(SafeSmartAccount account, Transaction memory transaction, Enum.Operation operation) + private + view + returns (bytes32) + { + return account.getTransactionHash( + transaction.to, + transaction.value, + transaction.data, + operation, + 0, + 0, + 0, + address(0), + address(0), + transaction.nonce + ); + } + + function _accountTransactionData(SafeSmartAccount account, Transaction memory transaction, Enum.Operation operation) + private + view + returns (bytes memory) + { + return account.encodeTransactionData( + transaction.to, + transaction.value, + transaction.data, + operation, + 0, + 0, + 0, + address(0), + address(0), + transaction.nonce + ); + } +} + +/// @notice Reject responses with unsupported signed fields or malformed required values. +contract SafeValidationNegativeTest is BaseProposalFixture { + using Safe for Safe.Client; + + Safe.Client safe; + string json; + address to; + bytes data; + + function setUp() public { + vm.chainId(BASE_CHAIN_ID); + safe.initialize(SAFE_ADDRESS); + json = vm.readFile("test/fixtures/base-multisig-transaction.json"); + to = vm.parseJsonAddress(json, ".to"); + data = vm.parseJsonBytes(json, ".data"); + } + + function test_Safe_validateProposedTransactionJson_revertsOnWrongValue() public { + _expectMismatch('"value": "0"', '"value": "1"', "value", "0", "1"); + } + + function test_Safe_validateProposedTransactionJson_revertsOnNonzeroSafeTxGas() public { + _expectMismatch('"safeTxGas": "0"', '"safeTxGas": "1"', "safeTxGas", "0", "1"); + } + + function test_Safe_validateProposedTransactionJson_revertsOnNonzeroBaseGas() public { + _expectMismatch('"baseGas": "0"', '"baseGas": "1"', "baseGas", "0", "1"); + } + + function test_Safe_validateProposedTransactionJson_revertsOnNonzeroGasPrice() public { + _expectMismatch('"gasPrice": "0"', '"gasPrice": "1"', "gasPrice", "0", "1"); + } + + function test_Safe_validateProposedTransactionJson_revertsOnNonzeroGasToken() public { + _expectMismatch( + '"gasToken": null', + '"gasToken": "0x0000000000000000000000000000000000000001"', + "gasToken", + vm.toString(address(0)), + vm.toString(address(1)) + ); + } + + function test_Safe_validateProposedTransactionJson_revertsOnNonzeroRefundReceiver() public { + _expectMismatch( + '"refundReceiver": null', + '"refundReceiver": "0x0000000000000000000000000000000000000001"', + "refundReceiver", + vm.toString(address(0)), + vm.toString(address(1)) + ); + } + + function test_Safe_validateProposedTransactionJson_recomputesMatchingResponseHash() public { + // The response and requested hash agree, but neither matches the signed transaction fields. + bytes32 dishonestHash = bytes32(uint256(1)); + string memory response = _replace(vm.toString(PROPOSAL_SAFE_TX_HASH), vm.toString(dishonestHash)); + vm.expectRevert( + abi.encodeWithSelector( + Safe.ProposedTransactionMismatch.selector, + "recomputed safeTxHash", + vm.toString(PROPOSAL_SAFE_TX_HASH), + vm.toString(dishonestHash) + ) + ); + this.exposedValidate(response, dishonestHash); + } + + function test_Safe_validateProposedTransactionJson_revertsOnMalformedJson() public { + vm.expectRevert(); + this.exposedValidate("{", PROPOSAL_SAFE_TX_HASH); + } + + function test_Safe_validateProposedTransactionJson_revertsOnInvalidOperation() public { + _expectRejected('"operation": 1', '"operation": 256'); + _expectRejected('"operation": 1', '"operation": -1'); + } + + function test_Safe_validateProposedTransactionJson_revertsOnInvalidNumbers() public { + _expectRejected('"nonce": "3"', '"nonce": []'); + _expectRejected('"nonce": "3"', '"nonce": null'); + _expectRejected('"gasPrice": "0"', '"gasPrice": "-1"'); + } + + function test_Safe_validateProposedTransactionJson_revertsOnMissingRequiredFields() public { + _expectRejected('"safeTxGas": "0",', ""); + _expectRejected(string.concat('"data": "', vm.toString(data), '",'), ""); + } + + function test_Safe_validateProposedTransactionJson_revertsOnInvalidAddresses() public { + _expectRejected('"gasToken": null', '"gasToken": {}'); + _expectRejected('"refundReceiver": null', '"refundReceiver": "invalid"'); + } + + function test_Safe_validateProposedTransactionJson_revertsOnInvalidData() public { + string memory original = string.concat('"data": "', vm.toString(data), '"'); + _expectRejected(original, '"data": null'); + _expectRejected(original, '"data": "nothex"'); + } + + function test_Safe_validateProposedTransactionJson_acceptsMissingNullableAddresses() public view { + string memory response = _replace('"gasToken": null,', ""); + response = vm.replace(response, '"refundReceiver": null,', ""); + Safe.SafeTxHashes memory hashes = this.exposedValidate(response, PROPOSAL_SAFE_TX_HASH); + assertEq(hashes.safeTxHash, PROPOSAL_SAFE_TX_HASH); + } + + function exposedValidate(string memory response, bytes32 safeTxHash) + external + view + returns (Safe.SafeTxHashes memory) + { + return safe.validateProposedTransactionJson( + response, safeTxHash, to, 0, data, Enum.Operation.DelegateCall, PROPOSAL_NONCE + ); + } + + function _expectMismatch( + string memory original, + string memory replacement, + string memory field, + string memory expected, + string memory actual + ) private { + string memory response = _replace(original, replacement); + vm.expectRevert(abi.encodeWithSelector(Safe.ProposedTransactionMismatch.selector, field, expected, actual)); + this.exposedValidate(response, PROPOSAL_SAFE_TX_HASH); + } + + function _expectRejected(string memory original, string memory replacement) private { + string memory response = _replace(original, replacement); + // Malformed JSON values can fail in the parser before the comparison reports a mismatch. + vm.expectRevert(); + this.exposedValidate(response, PROPOSAL_SAFE_TX_HASH); + } + + function _replace(string memory original, string memory replacement) private view returns (string memory) { + string memory response = vm.replace(json, original, replacement); + assertTrue(keccak256(bytes(response)) != keccak256(bytes(json)), "fixture replacement did not match"); + return response; + } +} From 48959661c0ca492ec16cdd491582171c0a7a3610 Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Wed, 4 Nov 2026 11:54:43 +0000 Subject: [PATCH 2/3] Clarify which simulation failures return false --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b2af8ec..9260f46 100644 --- a/README.md +++ b/README.md @@ -181,7 +181,7 @@ bool ok = safe.simulateTransactionMultiSigNoSign(target, data, signers); bool ok = safe.simulateTransactionsMultiSigNoSign(targets, datas, signers); ``` -All simulate functions return `true` on success and `false` on revert — they never throw, so you can inspect failures without aborting the script. +Simulation helpers return `true` when Safe execution succeeds and `false` when it fails. Errors before execution, such as invalid batch inputs or missing MultiSend code, can still revert. Mode detection helpers let you branch between simulation and broadcast in one script: From 1e1f06e4f3f3e739477b6ed931af966ebdf2acf9 Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Wed, 4 Nov 2026 13:03:09 +0000 Subject: [PATCH 3/3] Distinguish missing required fields from empty calldata in tests --- test/SafeValidationExtra.t.sol | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/SafeValidationExtra.t.sol b/test/SafeValidationExtra.t.sol index 89a4d4f..60ceae8 100644 --- a/test/SafeValidationExtra.t.sol +++ b/test/SafeValidationExtra.t.sol @@ -160,9 +160,24 @@ contract SafeValidationNegativeTest is BaseProposalFixture { function test_Safe_validateProposedTransactionJson_revertsOnMissingRequiredFields() public { _expectRejected('"safeTxGas": "0",', ""); + _expectRejected('"nonce": "3",', ""); + } + + function test_Safe_validateProposedTransactionJson_revertsOnMissingNonemptyData() public { _expectRejected(string.concat('"data": "', vm.toString(data), '",'), ""); } + function test_Safe_validateProposedTransactionJson_acceptsMissingEmptyData() public view { + Safe.SafeTxHashes memory expected = safe.getSafeTxHashes(to, 0, "", Enum.Operation.DelegateCall, PROPOSAL_NONCE); + string memory response = _replace(string.concat('"data": "', vm.toString(data), '",'), ""); + response = vm.replace(response, vm.toString(PROPOSAL_SAFE_TX_HASH), vm.toString(expected.safeTxHash)); + + Safe.SafeTxHashes memory actual = safe.validateProposedTransactionJson( + response, expected.safeTxHash, to, 0, "", Enum.Operation.DelegateCall, PROPOSAL_NONCE + ); + assertEq(actual.safeTxHash, expected.safeTxHash); + } + function test_Safe_validateProposedTransactionJson_revertsOnInvalidAddresses() public { _expectRejected('"gasToken": null', '"gasToken": {}'); _expectRejected('"refundReceiver": null', '"refundReceiver": "invalid"');