Merge pull request #711 from safe-global/optimizer-enabled

Making contracts `memory-safe`

.env.sample

@@ -10,4 +10,8 @@ ERC4337_TEST_NODE_URL=
 ERC4337_TEST_SINGLETON_ADDRESS=
 ERC4337_TEST_SAFE_FACTORY_ADDRESS=
 # (Optional) Tells the test runner which Safe Singleton Contract to use for testing: Safe or SafeL2. Defaults to Safe.
-SAFE_CONTRACT_UNDER_TEST="Safe"
+SAFE_CONTRACT_UNDER_TEST="Safe"
+# Used for compiling with different solidity testing
+SOLIDITY_VERSION= # Example: '0.8.19'
+# For running coverage tests, `details` section of solidity settings are required, else could be removed.
+SOLIDITY_SETTINGS= # Example: '{"viaIR":true,"optimizer":{"enabled":true, "details": {"yul": true, "yulDetails": { "optimizerSteps": ""}}}}'

.gitignore

@@ -13,6 +13,7 @@ coverage/
 coverage.json
 yarn-error.log
 typechain-types
+.vscode
 
 # Certora Formal Verification related files
 .certora_internal

certora/specs/Safe.spec

@@ -65,7 +65,7 @@ hook Sstore SafeHarness.(slot 24440054405305269366569402256811496959409073762505
     ghostSingletonAddress = newSingletonAddress;
 }
 
-invariant sigletonAddressNeverChanges()
+invariant singletonAddressNeverChanges()
     ghostSingletonAddress == 0
     filtered { f -> reachableOnly(f) && f.selector != sig:getStorageAt(uint256,uint256).selector }
 

contracts/base/FallbackManager.sol

@@ -69,33 +69,26 @@ abstract contract FallbackManager is SelfAuthorized {
             // memory is used, therefore we need to guarantee memory safety (keeping the free memory point 0x40 slot intact,
             // not going beyond the scratch space, etc)
             // Solidity docs: https://docs.soliditylang.org/en/latest/assembly.html#memory-safety
-            function allocate(length) -> pos {
-                pos := mload(0x40)
-                mstore(0x40, add(pos, length))
-            }
-
             let handler := sload(slot)
             if iszero(handler) {
                 return(0, 0)
             }
 
-            let calldataPtr := allocate(calldatasize())
-            calldatacopy(calldataPtr, 0, calldatasize())
+            let ptr := mload(0x40)
+            calldatacopy(ptr, 0, calldatasize())
 
             // The msg.sender address is shifted to the left by 12 bytes to remove the padding
             // Then the address without padding is stored right after the calldata
-            let senderPtr := allocate(20)
-            mstore(senderPtr, shl(96, caller()))
+            mstore(add(ptr, calldatasize()), shl(96, caller()))
 
             // Add 20 bytes for the address appended add the end
-            let success := call(gas(), handler, 0, calldataPtr, add(calldatasize(), 20), 0, 0)
+            let success := call(gas(), handler, 0, ptr, add(calldatasize(), 20), 0, 0)
 
-            let returnDataPtr := allocate(returndatasize())
-            returndatacopy(returnDataPtr, 0, returndatasize())
+            returndatacopy(ptr, 0, returndatasize())
             if iszero(success) {
-                revert(returnDataPtr, returndatasize())
+                revert(ptr, returndatasize())
             }
-            return(returnDataPtr, returndatasize())
+            return(ptr, returndatasize())
         }
         /* solhint-enable no-inline-assembly */
     }

contracts/base/ModuleManager.sol

@@ -123,16 +123,14 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager {
         /// @solidity memory-safe-assembly
         assembly {
             // Load free memory location
-            let ptr := mload(0x40)
+            returnData := mload(0x40)
             // We allocate memory for the return data by setting the free memory location to
             // current free memory location + data size + 32 bytes for data size value
-            mstore(0x40, add(ptr, add(returndatasize(), 0x20)))
+            mstore(0x40, add(returnData, add(returndatasize(), 0x20)))
             // Store the size
-            mstore(ptr, returndatasize())
+            mstore(returnData, returndatasize())
             // Store the data
-            returndatacopy(add(ptr, 0x20), 0, returndatasize())
-            // Point the return data to the correct memory location
-            returnData := ptr
+            returndatacopy(add(returnData, 0x20), 0, returndatasize())
         }
         /* solhint-enable no-inline-assembly */
     }

contracts/common/SecuredTokenTransfer.sol

