Merge branch 'main' into 1.4.1-zksync

.github/workflows/certora.yml

@@ -25,8 +25,8 @@ jobs:
               uses: actions/setup-java@v3
               with: { java-version: "17", java-package: jre, distribution: semeru }
 
-            - name: Install certora cli-beta
-              run: pip install -Iv certora-cli-beta==4.2.0
+            - name: Install certora cli
+              run: pip install -Iv certora-cli
 
             - name: Install solc
               run: |

.github/workflows/cla.yml

@@ -19,7 +19,7 @@ jobs:
           PERSONAL_ACCESS_TOKEN: ${{ secrets.PERSONAL_ACCESS_TOKEN }}
         with:
           path-to-signatures: 'signatures/version1/cla.json'
-          path-to-document: 'https://safe.global/cla/'
+          path-to-document: 'https://safe.global/cla'
           # branch should not be protected
           branch: 'cla-signatures'
           allowlist: rmeissner,Uxio0,*bot # may need to update this expression if we add new bots

README.md

@@ -2,8 +2,8 @@ Safe Contracts
 ==============
 
 [![npm version](https://badge.fury.io/js/%40gnosis.pm%2Fsafe-contracts.svg)](https://badge.fury.io/js/%40gnosis.pm%2Fsafe-contracts)
-[![Build Status](https://github.com/safe-global/safe-contracts/workflows/safe-contracts/badge.svg?branch=development)](https://github.com/safe-global/safe-contracts/actions)
-[![Coverage Status](https://coveralls.io/repos/github/safe-global/safe-contracts/badge.svg?branch=development)](https://coveralls.io/github/safe-global/safe-contracts)
+[![Build Status](https://github.com/safe-global/safe-contracts/workflows/safe-contracts/badge.svg?branch=main)](https://github.com/safe-global/safe-contracts/actions)
+[![Coverage Status](https://coveralls.io/repos/github/safe-global/safe-contracts/badge.svg?branch=main)](https://coveralls.io/github/safe-global/safe-contracts)
 
 > :warning: **This branch contains changes that are under development** To use the latest audited version make sure to use the correct commit. The tagged versions that are used by the Safe team can be found in the [releases](https://github.com/safe-global/safe-contracts/releases).
 

certora/harnesses/SafeHarness.sol

@@ -27,4 +27,12 @@ contract SafeHarness is Safe {
     function getNativeTokenBalance() public view returns (uint256) {
         return address(this).balance;
     }
+
+    function getOwnersCount() public view returns (uint256) {
+        return ownerCount;
+    }
+
+    function getOwnersCountFromArray() public view returns (uint256) {
+        return getOwners().length;
+    }
 }

certora/scripts/verifySafe.sh

@@ -10,7 +10,7 @@ certoraRun  certora/harnesses/SafeHarness.sol \
     --verify SafeHarness:certora/specs/Safe.spec \
     --solc solc7.6 \
     --optimistic_loop \
-    --prover_args "-optimisticFallback true" \
+    --prover_args '-optimisticFallback true' \
     --loop_iter 3 \
     --optimistic_hashing \
     --hashing_length_bound 352 \

certora/specs/Safe.spec

@@ -9,17 +9,19 @@ methods {
     function getModule(address) external returns (address) envfree;
     function getSafeGuard() external returns (address) envfree;
     function getNativeTokenBalance() external returns (uint256) envfree;
+    function getOwnersCount() external returns (uint256) envfree;
+    function getOwnersCountFromArray() external returns (uint256) envfree;
 
     // optional
-    function execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation) external returns (bool, bytes memory);
-    function execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation) external returns (bool);
-    function execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes) external returns (bool);
+    function execTransactionFromModuleReturnData(address,uint256,bytes,Enum.Operation) external returns (bool, bytes memory);
+    function execTransactionFromModule(address,uint256,bytes,Enum.Operation) external returns (bool);
+    function execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes) external returns (bool);
 }
 
 definition noHavoc(method f) returns bool =
-    f.selector != sig:execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation).selector
-    && f.selector != sig:execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation).selector 
-    && f.selector != sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector;
+    f.selector != sig:execTransactionFromModuleReturnData(address,uint256,bytes,Enum.Operation).selector
+    && f.selector != sig:execTransactionFromModule(address,uint256,bytes,Enum.Operation).selector 
+    && f.selector != sig:execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes).selector;
 
 definition reachableOnly(method f) returns bool =
     f.selector != sig:setup(address[],uint256,address,bytes,address,address,uint256,address).selector
@@ -46,7 +48,7 @@ rule nonceMonotonicity(method f) filtered {
     uint256 nonceAfter = nonce();
 
     assert nonceAfter != nonceBefore => 
-        to_mathint(nonceAfter) == nonceBefore + 1 && f.selector == sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector;
+        to_mathint(nonceAfter) == nonceBefore + 1 && f.selector == sig:execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes).selector;
 }
 
 
@@ -128,6 +130,28 @@ hook Sstore SafeHarness.(slot 49122629484629529244014240937346711770925847994644
     fallbackHandlerAddress = newFallbackHandlerAddress;
 }
 
+invariant threholdShouldBeLessThanOwners() getOwnersCount() >= getThreshold()
+    filtered { f -> reachableOnly(f) }
+    { preserved {
+        // The prover found a counterexample if the owners count is max uint256,
+        // but this is not a realistic scenario.
+        require getOwnersCount() >= 1 && getOwnersCount() < MAX_UINT256();
+      }
+    }
+
+invariant safeOwnerCannotBeItself(env e) !isOwner(e, currentContract)
+    filtered { f -> reachableOnly(f) }
+
+rule safeOwnerCannotBeSentinelAddress(method f) filtered {
+    f -> reachableOnly(f)
+} {
+    calldataarg args; env e;
+    f(e, args);
+
+    assert isOwner(e, 1) == false;
+}
+
+
 rule fallbackHandlerAddressChange(method f) filtered {
     f -> f.selector != sig:simulateAndRevert(address,bytes).selector &&
          f.selector != sig:getStorageAt(uint256,uint256).selector
@@ -174,16 +198,16 @@ rule nativeTokenBalanceSpending(method f) filtered {
     uint256 balanceAfter = getNativeTokenBalance();
 
     assert balanceAfter < balanceBefore => 
-        f.selector == sig:execTransaction(address,uint256,bytes,SafeHarness.Operation,uint256,uint256,uint256,address,address,bytes).selector
-        || f.selector == sig:execTransactionFromModule(address,uint256,bytes,SafeHarness.Operation).selector
-        || f.selector == sig:execTransactionFromModuleReturnData(address,uint256,bytes,SafeHarness.Operation).selector;
+        f.selector == sig:execTransaction(address,uint256,bytes,Enum.Operation,uint256,uint256,uint256,address,address,bytes).selector
+        || f.selector == sig:execTransactionFromModule(address,uint256,bytes,Enum.Operation).selector
+        || f.selector == sig:execTransactionFromModuleReturnData(address,uint256,bytes,Enum.Operation).selector;
 }
 
 rule nativeTokenBalanceSpendingExecTransaction(
         address to,
         uint256 value,
         bytes data,
-        SafeHarness.Operation operation,
+        Enum.Operation operation,
         uint256 safeTxGas,
         uint256 baseGas, 
         uint256 gasPrice, 
@@ -208,7 +232,7 @@ rule nativeTokenBalanceSpendingExecTransactionFromModule(
         address to,
         uint256 value,
         bytes data,
-        SafeHarness.Operation operation
+        Enum.Operation operation
     ) {
     uint256 balanceBefore = getNativeTokenBalance();
     env e;
@@ -226,7 +250,7 @@ rule nativeTokenBalanceSpendingExecTransactionFromModuleReturnData(
         address to,
         uint256 value,
         bytes data,
-        SafeHarness.Operation operation
+        Enum.Operation operation
 ) {
     uint256 balanceBefore = getNativeTokenBalance();
     env e;
@@ -238,3 +262,12 @@ rule nativeTokenBalanceSpendingExecTransactionFromModuleReturnData(
     assert balanceAfter < balanceBefore => 
         to_mathint(balanceBefore - value) <= to_mathint(balanceAfter);
 }
+
+rule safeOwnerCountConsistency(method f) filtered {
+    f -> reachableOnly(f)
+} {
+    calldataarg args; env e;
+    f(e, args);
+
+    assert getOwnersCount() == getOwnersCountFromArray();
+}

contracts/Safe.sol

@@ -416,7 +416,7 @@ contract Safe is
      * @param value Ether value.
      * @param data Data payload.
      * @param operation Operation type.
-     * @param safeTxGas Fas that should be used for the safe transaction.
+     * @param safeTxGas Gas that should be used for the safe transaction.
      * @param baseGas Gas costs for data used to trigger the safe transaction.
      * @param gasPrice Maximum gas price that should be used for this transaction.
      * @param gasToken Token address (or 0 if ETH) that is used for the payment.

contracts/libraries/MultiSend.sol

@@ -60,7 +60,9 @@ contract MultiSend {
                     success := delegatecall(gas(), to, data, dataLength, 0, 0)
                 }
                 if eq(success, 0) {
-                    revert(0, 0)
+                    let errorLength := returndatasize()
+                    returndatacopy(0, 0, errorLength)
+                    revert(0, errorLength)
                 }
                 // Next entry starts at 85 byte + data length
                 i := add(i, add(0x55, dataLength))

contracts/libraries/MultiSendCallOnly.sol

@@ -55,7 +55,9 @@ contract MultiSendCallOnly {
                     revert(0, 0)
                 }
                 if eq(success, 0) {
-                    revert(0, 0)
+                    let errorLength := returndatasize()
+                    returndatacopy(0, 0, errorLength)
+                    revert(0, errorLength)
                 }
                 // Next entry starts at 85 byte + data length
                 i := add(i, add(0x55, dataLength))

contracts/test/4337/Test4337ModuleAndHandler.sol

@@ -2,8 +2,6 @@
 pragma solidity >=0.7.0 <0.9.0;
 pragma abicoder v2;
 
-import "../../libraries/SafeStorage.sol";
-
 struct UserOperation {
     address sender;
     uint256 nonce;
@@ -19,14 +17,16 @@ struct UserOperation {
 }
 
 interface ISafe {
+    function enableModule(address module) external;
+
     function execTransactionFromModule(address to, uint256 value, bytes memory data, uint8 operation) external returns (bool success);
 }
 
 /// @dev A Dummy 4337 Module/Handler for testing purposes
 ///      ⚠️ ⚠️ ⚠️ DO NOT USE IN PRODUCTION ⚠️ ⚠️ ⚠️
 ///      The module does not perform ANY validation, it just executes validateUserOp and execTransaction
 ///      to perform the opcode level compliance by the bundler.
-contract Test4337ModuleAndHandler is SafeStorage {
+contract Test4337ModuleAndHandler {
     address public immutable myAddress;
     address public immutable entryPoint;
 
@@ -55,11 +55,6 @@ contract Test4337ModuleAndHandler is SafeStorage {
     }
 
     function enableMyself() public {
-        require(myAddress != address(this), "You need to DELEGATECALL, sir");
-
-        // Module cannot be added twice.
-        require(modules[myAddress] == address(0), "GS102");
-        modules[myAddress] = modules[SENTINEL_MODULES];
-        modules[SENTINEL_MODULES] = myAddress;
+        ISafe(address(this)).enableModule(myAddress);
     }
 }

contracts/test/DelegateCaller.sol

@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity >=0.7.0 <0.9.0;
+
+/**
+ * @title DelegateCaller - A test contract that executes delegatecalls
+ */
+contract DelegateCaller {
+    /**
+     * @notice makes a delegatecall
+     * @param _called The address to be delegate called
+     * @param _calldata the calldata of the call
+     */
+    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
+            assembly {
+                let length := returndatasize()
+                returndatacopy(0, 0, length)
+                revert(0, length)
+            }
+        }
+    }
+}

docs/safe_tx_gas.md

@@ -26,7 +26,7 @@ This also results in the `nonce` of this transaction being used, so it is not po
 
 #### Without gas refund
 
-If `gasPrice` is set to `0` then the Safe contracts will **not** issue a refund after the Safe transaction exection.
+If `gasPrice` is set to `0` then the Safe contracts will **not** issue a refund after the Safe transaction execution.
 
 Therefore it is not necessary to be as strict on the gas being passed along with the execution of the Safe transaction. As no refund is triggered the Safe will not pay for the execution costs, based on this the Safe contracts will send along all available case when no refund is used.
 

package.json

@@ -1,11 +1,11 @@
 {
     "name": "@safe-global/safe-contracts",
-    "version": "1.4.1",
+    "version": "1.4.1-build.0",
     "description": "Ethereum multisig contract",
     "homepage": "https://github.com/safe-global/safe-contracts/",
     "license": "LGPL-3.0",
-    "main": "dist/index.js",
-    "typings": "dist/index.d.ts",
+    "main": "dist/src/index.js",
+    "typings": "dist/src/index.d.ts",
     "files": [
         "contracts",
         "dist",

test/libraries/MultiSend.spec.ts

@@ -1,7 +1,7 @@
 import { expect } from "chai";
 import hre from "hardhat";
 import "@nomiclabs/hardhat-ethers";
-import { deployContract, getMock, getMultiSend, getSafeWithOwners, getWallets } from "../utils/setup";
+import { deployContract, getMock, getMultiSend, getSafeWithOwners, getWallets, getDelegateCaller } from "../utils/setup";
 import { buildContractCall, buildSafeTransaction, executeTx, MetaTransaction, safeApproveHash } from "../../src/utils/execution";
 import { buildMultiSendSafeTx, encodeMultiSend } from "../../src/utils/multisend";
 import { parseEther } from "@ethersproject/units";
@@ -26,6 +26,7 @@ describe("MultiSend", async () => {
             safe: await getSafeWithOwners([user1.address]),
             multiSend: await getMultiSend(),
             mock: await getMock(),
+            delegateCaller: await getDelegateCaller(),
             storageSetter,
         };
     });
@@ -181,5 +182,56 @@ describe("MultiSend", async () => {
                 ),
             ).to.be.eq("0x" + "baddad".padEnd(64, "0"));
         });
+
+        it("can bubble up revert message on call", async () => {
+            const { delegateCaller, multiSend, mock } = await setupTests();
+
+            const triggerCalldata = "0xbaddad";
+            const errorMessage = "Some random message";
+
+            await mock.givenCalldataRevertWithMessage(triggerCalldata, errorMessage);
+
+            const txs: MetaTransaction[] = [
+                {
+                    to: mock.address,
+                    value: 0,
+                    data: triggerCalldata,
+                    operation: 0,
+                },
+            ];
+            const { data } = buildMultiSendSafeTx(multiSend, txs, 0);
+
+            await expect(delegateCaller.makeDelegatecall(multiSend.address, data)).to.be.revertedWith(errorMessage);
+        });
+
+        it("can bubble up revert message on delegatecall", async () => {
+            const { delegateCaller, multiSend, mock } = await setupTests();
+
+            const triggerCalldata = "0xbaddad";
+            const errorMessage = "Some random message";
+
+            const setRevertMessageData = mock.interface.encodeFunctionData("givenCalldataRevertWithMessage", [
+                triggerCalldata,
+                errorMessage,
+            ]);
+
+            const txs: MetaTransaction[] = [
+                {
+                    to: mock.address,
+                    value: 0,
+                    data: setRevertMessageData as string,
+                    operation: 1,
+                },
+                {
+                    to: mock.address,
+                    value: 0,
+                    data: triggerCalldata,
+                    operation: 1,
+                },
+            ];
+            const { data } = buildMultiSendSafeTx(multiSend, txs, 0);
+
+            await expect(delegateCaller.callStatic.makeDelegatecall(multiSend.address, data)).to.be.revertedWith(errorMessage);
+        });
     });
 });