Merge 96e90c0 into 6b3784f

contracts/examples/guards/DebugTransactionGuard.sol

@@ -5,9 +5,11 @@ import "../../common/Enum.sol";
 import "../../base/GuardManager.sol";
 import "../../Safe.sol";
 
-/// @title Debug Transaction Guard - A guard that will emit events with extended information.
-/// @notice This guard is only meant as a development tool and example
-/// @author Richard Meissner - <richard@gnosis.pm>
+/**
+ * @title Debug Transaction Guard - A guard that will emit events with extended information.
+ * @dev This guard is only meant as a development tool and example
+ * @author Richard Meissner - @rmeissner
+ */
 contract DebugTransactionGuard is BaseGuard {
     // solhint-disable-next-line payable-fallback
     fallback() external {
@@ -24,13 +26,29 @@ contract DebugTransactionGuard is BaseGuard {
         Enum.Operation operation,
         uint256 safeTxGas,
         bool usesRefund,
-        uint256 nonce
+        uint256 nonce,
+        bytes signatures,
+        address executor
     );
 
     event GasUsage(address indexed safe, bytes32 indexed txHash, uint256 indexed nonce, bool success);
 
     mapping(bytes32 => uint256) public txNonces;
 
+    /**
+     * @notice Called by the Safe contract before a transaction is executed.
+     * @param to Destination address of Safe transaction.
+     * @param value Ether value of Safe transaction.
+     * @param data Data payload of Safe transaction.
+     * @param operation Operation type of Safe transaction.
+     * @param safeTxGas Gas that should be used for the Safe transaction.
+     * @param baseGas Gas costs that are independent of the transaction execution(e.g. base transaction fee, signature check, payment of the refund)
+     * @param gasPrice Gas price that should be used for the payment calculation.
+     * @param gasToken Token address (or 0 if ETH) that is used for the payment.
+     * @param refundReceiver Address of receiver of gas payment (or 0 if tx.origin).
+     * @param signatures Packed signature data ({bytes32 r}{bytes32 s}{uint8 v})
+     * @param executor Account executing the transaction.
+     */
     function checkTransaction(
         address to,
         uint256 value,
@@ -42,8 +60,8 @@ contract DebugTransactionGuard is BaseGuard {
         address gasToken,
         // solhint-disable-next-line no-unused-vars
         address payable refundReceiver,
-        bytes memory,
-        address
+        bytes memory signatures,
+        address executor
     ) external override {
         uint256 nonce;
         bytes32 txHash;
@@ -52,10 +70,15 @@ contract DebugTransactionGuard is BaseGuard {
             nonce = safe.nonce() - 1;
             txHash = safe.getTransactionHash(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, nonce);
         }
-        emit TransactionDetails(msg.sender, txHash, to, value, data, operation, safeTxGas, gasPrice > 0, nonce);
+        emit TransactionDetails(msg.sender, txHash, to, value, data, operation, safeTxGas, gasPrice > 0, nonce, signatures, executor);
         txNonces[txHash] = nonce;
     }
 
+    /**
+     * @notice Called by the Safe contract after a transaction is executed.
+     * @param txHash Hash of the executed transaction.
+     * @param success True if the transaction was successful.
+     */
     function checkAfterExecution(bytes32 txHash, bool success) external override {
         uint256 nonce = txNonces[txHash];
         require(nonce != 0, "Could not get nonce");

contracts/examples/guards/DelegateCallTransactionGuard.sol

@@ -5,6 +5,10 @@ import "../../common/Enum.sol";
 import "../../base/GuardManager.sol";
 import "../../Safe.sol";
 
+/**
+ * @title DelegateCallTransactionGuard - A transaction guard that limits delegate calls to a specific target.
+ * @author Richard Meissner - @rmeissner
+ */
 contract DelegateCallTransactionGuard is BaseGuard {
     address public immutable allowedTarget;
 
@@ -18,6 +22,12 @@ contract DelegateCallTransactionGuard is BaseGuard {
         // E.g. The expected check method might change and then the Safe would be locked.
     }
 
+    /**
+     * @notice Called by the Safe contract before a transaction is executed.
+     * @dev  Reverts if the transaction is a delegate call to contract other than the allowed one.
+     * @param to Destination address of Safe transaction.
+     * @param operation Operation type of Safe transaction.
+     */
     function checkTransaction(
         address to,
         uint256,

contracts/examples/guards/OnlyOwnersGuard.sol

@@ -9,11 +9,26 @@ interface ISafe {
     function getOwners() external view returns (address[] memory);
 }
 
+/**
+ * @title OnlyOwnersGuard - A guard that only allows owners to execute transactions.
+ * @author Richard Meissner - @rmeissner
+ */
 contract OnlyOwnersGuard is BaseGuard {
     ISafe public safe;
 
     constructor() {}
 
+    // solhint-disable-next-line payable-fallback
+    fallback() external {
+        // We don't revert on fallback to avoid issues in case of a Safe upgrade
+        // E.g. The expected check method might change and then the Safe would be locked.
+    }
+
+    /**
+     * @notice Called by the Safe contract before a transaction is executed.
+     * @dev Reverts if the transaction is not executed by an owner.
+     * @param msgSender Executor of the transaction.
+     */
     function checkTransaction(
         address,
         uint256,
@@ -27,7 +42,7 @@ contract OnlyOwnersGuard is BaseGuard {
         address payable,
         bytes memory,
         address msgSender
-    ) external override {
+    ) external view override {
         // Only owners can exec
         address[] memory owners = ISafe(msg.sender).getOwners();
         for (uint256 i = 0; i < owners.length; i++) {

contracts/examples/guards/ReentrancyTransactionGuard.sol

@@ -5,6 +5,10 @@ import "../../common/Enum.sol";
 import "../../base/GuardManager.sol";
 import "../../Safe.sol";
 
+/**
+ * @title ReentrancyTransactionGuard - A transaction guard that prevents reentrancy into the transaction execution function.
+ * @author Richard Meissner - @rmeissner
+ */
 contract ReentrancyTransactionGuard is BaseGuard {
     bytes32 internal constant GUARD_STORAGE_SLOT = keccak256("reentrancy_guard.guard.struct");
 
@@ -18,6 +22,11 @@ contract ReentrancyTransactionGuard is BaseGuard {
         // E.g. The expected check method might change and then the Safe would be locked.
     }
 
+    /**
+     * @notice Returns the guard value for the current context.
+     * @dev The guard value is stored in a slot that is unique to the contract instance and the function in which it is called.
+     * @return guard The guard value.
+     */
     function getGuard() internal pure returns (GuardValue storage guard) {
         bytes32 slot = GUARD_STORAGE_SLOT;
         // solhint-disable-next-line no-inline-assembly
@@ -26,6 +35,10 @@ contract ReentrancyTransactionGuard is BaseGuard {
         }
     }
 
+    /**
+     * @notice Called by the Safe contract before a transaction is executed.
+     * @dev Reverts if reentrancy is detected.
+     */
     function checkTransaction(
         address,
         uint256,
@@ -45,6 +58,10 @@ contract ReentrancyTransactionGuard is BaseGuard {
         guard.active = true;
     }
 
+    /**
+     * @notice Called by the Safe contract after a transaction is executed.
+     * @dev Resets the guard value.
+     */
     function checkAfterExecution(bytes32, bool) external override {
         getGuard().active = false;
     }

contracts/examples/libraries/Migrate_1_3_0_to_1_2_0.sol

@@ -2,28 +2,32 @@
 pragma solidity >=0.7.0 <0.9.0;
 import "../../libraries/SafeStorage.sol";
 
-/// @title Migration - migrates a Safe contract from 1.3.0 to 1.2.0
-/// @author Richard Meissner - <richard@gnosis.io>
+/**
+ * @title Migration - migrates a Safe contract from 1.3.0 to 1.2.0
+ * @author Richard Meissner - @rmeissner
+ */
 contract Migration is SafeStorage {
     bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = 0x035aff83d86937d35b32e04f0ddc6ff469290eef2f1b692d8a815c89404d4749;
 
     address public immutable migrationSingleton;
     address public immutable safe120Singleton;
 
     constructor(address targetSingleton) {
+        // Singleton address cannot be null.
         require(targetSingleton != address(0), "Invalid singleton address provided");
         safe120Singleton = targetSingleton;
         migrationSingleton = address(this);
     }
 
     event ChangedMasterCopy(address singleton);
 
-    bytes32 private guard;
-
-    /// @dev Allows to migrate the contract. This can only be called via a delegatecall.
+    /**
+     * @notice Migrates the Safe to the Singleton contract at `migrationSingleton`.
+     * @dev This can only be called via a delegatecall.
+     */
     function migrate() public {
         require(address(this) != migrationSingleton, "Migration should only be called via delegatecall");
-        // Master copy address cannot be null.
+
         singleton = safe120Singleton;
         _deprecatedDomainSeparator = keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, this));
         emit ChangedMasterCopy(singleton);

contracts/external/SafeMath.sol

@@ -3,12 +3,14 @@ pragma solidity >=0.7.0 <0.9.0;
 
 /**
  * @title SafeMath
- * @dev Math operations with safety checks that revert on error
- * TODO: remove once open zeppelin update to solc 0.5.0
+ * @notice Math operations with safety checks that revert on error (overflow/underflow)
  */
 library SafeMath {
     /**
-     * @dev Multiplies two numbers, reverts on overflow.
+     * @notice Multiplies two numbers, reverts on overflow.
+     * @param a First number
+     * @param b Second number
+     * @return Product of a and b
      */
     function mul(uint256 a, uint256 b) internal pure returns (uint256) {
         // Gas optimization: this is cheaper than requiring 'a' not being zero, but the
@@ -25,7 +27,10 @@ library SafeMath {
     }
 
     /**
-     * @dev Subtracts two numbers, reverts on overflow (i.e. if subtrahend is greater than minuend).
+     * @notice Subtracts two numbers, reverts on overflow (i.e. if subtrahend is greater than minuend).
+     * @param a First number
+     * @param b Second number
+     * @return Difference of a and b
      */
     function sub(uint256 a, uint256 b) internal pure returns (uint256) {
         require(b <= a);
@@ -35,7 +40,10 @@ library SafeMath {
     }
 
     /**
-     * @dev Adds two numbers, reverts on overflow.
+     * @notice Adds two numbers, reverts on overflow.
+     * @param a First number
+     * @param b Second number
+     * @return Sum of a and b
      */
     function add(uint256 a, uint256 b) internal pure returns (uint256) {
         uint256 c = a + b;
@@ -45,7 +53,10 @@ library SafeMath {
     }
 
     /**
-     * @dev Returns the largest of two numbers.
+     * @notice Returns the largest of two numbers.
+     * @param a First number
+     * @param b Second number
+     * @return Largest of a and b
      */
     function max(uint256 a, uint256 b) internal pure returns (uint256) {
         return a >= b ? a : b;

test/guards/DebugTransactionGuard.spec.ts

@@ -1,13 +1,9 @@
+import { signHash } from "./../../src/utils/execution";
 import { expect } from "chai";
 import hre, { deployments, waffle } from "hardhat";
 import "@nomiclabs/hardhat-ethers";
 import { getMock, getSafeWithOwners } from "../utils/setup";
-import {
-    buildSafeTransaction,
-    calculateSafeTransactionHash,
-    executeContractCallWithSigners,
-    executeTxWithSigners,
-} from "../../src/utils/execution";
+import { buildSafeTransaction, calculateSafeTransactionHash, executeContractCallWithSigners, executeTx } from "../../src/utils/execution";
 import { chainId } from "../utils/encoding";
 
 describe("DebugTransactionGuard", async () => {
@@ -54,8 +50,9 @@ describe("DebugTransactionGuard", async () => {
             const nonce = await safe.nonce();
             const safeTx = buildSafeTransaction({ to: mock.address, data: "0xbaddad42", nonce });
             const safeTxHash = calculateSafeTransactionHash(safe, safeTx, await chainId());
+            const signature = await signHash(user1, safeTxHash);
 
-            await expect(executeTxWithSigners(safe, safeTx, [user1]))
+            await expect(executeTx(safe, safeTx, [signature]))
                 .to.emit(guard, "TransactionDetails")
                 .withArgs(
                     safe.address,
@@ -67,6 +64,8 @@ describe("DebugTransactionGuard", async () => {
                     safeTx.safeTxGas,
                     false,
                     safeTx.nonce,
+                    signature.data,
+                    user1.address,
                 )
                 .and.to.emit(guard, "GasUsage")
                 .withArgs(safe.address, safeTxHash, nonce, true);