Merge pull request #599 from safe-global/release/v1.5.0

1.5.0 Release changes
safe-global · Aug 1, 2023 · e22af08 · e22af08
2 parents 9ffb6e3 + 8e015d7
commit e22af08
Show file tree

Hide file tree

Showing 35 changed files with 909 additions and 391 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -62,10 +62,10 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        solidity: ["0.7.6", "0.8.2"]
+        solidity: ["0.7.6", "0.8.19"]
         include:
-        - solidity: "0.8.2"
-          settings: '{"viaIR":true,"optimizer":{"enabled":true,"runs":10000}}'
+        - solidity: "0.8.19"
+          settings: '{"viaIR":true,"optimizer":{"enabled":true,"runs":1000000}}'
     env:
       SOLIDITY_VERSION: ${{ matrix.solidity }}
       SOLIDITY_SETTINGS: ${{ matrix.settings }}
@@ -78,4 +78,4 @@ jobs:
         with:
           path: "**/node_modules"
           key: ${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}
-      - run: (yarn --frozen-lockfile && yarn build && yarn hardhat codesize --contractname Safe && yarn benchmark) || echo "Benchmark failed"
+      - run: (yarn --frozen-lockfile && yarn build && yarn hardhat codesize --contractname Safe && yarn benchmark) || echo "Benchmark failed"
diff --git a/certora/applyHarness.patch b/certora/applyHarness.patch
@@ -1,29 +1,29 @@
-diff -druN Safe.sol Safe.sol
---- Safe.sol	2023-05-16 15:08:39
-+++ Safe.sol	2023-05-25 16:23:56
-@@ -76,7 +76,7 @@
-          * so we create a Safe with 0 owners and threshold 1.
-          * This is an unusable Safe, perfect for the singleton
-          */
--        threshold = 1;
-+        // threshold = 1; MUNGED: remove and add to constructor of the harness
-     }
-
-     /**
 diff -druN base/Executor.sol base/Executor.sol
---- base/Executor.sol	2023-05-16 15:08:39
-+++ base/Executor.sol	2023-05-25 16:23:31
-@@ -25,11 +25,9 @@
-         Enum.Operation operation,
+--- base/Executor.sol	2023-06-30 15:32:21.392860349 +0200
++++ base/Executor.sol	2023-06-30 15:37:58.671801994 +0200
+@@ -26,11 +26,8 @@
          uint256 txGas
      ) internal returns (bool success) {
-+        // MUNGED lets just be a bit more optimistic, `execute` does nothing for `DELEGATECALL` and always returns true
          if (operation == Enum.Operation.DelegateCall) {
 -            // solhint-disable-next-line no-inline-assembly
+-            /// @solidity memory-safe-assembly
 -            assembly {
 -                success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0)
 -            }
++            // MUNGED lets just be a bit more optimistic, `execute` does nothing for `DELEGATECALL` and always returns true
 +            return true;
          } else {
              // solhint-disable-next-line no-inline-assembly
-             assembly {
+             /// @solidity memory-safe-assembly
+diff -druN Safe.sol Safe.sol
+--- Safe.sol	2023-06-30 15:32:21.392860349 +0200
++++ Safe.sol	2023-06-30 15:37:17.198953773 +0200
+@@ -76,7 +76,7 @@
+          * so we create a Safe with 0 owners and threshold 1.
+          * This is an unusable Safe, perfect for the singleton
+          */
+-        threshold = 1;
++        // threshold = 1; MUNGED: remove and add to constructor of the harness
+     }
+
+     /**
diff --git a/certora/specs/Safe.spec b/certora/specs/Safe.spec
@@ -1,5 +1,4 @@
 methods {
-    // 
     function getThreshold() external returns (uint256) envfree;
     function disableModule(address,address) external;
     function nonce() external returns (uint256) envfree;

diff --git a/contracts/Safe.sol b/contracts/Safe.sol
@@ -39,8 +39,7 @@ contract Safe is
     SecuredTokenTransfer,
     ISignatureValidatorConstants,
     FallbackManager,
-    StorageAccessible,
-    GuardManager
+    StorageAccessible
 {
     using SafeMath for uint256;
 
@@ -151,8 +150,7 @@ contract Safe is
         bytes32 txHash;
         // Use scope here to limit variable lifetime and prevent `stack too deep` errors
         {
-            bytes memory txHashData = encodeTransactionData(
-                // Transaction info
+            txHash = getTransactionHash( // Transaction info
                 to,
                 value,
                 data,
@@ -164,12 +162,10 @@ contract Safe is
                 gasToken,
                 refundReceiver,
                 // Signature info
-                nonce
+                // We use the post-increment here, so the current nonce value is used and incremented afterwards.
+                nonce++
             );
-            // Increase nonce and execute transaction.
-            nonce++;
-            txHash = keccak256(txHashData);
-            checkSignatures(txHash, txHashData, signatures);
+            checkSignatures(txHash, "", signatures);
         }
         address guard = getGuard();
         {
@@ -192,6 +188,7 @@ contract Safe is
                 );
             }
         }
+
         // We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500)
         // We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150
         require(gasleft() >= ((safeTxGas * 64) / 63).max(safeTxGas + 2500) + 500, "GS010");
@@ -238,9 +235,10 @@ contract Safe is
         // solhint-disable-next-line avoid-tx-origin
         address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
         if (gasToken == address(0)) {
-            // For ETH we will only adjust the gas price to not be higher than the actual used gas price
+            // For native tokens, we will only adjust the gas price to not be higher than the actually used gas price
             payment = gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice);
-            require(receiver.send(payment), "GS011");
+            (bool refundSuccess, ) = receiver.call{value: payment}("");
+            require(refundSuccess, "GS011");
         } else {
             payment = gasUsed.add(baseGas).mul(gasPrice);
             require(transferToken(gasToken, receiver, payment), "GS012");
@@ -259,19 +257,30 @@ contract Safe is
         uint256 _threshold = threshold;
         // Check that a threshold is set
         require(_threshold > 0, "GS001");
-        checkNSignatures(dataHash, data, signatures, _threshold);
+        checkNSignatures(msg.sender, dataHash, data, signatures, _threshold);
     }
 
     /**
      * @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
      * @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.
+     *      The data parameter (bytes) is not used since v1.5.0 as it is not required anymore. Prior to v1.5.0,
+     *      data parameter was used in contract signature validation flow using legacy EIP-1271.
+     *      Version v1.5.0, uses dataHash parameter instead of data with updated EIP-1271 implementation.
+     * @param executor Address that executing the transaction.
+     *        ⚠️⚠️⚠️ Make sure that the executor address is a legitmate executor.
+     *        Incorrectly passed the executor might reduce the threshold by 1 signature. ⚠️⚠️⚠️
      * @param dataHash Hash of the data (could be either a message hash or transaction hash)
-     * @param data That should be signed (this is passed to an external validator contract)
      * @param signatures Signature data that should be verified.
      *                   Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
      * @param requiredSignatures Amount of required valid signatures.
      */
