Raise transfer stipend

hardhat.config.ts

@@ -90,6 +90,7 @@ export default {
   },
   networks: {
     hardhat: {
+      hardfork: "berlin",
       blockGasLimit: 12.5e6,
     },
     mainnet: {

src/contracts/libraries/GPv2Transfer.sol

@@ -155,7 +155,26 @@ library GPv2Transfer {
                     transfer.balance != GPv2Order.BALANCE_INTERNAL,
                     "GPv2: unsupported internal ETH"
                 );
-                payable(transfer.account).transfer(transfer.amount);
+
+                // NOTE: We transfer with a slightly higher stipend than the
+                // Solidity default of 2300 because of the recent Berlin hard
+                // fork. This is not strictly needed, as access lists would also
+                // allow sending Ether to SC wallets, but increasing the stipend
+                // seemed like an appropriate solution. The value of 5700 was
+                // chosen because:
+                // - it is small enough that no meaningful work can be done (no
+                //   storage writing for example)
+                // - it is enough for a cold storage read (2100), cold delegate
+                //   call (2600), and a little more for doing things (1000)
+                // - it is enough to transfer to a Gnosis Safe :)
+                // solhint-disable avoid-low-level-calls
+                (bool success, ) =
+                    payable(transfer.account).call{
+                        value: transfer.amount,
+                        gas: 5700
+                    }(hex"");
+                // solhint-enable avoid-low-level-calls
+                require(success, "GPv2: ether transfer failed");
             } else if (transfer.balance == GPv2Order.BALANCE_ERC20) {
                 transfer.token.safeTransfer(transfer.account, transfer.amount);
             } else {

src/contracts/test/FallbackWriteStorage.sol

@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: LGPL-3.0-or-later
+pragma solidity ^0.7.6;
+
+contract FallbackWriteStorage {
+    uint256 public receiveCount;
+
+    receive() external payable {
+        receiveCount += 1;
+    }
+}

test/GPv2Transfer.test.ts

@@ -1,13 +1,14 @@
 import IERC20 from "@openzeppelin/contracts/build/contracts/IERC20.json";
 import { expect } from "chai";
 import { MockContract } from "ethereum-waffle";
-import { BigNumber, Contract } from "ethers";
+import { BigNumber, Contract, ContractFactory } from "ethers";
 import { artifacts, ethers, waffle } from "hardhat";
 
 import { BUY_ETH_ADDRESS } from "../src/ts";
 
 import { UserBalanceOpKind } from "./balancer";
 import { OrderBalanceId } from "./encoding";
+import { GnosisSafeManager } from "./safe";
 
 describe("GPv2Transfer", () => {
   const [deployer, recipient, funder, ...traders] =
@@ -17,6 +18,9 @@ describe("GPv2Transfer", () => {
   let vault: MockContract;
   let token: MockContract;
 
+  let NonPayable: ContractFactory;
+  let FallbackWriteStorage: ContractFactory;
+
   beforeEach(async () => {
     const GPv2Transfer = await ethers.getContractFactory(
       "GPv2TransferTestInterface",
@@ -26,6 +30,11 @@ describe("GPv2Transfer", () => {
     const IVault = await artifacts.readArtifact("IVault");
     vault = await waffle.deployMockContract(deployer, IVault.abi);
     token = await waffle.deployMockContract(deployer, IERC20.abi);
+
+    NonPayable = await ethers.getContractFactory("NonPayable");
+    FallbackWriteStorage = await ethers.getContractFactory(
+      "FallbackWriteStorage",
+    );
   });
 
   const amount = ethers.utils.parseEther("0.1337");
@@ -508,6 +517,77 @@ describe("GPv2Transfer", () => {
       );
     });
 
+    it("should transfer Ether to a Smart Contract wallet", async () => {
+      await funder.sendTransaction({
+        to: transfer.address,
+        value: amount,
+      });
+
+      const safeManager = await GnosisSafeManager.init(deployer);
+      const safe = await safeManager.newSafe([traders[0].address], 1);
+
+      await transfer.transferToAccountsTest(vault.address, [
+        {
+          account: safe.address,
+          token: BUY_ETH_ADDRESS,
+          amount,
+          balance: OrderBalanceId.ERC20,
+        },
+      ]);
+
+      expect(await ethers.provider.getBalance(safe.address)).to.deep.equal(
+        amount,
+      );
+    });
+
+    it("should revert when transfering Ether to contract that reverts", async () => {
+      await funder.sendTransaction({
+        to: transfer.address,
+        value: amount,
+      });
+
+      const smartWallet = await NonPayable.deploy();
+      await expect(
+        transfer.transferToAccountsTest(vault.address, [
+          {
+            account: smartWallet.address,
+            token: BUY_ETH_ADDRESS,
+            amount,
+            balance: OrderBalanceId.ERC20,
+          },
+        ]),
+      ).to.be.reverted;
+    });
+
+    it("should revert when transfering Ether to contract writes storage", async () => {
+      await funder.sendTransaction({
+        to: transfer.address,
+        value: amount,
+      });
+
+      const smartWallet = await FallbackWriteStorage.deploy();
+
+      await expect(
+        funder.sendTransaction({
+          to: smartWallet.address,
+          value: 1,
+        }),
+      ).to.not.be.reverted;
+      expect(await smartWallet.receiveCount()).to.equal(1);
+
+      await expect(
+        transfer.transferToAccountsTest(vault.address, [
+          {
+            account: smartWallet.address,
+            token: BUY_ETH_ADDRESS,
+            amount,
+            balance: OrderBalanceId.ERC20,
+          },
+        ]),
+      ).to.be.reverted;
+      expect(await smartWallet.receiveCount()).to.not.equal(2);
+    });
+
     it("should transfer many external and internal amounts to recipient", async () => {
       // NOTE: Make sure we have enough traders for our test :)
       expect(traders).to.have.length.above(5);

test/e2e/contractOrdersWithGnosisSafe.test.ts

@@ -1,6 +1,3 @@
-import GnosisSafe from "@gnosis.pm/safe-contracts/build/artifacts/contracts/GnosisSafe.sol/GnosisSafe.json";
-import CompatibilityFallbackHandler from "@gnosis.pm/safe-contracts/build/artifacts/contracts/handler/CompatibilityFallbackHandler.sol/CompatibilityFallbackHandler.json";
-import GnosisSafeProxyFactory from "@gnosis.pm/safe-contracts/build/artifacts/contracts/proxies/GnosisSafeProxyFactory.sol/GnosisSafeProxyFactory.json";
 import ERC20 from "@openzeppelin/contracts/build/contracts/ERC20PresetMinterPauser.json";
 import { expect } from "chai";
 import { BytesLike, Signer, Contract, Wallet } from "ethers";
@@ -15,6 +12,7 @@ import {
   domain,
   hashOrder,
 } from "../../src/ts";
+import { GnosisSafeManager } from "../safe";
 
 import { deployTestContracts } from "./fixture";
 
@@ -23,57 +21,6 @@ interface SafeTransaction {
   data: BytesLike;
 }
 
-class GnosisSafeManager {
-  constructor(
-    readonly deployer: Signer,
-    readonly masterCopy: Contract,
-    readonly signingFallback: Contract,
-    readonly proxyFactory: Contract,
-  ) {}
-
-  static async init(deployer: Signer): Promise<GnosisSafeManager> {
-    const masterCopy = await waffle.deployContract(deployer, GnosisSafe);
-    const proxyFactory = await waffle.deployContract(
-      deployer,
-      GnosisSafeProxyFactory,
-    );
-    const signingFallback = await waffle.deployContract(
-      deployer,
-      CompatibilityFallbackHandler,
-    );
-    return new GnosisSafeManager(
-      deployer,
-      masterCopy,
-      signingFallback,
-      proxyFactory,
-    );
-  }
-
-  async newSafe(
-    owners: string[],
-    threshold: number,
-    fallback = ethers.constants.AddressZero,
-  ): Promise<Contract> {
-    const proxyAddress = await this.proxyFactory.callStatic.createProxy(
-      this.masterCopy.address,
-      "0x",
-    );
-    await this.proxyFactory.createProxy(this.masterCopy.address, "0x");
-    const safe = await ethers.getContractAt(GnosisSafe.abi, proxyAddress);
-    await safe.setup(
-      owners,
-      threshold,
-      ethers.constants.AddressZero,
-      "0x",
-      fallback,
-      ethers.constants.AddressZero,
-      0,
-      ethers.constants.AddressZero,
-    );
-    return safe;
-  }
-}
-
 async function gnosisSafeSign(
   message: BytesLike,
   signers: Signer[],

test/safe.ts

@@ -0,0 +1,56 @@
+import GnosisSafe from "@gnosis.pm/safe-contracts/build/artifacts/contracts/GnosisSafe.sol/GnosisSafe.json";
+import CompatibilityFallbackHandler from "@gnosis.pm/safe-contracts/build/artifacts/contracts/handler/CompatibilityFallbackHandler.sol/CompatibilityFallbackHandler.json";
+import GnosisSafeProxyFactory from "@gnosis.pm/safe-contracts/build/artifacts/contracts/proxies/GnosisSafeProxyFactory.sol/GnosisSafeProxyFactory.json";
+import { Signer, Contract } from "ethers";
+import { ethers, waffle } from "hardhat";
+
+export class GnosisSafeManager {
+  constructor(
+    readonly deployer: Signer,
+    readonly masterCopy: Contract,
+    readonly signingFallback: Contract,
+    readonly proxyFactory: Contract,
+  ) {}
+
+  static async init(deployer: Signer): Promise<GnosisSafeManager> {
+    const masterCopy = await waffle.deployContract(deployer, GnosisSafe);
+    const proxyFactory = await waffle.deployContract(
+      deployer,
+      GnosisSafeProxyFactory,
+    );
+    const signingFallback = await waffle.deployContract(
+      deployer,
+      CompatibilityFallbackHandler,
+    );
+    return new GnosisSafeManager(
+      deployer,
+      masterCopy,
+      signingFallback,
+      proxyFactory,
+    );
+  }
+
+  async newSafe(
+    owners: string[],
+    threshold: number,
+    fallback = ethers.constants.AddressZero,
+  ): Promise<Contract> {
+    const proxyAddress = await this.proxyFactory.callStatic.createProxy(
+      this.masterCopy.address,
+      "0x",
+    );
+    await this.proxyFactory.createProxy(this.masterCopy.address, "0x");
+    const safe = await ethers.getContractAt(GnosisSafe.abi, proxyAddress);
+    await safe.setup(
+      owners,
+      threshold,
+      ethers.constants.AddressZero,
+      "0x",
+      fallback,
+      ethers.constants.AddressZero,
+      0,
+      ethers.constants.AddressZero,
+    );
+    return safe;
+  }
+}