From c484f203b5702b7d18213a2df57668d5dbd7f7f6 Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Thu, 25 Mar 2021 11:14:17 +0100 Subject: [PATCH] Closes #227: Allow multisend when msg.value > 0; Add call only multisend (#286) --- contracts/libraries/MultiSend.sol | 3 + contracts/libraries/MultiSendCallOnly.sol | 56 ++++++++ src/deploy/deploy_libraries.ts | 7 + test/libraries/MultiSend.spec.ts | 17 +++ test/libraries/MultiSendCallOnly.spec.ts | 162 ++++++++++++++++++++++ test/utils/setup.ts | 6 + 6 files changed, 251 insertions(+) create mode 100644 contracts/libraries/MultiSendCallOnly.sol create mode 100644 test/libraries/MultiSendCallOnly.spec.ts diff --git a/contracts/libraries/MultiSend.sol b/contracts/libraries/MultiSend.sol index 8ccf73359..c47ac351c 100644 --- a/contracts/libraries/MultiSend.sol +++ b/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 diff --git a/contracts/libraries/MultiSendCallOnly.sol b/contracts/libraries/MultiSendCallOnly.sol new file mode 100644 index 000000000..b11b32866 --- /dev/null +++ b/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 - +/// @author Richard Meissner - +/// @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)) + } + } + } +} diff --git a/src/deploy/deploy_libraries.ts b/src/deploy/deploy_libraries.ts index 13b0a7b74..00523f81d 100644 --- a/src/deploy/deploy_libraries.ts +++ b/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'] diff --git a/test/libraries/MultiSend.spec.ts b/test/libraries/MultiSend.spec.ts index 36317b0b3..6d36068bf 100644 --- a/test/libraries/MultiSend.spec.ts +++ b/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() diff --git a/test/libraries/MultiSendCallOnly.spec.ts b/test/libraries/MultiSendCallOnly.spec.ts new file mode 100644 index 000000000..6a09866b8 --- /dev/null +++ b/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")) + }) + }) +}) \ No newline at end of file diff --git a/test/utils/setup.ts b/test/utils/setup.ts index 62c6c0ac9..15267460c 100644 --- a/test/utils/setup.ts +++ b/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");