From a817bc47b490fae6a07e030fa0d4e6c6581685ca Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Fri, 4 Jun 2021 14:43:37 +0200 Subject: [PATCH 1/4] Increase Ether transfer gas stipend --- src/contracts/libraries/GPv2Transfer.sol | 18 ++++++++- src/contracts/test/EtherReceiver.sol | 7 ++++ test/GPv2Transfer.test.ts | 51 +++++++++++++++++++++++- 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 src/contracts/test/EtherReceiver.sol diff --git a/src/contracts/libraries/GPv2Transfer.sol b/src/contracts/libraries/GPv2Transfer.sol index 360f4cc7..da4042c0 100644 --- a/src/contracts/libraries/GPv2Transfer.sol +++ b/src/contracts/libraries/GPv2Transfer.sol @@ -155,7 +155,23 @@ 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 15000 was + // chosen so that no storage can be written nor a new contract + // created. + // solhint-disable avoid-low-level-calls + (bool success, ) = + payable(transfer.account).call{ + value: transfer.amount, + gas: 15000 + }(hex""); + // solhint-enable avoid-low-level-calls + + require(success, "GPv2: transfer failed"); } else if (transfer.balance == GPv2Order.BALANCE_ERC20) { transfer.token.safeTransfer(transfer.account, transfer.amount); } else { diff --git a/src/contracts/test/EtherReceiver.sol b/src/contracts/test/EtherReceiver.sol new file mode 100644 index 00000000..0f166a3a --- /dev/null +++ b/src/contracts/test/EtherReceiver.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.7.6; + +contract EtherReceiver { + // solhint-disable-next-line no-empty-blocks + receive() external payable {} +} diff --git a/test/GPv2Transfer.test.ts b/test/GPv2Transfer.test.ts index 85d83a33..b3f24373 100644 --- a/test/GPv2Transfer.test.ts +++ b/test/GPv2Transfer.test.ts @@ -1,7 +1,7 @@ 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"; @@ -17,6 +17,9 @@ describe("GPv2Transfer", () => { let vault: MockContract; let token: MockContract; + let NonPayable: ContractFactory; + let EtherReceiver: ContractFactory; + beforeEach(async () => { const GPv2Transfer = await ethers.getContractFactory( "GPv2TransferTestInterface", @@ -26,6 +29,9 @@ 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"); + EtherReceiver = await ethers.getContractFactory("EtherReceiver"); }); const amount = ethers.utils.parseEther("0.1337"); @@ -508,6 +514,49 @@ describe("GPv2Transfer", () => { ); }); + it("should transfer Ether to contract", async () => { + await funder.sendTransaction({ + to: transfer.address, + value: amount, + }); + + const smartWallet = await EtherReceiver.deploy(); + const initialBalance = await ethers.provider.getBalance( + smartWallet.address, + ); + await transfer.transferToAccountsTest(vault.address, [ + { + account: smartWallet.address, + token: BUY_ETH_ADDRESS, + amount, + balance: OrderBalanceId.ERC20, + }, + ]); + + expect( + await ethers.provider.getBalance(smartWallet.address), + ).to.deep.equal(initialBalance.add(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 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); From 97c20f76d5339fa165d597d1b1c9f0edb42e00b4 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Fri, 4 Jun 2021 15:47:44 +0200 Subject: [PATCH 2/4] change stipend amount and add safe test --- hardhat.config.ts | 1 + src/contracts/libraries/GPv2Transfer.sol | 14 +++-- src/contracts/test/EtherReceiver.sol | 7 --- test/GPv2Transfer.test.ts | 20 +++---- test/e2e/contractOrdersWithGnosisSafe.test.ts | 55 +----------------- test/safe.ts | 56 +++++++++++++++++++ 6 files changed, 76 insertions(+), 77 deletions(-) delete mode 100644 src/contracts/test/EtherReceiver.sol create mode 100644 test/safe.ts diff --git a/hardhat.config.ts b/hardhat.config.ts index 6fc2ffae..4c523c8e 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -90,6 +90,7 @@ export default { }, networks: { hardhat: { + hardfork: "berlin", blockGasLimit: 12.5e6, }, mainnet: { diff --git a/src/contracts/libraries/GPv2Transfer.sol b/src/contracts/libraries/GPv2Transfer.sol index da4042c0..39843c76 100644 --- a/src/contracts/libraries/GPv2Transfer.sol +++ b/src/contracts/libraries/GPv2Transfer.sol @@ -160,18 +160,22 @@ library GPv2Transfer { // 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 15000 was - // chosen so that no storage can be written nor a new contract - // created. + // 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: 15000 + gas: 5700 }(hex""); // solhint-enable avoid-low-level-calls - require(success, "GPv2: transfer failed"); + require(success, "GPv2: ether transfer failed"); } else if (transfer.balance == GPv2Order.BALANCE_ERC20) { transfer.token.safeTransfer(transfer.account, transfer.amount); } else { diff --git a/src/contracts/test/EtherReceiver.sol b/src/contracts/test/EtherReceiver.sol deleted file mode 100644 index 0f166a3a..00000000 --- a/src/contracts/test/EtherReceiver.sol +++ /dev/null @@ -1,7 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-or-later -pragma solidity ^0.7.6; - -contract EtherReceiver { - // solhint-disable-next-line no-empty-blocks - receive() external payable {} -} diff --git a/test/GPv2Transfer.test.ts b/test/GPv2Transfer.test.ts index b3f24373..7058e702 100644 --- a/test/GPv2Transfer.test.ts +++ b/test/GPv2Transfer.test.ts @@ -8,6 +8,7 @@ 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] = @@ -18,7 +19,6 @@ describe("GPv2Transfer", () => { let token: MockContract; let NonPayable: ContractFactory; - let EtherReceiver: ContractFactory; beforeEach(async () => { const GPv2Transfer = await ethers.getContractFactory( @@ -31,7 +31,6 @@ describe("GPv2Transfer", () => { token = await waffle.deployMockContract(deployer, IERC20.abi); NonPayable = await ethers.getContractFactory("NonPayable"); - EtherReceiver = await ethers.getContractFactory("EtherReceiver"); }); const amount = ethers.utils.parseEther("0.1337"); @@ -514,28 +513,27 @@ describe("GPv2Transfer", () => { ); }); - it("should transfer Ether to contract", async () => { + it("should transfer Ether to a Smart Contract wallet", async () => { await funder.sendTransaction({ to: transfer.address, value: amount, }); - const smartWallet = await EtherReceiver.deploy(); - const initialBalance = await ethers.provider.getBalance( - smartWallet.address, - ); + const safeManager = await GnosisSafeManager.init(deployer); + const safe = await safeManager.newSafe([traders[0].address], 1); + await transfer.transferToAccountsTest(vault.address, [ { - account: smartWallet.address, + account: safe.address, token: BUY_ETH_ADDRESS, amount, balance: OrderBalanceId.ERC20, }, ]); - expect( - await ethers.provider.getBalance(smartWallet.address), - ).to.deep.equal(initialBalance.add(amount)); + expect(await ethers.provider.getBalance(safe.address)).to.deep.equal( + amount, + ); }); it("should revert when transfering Ether to contract that reverts", async () => { diff --git a/test/e2e/contractOrdersWithGnosisSafe.test.ts b/test/e2e/contractOrdersWithGnosisSafe.test.ts index 9d5a6093..3d9ebc01 100644 --- a/test/e2e/contractOrdersWithGnosisSafe.test.ts +++ b/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 { - 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 { - 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[], diff --git a/test/safe.ts b/test/safe.ts new file mode 100644 index 00000000..05e018cb --- /dev/null +++ b/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 { + 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 { + 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; + } +} From c3e7da155bd634bdd270d3a0523f49dbe84d457a Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Fri, 4 Jun 2021 15:48:51 +0200 Subject: [PATCH 3/4] formatting --- src/contracts/libraries/GPv2Transfer.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/src/contracts/libraries/GPv2Transfer.sol b/src/contracts/libraries/GPv2Transfer.sol index 39843c76..36c698f6 100644 --- a/src/contracts/libraries/GPv2Transfer.sol +++ b/src/contracts/libraries/GPv2Transfer.sol @@ -174,7 +174,6 @@ library GPv2Transfer { 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); From e1a11e3ccf7d248c7659736e0c559a4f69ccf00c Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Mon, 7 Jun 2021 12:23:02 +0200 Subject: [PATCH 4/4] Add test that storage can't be written --- src/contracts/test/FallbackWriteStorage.sol | 10 +++++++ test/GPv2Transfer.test.ts | 33 +++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 src/contracts/test/FallbackWriteStorage.sol diff --git a/src/contracts/test/FallbackWriteStorage.sol b/src/contracts/test/FallbackWriteStorage.sol new file mode 100644 index 00000000..3ef2d1ba --- /dev/null +++ b/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; + } +} diff --git a/test/GPv2Transfer.test.ts b/test/GPv2Transfer.test.ts index 7058e702..a9057360 100644 --- a/test/GPv2Transfer.test.ts +++ b/test/GPv2Transfer.test.ts @@ -19,6 +19,7 @@ describe("GPv2Transfer", () => { let token: MockContract; let NonPayable: ContractFactory; + let FallbackWriteStorage: ContractFactory; beforeEach(async () => { const GPv2Transfer = await ethers.getContractFactory( @@ -31,6 +32,9 @@ describe("GPv2Transfer", () => { token = await waffle.deployMockContract(deployer, IERC20.abi); NonPayable = await ethers.getContractFactory("NonPayable"); + FallbackWriteStorage = await ethers.getContractFactory( + "FallbackWriteStorage", + ); }); const amount = ethers.utils.parseEther("0.1337"); @@ -555,6 +559,35 @@ describe("GPv2Transfer", () => { ).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);