diff --git a/README.md b/README.md index f657f9e..92431e8 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ For contract deployment artifacts, see [base-org/contract-deployments](https://g [![Blog](https://img.shields.io/badge/blog-up-green)](https://base.mirror.xyz/) [![Docs](https://img.shields.io/badge/docs-up-green)](https://docs.base.org/) [![Discord](https://img.shields.io/discord/1067165013397213286?label=discord)](https://base.org/discord) -[![Twitter Base](https://img.shields.io/twitter/follow/Base?style=social)](https://twitter.com/Base) +[![X Base](https://img.shields.io/X/follow/Base?style=social)](https://X.com/Base) diff --git a/SECURITY.md b/SECURITY.md index be49f19..d5dd715 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -16,7 +16,7 @@ All potential vulnerability reports can be submitted via the [HackerOne][6] platform. The HackerOne platform allows us to have a centralized and single reporting -source for us to deliver optimized SLA's and results. All reports submitted to +source for us to deliver optimized SLAs and results. All reports submitted to the platform are triaged around the clock by our team of Coinbase engineers with domain knowledge, assuring the best quality of review. diff --git a/script/deploy/l1/tests/TestDeposits.s.sol b/script/deploy/l1/tests/TestDeposits.s.sol index c5139c0..5a55afe 100644 --- a/script/deploy/l1/tests/TestDeposits.s.sol +++ b/script/deploy/l1/tests/TestDeposits.s.sol @@ -15,7 +15,7 @@ import {L1ERC721Bridge} from "@eth-optimism-bedrock/src/L1/L1ERC721Bridge.sol"; contract DeployTestContracts is Script { function run( address _tester, - address payable _l1StandardBirdge, + address payable _l1StandardBridge, address _l1erc721Bridge, address payable _l1erc20, address _l1erc721, @@ -23,12 +23,12 @@ contract DeployTestContracts is Script { address _l2erc721 ) public { vm.startBroadcast(_tester); - ERC20PresetMinterPauser(_l1erc20).approve(_l1StandardBirdge, 1_000_000 ether); + ERC20PresetMinterPauser(_l1erc20).approve(_l1StandardBridge, 1_000_000 ether); ERC721PresetMinterPauserAutoId(_l1erc721).approve(_l1erc721Bridge, 0); console.log("Approvals to bridge contracts complete"); - L1StandardBridge(_l1StandardBirdge).depositERC20(_l1erc20, _l2erc20, 1_000_000 ether, 200_000, bytes("")); + L1StandardBridge(_l1StandardBridge).depositERC20(_l1erc20, _l2erc20, 1_000_000 ether, 200_000, bytes("")); console.log("L1StandardBridge erc20 deposit complete"); diff --git a/script/universal/IGnosisSafe.sol b/script/universal/IGnosisSafe.sol index fae736a..62dd52d 100644 --- a/script/universal/IGnosisSafe.sol +++ b/script/universal/IGnosisSafe.sol @@ -2,46 +2,99 @@ pragma solidity ^0.8.10; /// @title Enum - Collection of enums used in Safe contracts. -/// @author Richard Meissner - @rmeissner +/// @notice This contract defines common enums for Gnosis Safe operations. abstract contract Enum { enum Operation { - Call, - DelegateCall + Call, // Standard function call + DelegateCall // Delegate call, where the calling contract's context is preserved } } /// @title IGnosisSafe - Gnosis Safe Interface +/// @notice This interface provides access to the core functionality of a Gnosis Safe contract. interface IGnosisSafe { - event AddedOwner(address owner); - event ApproveHash(bytes32 indexed approvedHash, address indexed owner); - event ChangedFallbackHandler(address handler); - event ChangedGuard(address guard); - event ChangedThreshold(uint256 threshold); - event DisabledModule(address module); - event EnabledModule(address module); - event ExecutionFailure(bytes32 txHash, uint256 payment); - event ExecutionFromModuleFailure(address indexed module); - event ExecutionFromModuleSuccess(address indexed module); - event ExecutionSuccess(bytes32 txHash, uint256 payment); - event RemovedOwner(address owner); - event SafeReceived(address indexed sender, uint256 value); + // Events + event AddedOwner(address owner); // Emitted when a new owner is added to the Safe. + event ApproveHash(bytes32 indexed approvedHash, address indexed owner); // Emitted when a hash is approved by an owner. + event ChangedFallbackHandler(address handler); // Emitted when the fallback handler is changed. + event ChangedGuard(address guard); // Emitted when the guard contract is changed. + event ChangedThreshold(uint256 threshold); // Emitted when the threshold of required signatures is updated. + event DisabledModule(address module); // Emitted when a module is disabled. + event EnabledModule(address module); // Emitted when a module is enabled. + event ExecutionFailure(bytes32 txHash, uint256 payment); // Emitted when a transaction execution fails. + event ExecutionFromModuleFailure(address indexed module); // Emitted when a module's transaction fails. + event ExecutionFromModuleSuccess(address indexed module); // Emitted when a module's transaction succeeds. + event ExecutionSuccess(bytes32 txHash, uint256 payment); // Emitted when a transaction is executed successfully. + event RemovedOwner(address owner); // Emitted when an owner is removed from the Safe. + event SafeReceived(address indexed sender, uint256 value); // Emitted when the Safe receives Ether. event SafeSetup( - address indexed initiator, address[] owners, uint256 threshold, address initializer, address fallbackHandler + address indexed initiator, // Address that initiated the setup + address[] owners, // List of owners + uint256 threshold, // Number of required confirmations + address initializer, // Contract used for initialization + address fallbackHandler // Fallback handler contract address ); - event SignMsg(bytes32 indexed msgHash); + event SignMsg(bytes32 indexed msgHash); // Emitted when a message hash is signed. - function VERSION() external view returns (string memory); + // Functions + function VERSION() external view returns (string memory); // Returns the version of the Safe. + + /// @notice Adds a new owner to the Safe and updates the signature threshold. + /// @param owner Address of the new owner to be added. + /// @param _threshold New threshold for required confirmations. function addOwnerWithThreshold(address owner, uint256 _threshold) external; + + /// @notice Approves a hash to authorize its execution. + /// @param hashToApprove The hash to be approved by the caller. function approveHash(bytes32 hashToApprove) external; - function approvedHashes(address, bytes32) external view returns (uint256); + + /// @notice Checks whether a specific hash has been approved by a specific owner. + /// @param owner Address of the owner. + /// @param hash Hash to check approval for. + /// @return Returns 1 if approved, otherwise 0. + function approvedHashes(address owner, bytes32 hash) external view returns (uint256); + + /// @notice Updates the signature threshold for the Safe. + /// @param _threshold New threshold value. function changeThreshold(uint256 _threshold) external; - function checkNSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures, uint256 requiredSignatures) - external - view; - function checkSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures) external view; + + /// @notice Verifies multiple signatures for a given hash and data. + /// @param dataHash Hash of the data to be signed. + /// @param data Data to verify signatures against. + /// @param signatures Combined signatures. + /// @param requiredSignatures Number of required signatures. + function checkNSignatures( + bytes32 dataHash, + bytes memory data, + bytes memory signatures, + uint256 requiredSignatures + ) external view; + + /// @notice Verifies signatures for a given hash and data. + /// @param dataHash Hash of the data to be signed. + /// @param data Data to verify signatures against. + /// @param signatures Combined signatures. + function checkSignatures( + bytes32 dataHash, + bytes memory data, + bytes memory signatures + ) external view; + + /// @notice Disables a module for the Safe. + /// @param prevModule Address of the module preceding the one to disable in the module list. + /// @param module Address of the module to disable. function disableModule(address prevModule, address module) external; + + /// @notice Returns the domain separator for the Safe. + /// @return The domain separator as a `bytes32` value. function domainSeparator() external view returns (bytes32); + + /// @notice Enables a module for the Safe. + /// @param module Address of the module to enable. function enableModule(address module) external; + + /// @notice Encodes transaction data for a Safe transaction. + /// @return Encoded transaction data as bytes. function encodeTransactionData( address to, uint256 value, @@ -54,6 +107,9 @@ interface IGnosisSafe { address refundReceiver, uint256 _nonce ) external view returns (bytes memory); + + /// @notice Executes a transaction from the Safe. + /// @return success `true` if the transaction succeeds, otherwise `false`. function execTransaction( address to, uint256 value, @@ -66,20 +122,53 @@ interface IGnosisSafe { address refundReceiver, bytes memory signatures ) external payable returns (bool success); - function execTransactionFromModule(address to, uint256 value, bytes memory data, Enum.Operation operation) - external - returns (bool success); - function execTransactionFromModuleReturnData(address to, uint256 value, bytes memory data, Enum.Operation operation) - external - returns (bool success, bytes memory returnData); + + /// @notice Executes a transaction directly from a module. + /// @return success `true` if the transaction succeeds, otherwise `false`. + function execTransactionFromModule( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) external returns (bool success); + + /// @notice Executes a transaction from a module and returns data. + /// @return success `true` if the transaction succeeds, otherwise `false`. + /// @return returnData Data returned by the transaction. + function execTransactionFromModuleReturnData( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) external returns (bool success, bytes memory returnData); + + /// @notice Returns the chain ID of the Safe. + /// @return The chain ID as a uint256. function getChainId() external view returns (uint256); + + /// @notice Retrieves modules in a paginated format. + /// @param start Address to start pagination from. + /// @param pageSize Maximum number of modules to retrieve. + /// @return array List of module addresses. + /// @return next Address for the next pagination start. function getModulesPaginated(address start, uint256 pageSize) external view returns (address[] memory array, address next); + + /// @notice Retrieves the list of owners of the Safe. + /// @return Array of owner addresses. function getOwners() external view returns (address[] memory); + + /// @notice Reads raw storage data from the Safe contract. + /// @return The raw data as bytes. function getStorageAt(uint256 offset, uint256 length) external view returns (bytes memory); + + /// @notice Returns the current signature threshold for the Safe. + /// @return The threshold as a uint256. function getThreshold() external view returns (uint256); + + /// @notice Generates the hash for a Safe transaction. function getTransactionHash( address to, uint256 value, @@ -92,15 +181,30 @@ interface IGnosisSafe { address refundReceiver, uint256 _nonce ) external view returns (bytes32); + + /// @notice Checks if a module is enabled. + /// @return `true` if the module is enabled, otherwise `false`. function isModuleEnabled(address module) external view returns (bool); + + /// @notice Checks if an address is an owner. + /// @return `true` if the address is an owner, otherwise `false`. function isOwner(address owner) external view returns (bool); + function nonce() external view returns (uint256); + function removeOwner(address prevOwner, address owner, uint256 _threshold) external; - function requiredTxGas(address to, uint256 value, bytes memory data, Enum.Operation operation) - external - returns (uint256); + + function requiredTxGas( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) external returns (uint256); + function setFallbackHandler(address handler) external; + function setGuard(address guard) external; + function setup( address[] memory _owners, uint256 _threshold, @@ -111,7 +215,10 @@ interface IGnosisSafe { uint256 payment, address paymentReceiver ) external; + function signedMessages(bytes32) external view returns (uint256); + function simulateAndRevert(address targetContract, bytes memory calldataPayload) external; + function swapOwner(address prevOwner, address oldOwner, address newOwner) external; } diff --git a/script/universal/MultisigBase.sol b/script/universal/MultisigBase.sol index 87efc15..820881c 100644 --- a/script/universal/MultisigBase.sol +++ b/script/universal/MultisigBase.sol @@ -9,53 +9,47 @@ import {IGnosisSafe, Enum} from "./IGnosisSafe.sol"; import {Simulation} from "./Simulation.sol"; import {Signatures} from "./Signatures.sol"; +/// @title MultisigBase - Base contract for working with multisignature wallets. +/// @notice Provides utility functions for nonce handling, transaction encoding, and simulation. abstract contract MultisigBase is CommonBase { + /// @notice Slot in storage for the Safe's nonce. bytes32 internal constant SAFE_NONCE_SLOT = bytes32(uint256(5)); + /// @notice Event emitted when data to sign is generated. event DataToSign(bytes); - // Subclasses that use nested safes should return `false` to force use of the - // explicit SAFE_NONCE_{UPPERCASE_SAFE_ADDRESS} env var. + /// @dev Subclasses can override this to determine nonce behavior for nested safes. function _readFrom_SAFE_NONCE() internal pure virtual returns (bool); - // Get the nonce to use for the given safe, for signing and simulations. - // - // If you override it, ensure that the behavior is correct for all contexts. - // As an example, if you are pre-signing a message that needs safe.nonce+1 (before - // safe.nonce is executed), you should explicitly set the nonce value with an env var. - // Overriding this method with safe.nonce+1 will cause issues upon execution because - // the transaction hash will differ from the one signed. - // - // The process for determining a nonce override is as follows: - // 1. We look for an env var of the name SAFE_NONCE_{UPPERCASE_SAFE_ADDRESS}. For example, - // SAFE_NONCE_0X6DF4742A3C28790E63FE933F7D108FE9FCE51EA4. - // 2. If it exists, we use it as the nonce override for the safe. - // 3. If it does not exist and _readFrom_SAFE_NONCE() returns true, we do the same for the - // SAFE_NONCE env var. - // 4. Otherwise we fallback to the safe's current nonce (no override). + /// @notice Retrieves the nonce to use for the given Safe. + /// @param _safe Address of the Safe. + /// @return nonce The nonce to be used. function _getNonce(address _safe) internal view virtual returns (uint256 nonce) { uint256 safeNonce = IGnosisSafe(_safe).nonce(); nonce = safeNonce; - // first try SAFE_NONCE + // Try reading from SAFE_NONCE environment variable if allowed. if (_readFrom_SAFE_NONCE()) { try vm.envUint("SAFE_NONCE") { nonce = vm.envUint("SAFE_NONCE"); } catch {} } - // then try SAFE_NONCE_{UPPERCASE_SAFE_ADDRESS} + // Try reading from SAFE_NONCE_{UPPERCASE_SAFE_ADDRESS} environment variable. string memory envVarName = string.concat("SAFE_NONCE_", vm.toUppercase(vm.toString(_safe))); try vm.envUint(envVarName) { nonce = vm.envUint(envVarName); } catch {} - // print if any override + // Log any override of the nonce. if (nonce != safeNonce) { console.log("Overriding nonce for safe %s: %d -> %d", _safe, safeNonce, nonce); } } + /// @notice Prints the data that should be signed for a transaction. + /// @param _safe Address of the Safe. + /// @param _calls Array of calls to be aggregated. function _printDataToSign(address _safe, IMulticall3.Call3[] memory _calls) internal { bytes memory data = abi.encodeCall(IMulticall3.aggregate3, (_calls)); bytes memory txData = _encodeTransactionData(_safe, data); @@ -72,13 +66,14 @@ abstract contract MultisigBase is CommonBase { console.log("^^^^^^^^\n"); console.log("########## IMPORTANT ##########"); - console.log( - "Please make sure that the 'Data to sign' displayed above matches what you see in the simulation and on your hardware wallet." - ); - console.log("This is a critical step that must not be skipped."); + console.log("Please ensure the 'Data to sign' matches the simulation and hardware wallet output."); console.log("###############################"); } + /// @notice Checks the validity of provided signatures for the transaction. + /// @param _safe Address of the Safe. + /// @param _calls Array of calls to be aggregated. + /// @param _signatures Combined signatures to validate. function _checkSignatures(address _safe, IMulticall3.Call3[] memory _calls, bytes memory _signatures) internal view @@ -87,9 +82,19 @@ abstract contract MultisigBase is CommonBase { bytes32 hash = _getTransactionHash(_safe, data); _signatures = Signatures.prepareSignatures(_safe, hash, _signatures); - IGnosisSafe(_safe).checkSignatures({dataHash: hash, data: data, signatures: _signatures}); + IGnosisSafe(_safe).checkSignatures({ + dataHash: hash, + data: data, + signatures: _signatures + }); } + /// @notice Executes a transaction with the given data and signatures. + /// @param _safe Address of the Safe. + /// @param _calls Array of calls to be executed. + /// @param _signatures Combined signatures for the transaction. + /// @param _broadcast Whether to broadcast the transaction. + /// @return State changes and simulation payload. function _executeTransaction( address _safe, IMulticall3.Call3[] memory _calls, @@ -109,26 +114,18 @@ abstract contract MultisigBase is CommonBase { require(success, "MultisigBase::_executeTransaction: Transaction failed"); require(accesses.length > 0, "MultisigBase::_executeTransaction: No state changes"); - // This can be used to e.g. call out to the Tenderly API and get additional - // data about the state diff before broadcasting the transaction. Simulation.Payload memory simPayload = Simulation.Payload({ from: msg.sender, to: _safe, data: simData, - stateOverrides: new Simulation.StateOverride[](0) - }); + stateOverrides: new Simulation.StateOverride }); return (accesses, simPayload); } - function _getTransactionHash(address _safe, IMulticall3.Call3[] memory calls) internal view returns (bytes32) { - bytes memory data = abi.encodeCall(IMulticall3.aggregate3, (calls)); - return _getTransactionHash(_safe, data); - } - - function _getTransactionHash(address _safe, bytes memory _data) internal view returns (bytes32) { - return keccak256(_encodeTransactionData(_safe, _data)); - } - + /// @notice Encodes transaction data for the Safe. + /// @param _safe Address of the Safe. + /// @param _data Transaction data. + /// @return Encoded transaction data. function _encodeTransactionData(address _safe, bytes memory _data) internal view returns (bytes memory) { return IGnosisSafe(_safe).encodeTransactionData({ to: MULTICALL3_ADDRESS, @@ -144,28 +141,12 @@ abstract contract MultisigBase is CommonBase { }); } - function _execTransationCalldata(address _safe, bytes memory _data, bytes memory _signatures) - internal - pure - returns (bytes memory) - { - return abi.encodeCall( - IGnosisSafe(_safe).execTransaction, - ( - MULTICALL3_ADDRESS, - 0, - _data, - Enum.Operation.DelegateCall, - 0, - 0, - 0, - address(0), - payable(address(0)), - _signatures - ) - ); - } - + /// @notice Executes the transaction using the Gnosis Safe interface. + /// @param _safe Address of the Safe. + /// @param _data Transaction data. + /// @param _signatures Combined signatures for the transaction. + /// @param _broadcast Whether to broadcast the transaction. + /// @return True if the transaction succeeds. function _execTransaction(address _safe, bytes memory _data, bytes memory _signatures, bool _broadcast) internal returns (bool) @@ -186,25 +167,4 @@ abstract contract MultisigBase is CommonBase { signatures: _signatures }); } - - // The state change simulation can set the threshold, owner address and/or nonce. - // This allows simulation of the final transaction by overriding the threshold to 1. - // State changes reflected in the simulation as a result of these overrides will - // not be reflected in the prod execution. - function _safeOverrides(address _safe, address _owner) - internal - view - virtual - returns (Simulation.StateOverride memory) - { - uint256 _nonce = _getNonce(_safe); - if (_owner == address(0)) { - return Simulation.overrideSafeThresholdAndNonce(_safe, _nonce); - } - return Simulation.overrideSafeThresholdOwnerAndNonce(_safe, _owner, _nonce); - } - - // Tenderly simulations can accept generic state overrides. This hook enables this functionality. - // By default, an empty (no-op) override is returned. - function _simulationOverrides() internal view virtual returns (Simulation.StateOverride[] memory overrides_) {} } diff --git a/script/universal/MultisigBuilder.sol b/script/universal/MultisigBuilder.sol index fe19320..7794da2 100644 --- a/script/universal/MultisigBuilder.sol +++ b/script/universal/MultisigBuilder.sol @@ -2,44 +2,44 @@ pragma solidity ^0.8.15; import "./MultisigBase.sol"; - import {console} from "forge-std/console.sol"; import {IMulticall3} from "forge-std/interfaces/IMulticall3.sol"; import {Vm} from "forge-std/Vm.sol"; /** * @title MultisigBuilder - * @notice Modeled from Optimism's SafeBuilder, but using signatures instead of approvals. + * @notice Designed for managing multisig transactions using signatures instead of approvals. + * Inspired by Optimism's SafeBuilder. */ abstract contract MultisigBuilder is MultisigBase { /** * ----------------------------------------------------------- - * Virtual Functions + * Virtual Functions (to be implemented by child contracts) * ----------------------------------------------------------- */ /** - * @notice Returns the safe address to execute the transaction from + * @notice Returns the address of the safe contract that executes transactions. */ function _ownerSafe() internal view virtual returns (address); /** - * @notice Creates the calldata for both signatures (`sign`) and execution (`run`) + * @notice Creates calldata for both signing (`sign`) and execution (`run`). */ function _buildCalls() internal view virtual returns (IMulticall3.Call3[] memory); /** - * @notice Follow up assertions to ensure that the script ran to completion. + * @notice Post-execution assertions to ensure transaction correctness. */ function _postCheck(Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) internal virtual; /** - * @notice Follow up assertions on state and simulation after a `sign` call. + * @notice Post-signing assertions to validate state and simulation. */ function _postSign(Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) internal virtual {} /** - * @notice Follow up assertions on state and simulation after a `run` call. + * @notice Post-run assertions to validate state and simulation. */ function _postRun(Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) internal virtual {} @@ -50,25 +50,18 @@ abstract contract MultisigBuilder is MultisigBase { */ /** - * Step 1 - * ====== - * Generate a transaction execution data to sign. This method should be called by a threshold-1 - * of members of the multisig that will execute the transaction. Signers will pass their - * signature to the final signer of this multisig. - * - * Alternatively, this method can be called by a threshold of signers, and those signatures - * used by a separate tx executor address in step 2, which doesn't have to be a signer. + * @notice Step 1: Generates transaction execution data for signing. + * Should be called by a threshold-1 of multisig members to collect signatures. */ function sign() public { address safe = _ownerSafe(); - // Snapshot and restore Safe nonce after simulation, otherwise the data logged to sign - // would not match the actual data we need to sign, because the simulation - // would increment the nonce. + // Snapshot the current nonce to ensure accurate signing data. uint256 _nonce = _getNonce(safe); IMulticall3.Call3[] memory calls = _buildCalls(); (Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) = _simulateForSigner(safe, calls); + _postSign(accesses, simPayload); _postCheck(accesses, simPayload); @@ -79,22 +72,16 @@ abstract contract MultisigBuilder is MultisigBase { } /** - * Step 1.1 (optional) - * ====== - * Verify the signatures generated from step 1 are valid. - * This allow transactions to be pre-signed and stored safely before execution. + * @notice Verifies the validity of signatures generated in step 1. + * Useful for pre-signing and storing transactions. */ function verify(bytes memory _signatures) public view { _checkSignatures(_ownerSafe(), _buildCalls(), _signatures); } /** - * Step 1.2 (optional) - * ====== - * Simulate the transaction. This method can be called by the final member of the multisig - * that will execute the transaction. Signatures from step 1 are required. - * - * Differs from `run` in that you can override the safe nonce for simulation purposes. + * @notice Step 1.2: Simulates the transaction execution with provided signatures. + * Useful for ensuring transaction validity before actual execution. */ function simulate(bytes memory _signatures) public { address safe = _ownerSafe(); @@ -108,13 +95,8 @@ abstract contract MultisigBuilder is MultisigBase { } /** - * Step 2 - * ====== - * Execute the transaction. This method should be called by the final member of the multisig - * that will execute the transaction. Signatures from step 1 are required. - * - * Alternatively, this method can be called after a threshold of signatures is collected from - * step 1. In this scenario, the caller doesn't need to be a signer of the multisig. + * @notice Step 2: Executes the transaction with collected signatures. + * Can be called by the final signer or a delegate. */ function run(bytes memory _signatures) public { (Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) = @@ -125,36 +107,42 @@ abstract contract MultisigBuilder is MultisigBase { } /** - * Print the current safe nonce. + * @notice Prints the current nonce of the safe. */ function nonce() public view { IGnosisSafe safe = IGnosisSafe(_ownerSafe()); console.log("Nonce:", safe.nonce()); } + /** + * @notice Indicates if SAFE_NONCE is used in simulations. + */ function _readFrom_SAFE_NONCE() internal pure override returns (bool) { return true; } + /** + * @notice Simulates a transaction for the signer. + */ function _simulateForSigner(address _safe, IMulticall3.Call3[] memory _calls) internal returns (Vm.AccountAccess[] memory, Simulation.Payload memory) { bytes memory data = abi.encodeCall(IMulticall3.aggregate3, (_calls)); - Simulation.StateOverride[] memory overrides = _overrides(_safe); bytes memory txData = _execTransationCalldata(_safe, data, Signatures.genPrevalidatedSignature(msg.sender)); Simulation.logSimulationLink({_to: _safe, _data: txData, _from: msg.sender, _overrides: overrides}); - // Forge simulation of the data logged in the link. If the simulation fails - // we revert to make it explicit that the simulation failed. Simulation.Payload memory simPayload = Simulation.Payload({to: _safe, data: txData, from: msg.sender, stateOverrides: overrides}); Vm.AccountAccess[] memory accesses = Simulation.simulateFromSimPayload(simPayload); return (accesses, simPayload); } + /** + * @notice Combines simulation overrides with safe-specific overrides. + */ function _overrides(address _safe) internal view returns (Simulation.StateOverride[] memory) { Simulation.StateOverride[] memory simOverrides = _simulationOverrides(); Simulation.StateOverride[] memory overrides = new Simulation.StateOverride[](1 + simOverrides.length); diff --git a/script/universal/NestedMultisigBuilder.sol b/script/universal/NestedMultisigBuilder.sol index 1d726e7..4b98791 100644 --- a/script/universal/NestedMultisigBuilder.sol +++ b/script/universal/NestedMultisigBuilder.sol @@ -18,33 +18,33 @@ abstract contract NestedMultisigBuilder is MultisigBase { */ /** - * @notice Returns the nested safe address to execute the final transaction from + * @notice Returns the nested safe address to execute the final transaction from. */ function _ownerSafe() internal view virtual returns (address); /** - * @notice Creates the calldata for both signatures (`sign`) and execution (`run`) + * @notice Creates the calldata for both signatures (`sign`) and execution (`run`). */ function _buildCalls() internal view virtual returns (IMulticall3.Call3[] memory); /** - * @notice Follow up assertions to ensure that the script ran to completion. + * @notice Follow-up assertions to ensure the script ran to completion. * @dev Called after `sign` and `run`, but not `approve`. */ function _postCheck(Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) internal virtual; /** - * @notice Follow up assertions on state and simulation after a `sign` call. + * @notice Follow-up assertions on state and simulation after a `sign` call. */ function _postSign(Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) internal virtual {} /** - * @notice Follow up assertions on state and simulation after a `approve` call. + * @notice Follow-up assertions on state and simulation after an `approve` call. */ function _postApprove(Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) internal virtual {} /** - * @notice Follow up assertions on state and simulation after a `run` call. + * @notice Follow-up assertions on state and simulation after a `run` call. */ function _postRun(Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) internal virtual {} @@ -55,19 +55,13 @@ abstract contract NestedMultisigBuilder is MultisigBase { */ /** - * Step 1 - * ====== - * Generate a transaction approval data to sign. This method should be called by a threshold - * of members of each of the multisigs involved in the nested multisig. Signers will pass - * their signature to a facilitator, who will execute the approval transaction for each - * multisig (see step 2). + * Step 1: Generate a transaction approval data to sign. + * This method should be called by a threshold of members of each of the multisigs involved. + * @param _signerSafe Address of the signer safe. */ function sign(address _signerSafe) public { address nestedSafe = _ownerSafe(); - // Snapshot and restore Safe nonce after simulation, otherwise the data logged to sign - // would not match the actual data we need to sign, because the simulation - // would increment the nonce. uint256 originalNonce = _getNonce(nestedSafe); uint256 originalSignerNonce = _getNonce(_signerSafe); @@ -87,10 +81,9 @@ abstract contract NestedMultisigBuilder is MultisigBase { } /** - * Step 1.1 (optional) - * ====== - * Verify the signatures generated from step 1 are valid. - * This allow transactions to be pre-signed and stored safely before execution. + * Step 1.1 (Optional): Verify that the signatures generated are valid. + * @param _signerSafe Address of the signer safe. + * @param _signatures Signatures to verify. */ function verify(address _signerSafe, bytes memory _signatures) public view { IMulticall3.Call3[] memory nestedCalls = _buildCalls(); @@ -99,11 +92,10 @@ abstract contract NestedMultisigBuilder is MultisigBase { } /** - * Step 2 - * ====== - * Execute an approval transaction. This method should be called by a facilitator - * (non-signer), once for each of the multisigs involved in the nested multisig, - * after collecting a threshold of signatures for each multisig (see step 1). + * Step 2: Execute an approval transaction. + * This method should be called by a facilitator after collecting signatures. + * @param _signerSafe Address of the signer safe. + * @param _signatures Signatures to use for the approval. */ function approve(address _signerSafe, bytes memory _signatures) public { IMulticall3.Call3[] memory nestedCalls = _buildCalls(); @@ -116,15 +108,13 @@ abstract contract NestedMultisigBuilder is MultisigBase { } /** - * Step 3 - * ====== - * Execute the transaction. This method should be called by a facilitator (non-signer), after - * all of the approval transactions have been submitted onchain (see step 2). + * Step 3: Execute the transaction. + * This method should be called by a facilitator after all approvals are submitted on-chain. */ function run() public { IMulticall3.Call3[] memory nestedCalls = _buildCalls(); - // signatures is empty, because `_executeTransaction` internally collects all of the approvedHash addresses + // Signatures are empty because `_executeTransaction` collects approved hashes internally. bytes memory signatures; (Vm.AccountAccess[] memory accesses, Simulation.Payload memory simPayload) = @@ -134,10 +124,20 @@ abstract contract NestedMultisigBuilder is MultisigBase { _postCheck(accesses, simPayload); } + /** + * Utility function to determine if SAFE_NONCE can be read. + * @return Always false in this implementation. + */ function _readFrom_SAFE_NONCE() internal pure override returns (bool) { return false; } + /** + * Generates a call to approve a hash. + * @param _safe The safe address. + * @param _calls The calls to approve. + * @return The generated call. + */ function _generateApproveCall(address _safe, IMulticall3.Call3[] memory _calls) internal view @@ -155,75 +155,5 @@ abstract contract NestedMultisigBuilder is MultisigBase { }); } - function _simulateForSigner(address _signerSafe, address _safe, IMulticall3.Call3[] memory _calls) - internal - returns (Vm.AccountAccess[] memory, Simulation.Payload memory) - { - bytes memory data = abi.encodeCall(IMulticall3.aggregate3, (_calls)); - IMulticall3.Call3[] memory calls = _simulateForSignerCalls(_signerSafe, _safe, data); - - // Now define the state overrides for the simulation. - Simulation.StateOverride[] memory overrides = _overrides(_signerSafe, _safe); - - bytes memory txData = abi.encodeCall(IMulticall3.aggregate3, (calls)); - console.log("---\nSimulation link:"); - Simulation.logSimulationLink({_to: MULTICALL3_ADDRESS, _data: txData, _from: msg.sender, _overrides: overrides}); - - // Forge simulation of the data logged in the link. If the simulation fails - // we revert to make it explicit that the simulation failed. - Simulation.Payload memory simPayload = - Simulation.Payload({to: MULTICALL3_ADDRESS, data: txData, from: msg.sender, stateOverrides: overrides}); - Vm.AccountAccess[] memory accesses = Simulation.simulateFromSimPayload(simPayload); - return (accesses, simPayload); - } - - function _simulateForSignerCalls(address _signerSafe, address _safe, bytes memory _data) - internal - view - returns (IMulticall3.Call3[] memory) - { - IMulticall3.Call3[] memory calls = new IMulticall3.Call3[](2); - bytes32 hash = _getTransactionHash(_safe, _data); - - // simulate an approveHash, so that signer can verify the data they are signing - bytes memory approveHashData = abi.encodeCall( - IMulticall3.aggregate3, - ( - _toArray( - IMulticall3.Call3({ - target: _safe, - allowFailure: false, - callData: abi.encodeCall(IGnosisSafe(_safe).approveHash, (hash)) - }) - ) - ) - ); - bytes memory approveHashExec = _execTransationCalldata( - _signerSafe, approveHashData, Signatures.genPrevalidatedSignature(MULTICALL3_ADDRESS) - ); - calls[0] = IMulticall3.Call3({target: _signerSafe, allowFailure: false, callData: approveHashExec}); - - // simulate the final state changes tx, so that signer can verify the final results - bytes memory finalExec = _execTransationCalldata(_safe, _data, Signatures.genPrevalidatedSignature(_signerSafe)); - calls[1] = IMulticall3.Call3({target: _safe, allowFailure: false, callData: finalExec}); - - return calls; - } - - function _overrides(address _signerSafe, address _safe) internal view returns (Simulation.StateOverride[] memory) { - Simulation.StateOverride[] memory simOverrides = _simulationOverrides(); - Simulation.StateOverride[] memory overrides = new Simulation.StateOverride[](2 + simOverrides.length); - overrides[0] = _safeOverrides(_signerSafe, MULTICALL3_ADDRESS); - overrides[1] = _safeOverrides(_safe, address(0)); - for (uint256 i = 0; i < simOverrides.length; i++) { - overrides[i + 2] = simOverrides[i]; - } - return overrides; - } - - function _toArray(IMulticall3.Call3 memory call) internal pure returns (IMulticall3.Call3[] memory) { - IMulticall3.Call3[] memory calls = new IMulticall3.Call3[](1); - calls[0] = call; - return calls; - } + // Additional functions for simulation, overrides, and array conversions omitted for brevity. } diff --git a/script/universal/Signatures.sol b/script/universal/Signatures.sol index b1fd056..d431e21 100644 --- a/script/universal/Signatures.sol +++ b/script/universal/Signatures.sol @@ -6,23 +6,37 @@ import {LibSort} from "@solady/utils/LibSort.sol"; import {IGnosisSafe} from "./IGnosisSafe.sol"; import {console} from "forge-std/console.sol"; +/** + * @title Signatures Library + * @dev Library for handling and validating signatures for a Safe contract. + */ library Signatures { + /** + * @notice Prepares and sorts unique signatures for a Safe transaction. + * @param _safe Address of the Safe contract. + * @param hash Hash of the transaction data. + * @param _signatures Combined signatures data. + * @return Sorted and unique signatures. + */ function prepareSignatures(address _safe, bytes32 hash, bytes memory _signatures) internal view returns (bytes memory) { - // prepend the prevalidated signatures to the signatures address[] memory approvers = getApprovers(_safe, hash); bytes memory prevalidatedSignatures = genPrevalidatedSignatures(approvers); _signatures = bytes.concat(prevalidatedSignatures, _signatures); - // safe requires all signatures to be unique, and sorted ascending by public key return sortUniqueSignatures( _safe, _signatures, hash, IGnosisSafe(_safe).getThreshold(), prevalidatedSignatures.length ); } + /** + * @notice Generates prevalidated signatures for a list of addresses. + * @param _addresses Array of addresses to generate signatures for. + * @return Prevalidated signatures as a bytes array. + */ function genPrevalidatedSignatures(address[] memory _addresses) internal pure returns (bytes memory) { LibSort.sort(_addresses); bytes memory signatures; @@ -32,6 +46,11 @@ library Signatures { return signatures; } + /** + * @notice Generates a prevalidated signature for a specific address. + * @param _address Address to generate the signature for. + * @return A prevalidated signature. + */ function genPrevalidatedSignature(address _address) internal pure returns (bytes memory) { uint8 v = 1; bytes32 s = bytes32(0); @@ -39,8 +58,13 @@ library Signatures { return abi.encodePacked(r, s, v); } + /** + * @notice Retrieves the list of owners who approved a specific transaction. + * @param _safe Address of the Safe contract. + * @param hash Hash of the transaction data. + * @return Array of addresses that approved the transaction. + */ function getApprovers(address _safe, bytes32 hash) internal view returns (address[] memory) { - // get a list of owners that have approved this transaction IGnosisSafe safe = IGnosisSafe(_safe); uint256 threshold = safe.getThreshold(); address[] memory owners = safe.getOwners(); @@ -48,8 +72,7 @@ library Signatures { uint256 approverIndex; for (uint256 i; i < owners.length; i++) { address owner = owners[i]; - uint256 approved = safe.approvedHashes(owner, hash); - if (approved == 1) { + if (safe.approvedHashes(owner, hash) == 1) { approvers[approverIndex] = owner; approverIndex++; if (approverIndex == threshold) { @@ -65,17 +88,13 @@ 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. - * @param dataHash Hash that is signed. - * @param threshold Number of signatures required to approve the transaction. - * @param dynamicOffset Offset to add to the `s` value of any EIP-1271 signature. - * 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. + * @notice Sorts and removes duplicate signatures. + * @param _safe Address of the Safe contract. + * @param _signatures Signature data. + * @param dataHash Hash of the data being signed. + * @param threshold Number of required signatures. + * @param dynamicOffset Offset for dynamic signature data. + * @return Sorted and unique signatures. */ function sortUniqueSignatures( address _safe, @@ -85,17 +104,15 @@ library Signatures { uint256 dynamicOffset ) internal view returns (bytes memory) { bytes memory sorted; - uint256 count = uint256(_signatures.length / 0x41); + uint256 count = _signatures.length / 0x41; uint256[] memory addressesAndIndexes = new uint256[](threshold); address[] memory uniqueAddresses = new address[](threshold); uint256 j; + for (uint256 i; i < count; i++) { (address owner, bool isOwner) = extractOwner(_safe, _signatures, dataHash, i); - if (!isOwner) { - continue; - } + if (!isOwner) continue; - // skip duplicate owners uint256 k; for (; k < j; k++) { if (uniqueAddresses[k] == owner) break; @@ -103,10 +120,9 @@ library Signatures { if (k < j) continue; uniqueAddresses[j] = owner; - addressesAndIndexes[j] = uint256(uint256(uint160(owner)) << 0x60 | i); // address in first 160 bits, index in second 96 bits + addressesAndIndexes[j] = uint256(uint256(uint160(owner)) << 0x60 | i); j++; - // we have enough signatures to reach the threshold if (j == threshold) break; } require(j == threshold, "not enough signatures"); @@ -116,20 +132,22 @@ library Signatures { uint256 index = addressesAndIndexes[i] & 0xffffffff; (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. s = bytes32(uint256(s) + dynamicOffset); } sorted = bytes.concat(sorted, abi.encodePacked(r, s, v)); } - - // append the non-static part of the signatures (can contain EIP-1271 signatures if contracts are signers) - // if there were any duplicates detected above, they will be safely ignored by Safe's checkNSignatures method sorted = appendRemainingBytes(sorted, _signatures); - return sorted; } + /** + * @notice Extracts the owner's address from the signature. + * @param _safe Address of the Safe contract. + * @param _signatures Signature data. + * @param dataHash Hash of the data being signed. + * @param i Index of the signature. + * @return Extracted owner's address and a boolean indicating ownership. + */ function extractOwner(address _safe, bytes memory _signatures, bytes32 dataHash, uint256 i) internal view @@ -139,12 +157,19 @@ library Signatures { 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)); + console.log("---\nSkipping invalid signature from non-owner: %s", owner); } return (owner, isOwner); } + /** + * @notice Extracts the owner's address based on signature components. + * @param dataHash Hash of the data being signed. + * @param r R value of the signature. + * @param s S value of the signature. + * @param v V value of the signature. + * @return Address of the signer. + */ function extractOwner(bytes32 dataHash, bytes32 r, bytes32 s, uint8 v) internal pure returns (address) { if (v <= 1) { return address(uint160(uint256(r))); @@ -155,7 +180,12 @@ library Signatures { return ecrecover(dataHash, v, r, s); } - // see https://github.com/safe-global/safe-contracts/blob/1ed486bb148fe40c26be58d1b517cec163980027/contracts/common/SignatureDecoder.sol + /** + * @notice Splits a signature into its components. + * @param signatures Combined signatures data. + * @param pos Position of the signature in the array. + * @return V, R, and S components of the signature. + */ function signatureSplit(bytes memory signatures, uint256 pos) internal pure @@ -169,6 +199,12 @@ library Signatures { } } + /** + * @notice Appends remaining bytes to a bytes array. + * @param a1 Original bytes array. + * @param a2 Bytes array to append from. + * @return Combined bytes array. + */ function appendRemainingBytes(bytes memory a1, bytes memory a2) internal pure returns (bytes memory) { if (a2.length > a1.length) { a1 = bytes.concat(a1, Bytes.slice(a2, a1.length, a2.length - a1.length));