Fix simulation contract deployment during broadcast

script/universal/MultisigBase.sol

@@ -90,10 +90,12 @@ abstract contract MultisigBase is CommonBase {
         IGnosisSafe(_safe).checkSignatures({dataHash: hash, data: data, signatures: _signatures});
     }
 
-    function _executeTransaction(address _safe, IMulticall3.Call3[] memory _calls, bytes memory _signatures)
-        internal
-        returns (Vm.AccountAccess[] memory, Simulation.Payload memory)
-    {
+    function _executeTransaction(
+        address _safe,
+        IMulticall3.Call3[] memory _calls,
+        bytes memory _signatures,
+        bool _broadcast
+    ) internal returns (Vm.AccountAccess[] memory, Simulation.Payload memory) {
         bytes memory data = abi.encodeCall(IMulticall3.aggregate3, (_calls));
         bytes32 hash = _getTransactionHash(_safe, data);
         _signatures = Signatures.prepareSignatures(_safe, hash, _signatures);
@@ -102,7 +104,7 @@ abstract contract MultisigBase is CommonBase {
         Simulation.logSimulationLink({_to: _safe, _from: msg.sender, _data: simData});
 
         vm.startStateDiffRecording();
-        bool success = _execTransaction(_safe, data, _signatures);
+        bool success = _execTransaction(_safe, data, _signatures, _broadcast);
         Vm.AccountAccess[] memory accesses = vm.stopAndReturnStateDiff();
         require(success, "MultisigBase::_executeTransaction: Transaction failed");
         require(accesses.length > 0, "MultisigBase::_executeTransaction: No state changes");
@@ -164,7 +166,13 @@ abstract contract MultisigBase is CommonBase {
         );
     }
 
-    function _execTransaction(address _safe, bytes memory _data, bytes memory _signatures) internal returns (bool) {
+    function _execTransaction(address _safe, bytes memory _data, bytes memory _signatures, bool _broadcast)
+        internal
+        returns (bool)
+    {
+        if (_broadcast) {
+            vm.broadcast();
+        }
         return IGnosisSafe(_safe).execTransaction({
             to: MULTICALL3_ADDRESS,
             value: 0,

script/universal/MultisigBuilder.sol

@@ -101,7 +101,7 @@ abstract contract MultisigBuilder is MultisigBase {
         vm.store(safe, SAFE_NONCE_SLOT, bytes32(_getNonce(safe)));
 
         (Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) =
-            _executeTransaction(safe, _buildCalls(), _signatures);
+            _executeTransaction(safe, _buildCalls(), _signatures, false);
 
         _postRun(accesses, simPayload);
         _postCheck(accesses, simPayload);
@@ -117,10 +117,8 @@ abstract contract MultisigBuilder is MultisigBase {
      * step 1. In this scenario, the caller doesn't need to be a signer of the multisig.
      */
     function run(bytes memory _signatures) public {
-        vm.startBroadcast();
         (Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) =
-            _executeTransaction(_ownerSafe(), _buildCalls(), _signatures);
-        vm.stopBroadcast();
+            _executeTransaction(_ownerSafe(), _buildCalls(), _signatures, true);
 
         _postRun(accesses, simPayload);
         _postCheck(accesses, simPayload);

script/universal/NestedMultisigBuilder.sol

@@ -109,10 +109,8 @@ abstract contract NestedMultisigBuilder is MultisigBase {
         IMulticall3.Call3[] memory nestedCalls = _buildCalls();
         IMulticall3.Call3 memory call = _generateApproveCall(_ownerSafe(), nestedCalls);
 
-        vm.startBroadcast();
         (Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) =
-            _executeTransaction(_signerSafe, _toArray(call), _signatures);
-        vm.stopBroadcast();
+            _executeTransaction(_signerSafe, _toArray(call), _signatures, true);
 
         _postApprove(accesses, simPayload);
     }
@@ -129,10 +127,8 @@ abstract contract NestedMultisigBuilder is MultisigBase {
         // signatures is empty, because `_executeTransaction` internally collects all of the approvedHash addresses
         bytes memory signatures;
 
-        vm.startBroadcast();
         (Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) =
-            _executeTransaction(_ownerSafe(), nestedCalls, signatures);
-        vm.stopBroadcast();
+            _executeTransaction(_ownerSafe(), nestedCalls, signatures, true);
 
         _postRun(accesses, simPayload);
         _postCheck(accesses, simPayload);

script/universal/Signatures.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.15;
 import {Bytes} from "@eth-optimism-bedrock/src/libraries/Bytes.sol";
 import {LibSort} from "@solady/utils/LibSort.sol";
 import {IGnosisSafe} from "./IGnosisSafe.sol";
+import {console} from "forge-std/console.sol";
 
 library Signatures {
     function prepareSignatures(address _safe, bytes32 hash, bytes memory _signatures)
@@ -17,7 +18,9 @@ library Signatures {
         _signatures = bytes.concat(prevalidatedSignatures, _signatures);
 
         // safe requires all signatures to be unique, and sorted ascending by public key
-        return sortUniqueSignatures(_signatures, hash, IGnosisSafe(_safe).getThreshold(), prevalidatedSignatures.length);
+        return sortUniqueSignatures(
+            _safe, _signatures, hash, IGnosisSafe(_safe).getThreshold(), prevalidatedSignatures.length
+        );
     }
 
     function genPrevalidatedSignatures(address[] memory _addresses) internal pure returns (bytes memory) {
@@ -64,6 +67,7 @@ library Signatures {
     /**
      * @notice Sorts the signatures in ascending order of the signer's address, and removes any duplicates.
      * @dev see https://github.com/safe-global/safe-smart-account/blob/1ed486bb148fe40c26be58d1b517cec163980027/contracts/Safe.sol#L265-L334
+     * @param _safe Address of the Safe that should verify the signatures.
      * @param _signatures Signature data that should be verified.
      *                    Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
      *                    Can be suffixed with EIP-1271 signatures after threshold*65 bytes.
@@ -73,22 +77,23 @@ library Signatures {
      *                      Can be used to accomodate any additional signatures prepended to the array.
      *                      If prevalidated signatures were prepended, this should be the length of those signatures.
      */
-    function sortUniqueSignatures(bytes memory _signatures, bytes32 dataHash, uint256 threshold, uint256 dynamicOffset)
-        internal
-        pure
-        returns (bytes memory)
-    {
+    function sortUniqueSignatures(
+        address _safe,
+        bytes memory _signatures,
+        bytes32 dataHash,
+        uint256 threshold,
+        uint256 dynamicOffset
+    ) internal view returns (bytes memory) {
         bytes memory sorted;
         uint256 count = uint256(_signatures.length / 0x41);
         uint256[] memory addressesAndIndexes = new uint256[](threshold);
         address[] memory uniqueAddresses = new address[](threshold);
-        uint8 v;
-        bytes32 r;
-        bytes32 s;
         uint256 j;
         for (uint256 i; i < count; i++) {
-            (v, r, s) = signatureSplit(_signatures, i);
-            address owner = extractOwner(dataHash, r, s, v);
+            (address owner, bool isOwner) = extractOwner(_safe, _signatures, dataHash, i);
+            if (!isOwner) {
+                continue;
+            }
 
             // skip duplicate owners
             uint256 k;
@@ -109,7 +114,7 @@ library Signatures {
         LibSort.sort(addressesAndIndexes);
         for (uint256 i; i < count; i++) {
             uint256 index = addressesAndIndexes[i] & 0xffffffff;
-            (v, r, s) = signatureSplit(_signatures, index);
+            (uint8 v, bytes32 r, bytes32 s) = signatureSplit(_signatures, index);
             if (v == 0) {
                 // The `s` value is used by safe as a lookup into the signature bytes.
                 // Increment by the offset so that the lookup location remains correct.
@@ -125,6 +130,21 @@ library Signatures {
         return sorted;
     }
 
+    function extractOwner(address _safe, bytes memory _signatures, bytes32 dataHash, uint256 i)
+        internal
+        view
+        returns (address, bool)
+    {
+        (uint8 v, bytes32 r, bytes32 s) = signatureSplit(_signatures, i);
+        address owner = extractOwner(dataHash, r, s, v);
+        bool isOwner = IGnosisSafe(_safe).isOwner(owner);
+        if (!isOwner) {
+            console.log("---\nSkipping the following signature, which was recovered to a non-owner address %s:", owner);
+            console.logBytes(abi.encodePacked(r, s, v));
+        }
+        return (owner, isOwner);
+    }
+
     function extractOwner(bytes32 dataHash, bytes32 r, bytes32 s, uint8 v) internal pure returns (address) {
         if (v <= 1) {
             return address(uint160(uint256(r)));

script/universal/Simulation.sol

@@ -51,15 +51,19 @@ library Simulation {
         return accesses;
     }
 
-    function overrideSafeThresholdAndNonce(address _safe, uint256 _nonce) public view returns (StateOverride memory) {
+    function overrideSafeThresholdAndNonce(address _safe, uint256 _nonce)
+        internal
+        view
+        returns (StateOverride memory)
+    {
         StateOverride memory state = StateOverride({contractAddress: _safe, overrides: new StorageOverride[](0)});
         state = addThresholdOverride(_safe, state);
         state = addNonceOverride(_safe, state, _nonce);
         return state;
     }
 
     function overrideSafeThresholdOwnerAndNonce(address _safe, address _owner, uint256 _nonce)
-        public
+        internal
         view
         returns (StateOverride memory)
     {
@@ -133,12 +137,12 @@ library Simulation {
         return StateOverride({contractAddress: _state.contractAddress, overrides: overrides});
     }
 
-    function logSimulationLink(address _to, bytes memory _data, address _from) public view {
+    function logSimulationLink(address _to, bytes memory _data, address _from) internal view {
         logSimulationLink(_to, _data, _from, new StateOverride[](0));
     }
 
     function logSimulationLink(address _to, bytes memory _data, address _from, StateOverride[] memory _overrides)
-        public
+        internal
         view
     {
         string memory proj = vm.envOr("TENDERLY_PROJECT", string("TENDERLY_PROJECT"));

test/universal/NestedMultisigBuilder.t.sol

@@ -97,7 +97,7 @@ contract NestedMultisigBuilderTest is Test, NestedMultisigBuilder {
         bytes memory data = abi.encodeCall(this.approve, (safe2, abi.encodePacked(r, s, v)));
         (bool success, bytes memory result) = address(this).call(data);
         assertFalse(success);
-        assertEq(result, abi.encodeWithSignature("Error(string)", "GS026"));
+        assertEq(result, abi.encodeWithSignature("Error(string)", "not enough signatures"));
     }
 
     function test_run() external {