Assembly Memory Safety

This PR does a once-over on memory Safety for the 1.5.0 contracts. In
particular, there were a couple of missing `memory-safe` tags for some
assembly blocks which were preventing the IR assembler from working
correctly.

Additionally, since the `MultiSend*` contracts changed anyway in 1.5.0,
I took this opportunity to change the assembly to be memory-safe so that
we can tag it. Note that it only adds more code in the revert case, so
it should not have a negative impact for most use-cases.

Furthermore, I added a comment explaining why we did not make the
`SafeProxy` contract memory-safe (that is one intentional).

Lastly, I noticed that there were some `eq(..., 0)` assembly calls which
can be written as `iszero(...)` to save some gas and code.

contracts/Safe.sol

@@ -179,9 +179,9 @@ contract Safe is
                 /* solhint-disable no-inline-assembly */
                 /// @solidity memory-safe-assembly
                 assembly {
-                    let p := mload(0x40)
-                    returndatacopy(p, 0, returndatasize())
-                    revert(p, returndatasize())
+                    let ptr := mload(0x40)
+                    returndatacopy(ptr, 0, returndatasize())
+                    revert(ptr, returndatasize())
                 }
                 /* solhint-enable no-inline-assembly */
             }
@@ -403,9 +403,10 @@ contract Safe is
     ) public view override returns (bytes32 txHash) {
         bytes32 domainHash = domainSeparator();
 
-        // We opted out for using assembly code here, because the way Solidity compiler we use (0.7.6)
-        // allocates memory is inefficient. We only need to allocate memory for temporary variables to be used in the keccak256 call.
+        // We opted for using assembly code here, because the way Solidity compiler we use (0.7.6) allocates memory is
+        // inefficient. We do not need to allocate memory for temporary variables to be used in the keccak256 call.
         /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             // Get the free memory pointer
             let ptr := mload(0x40)

contracts/base/ModuleManager.sol

@@ -260,10 +260,12 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager {
             revertWithError("GS301");
 
         bytes32 slot = MODULE_GUARD_STORAGE_SLOT;
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             sstore(slot, moduleGuard)
         }
+        /* solhint-enable no-inline-assembly */
         emit ChangedModuleGuard(moduleGuard);
     }
 
@@ -273,10 +275,12 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager {
      */
     function getModuleGuard() internal view returns (address moduleGuard) {
         bytes32 slot = MODULE_GUARD_STORAGE_SLOT;
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             moduleGuard := sload(slot)
         }
+        /* solhint-enable no-inline-assembly */
     }
 
     /**

contracts/handler/extensible/MarshalLib.sol

@@ -33,12 +33,14 @@ library MarshalLib {
      * @return handler The address of the handler contract implementing the `IFallbackMethod` or `IStaticFallbackMethod` interface
      */
     function decode(bytes32 data) internal pure returns (bool isStatic, address handler) {
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             // set isStatic to true if the left-most byte of the data is 0x00
             isStatic := iszero(shr(248, data))
             handler := shr(96, shl(96, data))
         }
+        /* solhint-enable no-inline-assembly */
     }
 
     /**
@@ -49,12 +51,14 @@ library MarshalLib {
      * @return handler The address of the handler contract implementing the `IFallbackMethod` or `IStaticFallbackMethod` interface
      */
     function decodeWithSelector(bytes32 data) internal pure returns (bool isStatic, bytes4 selector, address handler) {
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             // set isStatic to true if the left-most byte of the data is 0x00
             isStatic := iszero(shr(248, data))
             handler := shr(96, shl(96, data))
             selector := shl(168, shr(160, data))
         }
+        /* solhint-enable no-inline-assembly */
     }
 }

contracts/handler/extensible/SignatureVerifierMuxer.sol