-    function checkNSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures, uint256 requiredSignatures) public view {
+    function checkNSignatures(
+        address executor,
+        bytes32 dataHash,
+        bytes memory /* data */,
+        bytes memory signatures,
+        uint256 requiredSignatures
+    ) public view {
         // Check that the provided signature data is not too short
         require(signatures.length >= requiredSignatures.mul(65), "GS020");
         // There cannot be an owner with address 0.
@@ -284,7 +293,6 @@ contract Safe is
         for (i = 0; i < requiredSignatures; i++) {
             (v, r, s) = signatureSplit(signatures, i);
             if (v == 0) {
-                require(keccak256(data) == dataHash, "GS027");
                 // If v is 0 then it is a contract signature
                 // When handling contract signatures the address of the contract is encoded into r
                 currentOwner = address(uint160(uint256(r)));
@@ -300,6 +308,7 @@ contract Safe is
                 // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
                 uint256 contractSignatureLen;
                 // solhint-disable-next-line no-inline-assembly
+                /// @solidity memory-safe-assembly
                 assembly {
                     contractSignatureLen := mload(add(add(signatures, s), 0x20))
                 }
@@ -308,17 +317,18 @@ contract Safe is
                 // Check signature
                 bytes memory contractSignature;
                 // solhint-disable-next-line no-inline-assembly
+                /// @solidity memory-safe-assembly
                 assembly {
                     // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
                     contractSignature := add(add(signatures, s), 0x20)
                 }
-                require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");
+                require(ISignatureValidator(currentOwner).isValidSignature(dataHash, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");
             } else if (v == 1) {
                 // If v is 1 then it is an approved hash
                 // When handling approved hashes the address of the approver is encoded into r
                 currentOwner = address(uint160(uint256(r)));
                 // Hashes are automatically approved by the sender of the message or when they have been pre-approved via a separate transaction
-                require(msg.sender == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025");
+                require(executor == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025");
             } else if (v > 30) {
                 // If v > 30 then default va (27,28) has been adjusted for eth_sign flow
                 // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
@@ -346,24 +356,18 @@ contract Safe is
     }
 
     /**
-     * @notice Returns the ID of the chain the contract is currently deployed on.
-     * @return The ID of the current chain as a uint256.
+     * @dev Returns the domain separator for this contract, as defined in the EIP-712 standard.
+     * @return bytes32 The domain separator hash.
      */
-    function getChainId() public view returns (uint256) {
-        uint256 id;
+    function domainSeparator() public view returns (bytes32) {
+        uint256 chainId;
         // solhint-disable-next-line no-inline-assembly
+        /// @solidity memory-safe-assembly
         assembly {
-            id := chainid()
+            chainId := chainid()
         }
-        return id;
-    }
 
-    /**
-     * @dev Returns the domain separator for this contract, as defined in the EIP-712 standard.
-     * @return bytes32 The domain separator hash.
-     */
-    function domainSeparator() public view returns (bytes32) {
-        return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this));
+        return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, chainId, this));
     }
 
     /**
@@ -391,7 +395,7 @@ contract Safe is
         address gasToken,
         address refundReceiver,
         uint256 _nonce
-    ) public view returns (bytes memory) {
+    ) private view returns (bytes memory) {
         bytes32 safeTxHash = keccak256(
             abi.encode(
                 SAFE_TX_TYPEHASH,

diff --git a/contracts/accessors/SimulateTxAccessor.sol b/contracts/accessors/SimulateTxAccessor.sol
@@ -49,6 +49,7 @@ contract SimulateTxAccessor is Executor {
         success = execute(to, value, data, operation, gasleft());
         estimate = startGas - gasleft();
         // solhint-disable-next-line no-inline-assembly
+        /// @solidity memory-safe-assembly
         assembly {
             // Load free memory location
             let ptr := mload(0x40)

diff --git a/contracts/base/Executor.sol b/contracts/base/Executor.sol
@@ -27,11 +27,13 @@ abstract contract Executor {
     ) internal returns (bool success) {
         if (operation == Enum.Operation.DelegateCall) {
             // solhint-disable-next-line no-inline-assembly
+            /// @solidity memory-safe-assembly
             assembly {
                 success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0)
             }
         } else {
             // solhint-disable-next-line no-inline-assembly
+            /// @solidity memory-safe-assembly
             assembly {
                 success := call(txGas, to, value, add(data, 0x20), mload(data), 0, 0)
             }

diff --git a/contracts/base/FallbackManager.sol b/contracts/base/FallbackManager.sol
@@ -35,6 +35,7 @@ abstract contract FallbackManager is SelfAuthorized {
 
         bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
         // solhint-disable-next-line no-inline-assembly
+        /// @solidity memory-safe-assembly
         assembly {
             sstore(slot, handler)
         }
@@ -61,22 +62,39 @@ abstract contract FallbackManager is SelfAuthorized {
     fallback() external {
         bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
         // solhint-disable-next-line no-inline-assembly
+        /// @solidity memory-safe-assembly
         assembly {
+            // When compiled with the optimizer, the compiler relies on a certain assumptions on how the
+            // 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)
             }
-            calldatacopy(0, 0, calldatasize())
+
+            let calldataPtr := allocate(calldatasize())
+            calldatacopy(calldataPtr, 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
-            mstore(calldatasize(), shl(96, caller()))
+            let senderPtr := allocate(20)
+            mstore(senderPtr, shl(96, caller()))
+
             // Add 20 bytes for the address appended add the end
-            let success := call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0)
-            returndatacopy(0, 0, returndatasize())
+            let success := call(gas(), handler, 0, calldataPtr, add(calldatasize(), 20), 0, 0)
+
+            let returnDataPtr := allocate(returndatasize())
+            returndatacopy(returnDataPtr, 0, returndatasize())
             if iszero(success) {
-                revert(0, returndatasize())
+                revert(returnDataPtr, returndatasize())
             }
-            return(0, returndatasize())
+            return(returnDataPtr, returndatasize())
         }
     }
 }
diff --git a/contracts/base/GuardManager.sol b/contracts/base/GuardManager.sol
@@ -5,7 +5,21 @@ import "../common/Enum.sol";
 import "../common/SelfAuthorized.sol";
 import "../interfaces/IERC165.sol";
 
+/// @title Guard Interface
 interface Guard is IERC165 {
+    /// @notice Checks the transaction details.
+    /// @dev The function needs to implement transaction validation logic.
+    /// @param to The address to which the transaction is intended.
+    /// @param value The value of the transaction in Wei.
+    /// @param data The transaction data.
+    /// @param operation The type of operation of the transaction.
+    /// @param safeTxGas Gas used for the transaction.
+    /// @param baseGas The base gas for the transaction.
+    /// @param gasPrice The price of gas in Wei for the transaction.
+    /// @param gasToken The token used to pay for gas.
+    /// @param refundReceiver The address which should receive the refund.
+    /// @param signatures The signatures of the transaction.
+    /// @param msgSender The address of the message sender.
     function checkTransaction(
         address to,
         uint256 value,
@@ -20,13 +34,33 @@ interface Guard is IERC165 {
         address msgSender
     ) external;
 
-    function checkAfterExecution(bytes32 txHash, bool success) external;
+    /// @notice Checks the module transaction details.
+    /// @dev The function needs to implement module transaction validation logic.
+    /// @param to The address to which the transaction is intended.
+    /// @param value The value of the transaction in Wei.
+    /// @param data The transaction data.
+    /// @param operation The type of operation of the transaction.
+    /// @param module The module involved in the transaction.
+    /// @return moduleTxHash The hash of the module transaction.
+    function checkModuleTransaction(
+        address to,
+        uint256 value,
+        bytes memory data,
+        Enum.Operation operation,
+        address module
+    ) external returns (bytes32 moduleTxHash);
+
+    /// @notice Checks after execution of transaction.
+    /// @dev The function needs to implement a check after the execution of the transaction.
+    /// @param hash The hash of the transaction.
+    /// @param success The status of the transaction execution.
+    function checkAfterExecution(bytes32 hash, bool success) external;
 }
 
 abstract contract BaseGuard is Guard {
     function supportsInterface(bytes4 interfaceId) external view virtual override returns (bool) {
         return
-            interfaceId == type(Guard).interfaceId || // 0xe6d7a83a
+            interfaceId == type(Guard).interfaceId || // 0x945b8148
             interfaceId == type(IERC165).interfaceId; // 0x01ffc9a7
     }
 }
@@ -56,6 +90,7 @@ abstract contract GuardManager is SelfAuthorized {
         }
         bytes32 slot = GUARD_STORAGE_SLOT;
         // solhint-disable-next-line no-inline-assembly
+        /// @solidity memory-safe-assembly
         assembly {
             sstore(slot, guard)
         }
@@ -72,6 +107,7 @@ abstract contract GuardManager is SelfAuthorized {
     function getGuard() internal view returns (address guard) {
         bytes32 slot = GUARD_STORAGE_SLOT;
         // solhint-disable-next-line no-inline-assembly
+        /// @solidity memory-safe-assembly
         assembly {
             guard := sload(slot)
         }