Merge branch 'next' into 514_testing_improvements

contracts/common/DepositableDelegateProxy.sol

@@ -8,14 +8,37 @@ 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 {
+            // Continue only 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) {
+                // Only accept the deposit and emit an event if all of the following are true:
+                // the proxy accepts deposits (isDepositable), msg.data.length == 0, and msg.value > 0
+                if and(and(sload(isDepositablePosition), iszero(calldatasize)), gt(callvalue, 0)) {
+                    // Equivalent Solidity code for emitting the event:
+                    // emit ProxyDeposit(msg.sender, msg.value);
+
+                    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);
+    }
+}

contracts/test/helpers/ProxyTarget.sol

@@ -0,0 +1,17 @@
+pragma solidity 0.4.24;
+
+contract ProxyTargetWithoutFallback {
+    event Pong();
+
+    function ping() external {
+      emit Pong();
+    }
+}
+
+contract ProxyTargetWithFallback is ProxyTargetWithoutFallback {
+    event ReceivedEth();
+
+    function () external payable {
+      emit ReceivedEth();
+    }
+}

contracts/test/mocks/apps/DepositableDelegateProxyMock.sol

@@ -0,0 +1,24 @@
+pragma solidity 0.4.24;
+
+import "../../../common/DepositableDelegateProxy.sol";
+
+
+contract DepositableDelegateProxyMock is DepositableDelegateProxy {
+    address private implementationMock;
+
+    function enableDepositsOnMock() external {
+        setDepositable(true);
+    }
+
+    function setImplementationOnMock(address _implementationMock) external {
+        implementationMock = _implementationMock;
+    }
+
+    function implementation() public view returns (address) {
+        return implementationMock;
+    }
+
+    function proxyType() public pure returns (uint256 proxyTypeId) {
+        return UPGRADEABLE;
+    }
+}

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@aragon/os",
-  "version": "4.2.1",
+  "version": "4.3.0",
   "description": "Core contracts for Aragon",
   "scripts": {
     "compile": "truffle compile",

test/contracts/apps/app_funds.js

@@ -17,7 +17,8 @@ const UnsafeAppStubDepositable = artifacts.require('UnsafeAppStubDepositable')
 
 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 + 9999 // <10k gas is the threshold for depositing
 
 contract('App funds', ([permissionsRoot]) => {
   let aclBase, kernelBase

test/contracts/apps/recovery_to_vault.js

@@ -20,7 +20,8 @@ const KernelDepositableMock = artifacts.require('KernelDepositableMock')
 
 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 + 9999 // <10k gas is the threshold for depositing
 
 contract('Recovery to vault', ([permissionsRoot]) => {
   let aclBase, appBase, appConditionalRecoveryBase
@@ -338,4 +339,4 @@ contract('Recovery to vault', ([permissionsRoot]) => {
       await recoverEth({ target: kernel, vault })
     })
   })
-})
+})

test/contracts/common/depositable_delegate_proxy.js

