Implement audit notes (#294)

Author: rmeissner

contracts/GnosisSafe.sol

@@ -102,7 +102,7 @@ contract GnosisSafe is
     }
 
     /// @dev Allows to execute a Safe transaction confirmed by required number of owners and then pays the account that submitted the transaction.
-    ///      Note: The fees are always transfered, even if the user transaction fails.
+    ///      Note: The fees are always transferred, even if the user transaction fails.
     /// @param to Destination address of Safe transaction.
     /// @param value Ether value of Safe transaction.
     /// @param data Data payload of Safe transaction.

contracts/GnosisSafeL2.sol

@@ -26,7 +26,7 @@ contract GnosisSafeL2 is GnosisSafe {
     event SafeModuleTransaction(address module, address to, uint256 value, bytes data, Enum.Operation operation);
 
     /// @dev Allows to execute a Safe transaction confirmed by required number of owners and then pays the account that submitted the transaction.
-    ///      Note: The fees are always transfered, even if the user transaction fails.
+    ///      Note: The fees are always transferred, even if the user transaction fails.
     /// @param to Destination address of Safe transaction.
     /// @param value Ether value of Safe transaction.
     /// @param data Data payload of Safe transaction.

contracts/base/FallbackManager.sol

@@ -25,6 +25,7 @@ contract FallbackManager is SelfAuthorized {
         internalSetFallbackHandler(handler);
     }
 
+    // solhint-disable-next-line payable-fallback,no-complex-fallback
     fallback() external {
         bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
         // solhint-disable-next-line no-inline-assembly

contracts/base/OwnerManager.sol

@@ -13,7 +13,7 @@ contract OwnerManager is SelfAuthorized {
     address internal constant SENTINEL_OWNERS = address(0x1);
 
     mapping(address => address) internal owners;
-    uint256 ownerCount;
+    uint256 internal ownerCount;
     uint256 internal threshold;
 
     /// @dev Setup function sets initial storage of contract.

contracts/common/SecuredTokenTransfer.sol

@@ -17,16 +17,15 @@ contract SecuredTokenTransfer {
         bytes memory data = abi.encodeWithSelector(0xa9059cbb, receiver, amount);
         // solhint-disable-next-line no-inline-assembly
         assembly {
-            let success := call(sub(gas(), 10000), token, 0, add(data, 0x20), mload(data), 0, 0)
-            let ptr := mload(0x40)
-            mstore(0x40, add(ptr, returndatasize()))
-            returndatacopy(ptr, 0, returndatasize())
+            // We write the return value to scratch space.
+            // See https://docs.soliditylang.org/en/v0.7.6/internals/layout_in_memory.html#layout-in-memory
+            let success := call(sub(gas(), 10000), token, 0, add(data, 0x20), mload(data), 0, 0x20)
             switch returndatasize()
                 case 0 {
                     transferred := success
                 }
                 case 0x20 {
-                    transferred := iszero(or(iszero(success), iszero(mload(ptr))))
+                    transferred := iszero(or(iszero(success), iszero(mload(0))))
                 }
                 default {
                     transferred := 0

contracts/common/StorageAccessible.sol

@@ -13,6 +13,7 @@ contract StorageAccessible {
     function getStorageAt(uint256 offset, uint256 length) public view returns (bytes memory) {
         bytes memory result = new bytes(length * 32);
         for (uint256 index = 0; index < length; index++) {
+            // solhint-disable-next-line no-inline-assembly
             assembly {
                 let word := sload(add(offset, index))
                 mstore(add(add(result, 0x20), mul(index, 0x20)), word)

contracts/guards/DelegateCallTransactionGuard.sol

@@ -12,6 +12,7 @@ contract DelegateCallTransactionGuard is Guard {
         allowedTarget = target;
     }
 
+    // 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.
@@ -26,6 +27,7 @@ contract DelegateCallTransactionGuard is Guard {
         uint256,
         uint256,
         address,
+        // solhint-disable-next-line no-unused-vars
         address payable,
         bytes memory,
         address

contracts/handler/CompatibilityFallbackHandler.sol

@@ -63,10 +63,13 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
      * @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, // solhint-disable-line no-unused-var
-        bytes calldata calldataPayload // solhint-disable-line no-unused-var
-    ) public returns (bytes memory response) {
+    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.
+        targetContract;
+        calldataPayload;
+
         // solhint-disable-next-line no-inline-assembly
         assembly {
             let internalCalldata := mload(0x40)
@@ -87,7 +90,7 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
             pop(
                 call(
                     gas(),
-                    // address() has been changed to caller() to use the implemtation of the Safe
+                    // address() has been changed to caller() to use the implementation of the Safe
                     caller(),
                     0,
                     internalCalldata,

contracts/handler/HandlerContext.sol

@@ -10,6 +10,7 @@ contract HandlerContext {
     ///         When using this functionality make sure that the linked _manager (aka msg.sender) supports this.
     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
         assembly {
             sender := shr(96, calldataload(sub(calldatasize(), 20)))
         }