DepositableDelegateProxy: optimize for EIP-1884

contracts/common/DepositableDelegateProxy.sol

@@ -8,14 +8,34 @@ contract DepositableDelegateProxy is DepositableStorage, DelegateProxy {
     event ProxyDeposit(address sender, uint256 value);
 
     function () external payable {
-        // send / transfer
-        if (gasleft() < FWD_GAS_LIMIT) {
-            require(msg.value > 0 && msg.data.length == 0);
-            require(isDepositable());
-            emit ProxyDeposit(msg.sender, msg.value);
-        } else { // all calls except for send or transfer
-            address target = implementation();
-            delegatedFwd(target, msg.data);
+        uint256 forwardGasThreshold = FWD_GAS_LIMIT;
+        bytes32 isDepositablePosition = DEPOSITABLE_POSITION;
+
+        // Optimized assembly implementation to prevent EIP-1884 from breaking deposits, reference code in Solidity:
+        // https://github.com/aragon/aragonOS/blob/v4.2.1/contracts/common/DepositableDelegateProxy.sol#L10-L20
+        assembly {
+            // If the gas left is lower than the threshold for forwarding to the implementation code,
+            // otherwise continue outside of the assembly block.
+            if lt(gas, forwardGasThreshold) {
+                // If the proxy accepts deposits, msg.data.length == 0 and msg.value > 0,
+                // accept the deposit and emit an event
+                if and(and(sload(isDepositablePosition), iszero(calldatasize)), gt(callvalue, 0)) {
+                    let logData := mload(0x40) // free memory pointer
+                    mstore(logData, caller) // add 'msg.sender' to the log data (first event param)
+                    mstore(add(logData, 0x20), callvalue) // add 'msg.value' to the log data (second event param)
+
+                    // Emit an event with one topic to identify the event: keccak256('ProxyDeposit(address,uint256)') = 0x15ee...dee1
+                    log1(logData, 0x40, 0x15eeaa57c7bd188c1388020bcadc2c436ec60d647d36ef5b9eb3c742217ddee1)
+
+                    stop() // Stop. Exits execution context
+                }
+
+                // If any of above checks failed, revert the execution (if ETH was sent, it is returned to the sender)
+                revert(0, 0)
+            }
         }
+
+        address target = implementation();
+        delegatedFwd(target, msg.data);
     }
 }

contracts/test/helpers/EthSender.sol

@@ -0,0 +1,8 @@
+pragma solidity 0.4.24;
+
+
+contract EthSender {
+    function sendEth(address to) external payable {
+        to.transfer(msg.value);
+    }
+}

test/contracts/apps/recovery_to_vault.js

@@ -1,12 +1,15 @@
 const { hash } = require('eth-ens-namehash')
+const { toChecksumAddress } = require('web3-utils')
 const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3)
-const { assertRevert } = require('../../helpers/assertThrow')
+const { assertRevert, assertOutOfGas } = require('../../helpers/assertThrow')
 const { getNewProxyAddress } = require('../../helpers/events')
 const { getBalance } = require('../../helpers/web3')
+const { decodeEventsOfType } = require('../../helpers/decodeEvent')
 
 const ACL = artifacts.require('ACL')
 const Kernel = artifacts.require('Kernel')
 const KernelProxy = artifacts.require('KernelProxy')
+const DepositableDelegateProxy = artifacts.require('DepositableDelegateProxy')
 
 // Mocks
 const AppStubDepositable = artifacts.require('AppStubDepositable')
@@ -17,22 +20,53 @@ const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock')
 const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock')
 const VaultMock = artifacts.require('VaultMock')
 const KernelDepositableMock = artifacts.require('KernelDepositableMock')
+const EthSender = artifacts.require('EthSender')
 
 const APP_ID = hash('stub.aragonpm.test')
 const EMPTY_BYTES = '0x'
-const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies
+const TX_BASE_GAS = 21000
+const SEND_ETH_GAS = TX_BASE_GAS + 10000 // 10k limit on depositable proxies
+const SOLIDITY_TRANSFER_GAS = 2300
+const ISTANBUL_SLOAD_GAS_INCREASE = 600
 