@@ -0,0 +1,173 @@
+const { toChecksumAddress } = require('web3-utils')
+const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3)
+const { decodeEventsOfType } = require('../../helpers/decodeEvent')
+const { assertRevert, assertOutOfGas } = require('../../helpers/assertThrow')
+const { getBalance } = require('../../helpers/web3')
+
+// Mocks
+const DepositableDelegateProxyMock = artifacts.require('DepositableDelegateProxyMock')
+const EthSender = artifacts.require('EthSender')
+const ProxyTargetWithoutFallback = artifacts.require('ProxyTargetWithoutFallback')
+const ProxyTargetWithFallback = artifacts.require('ProxyTargetWithFallback')
+
+const TX_BASE_GAS = 21000
+const SEND_ETH_GAS = TX_BASE_GAS + 9999 // <10k gas is the threshold for depositing
+const PROXY_FORWARD_GAS = TX_BASE_GAS + 2e6 // high gas amount to ensure that the proxy forwards the call
+const FALLBACK_SETUP_GAS = process.env.SOLIDITY_COVERAGE ? 5000 : 100 // rough estimation of how much gas it spends before executing the fallback code
+const SOLIDITY_TRANSFER_GAS = 2300
+const ISTANBUL_SLOAD_GAS_INCREASE = 600
+
+contract('DepositableDelegateProxy', ([ sender ]) => {
+  let ethSender, proxy, target, proxyTargetWithoutFallbackBase, proxyTargetWithFallbackBase
+
+  // Initial setup
+  before(async () => {
+    ethSender = await EthSender.new()
+    proxyTargetWithoutFallbackBase = await ProxyTargetWithoutFallback.new()
+    proxyTargetWithFallbackBase = await ProxyTargetWithFallback.new()
+  })
+
+  beforeEach(async () => {
+    proxy = await DepositableDelegateProxyMock.new()
+    target = ProxyTargetWithFallback.at(proxy.address)
+  })
+
+  const itForwardsToImplementationIfGasIsOverThreshold = () => {
+    context('when implementation address is set', () => {
+      const itSuccessfullyForwardsCall = () => {
+        it('forwards call with data', async () => {
+          const receipt = await target.ping({ gas: PROXY_FORWARD_GAS })
+          assertAmountOfEvents(receipt, 'Pong')
+        })
+      }
+
+      context('when implementation has a fallback', () => {
+        beforeEach(async () => {
+          await proxy.setImplementationOnMock(proxyTargetWithFallbackBase.address)
+        })
+
+        itSuccessfullyForwardsCall()
+
+        it('can receive ETH', async () => {
+          const receipt = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS + FALLBACK_SETUP_GAS })
+          assertAmountOfEvents(receipt, 'ReceivedEth')
+        })
+      })
+
+      context('when implementation doesn\'t have a fallback', () => {
+        beforeEach(async () => {
+          await proxy.setImplementationOnMock(proxyTargetWithoutFallbackBase.address)
+        })
+
+        itSuccessfullyForwardsCall()
+
+        it('reverts when sending ETH', async () => {
+          await assertRevert(target.sendTransaction({ value: 1, gas: PROXY_FORWARD_GAS }))
+        })
+      })
+    })
+
+    context('when implementation address is not set', () => {
+      it('reverts when a function is called', async () => {
+        await assertRevert(target.ping({ gas: PROXY_FORWARD_GAS }))
+      })
+
+      it('reverts when sending ETH', async () => {
+        await assertRevert(target.sendTransaction({ value: 1, gas: PROXY_FORWARD_GAS }))
+      })
+    })
+  }
+
+  const itRevertsOnInvalidDeposits = () => {
+    it('reverts when call has data', async () => {
+      await proxy.setImplementationOnMock(proxyTargetWithoutFallbackBase.address)
+
+      await assertRevert(target.ping({ gas: SEND_ETH_GAS }))
+    })
+
+    it('reverts when call sends 0 value', async () => {
+      await assertRevert(proxy.sendTransaction({ value: 0, gas: SEND_ETH_GAS }))
+    })
+  }
+
+  context('when proxy is set as depositable', () => {
+    beforeEach(async () => {
+      await proxy.enableDepositsOnMock()
+    })
+
+    context('when call gas is below the forwarding threshold', () => {
+      const value = 100
+
+      const assertSendEthToProxy = async ({ value, gas, shouldOOG }) => {
+        const initialBalance = await getBalance(proxy.address)
+
+        const sendEthAction = () => proxy.sendTransaction({ from: sender, gas, value })
+
+        if (shouldOOG) {
+          await assertOutOfGas(sendEthAction())
+          assert.equal((await getBalance(proxy.address)).valueOf(), initialBalance, 'Target balance should be the same as before')
+        } else {
+          const { receipt, logs } = await sendEthAction()
+
+          assert.equal((await getBalance(proxy.address)).valueOf(), initialBalance.plus(value), 'Target balance should be correct')
+          assertAmountOfEvents({ logs }, 'ProxyDeposit')
+          assertEvent({ logs }, 'ProxyDeposit', { sender, value })
+
+          return receipt
+        }
+      }
+
+      it('can receive ETH (Constantinople)', async () => {
+        const { gasUsed } = await assertSendEthToProxy({ value, gas: SEND_ETH_GAS })
+        console.log('Used gas:', gasUsed - TX_BASE_GAS)
+      })
+
+      // TODO: Remove when the targetted EVM has been upgraded to Istanbul (EIP-1884)
+      it('can receive ETH (Istanbul, EIP-1884)', async () => {
+        const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE
+        const { gasUsed } = await assertSendEthToProxy({ value, gas })
+        const gasUsedIstanbul = gasUsed - TX_BASE_GAS + ISTANBUL_SLOAD_GAS_INCREASE
+        console.log('Used gas (Istanbul):', gasUsedIstanbul)
+
+        assert.isBelow(gasUsedIstanbul, 2300, 'Gas cost under Istanbul cannot be above 2300 gas')
+      })
+
+      // TODO: Remove when the targetted EVM has been upgraded to Istanbul (EIP-1884)
+      it('cannot receive ETH if sent with a small amount of gas', async () => {
+        // solidity-coverage seems to be increasing the gas amount to prevent failures
+        const oogDecrease = process.env.SOLIDITY_COVERAGE ? 600 : 250
+        // deposit cannot be done with this amount of gas
+        const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE - oogDecrease
+        await assertSendEthToProxy({ shouldOOG: true, value, gas })
+      })
+
+      it('can receive ETH from contract', async () => {
+        const { tx } = await ethSender.sendEth(proxy.address, { value })
+        const receipt = await web3.eth.getTransactionReceipt(tx)
+        const logs = decodeEventsOfType(receipt, DepositableDelegateProxyMock.abi, 'ProxyDeposit')
+        assertAmountOfEvents({ logs }, 'ProxyDeposit')
+        assertEvent({ logs }, 'ProxyDeposit', { sender: toChecksumAddress(ethSender.address), value })
+      })
+
+      itRevertsOnInvalidDeposits()
+    })
+
+    context('when call gas is over forwarding threshold', () => {
+      itForwardsToImplementationIfGasIsOverThreshold()
+    })
+  })
+
+  context('when proxy is not set as depositable', () => {
+    context('when call gas is below the forwarding threshold', () => {
+      it('reverts when depositing ETH', async () => {
+        await assertRevert(proxy.sendTransaction({ value: 1, gas: SEND_ETH_GAS }))
+      })
+
+      itRevertsOnInvalidDeposits()
+    })
+
+    context('when call gas is over forwarding threshold', () => {
+      itForwardsToImplementationIfGasIsOverThreshold()
+    })
+  })
+})

test/contracts/kernel/kernel_funds.js

@@ -9,7 +9,8 @@ const KernelProxy = artifacts.require('KernelProxy')
 // Mocks
 const KernelDepositableMock = artifacts.require('KernelDepositableMock')
 
-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 + 9999 // <10k gas is the threshold for depositing
 
 contract('Kernel funds', ([permissionsRoot]) => {
   let aclBase

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`