Closes #227: Allow multisend when msg.value > 0; Add call only multis…

…end (#286)

contracts/libraries/MultiSend.sol

@@ -25,8 +25,11 @@ contract MultiSend {
     ///                     data length as a uint256 (=> 32 bytes),
     ///                     data as bytes.
     ///                     see abi.encodePacked for more information on packed encoding
+    /// @notice This method is payable as delegatecalls keep the msg.value from the previous call
+    ///         If the calling method (e.g. execTransaction) received ETH this would revert otherwise
     function multiSend(bytes memory transactions)
         public
+        payable
     {
         require(guard != GUARD_VALUE, "MultiSend should only be called via delegatecall");
         // solium-disable-next-line security/no-inline-assembly

contracts/libraries/MultiSendCallOnly.sol

@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: LGPL-3.0-only
+pragma solidity >=0.7.0 <0.9.0;
+
+
+/// @title Multi Send Call Only - Allows to batch multiple transactions into one, but only calls
+/// @author Stefan George - <stefan@gnosis.io>
+/// @author Richard Meissner - <richard@gnosis.io>
+/// @notice The guard logic is not required here as this contract doesn't support nested delegate calls
+contract MultiSendCallOnly {
+
+    /// @dev Sends multiple transactions and reverts all if one fails.
+    /// @param transactions Encoded transactions. Each transaction is encoded as a packed bytes of
+    ///                     operation has to be uint8(0) in this version (=> 1 byte),
+    ///                     to as a address (=> 20 bytes),
+    ///                     value as a uint256 (=> 32 bytes),
+    ///                     data length as a uint256 (=> 32 bytes),
+    ///                     data as bytes.
+    ///                     see abi.encodePacked for more information on packed encoding
+    /// @notice The code is for most part the same as the normal MultiSend (to keep compatibility), 
+    ///         but reverts if a transaction tries to use a delegatecall.
+    /// @notice This method is payable as delegatecalls keep the msg.value from the previous call
+    ///         If the calling method (e.g. execTransaction) received ETH this would revert otherwise
+    function multiSend(bytes memory transactions)
+        public
+        payable
+    {
+        // solium-disable-next-line security/no-inline-assembly
+        assembly {
+            let length := mload(transactions)
+            let i := 0x20
+            for { } lt(i, length) { } {
+                // First byte of the data is the operation.
+                // We shift by 248 bits (256 - 8 [operation byte]) it right since mload will always load 32 bytes (a word).
+                // This will also zero out unused data.
+                let operation := shr(0xf8, mload(add(transactions, i)))
+                // We offset the load address by 1 byte (operation byte)
+                // We shift it right by 96 bits (256 - 160 [20 address bytes]) to right-align the data and zero out unused data.
+                let to := shr(0x60, mload(add(transactions, add(i, 0x01))))
+                // We offset the load address by 21 byte (operation byte + 20 address bytes)
+                let value := mload(add(transactions, add(i, 0x15)))
+                // We offset the load address by 53 byte (operation byte + 20 address bytes + 32 value bytes)
+                let dataLength := mload(add(transactions, add(i, 0x35)))
+                // We offset the load address by 85 byte (operation byte + 20 address bytes + 32 value bytes + 32 data length bytes)
+                let data := add(transactions, add(i, 0x55))
+                let success := 0
+                switch operation
+                case 0 { success := call(gas(), to, value, data, dataLength, 0, 0) }
+                // This version does not allow delegatecalls
+                case 1 { revert(0, 0) }
+                if eq(success, 0) { revert(0, 0) }
+                // Next entry starts at 85 byte + data length
+                i := add(i, add(0x55, dataLength))
+            }
+        }
+    }
+}

src/deploy/deploy_libraries.ts

@@ -21,6 +21,13 @@ const deploy: DeployFunction = async function (
     log: true,
     deterministicDeployment: true,
   });
+
+  await deploy("MultiSendCallOnly", {
+    from: deployer,
+    args: [],
+    log: true,
+    deterministicDeployment: true,
+  });
 };
 
 deploy.tags = ['libraries']

test/libraries/MultiSend.spec.ts

@@ -108,6 +108,23 @@ describe("MultiSend", async () => {
             await expect(await hre.ethers.provider.getBalance(user2.address)).to.be.deep.eq(userBalance)
         })
 
+        it('can be used when ETH is sent with execution', async () => {
+            const { safe, multiSend, storageSetter } = await setupTests()
+
+            const txs: MetaTransaction[] = [
+                buildContractCall(storageSetter, "setStorage", ["0xbaddad"], 0)
+            ]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("0"))
+
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ], { value: parseEther("1") })
+            ).to.emit(safe, "ExecutionSuccess")
+
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("1"))
+        })
+
         it('can execute contract calls', async () => {
             const { safe, multiSend, storageSetter } = await setupTests()
 

test/libraries/MultiSendCallOnly.spec.ts

@@ -0,0 +1,162 @@
+import { expect } from "chai";
+import hre, { deployments, waffle } from "hardhat";
+import "@nomiclabs/hardhat-ethers";
+import { deployContract, getMock, getMultiSendCallOnly, getSafeWithOwners } from "../utils/setup";
+import { buildContractCall, buildSafeTransaction, executeTx, MetaTransaction, safeApproveHash } from "../utils/execution";
+import { buildMultiSendSafeTx } from "../utils/multisend";
+import { parseEther } from "@ethersproject/units";
+
+describe("MultiSendCallOnly", async () => {
+
+    const [user1, user2] = waffle.provider.getWallets();
+
+    const setupTests = deployments.createFixture(async ({ deployments }) => {
+        await deployments.fixture();
+        const setterSource = `
+            contract StorageSetter {
+                function setStorage(bytes3 data) public {
+                    bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
+                    // solium-disable-next-line security/no-inline-assembly
+                    assembly {
+                        sstore(slot, data)
+                    }
+                }
+            }`
+        const storageSetter = await deployContract(user1, setterSource);
+        return {
+            safe: await getSafeWithOwners([user1.address]),
+            multiSend: await getMultiSendCallOnly(),
+            mock: await getMock(),
+            storageSetter
+        }
+    })
+
+    describe("multiSend", async () => {
+
+        it('Should fail when using invalid operation', async () => {
+            const { safe, multiSend } = await setupTests()
+
+            const txs = [buildSafeTransaction({to: user2.address, operation: 2, nonce: 0})]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ])
+            ).to.revertedWith("GS013")
+        })
+
+        it('Should fail when using delegatecall operation', async () => {
+            const { safe, multiSend } = await setupTests()
+
+            const txs = [buildSafeTransaction({to: user2.address, operation: 1, nonce: 0})]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ])
+            ).to.revertedWith("GS013")
+        })
+
+        it('Can execute empty multisend', async () => {
+            const { safe, multiSend } = await setupTests()
+
+            const txs: MetaTransaction[] = []
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ])
+            ).to.emit(safe, "ExecutionSuccess")
+        })
+
+        it('Can execute single ether transfer', async () => {
+            const { safe, multiSend } = await setupTests()
+            await user1.sendTransaction({to: safe.address, value: parseEther("1")})
+            const userBalance = await hre.ethers.provider.getBalance(user2.address)
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("1"))
+
+            const txs: MetaTransaction[] = [buildSafeTransaction({to: user2.address, value: parseEther("1"), nonce: 0})]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ])
+            ).to.emit(safe, "ExecutionSuccess")
+
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("0"))
+            await expect(await hre.ethers.provider.getBalance(user2.address)).to.be.deep.eq(userBalance.add(parseEther("1")))
+        })
+
+        it('reverts all tx if any fails', async () => {
+            const { safe, multiSend } = await setupTests()
+            await user1.sendTransaction({to: safe.address, value: parseEther("1")})
+            const userBalance = await hre.ethers.provider.getBalance(user2.address)
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("1"))
+
+            const txs: MetaTransaction[] = [
+                buildSafeTransaction({to: user2.address, value: parseEther("1"), nonce: 0}),
+                buildSafeTransaction({to: user2.address, value: parseEther("1"), nonce: 0}),
+            ]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce(), { safeTxGas: 1 })
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ])
+            ).to.emit(safe, "ExecutionFailure")
+
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("1"))
+            await expect(await hre.ethers.provider.getBalance(user2.address)).to.be.deep.eq(userBalance)
+        })
+
+        it('can be used when ETH is sent with execution', async () => {
+            const { safe, multiSend, storageSetter } = await setupTests()
+
+            const txs: MetaTransaction[] = [
+                buildContractCall(storageSetter, "setStorage", ["0xbaddad"], 0)
+            ]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("0"))
+
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ], { value: parseEther("1") })
+            ).to.emit(safe, "ExecutionSuccess")
+
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("1"))
+        })
+
+        it('can execute contract calls', async () => {
+            const { safe, multiSend, storageSetter } = await setupTests()
+
+            const txs: MetaTransaction[] = [
+                buildContractCall(storageSetter, "setStorage", ["0xbaddad"], 0)
+            ]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ])
+            ).to.emit(safe, "ExecutionSuccess")
+
+            await expect(
+                await hre.ethers.provider.getStorageAt(safe.address, "0x4242424242424242424242424242424242424242424242424242424242424242")
+            ).to.be.eq("0x" + "".padEnd(64, "0"))
+            await expect(
+                await hre.ethers.provider.getStorageAt(storageSetter.address, "0x4242424242424242424242424242424242424242424242424242424242424242")
+            ).to.be.eq("0x" + "baddad".padEnd(64, "0"))
+        })
+
+        it('can execute combinations', async () => {
+            const { safe, multiSend, storageSetter } = await setupTests()
+            await user1.sendTransaction({to: safe.address, value: parseEther("1")})
+            const userBalance = await hre.ethers.provider.getBalance(user2.address)
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("1"))
+
+            const txs: MetaTransaction[] = [
+                buildSafeTransaction({to: user2.address, value: parseEther("1"), nonce: 0}),
+                buildContractCall(storageSetter, "setStorage", ["0xbaddad"], 0)
+            ]
+            const safeTx = buildMultiSendSafeTx(multiSend, txs, await safe.nonce())
+            await expect(
+                executeTx(safe, safeTx, [ await safeApproveHash(user1, safe, safeTx, true) ])
+            ).to.emit(safe, "ExecutionSuccess")
+
+            await expect(await hre.ethers.provider.getBalance(safe.address)).to.be.deep.eq(parseEther("0"))
+            await expect(await hre.ethers.provider.getBalance(user2.address)).to.be.deep.eq(userBalance.add(parseEther("1")))
+            await expect(
+                await hre.ethers.provider.getStorageAt(safe.address, "0x4242424242424242424242424242424242424242424242424242424242424242")
+            ).to.be.eq("0x" + "".padEnd(64, "0"))
+            await expect(
+                await hre.ethers.provider.getStorageAt(storageSetter.address, "0x4242424242424242424242424242424242424242424242424242424242424242")
+            ).to.be.eq("0x" + "baddad".padEnd(64, "0"))
+        })
+    })
+})

test/utils/setup.ts

@@ -44,6 +44,12 @@ export const getMultiSend = async () => {
     return MultiSend.attach(MultiSendDeployment.address);
 }
 
+export const getMultiSendCallOnly = async () => {
+    const MultiSendDeployment = await deployments.get("MultiSendCallOnly");
+    const MultiSend = await hre.ethers.getContractFactory("MultiSendCallOnly");
+    return MultiSend.attach(MultiSendDeployment.address);
+}
+
 export const getCreateCall = async () => {
     const CreateCallDeployment = await deployments.get("CreateCall");
     const CreateCall = await hre.ethers.getContractFactory("CreateCall");