Merge ce1b3d1 into e59cf7a

contracts/base/FallbackManager.sol

@@ -37,6 +37,10 @@ contract FallbackManager is SelfAuthorized {
     }
 
     // @notice Forwards all calls to the fallback handler if set. Returns 0 if no handler is set.
+    // @dev Appends the non-padded caller address to the calldata to be optionally used in the handler
+    //      The handler can make us of `HandlerContext.sol` to extract the address.
+    //      This is done because in the next call frame the `msg.sender` will be FallbackManager's address
+    //      and having the original caller address may enable additional verification scenarios.
     // solhint-disable-next-line payable-fallback,no-complex-fallback
     fallback() external {
         bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;

contracts/base/OwnerManager.sol

@@ -75,7 +75,11 @@ contract OwnerManager is SelfAuthorized {
      * @param owner Owner address to be removed.
      * @param _threshold New threshold.
      */
-    function removeOwner(address prevOwner, address owner, uint256 _threshold) public authorized {
+    function removeOwner(
+        address prevOwner,
+        address owner,
+        uint256 _threshold
+    ) public authorized {
         // Only allow to remove an owner, if threshold can still be reached.
         require(ownerCount - 1 >= _threshold, "GS201");
         // Validate owner address and check that it corresponds to owner index.
@@ -96,7 +100,11 @@ contract OwnerManager is SelfAuthorized {
      * @param oldOwner Owner address to be replaced.
      * @param newOwner New owner address.
      */
-    function swapOwner(address prevOwner, address oldOwner, address newOwner) public authorized {
+    function swapOwner(
+        address prevOwner,
+        address oldOwner,
+        address newOwner
+    ) public authorized {
         // Owner address cannot be null, the sentinel or the Safe itself.
         require(newOwner != address(0) && newOwner != SENTINEL_OWNERS && newOwner != address(this), "GS203");
         // No duplicate owners allowed.

contracts/handler/CompatibilityFallbackHandler.sol

@@ -5,12 +5,12 @@ import "./DefaultCallbackHandler.sol";
 import "../interfaces/ISignatureValidator.sol";
 import "../Safe.sol";
 
-/// @title Compatibility Fallback Handler - fallback handler to provider compatibility between pre 1.3.0 and 1.3.0+ Safe contracts
-/// @author Richard Meissner - <richard@gnosis.pm>
+/**
+ * @title Compatibility Fallback Handler - Provides compatibility between pre 1.3.0 and 1.3.0+ Safe contracts.
+ * @author Richard Meissner - @rmeissner
+ */
 contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValidator {
-    //keccak256(
-    //    "SafeMessage(bytes message)"
-    //);
+    // keccak256("SafeMessage(bytes message)");
     bytes32 private constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca;
 
     bytes4 internal constant SIMULATE_SELECTOR = bytes4(keccak256("simulate(address,bytes)"));
@@ -19,11 +19,11 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
     bytes4 internal constant UPDATED_MAGIC_VALUE = 0x1626ba7e;
 
     /**
-     * Implementation of ISignatureValidator (see `interfaces/ISignatureValidator.sol`)
-     * @dev Should return whether the signature provided is valid for the provided data.
-     * @param _data Arbitrary length data signed on the behalf of address(msg.sender)
-     * @param _signature Signature byte array associated with _data
-     * @return a bool upon valid or invalid signature with corresponding _data
+     * @notice Legacy EIP-1271 signature validation method.
+     * @dev Implementation of ISignatureValidator (see `interfaces/ISignatureValidator.sol`)
+     * @param _data Arbitrary length data signed on the behalf of address(msg.sender).
+     * @param _signature Signature byte array associated with _data.
+     * @return a bool upon valid or invalid signature with corresponding _data.
      */
     function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
         // Caller should be a Safe
@@ -38,48 +38,55 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
         return EIP1271_MAGIC_VALUE;
     }
 
-    /// @dev Returns hash of a message that can be signed by owners.
-    /// @param message Message that should be hashed
-    /// @return Message hash.
+    /**
+     * @dev Returns the hash of a message to be signed by owners.
+     * @param message Raw message bytes.
+     * @return Message hash.
+     */
     function getMessageHash(bytes memory message) public view returns (bytes32) {
         return getMessageHashForSafe(Safe(payable(msg.sender)), message);
     }
 
-    /// @dev Returns the pre-image of the message hash (see getMessageHashForSafe)
-    /// @param safe Safe to which the message is targeted
-    /// @param message Message that should be encoded
-    /// @return Encoded message.
+    /**
+     * @dev Returns the pre-image of the message hash (see getMessageHashForSafe).
+     * @param safe Safe to which the message is targeted.
+     * @param message Message that should be encoded.
+     * @return Encoded message.
+     */
     function encodeMessageDataForSafe(Safe safe, bytes memory message) public view returns (bytes memory) {
         bytes32 safeMessageHash = keccak256(abi.encode(SAFE_MSG_TYPEHASH, keccak256(message)));
         return abi.encodePacked(bytes1(0x19), bytes1(0x01), safe.domainSeparator(), safeMessageHash);
     }
 
-    /// @dev Returns hash of a message that can be signed by owners.
-    /// @param safe Safe to which the message is targeted
-    /// @param message Message that should be hashed
-    /// @return Message hash.
+    /**
+     * @dev Returns hash of a message that can be signed by owners.
+     * @param safe Safe to which the message is targeted.
+     * @param message Message that should be hashed.
+     * @return Message hash.
+     */
     function getMessageHashForSafe(Safe safe, bytes memory message) public view returns (bytes32) {
         return keccak256(encodeMessageDataForSafe(safe, message));
     }
 
     /**
-     * Implementation of updated EIP-1271
+     * @notice Implementation of updated EIP-1271 signature validation method.
      * @dev Should return whether the signature provided is valid for the provided data.
-     *       The save does not implement the interface since `checkSignatures` is not a view method.
-     *       The method will not perform any state changes (see parameters of `checkSignatures`)
+     *      The Safe does not implement the interface since `checkSignatures` is not a view method.
+     *      The method will not perform any state changes (see parameters of `checkSignatures`)
      * @param _dataHash Hash of the data signed on the behalf of address(msg.sender)
      * @param _signature Signature byte array associated with _dataHash
-     * @return a bool upon valid or invalid signature with corresponding _dataHash
-     * @notice See https://github.com/gnosis/util-contracts/blob/bb5fe5fb5df6d8400998094fb1b32a178a47c3a1/contracts/StorageAccessible.sol
+     * @return Updated EIP1271 magic value if signature is valid, otherwise 0x0
      */
     function isValidSignature(bytes32 _dataHash, bytes calldata _signature) external view returns (bytes4) {
         ISignatureValidator validator = ISignatureValidator(msg.sender);
         bytes4 value = validator.isValidSignature(abi.encode(_dataHash), _signature);
         return (value == EIP1271_MAGIC_VALUE) ? UPDATED_MAGIC_VALUE : bytes4(0);
     }
 
-    /// @dev Returns array of first 10 modules.
-    /// @return Array of modules.
+    /**
+     * @dev Returns array of first 10 modules.
+     * @return Array of modules.
+     */
     function getModules() external view returns (address[] memory) {
         // Caller should be a Safe
         Safe safe = Safe(payable(msg.sender));
@@ -90,33 +97,42 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
     /**
      * @dev Performs a delegatecall on a targetContract in the context of self.
      * Internally reverts execution to avoid side effects (making it static). Catches revert and returns encoded result as bytes.
+     * @dev Inspired by https://github.com/gnosis/util-contracts/blob/bb5fe5fb5df6d8400998094fb1b32a178a47c3a1/contracts/StorageAccessible.sol
      * @param targetContract Address of the contract containing the code to execute.
      * @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments).
      */
     function simulate(address targetContract, bytes calldata calldataPayload) external returns (bytes memory response) {
-        // Suppress compiler warnings about not using parameters, while allowing
-        // parameters to keep names for documentation purposes. This does not
-        // generate code.
+        /**
+         * Suppress compiler warnings about not using parameters, while allowing
+         * parameters to keep names for documentation purposes. This does not
+         * generate code.
+         */
         targetContract;
         calldataPayload;
 
         // solhint-disable-next-line no-inline-assembly
         assembly {
             let internalCalldata := mload(0x40)
-            // Store `simulateAndRevert.selector`.
-            // String representation is used to force right padding
+            /**
+             * Store `simulateAndRevert.selector`.
+             * String representation is used to force right padding
+             */
             mstore(internalCalldata, "\xb4\xfa\xba\x09")
-            // Abuse the fact that both this and the internal methods have the
-            // same signature, and differ only in symbol name (and therefore,
-            // selector) and copy calldata directly. This saves us approximately
-            // 250 bytes of code and 300 gas at runtime over the
-            // `abi.encodeWithSelector` builtin.
+            /**
+             * Abuse the fact that both this and the internal methods have the
+             * same signature, and differ only in symbol name (and therefore,
+             * selector) and copy calldata directly. This saves us approximately
+             * 250 bytes of code and 300 gas at runtime over the
+             * `abi.encodeWithSelector` builtin.
+             */
             calldatacopy(add(internalCalldata, 0x04), 0x04, sub(calldatasize(), 0x04))
 
-            // `pop` is required here by the compiler, as top level expressions
-            // can't have return values in inline assembly. `call` typically
-            // returns a 0 or 1 value indicated whether or not it reverted, but
-            // since we know it will always revert, we can safely ignore it.
+            /**
+             * `pop` is required here by the compiler, as top level expressions
+             * can't have return values in inline assembly. `call` typically
+             * returns a 0 or 1 value indicated whether or not it reverted, but
+             * since we know it will always revert, we can safely ignore it.
+             */
             pop(
                 call(
                     gas(),
@@ -125,21 +141,25 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
                     0,
                     internalCalldata,
                     calldatasize(),
-                    // The `simulateAndRevert` call always reverts, and
-                    // instead encodes whether or not it was successful in the return
-                    // data. The first 32-byte word of the return data contains the
-                    // `success` value, so write it to memory address 0x00 (which is
-                    // reserved Solidity scratch space and OK to use).
+                    /**
+                     * The `simulateAndRevert` call always reverts, and
+                     * instead encodes whether or not it was successful in the return
+                     * data. The first 32-byte word of the return data contains the
+                     * `success` value, so write it to memory address 0x00 (which is
+                     * reserved Solidity scratch space and OK to use).
+                     */
                     0x00,
                     0x20
                 )
             )
 
-            // Allocate and copy the response bytes, making sure to increment
-            // the free memory pointer accordingly (in case this method is
-            // called as an internal function). The remaining `returndata[0x20:]`
-            // contains the ABI encoded response bytes, so we can just write it
-            // as is to memory.
+            /**
+             * Allocate and copy the response bytes, making sure to increment
+             * the free memory pointer accordingly (in case this method is
+             * called as an internal function). The remaining `returndata[0x20:]`
+             * contains the ABI encoded response bytes, so we can just write it
+             * as is to memory.
+             */
             let responseSize := sub(returndatasize(), 0x20)
             response := mload(0x40)
             mstore(0x40, add(response, responseSize))

contracts/handler/DefaultCallbackHandler.sol

@@ -6,16 +6,32 @@ import "../interfaces/ERC721TokenReceiver.sol";
 import "../interfaces/ERC777TokensRecipient.sol";
 import "../interfaces/IERC165.sol";
 
-/// @title Default Callback Handler - returns true for known token callbacks
-/// @author Richard Meissner - <richard@gnosis.pm>
+/**
+ * @title Default Callback Handler - Handles supported tokens' callbacks, allowing Safes receiving these tokens.
+ * @author Richard Meissner - @rmeissner
+ */
 contract DefaultCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient, ERC721TokenReceiver, IERC165 {
     string public constant NAME = "Default Callback Handler";
     string public constant VERSION = "1.0.0";
 
-    function onERC1155Received(address, address, uint256, uint256, bytes calldata) external pure override returns (bytes4) {
+    /**
+     * @notice Handles ERC1155 Token callback.
+     * return Standardized onERC1155Received return value.
+     */
+    function onERC1155Received(
+        address,
+        address,
+        uint256,
+        uint256,
+        bytes calldata
+    ) external pure override returns (bytes4) {
         return 0xf23a6e61;
     }
 
+    /**
+     * @notice Handles ERC1155 Token batch callback.
+     * return Standardized onERC1155BatchReceived return value.
+     */
     function onERC1155BatchReceived(
         address,
         address,
@@ -26,14 +42,39 @@ contract DefaultCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient,
         return 0xbc197c81;
     }
 
-    function onERC721Received(address, address, uint256, bytes calldata) external pure override returns (bytes4) {
+    /**
+     * @notice Handles ERC721 Token callback.
+     *  return Standardized onERC721Received return value.
+     */
+    function onERC721Received(
+        address,
+        address,
+        uint256,
+        bytes calldata
+    ) external pure override returns (bytes4) {
         return 0x150b7a02;
     }
 
-    function tokensReceived(address, address, address, uint256, bytes calldata, bytes calldata) external pure override {
+    /**
+     * @notice Handles ERC777 Token callback.
+     * return nothing (not standardized)
+     */
+    function tokensReceived(
+        address,
+        address,
+        address,
+        uint256,
+        bytes calldata,
+        bytes calldata
+    ) external pure override {
         // We implement this for completeness, doesn't really have any value
     }
 
+    /**
+     * @notice Implements ERC165 interface support for ERC1155TokenReceiver, ERC721TokenReceiver and IERC165.
+     * @param interfaceId Id of the interface.
+     * @return if the interface is supported.
+     */
     function supportsInterface(bytes4 interfaceId) external view virtual override returns (bool) {
         return
             interfaceId == type(ERC1155TokenReceiver).interfaceId ||

contracts/handler/HandlerContext.sol

@@ -1,13 +1,22 @@
 // SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity >=0.7.0 <0.9.0;
 
-/// @title Handler Context - allows to extract calling context
-/// @author Richard Meissner - <richard@gnosis.pm>
-/// @notice based on https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f8cc8b844a9f92f63dc55aa581f7d643a1bc5ac1/contracts/metatx/ERC2771Context.sol
+/**
+ * @title Handler Context - Allows the fallback handler to extract addition context from the calldata
+ * @dev The fallback manager appends the following context to the calldata:
+ *      1. Fallback manager caller address (non-padded)
+ * based on https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f8cc8b844a9f92f63dc55aa581f7d643a1bc5ac1/contracts/metatx/ERC2771Context.sol
+ * @author Richard Meissner - @rmeissner
+ */
 contract HandlerContext {
-    // This function does not rely on a trusted forwarder. Use the returned value only to check information against the calling manager.
-    /// @notice This is only reliable in combination with a FallbackManager that supports this (e.g. Safe contract >=1.3.0).
-    ///         When using this functionality make sure that the linked _manager (aka msg.sender) supports this.
+    /**
+     * @notice Allows fetching the original caller address.
+     * @dev This is only reliable in combination with a FallbackManager that supports this (e.g. Safe contract >=1.3.0).
+     *      When using this functionality make sure that the linked _manager (aka msg.sender) supports this.
+     *      This function does not rely on a trusted forwarder. Use the returned value only to
+     *      check information against the calling manager.
+     * @return sender Original caller address.
+     */
     function _msgSender() internal pure returns (address sender) {
         // The assembly code is more direct than the Solidity version using `abi.decode`.
         // solhint-disable-next-line no-inline-assembly
@@ -16,7 +25,10 @@ contract HandlerContext {
         }
     }
 
-    // Function do differentiate more clearly between msg.sender and the calling manager
+    /**
+     * @notice Returns the FallbackManager address
+     * @return Fallback manager address
+     */
     function _manager() internal view returns (address) {
         return msg.sender;
     }