@@ -18,7 +18,8 @@ abstract contract SecuredTokenTransfer {
     function transferToken(address token, address receiver, uint256 amount) internal returns (bool transferred) {
         // 0xa9059cbb - keccack("transfer(address,uint256)")
         bytes memory data = abi.encodeWithSelector(0xa9059cbb, receiver, amount);
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             // 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
@@ -34,5 +35,6 @@ abstract contract SecuredTokenTransfer {
                 transferred := 0
             }
         }
+        /* solhint-enable no-inline-assembly */
     }
 }

contracts/handler/CompatibilityFallbackHandler.sol

@@ -97,20 +97,21 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
         /* solhint-disable no-inline-assembly */
         /// @solidity memory-safe-assembly
         assembly {
-            let internalCalldata := mload(0x40)
+            let ptr := mload(0x40)
             /**
              * Store `simulateAndRevert.selector`.
              * String representation is used to force right padding
              */
-            mstore(internalCalldata, "\xb4\xfa\xba\x09")
+            mstore(ptr, "\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.
              */
-            calldatacopy(add(internalCalldata, 0x04), 0x04, sub(calldatasize(), 0x04))
+            calldatacopy(add(ptr, 0x04), 0x04, sub(calldatasize(), 0x04))
 
             /**
              * `pop` is required here by the compiler, as top level expressions
@@ -124,7 +125,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
                     // address() has been changed to caller() to use the implementation of the Safe
                     caller(),
                     0,
-                    internalCalldata,
+                    ptr,
                     calldatasize(),
                     /**
                      * The `simulateAndRevert` call always reverts, and

contracts/interfaces/ViewStorageAccessible.sol

@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity >=0.5.0 <0.9.0;
 
 /// @title ViewStorageAccessible - Interface on top of StorageAccessible base class to allow simulations from view functions.

contracts/libraries/MultiSend.sol

@@ -30,7 +30,6 @@ 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

contracts/libraries/MultiSendCallOnly.sol

@@ -24,7 +24,6 @@ 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

contracts/libraries/Safe130To141Migration.sol

@@ -125,10 +125,12 @@ contract Safe130To141Migration is SafeStorage {
      */
     function isContract(address account) internal view returns (bool) {
         uint256 size;
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             size := extcodesize(account)
         }
+        /* solhint-enable no-inline-assembly */
 
         // If the code size is greater than 0, it is a contract; otherwise, it is an EOA.
         return size > 0;

contracts/libraries/Safe150Migration.sol

@@ -160,10 +160,12 @@ contract Safe150Migration is SafeStorage {
      */
     function getGuard() internal view returns (address guard) {
         bytes32 slot = GUARD_STORAGE_SLOT;
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             guard := sload(slot)
         }
+        /* solhint-enable no-inline-assembly */
     }
 
     /**
@@ -176,10 +178,12 @@ contract Safe150Migration is SafeStorage {
      */
     function isContract(address account) internal view returns (bool) {
         uint256 size;
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             size := extcodesize(account)
         }
+        /* solhint-enable no-inline-assembly */
 
         // If the code size is greater than 0, it is a contract; otherwise, it is an EOA.
         return size > 0;

contracts/libraries/SafeToL2Migration.sol

@@ -96,7 +96,7 @@ contract SafeToL2Migration is SafeStorage {
             0,
             0,
             address(0),
-            address(0),
+            payable(address(0)),
             "", // We cannot detect signatures
             additionalInfo
         );
@@ -166,10 +166,12 @@ contract SafeToL2Migration is SafeStorage {
      */
     function isContract(address account) internal view returns (bool) {
         uint256 size;
-        // solhint-disable-next-line no-inline-assembly
+        /* solhint-disable no-inline-assembly */
+        /// @solidity memory-safe-assembly
         assembly {
             size := extcodesize(account)
         }
+        /* solhint-enable no-inline-assembly */
 
         // If the code size is greater than 0, it is a contract; otherwise, it is an EOA.
         return size > 0;

contracts/test/DelegateCaller.sol

@@ -13,12 +13,13 @@ contract DelegateCaller {
     function makeDelegatecall(address _called, bytes memory _calldata) external returns (bool success, bytes memory returnData) {
         (success, returnData) = _called.delegatecall(_calldata);
         if (!success) {
-            // solhint-disable-next-line no-inline-assembly
+            /* solhint-disable no-inline-assembly */
             assembly {
                 let length := returndatasize()
                 returndatacopy(0, 0, length)
                 revert(0, length)
             }
+            /* solhint-enable no-inline-assembly */
         }
     }
 }

contracts/test/Token.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: LGPL-3.0-only
-pragma solidity >=0.6.0 <0.7.0;
+pragma solidity >=0.6.0 <0.9.0;
 
 interface Token {
     function transfer(address _to, uint256 value) external returns (bool);

docs/guidelines.md

@@ -10,20 +10,24 @@ The data can be stored to this location with
 
 ```
 bytes32 slot = VARIABLE_SLOT;
-// solhint-disable-next-line no-inline-assembly
+/* solhint-disable no-inline-assembly */
+/// @solidity memory-safe-assembly
 assembly {
     sstore(slot, value)
 }
+/* solhint-enable no-inline-assembly */
 ```
 
 and read with
 
 ```
 bytes32 slot = VARIABLE_SLOT;
-// solhint-disable-next-line no-inline-assembly
+/* solhint-disable no-inline-assembly */
+/// @solidity memory-safe-assembly
 assembly {
     value := sload(slot)
 }
+/* solhint-enable no-inline-assembly */
 ```
 
 Note: Make sure to use a unique identifier else unexpected behaviour will occur

hardhat.config.ts

@@ -39,7 +39,8 @@ import "./src/tasks/show_codesize";
 import { BigNumber } from "@ethersproject/bignumber";
 import { DeterministicDeploymentInfo } from "hardhat-deploy/dist/types";
 
-const primarySolidityVersion = SOLIDITY_VERSION || "0.7.6";
+const defaultSolidityVersion = "0.7.6";
+const primarySolidityVersion = SOLIDITY_VERSION || defaultSolidityVersion;
 const soliditySettings = SOLIDITY_SETTINGS ? JSON.parse(SOLIDITY_SETTINGS) : undefined;
 
 const deterministicDeployment = (network: string): DeterministicDeploymentInfo => {
@@ -70,7 +71,7 @@ const userConfig: HardhatUserConfig = {
         target: "ethers-v6",
     },
     solidity: {
-        compilers: [{ version: primarySolidityVersion, settings: soliditySettings }, { version: "0.6.12" }, { version: "0.5.17" }],
+        compilers: [{ version: primarySolidityVersion, settings: soliditySettings }, { version: defaultSolidityVersion }],
     },
     networks: {
         hardhat: {

test/core/Safe.Execution.spec.ts

@@ -22,10 +22,12 @@ describe("Safe", () => {
             contract StorageSetter {
                 function setStorage(bytes3 data) public {
                     bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
-                    // solhint-disable-next-line no-inline-assembly
+                    /* solhint-disable no-inline-assembly */
+                    /// @solidity memory-safe-assembly
                     assembly {
                         sstore(slot, data)
                     }
+                    /* solhint-enable no-inline-assembly */
                 }
             }`;
         const storageSetter = await deployContract(user1, setterSource);

test/core/Safe.Setup.spec.ts

@@ -216,10 +216,12 @@ describe("Safe", () => {
             contract Initializer {
                 function init(bytes4 data) public {
                     bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
-                    // solhint-disable-next-line no-inline-assembly
+                    /* solhint-disable no-inline-assembly */
+                    /// @solidity memory-safe-assembly
                     assembly {
                         sstore(slot, data)
                     }
+                    /* solhint-enable no-inline-assembly */
                 }
             }`;
             const testIntializer = await deployContract(user1, source);

test/libraries/MultiSend.spec.ts

@@ -18,10 +18,12 @@ describe("MultiSend", () => {
             contract StorageSetter {
                 function setStorage(bytes3 data) public {
                     bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
-                    // solhint-disable-next-line no-inline-assembly
+                    /* solhint-disable no-inline-assembly */
+                    /// @solidity memory-safe-assembly
                     assembly {
                         sstore(slot, data)
                     }
+                    /* solhint-enable no-inline-assembly */
                 }
             }`;
         const signers = await ethers.getSigners();

test/libraries/MultiSendCallOnly.spec.ts

@@ -18,10 +18,12 @@ describe("MultiSendCallOnly", () => {
             contract StorageSetter {
                 function setStorage(bytes3 data) public {
                     bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
-                    // solhint-disable-next-line no-inline-assembly
+                    /* solhint-disable no-inline-assembly */
+                    /// @solidity memory-safe-assembly
                     assembly {
                         sstore(slot, data)
                     }
+                    /* solhint-enable no-inline-assembly */
                 }
             }`;
         const signers = await ethers.getSigners();