diff --git a/script/universal/MultisigBase.sol b/script/universal/MultisigBase.sol index d2d79d6..87efc15 100644 --- a/script/universal/MultisigBase.sol +++ b/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, diff --git a/script/universal/MultisigBuilder.sol b/script/universal/MultisigBuilder.sol index 7601e58..fe19320 100644 --- a/script/universal/MultisigBuilder.sol +++ b/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); diff --git a/script/universal/NestedMultisigBuilder.sol b/script/universal/NestedMultisigBuilder.sol index c977885..1d726e7 100644 --- a/script/universal/NestedMultisigBuilder.sol +++ b/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); diff --git a/script/universal/Signatures.sol b/script/universal/Signatures.sol index 42efd0b..b1fd056 100644 --- a/script/universal/Signatures.sol +++ b/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))); diff --git a/script/universal/Simulation.sol b/script/universal/Simulation.sol index ca51288..d77c79b 100644 --- a/script/universal/Simulation.sol +++ b/script/universal/Simulation.sol @@ -51,7 +51,11 @@ 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); @@ -59,7 +63,7 @@ library Simulation { } 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")); diff --git a/test/universal/NestedMultisigBuilder.t.sol b/test/universal/NestedMultisigBuilder.t.sol index 98d70bb..5350a34 100644 --- a/test/universal/NestedMultisigBuilder.t.sol +++ b/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 {