Skip to content

Commit

Permalink
Fix simulation contract deployment during broadcast (#110)
Browse files Browse the repository at this point in the history 
* Fix simulation contract deployment during broadcast

* Add support for skipping signatures that aren't from owners (and warning via console)

* Fix tests
  • Loading branch information
@mdehoog
mdehoog authored Dec 12, 2024
1 parent ed36aac commit 4945865
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 33 deletions.
20 changes: 14 additions & 6 deletions script/universal/MultisigBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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");
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 2 additions & 4 deletions script/universal/MultisigBuilder.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
8 changes: 2 additions & 6 deletions script/universal/NestedMultisigBuilder.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down
44 changes: 32 additions & 12 deletions script/universal/Signatures.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Expand All @@ -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.
Expand All @@ -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)));
Expand Down
12 changes: 8 additions & 4 deletions script/universal/Simulation.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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"));
Expand Down
2 changes: 1 addition & 1 deletion test/universal/NestedMultisigBuilder.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 4945865

Please sign in to comment.