@@ -107,10 +107,12 @@ abstract contract SignatureVerifierMuxer is ExtensibleBase, ERC1271, ISignatureV
         // Check if the signature is for an `ISafeSignatureVerifier` and if it is valid for the domain.
         if (signature.length >= 4) {
             bytes4 sigSelector;
-            // solhint-disable-next-line no-inline-assembly
+            /* solhint-disable no-inline-assembly */
+            /// @solidity memory-safe-assembly
             assembly {
                 sigSelector := shl(224, shr(224, calldataload(signature.offset)))
             }
+            /* solhint-enable no-inline-assembly */
 
             // Guard against short signatures that would cause abi.decode to revert.
             if (sigSelector == SAFE_SIGNATURE_MAGIC_VALUE && signature.length >= 68) {

contracts/libraries/MultiSend.sol

@@ -30,6 +30,7 @@ contract MultiSend {
     function multiSend(bytes memory transactions) public payable {
         require(address(this) != MULTISEND_SINGLETON, "MultiSend should only be called via delegatecall");
         /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             let length := mload(transactions)
             let i := 0x20
@@ -61,9 +62,10 @@ contract MultiSend {
                 case 1 {
                     success := delegatecall(gas(), to, data, dataLength, 0, 0)
                 }
-                if eq(success, 0) {
-                    returndatacopy(0, 0, returndatasize())
-                    revert(0, returndatasize())
+                if iszero(success) {
+                    let ptr := mload(0x40)
+                    returndatacopy(ptr, 0, returndatasize())
+                    revert(ptr, returndatasize())
                 }
                 // Next entry starts at 85 byte + data length
                 i := add(i, add(0x55, dataLength))

contracts/libraries/MultiSendCallOnly.sol

@@ -24,6 +24,7 @@ contract MultiSendCallOnly {
      */
     function multiSend(bytes memory transactions) public payable {
         /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             let length := mload(transactions)
             let i := 0x20
@@ -56,9 +57,10 @@ contract MultiSendCallOnly {
                 case 1 {
                     revert(0, 0)
                 }
-                if eq(success, 0) {
-                    returndatacopy(0, 0, returndatasize())
-                    revert(0, returndatasize())
+                if iszero(success) {
+                    let ptr := mload(0x40)
+                    returndatacopy(ptr, 0, returndatasize())
+                    revert(ptr, returndatasize())
                 }
                 // Next entry starts at 85 byte + data length
                 i := add(i, add(0x55, dataLength))

contracts/proxies/SafeProxy.sol

@@ -31,7 +31,11 @@ contract SafeProxy {
 
     /// @dev Fallback function forwards all transactions and returns all received return data.
     fallback() external payable {
-        // solhint-disable-next-line no-inline-assembly
+        // Note that this assembly block is **intentionally** not marked as memory-safe. First of all, it isn't memory
+        // safe to begin with, and turning this into memory-safe assembly would just make it less gas efficient.
+        // Additionally, we noticed that converting this to memory-safe assembly had no affect on optimizations of other
+        // contracts (as it always gets compiles alone in its own compilation unit anyway).
+        /* solhint-disable no-inline-assembly */
         assembly {
             let _singleton := sload(0)
             // 0xa619486e == keccak("masterCopy()"). The value is right padded to 32-bytes with 0s
@@ -42,10 +46,11 @@ contract SafeProxy {
             calldatacopy(0, 0, calldatasize())
             let success := delegatecall(gas(), _singleton, 0, calldatasize(), 0, 0)
             returndatacopy(0, 0, returndatasize())
-            if eq(success, 0) {
+            if iszero(success) {
                 revert(0, returndatasize())
             }
             return(0, returndatasize())
         }
+        /* solhint-enable no-inline-assembly */
     }
 }

contracts/test/DelegateCaller.sol

@@ -14,9 +14,11 @@ contract DelegateCaller {
         (success, returnData) = _called.delegatecall(_calldata);
         if (!success) {
             /* solhint-disable no-inline-assembly */
+            /// @solidity memory-safe-assembly
             assembly {
-                returndatacopy(0, 0, returndatasize())
-                revert(0, returndatasize())
+                let ptr := mload(0x40)
+                returndatacopy(ptr, 0, returndatasize())
+                revert(ptr, returndatasize())
             }
             /* solhint-enable no-inline-assembly */
         }

docs/safe_tx_gas.md

@@ -56,9 +56,9 @@ if (!success && safeTxGas == 0 && gasPrice == 0) {
     /* solhint-disable no-inline-assembly */
     /// @solidity memory-safe-assembly
     assembly {
-        let p := mload(0x40)
-        returndatacopy(p, 0, returndatasize())
-        revert(p, returndatasize())
+        let ptr := mload(0x40)
+        returndatacopy(ptr, 0, returndatasize())
+        revert(ptr, returndatasize())
     }
     /* solhint-enable no-inline-assembly */
 }