-contract('Recovery to vault', ([permissionsRoot]) => {
-  let aclBase, appBase, appConditionalRecoveryBase
+contract('Recovery to vault', ([ permissionsRoot ]) => {
+  let aclBase, appBase, appConditionalRecoveryBase, ethSender
   let APP_ADDR_NAMESPACE, ETH
 
   // Helpers
+  const sendEth = async ({ target, value, gas = SOLIDITY_TRANSFER_GAS, shouldOOG }) => {
+    const initialBalance = await getBalance(target.address)
+
+    const useSenderContract = gas === SOLIDITY_TRANSFER_GAS
+    const sender = useSenderContract ? ethSender.address : permissionsRoot
+
+    const sendAction = () => useSenderContract
+      ? ethSender.sendEth(target.address, { value })
+      : target.sendTransaction({ gas: gas + TX_BASE_GAS, value })
+
+    if (shouldOOG) {
+      await assertOutOfGas(sendAction)
+      assert.equal((await getBalance(target.address)).valueOf(), initialBalance, 'Target balance should be the same as before')
+    } else {
+      const { tx } = await sendAction()
+      const receipt = await web3.eth.getTransactionReceipt(tx)
+
+      assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(value), 'Target balance should be correct')
+
+      const logs = decodeEventsOfType(receipt, DepositableDelegateProxy.abi, 'ProxyDeposit')
+      assertAmountOfEvents({ logs }, 'ProxyDeposit')
+      assertEvent({ logs }, 'ProxyDeposit', { sender: toChecksumAddress(sender), value })
+
+      return receipt
+    }
+  }
+
   const recoverEth = async ({ shouldFail, target, vault }) => {
     const amount = 1
     const initialBalance = await getBalance(target.address)
     const initialVaultBalance = await getBalance(vault.address)
-    await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS })
-    assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct')
+
+    await sendEth({ target, value: amount })
 
     const recoverAction = () => target.transferToVault(ETH)
 
@@ -123,6 +157,7 @@ contract('Recovery to vault', ([permissionsRoot]) => {
     aclBase = await ACL.new()
     appBase = await AppStubDepositable.new()
     appConditionalRecoveryBase = await AppStubConditionalRecovery.new()
+    ethSender = await EthSender.new()
 
     // Setup constants
     const kernel = await Kernel.new(true)
@@ -241,6 +276,20 @@ contract('Recovery to vault', ([permissionsRoot]) => {
               await assertRevert(target.sendTransaction({ value: 1, data: '0x01', gas: SEND_ETH_GAS }))
             })
 
+            it('can receive ETH (Constantinople)', async () => {
+              await sendEth({ target, value: 1 })
+            })
+
+            // TODO: Remove when the targetted EVM has been upgraded to Istanbul (EIP-1884)
+            it('can receive ETH (Istanbul, EIP-1884)', async () => {
+              const { gasUsed } = await sendEth({ target, value: 1, gas: SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE })
+              console.log('Used gas:', gasUsed)
+            })
+
+            it('OOGs if sending ETH with too little gas', async () => {
+              await sendEth({ target, value: 1, gas: SOLIDITY_TRANSFER_GAS - 850, shouldOOG: true })
+            })
+
             it('recovers ETH', async () =>
               await recoverEth({ target, vault })
             )
@@ -334,6 +383,20 @@ contract('Recovery to vault', ([permissionsRoot]) => {
       await kernel.setRecoveryVaultAppId(vaultId)
     })
 
+    it('can receive ETH (Constantinople)', async () => {
+      await sendEth({ target: kernel, value: 1 })
+    })
+
+    // TODO: Remove when the targetted EVM has been upgraded to Istanbul (EIP-1884)
+    it('can receive ETH (Istanbul, EIP-1884)', async () => {
+      const { gasUsed } = await sendEth({ target: kernel, value: 1, gas: SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE })
+      console.log('Used gas:', gasUsed)
+    })
+
+    it('OOGs if sending ETH with too little gas', async () => {
+      await sendEth({ target: kernel, value: 1, gas: SOLIDITY_TRANSFER_GAS - 850, shouldOOG: true })
+    })
+
     it('recovers ETH from the kernel', async () => {
       await recoverEth({ target: kernel, vault })
     })

test/helpers/assertThrow.js

@@ -24,6 +24,10 @@ module.exports = {
     return assertThrows(blockOrPromise, 'invalid opcode')
   },
 
+  async assertOutOfGas(blockOrPromise) {
+    return assertThrows(blockOrPromise, 'out of gas')
+  },
+
   async assertRevert(blockOrPromise, reason) {
     const error = await assertThrows(blockOrPromise, 'revert', reason)
     const errorPrefix = `${THROW_ERROR_PREFIX} revert`