From 29d2986fa48f5a24fbc399d352e0f366e8e5b836 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Tue, 27 Aug 2019 23:50:32 +0200 Subject: [PATCH 1/9] DepositableDelegateProxy: optimize for EIP-1884 --- contracts/common/DepositableDelegateProxy.sol | 36 +++++++-- contracts/test/helpers/EthSender.sol | 8 ++ test/contracts/apps/recovery_to_vault.js | 75 +++++++++++++++++-- test/helpers/assertThrow.js | 4 + 4 files changed, 109 insertions(+), 14 deletions(-) create mode 100644 contracts/test/helpers/EthSender.sol diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol index eda7ca5a7..774481f30 100644 --- a/contracts/common/DepositableDelegateProxy.sol +++ b/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); } } diff --git a/contracts/test/helpers/EthSender.sol b/contracts/test/helpers/EthSender.sol new file mode 100644 index 000000000..c9ee9e78f --- /dev/null +++ b/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); + } +} \ No newline at end of file diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js index 626bc67de..971d362f8 100644 --- a/test/contracts/apps/recovery_to_vault.js +++ b/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 }) }) diff --git a/test/helpers/assertThrow.js b/test/helpers/assertThrow.js index e70f6c26e..d13dc842a 100644 --- a/test/helpers/assertThrow.js +++ b/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` From 03e6e9ecf3acc6524efba731e474d97d48469a3e Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 28 Aug 2019 12:57:38 +0200 Subject: [PATCH 2/9] Update contracts/common/DepositableDelegateProxy.sol Co-Authored-By: Brett Sun --- contracts/common/DepositableDelegateProxy.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol index 774481f30..80a266672 100644 --- a/contracts/common/DepositableDelegateProxy.sol +++ b/contracts/common/DepositableDelegateProxy.sol @@ -14,7 +14,7 @@ contract DepositableDelegateProxy is DepositableStorage, DelegateProxy { // 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, + // 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) { // If the proxy accepts deposits, msg.data.length == 0 and msg.value > 0, From aa40a2fbca0b0000287ec2bfb487c147aad02a51 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 28 Aug 2019 13:01:46 +0200 Subject: [PATCH 3/9] Clarify comment --- contracts/common/DepositableDelegateProxy.sol | 4 ++-- test/contracts/apps/recovery_to_vault.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol index 80a266672..14b543906 100644 --- a/contracts/common/DepositableDelegateProxy.sol +++ b/contracts/common/DepositableDelegateProxy.sol @@ -17,8 +17,8 @@ contract DepositableDelegateProxy is DepositableStorage, DelegateProxy { // 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) { - // If the proxy accepts deposits, msg.data.length == 0 and msg.value > 0, - // accept the deposit and emit an event + // 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)) { let logData := mload(0x40) // free memory pointer mstore(logData, caller) // add 'msg.sender' to the log data (first event param) diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js index 971d362f8..cd26e2580 100644 --- a/test/contracts/apps/recovery_to_vault.js +++ b/test/contracts/apps/recovery_to_vault.js @@ -283,7 +283,7 @@ contract('Recovery to vault', ([ permissionsRoot ]) => { // 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) + console.log('Used gas:', gasUsed - TX_BASE_GAS) }) it('OOGs if sending ETH with too little gas', async () => { @@ -390,7 +390,7 @@ contract('Recovery to vault', ([ permissionsRoot ]) => { // 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) + console.log('Used gas:', gasUsed - TX_BASE_GAS) }) it('OOGs if sending ETH with too little gas', async () => { From 6cd3c55805d1337cc2e67fbcae2cff3cdee78c5e Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 28 Aug 2019 13:02:59 +0200 Subject: [PATCH 4/9] Revert changes in recover_to_vault tests --- test/contracts/apps/recovery_to_vault.js | 77 +++--------------------- 1 file changed, 7 insertions(+), 70 deletions(-) diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js index cd26e2580..6d6cdb520 100644 --- a/test/contracts/apps/recovery_to_vault.js +++ b/test/contracts/apps/recovery_to_vault.js @@ -1,15 +1,12 @@ const { hash } = require('eth-ens-namehash') -const { toChecksumAddress } = require('web3-utils') const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3) -const { assertRevert, assertOutOfGas } = require('../../helpers/assertThrow') +const { assertRevert } = 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') @@ -20,53 +17,22 @@ 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 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 +const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies -contract('Recovery to vault', ([ permissionsRoot ]) => { - let aclBase, appBase, appConditionalRecoveryBase, ethSender +contract('Recovery to vault', ([permissionsRoot]) => { + let aclBase, appBase, appConditionalRecoveryBase 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 sendEth({ target, value: amount }) + 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') const recoverAction = () => target.transferToVault(ETH) @@ -157,7 +123,6 @@ 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) @@ -276,20 +241,6 @@ 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 - TX_BASE_GAS) - }) - - 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 }) ) @@ -383,22 +334,8 @@ 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 - TX_BASE_GAS) - }) - - 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 }) }) }) -}) +}) \ No newline at end of file From 67a7ef40eddae0c3f4ae0b6dea7debee72ce67b1 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 28 Aug 2019 13:21:17 +0200 Subject: [PATCH 5/9] Add DepositableDelegateProxy tests --- contracts/test/helpers/ProxyTarget.sol | 17 ++ .../apps/DepositableDelegateProxyMock.sol | 24 +++ .../common/depositable_delegate_proxy.js | 173 ++++++++++++++++++ 3 files changed, 214 insertions(+) create mode 100644 contracts/test/helpers/ProxyTarget.sol create mode 100644 contracts/test/mocks/apps/DepositableDelegateProxyMock.sol create mode 100644 test/contracts/common/depositable_delegate_proxy.js diff --git a/contracts/test/helpers/ProxyTarget.sol b/contracts/test/helpers/ProxyTarget.sol new file mode 100644 index 000000000..33106aed5 --- /dev/null +++ b/contracts/test/helpers/ProxyTarget.sol @@ -0,0 +1,17 @@ +pragma solidity 0.4.24; + +contract ProxyTarget { + event Pong(); + + function ping() external { + emit Pong(); + } +} + +contract ProxyTargetWithFallback is ProxyTarget { + event ReceivedEth(); + + function () external payable { + emit ReceivedEth(); + } +} \ No newline at end of file diff --git a/contracts/test/mocks/apps/DepositableDelegateProxyMock.sol b/contracts/test/mocks/apps/DepositableDelegateProxyMock.sol new file mode 100644 index 000000000..c6365dfe9 --- /dev/null +++ b/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; + } +} diff --git a/test/contracts/common/depositable_delegate_proxy.js b/test/contracts/common/depositable_delegate_proxy.js new file mode 100644 index 000000000..35bac043a --- /dev/null +++ b/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 ProxyTarget = artifacts.require('ProxyTarget') +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 FALLBACK_SETUP_GAS = 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, proxyTargetBase, proxyTargetWithFallbackBase + + // Initial setup + before(async () => { + ethSender = await EthSender.new() + proxyTargetBase = await ProxyTarget.new() + proxyTargetWithFallbackBase = await ProxyTargetWithFallback.new() + }) + + beforeEach(async () => { + proxy = await DepositableDelegateProxyMock.new() + }) + + const itForwardsToImplementationIfGasIsOverThreshold = () => { + let target + + beforeEach(() => { + target = ProxyTargetWithFallback.at(proxy.address) + }) + + context('when implementation address is set', () => { + const itSuccessfullyForwardsCall = () => { + it('forwards call with data', async () => { + const receipt = await target.ping() + 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(proxyTargetBase.address) + }) + + itSuccessfullyForwardsCall() + + it('reverts when sending ETH', async () => { + await assertRevert(target.sendTransaction({ value: 1 })) + }) + }) + }) + + context('when implementation address is not set', () => { + it('reverts when a function is called', async () => { + await assertRevert(target.ping()) + }) + + it('reverts when sending ETH', async () => { + await assertRevert(target.sendTransaction({ value: 1 })) + }) + }) + } + + const itRevertsOnInvalidDeposits = () => { + it('reverts when call has data', async () => { + await assertRevert(proxy.sendTransaction({ value: 1, data: '0x01', 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 sendEthToProxy = 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 sendEthToProxy({ 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 sendEthToProxy({ 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 () => { + // deposit cannot be done with this amount of gas + const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE - 250 + await sendEthToProxy({ 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() + }) + }) +}) \ No newline at end of file From 95c9b5400d7553e45a7ab228b258f4c6a4c4bae1 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 28 Aug 2019 16:31:52 +0200 Subject: [PATCH 6/9] Address review comments --- contracts/test/helpers/ProxyTarget.sol | 4 +-- .../common/depositable_delegate_proxy.js | 31 +++++++++---------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/contracts/test/helpers/ProxyTarget.sol b/contracts/test/helpers/ProxyTarget.sol index 33106aed5..177fb65c9 100644 --- a/contracts/test/helpers/ProxyTarget.sol +++ b/contracts/test/helpers/ProxyTarget.sol @@ -1,6 +1,6 @@ pragma solidity 0.4.24; -contract ProxyTarget { +contract ProxyTargetWithoutFallback { event Pong(); function ping() external { @@ -8,7 +8,7 @@ contract ProxyTarget { } } -contract ProxyTargetWithFallback is ProxyTarget { +contract ProxyTargetWithFallback is ProxyTargetWithoutFallback { event ReceivedEth(); function () external payable { diff --git a/test/contracts/common/depositable_delegate_proxy.js b/test/contracts/common/depositable_delegate_proxy.js index 35bac043a..2f996fee6 100644 --- a/test/contracts/common/depositable_delegate_proxy.js +++ b/test/contracts/common/depositable_delegate_proxy.js @@ -7,36 +7,31 @@ const { getBalance } = require('../../helpers/web3') // Mocks const DepositableDelegateProxyMock = artifacts.require('DepositableDelegateProxyMock') const EthSender = artifacts.require('EthSender') -const ProxyTarget = artifacts.require('ProxyTarget') +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 FALLBACK_SETUP_GAS = 100 // rough estimation of how much gas it spends before executing the fallback code +const FALLBACK_SETUP_GAS = 300 // rough estimation of how much gas it spends before executing the fallback code (increased for coverage) const SOLIDITY_TRANSFER_GAS = 2300 const ISTANBUL_SLOAD_GAS_INCREASE = 600 -contract('DepositableDelegateProxy', ([ sender ]) => { - let ethSender, proxy, proxyTargetBase, proxyTargetWithFallbackBase +contract.only('DepositableDelegateProxy', ([ sender ]) => { + let ethSender, proxy, target, proxyTargetWithoutFallbackBase, proxyTargetWithFallbackBase // Initial setup before(async () => { ethSender = await EthSender.new() - proxyTargetBase = await ProxyTarget.new() + proxyTargetWithoutFallbackBase = await ProxyTargetWithoutFallback.new() proxyTargetWithFallbackBase = await ProxyTargetWithFallback.new() }) beforeEach(async () => { proxy = await DepositableDelegateProxyMock.new() + target = ProxyTargetWithFallback.at(proxy.address) }) const itForwardsToImplementationIfGasIsOverThreshold = () => { - let target - - beforeEach(() => { - target = ProxyTargetWithFallback.at(proxy.address) - }) - context('when implementation address is set', () => { const itSuccessfullyForwardsCall = () => { it('forwards call with data', async () => { @@ -60,7 +55,7 @@ contract('DepositableDelegateProxy', ([ sender ]) => { context('when implementation doesn\'t have a fallback', () => { beforeEach(async () => { - await proxy.setImplementationOnMock(proxyTargetBase.address) + await proxy.setImplementationOnMock(proxyTargetWithoutFallbackBase.address) }) itSuccessfullyForwardsCall() @@ -84,7 +79,9 @@ contract('DepositableDelegateProxy', ([ sender ]) => { const itRevertsOnInvalidDeposits = () => { it('reverts when call has data', async () => { - await assertRevert(proxy.sendTransaction({ value: 1, data: '0x01', gas: SEND_ETH_GAS })) + await proxy.setImplementationOnMock(proxyTargetWithoutFallbackBase.address) + + await assertRevert(target.ping({ gas: SEND_ETH_GAS })) }) it('reverts when call sends 0 value', async () => { @@ -100,7 +97,7 @@ contract('DepositableDelegateProxy', ([ sender ]) => { context('when call gas is below the forwarding threshold', () => { const value = 100 - const sendEthToProxy = async ({ value, gas, shouldOOG }) => { + const assertSendEthToProxy = async ({ value, gas, shouldOOG }) => { const initialBalance = await getBalance(proxy.address) const sendEthAction = () => proxy.sendTransaction({ from: sender, gas, value }) @@ -120,14 +117,14 @@ contract('DepositableDelegateProxy', ([ sender ]) => { } it('can receive ETH (Constantinople)', async () => { - const { gasUsed } = await sendEthToProxy({ value, gas: SEND_ETH_GAS }) + 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 sendEthToProxy({ value, gas }) + const { gasUsed } = await assertSendEthToProxy({ value, gas }) const gasUsedIstanbul = gasUsed - TX_BASE_GAS + ISTANBUL_SLOAD_GAS_INCREASE console.log('Used gas (Istanbul):', gasUsedIstanbul) @@ -138,7 +135,7 @@ contract('DepositableDelegateProxy', ([ sender ]) => { it('cannot receive ETH if sent with a small amount of gas', async () => { // deposit cannot be done with this amount of gas const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE - 250 - await sendEthToProxy({ shouldOOG: true, value, gas }) + await assertSendEthToProxy({ shouldOOG: true, value, gas }) }) it('can receive ETH from contract', async () => { From 19756078c111e3ee4a7d06aab39a7ceacc919c61 Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Wed, 28 Aug 2019 18:12:37 +0200 Subject: [PATCH 7/9] Fix coverage --- test/contracts/common/depositable_delegate_proxy.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/contracts/common/depositable_delegate_proxy.js b/test/contracts/common/depositable_delegate_proxy.js index 2f996fee6..44bd6ce94 100644 --- a/test/contracts/common/depositable_delegate_proxy.js +++ b/test/contracts/common/depositable_delegate_proxy.js @@ -12,11 +12,11 @@ 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 FALLBACK_SETUP_GAS = 300 // rough estimation of how much gas it spends before executing the fallback code (increased for coverage) +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.only('DepositableDelegateProxy', ([ sender ]) => { +contract('DepositableDelegateProxy', ([ sender ]) => { let ethSender, proxy, target, proxyTargetWithoutFallbackBase, proxyTargetWithFallbackBase // Initial setup @@ -133,8 +133,10 @@ contract.only('DepositableDelegateProxy', ([ sender ]) => { // 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 - 250 + const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE - oogDecrease await assertSendEthToProxy({ shouldOOG: true, value, gas }) }) From 948aceafaf6444dafa0823f09db8881004666eaf Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Mon, 2 Sep 2019 20:39:24 +0200 Subject: [PATCH 8/9] Address review comments --- contracts/common/DepositableDelegateProxy.sol | 3 +++ test/contracts/apps/app_funds.js | 3 ++- test/contracts/apps/recovery_to_vault.js | 3 ++- test/contracts/common/depositable_delegate_proxy.js | 11 ++++++----- test/contracts/kernel/kernel_funds.js | 3 ++- 5 files changed, 15 insertions(+), 8 deletions(-) diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol index 14b543906..d7291859d 100644 --- a/contracts/common/DepositableDelegateProxy.sol +++ b/contracts/common/DepositableDelegateProxy.sol @@ -20,6 +20,9 @@ contract DepositableDelegateProxy is DepositableStorage, DelegateProxy { // 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) diff --git a/test/contracts/apps/app_funds.js b/test/contracts/apps/app_funds.js index f94433287..df4f0b8db 100644 --- a/test/contracts/apps/app_funds.js +++ b/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 diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js index 6d6cdb520..d913260ad 100644 --- a/test/contracts/apps/recovery_to_vault.js +++ b/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 diff --git a/test/contracts/common/depositable_delegate_proxy.js b/test/contracts/common/depositable_delegate_proxy.js index 44bd6ce94..c05231b3a 100644 --- a/test/contracts/common/depositable_delegate_proxy.js +++ b/test/contracts/common/depositable_delegate_proxy.js @@ -11,7 +11,8 @@ 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 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 @@ -35,7 +36,7 @@ contract('DepositableDelegateProxy', ([ sender ]) => { context('when implementation address is set', () => { const itSuccessfullyForwardsCall = () => { it('forwards call with data', async () => { - const receipt = await target.ping() + const receipt = await target.ping({ gas: PROXY_FORWARD_GAS }) assertAmountOfEvents(receipt, 'Pong') }) } @@ -61,18 +62,18 @@ contract('DepositableDelegateProxy', ([ sender ]) => { itSuccessfullyForwardsCall() it('reverts when sending ETH', async () => { - await assertRevert(target.sendTransaction({ value: 1 })) + 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()) + await assertRevert(target.ping({ gas: PROXY_FORWARD_GAS })) }) it('reverts when sending ETH', async () => { - await assertRevert(target.sendTransaction({ value: 1 })) + await assertRevert(target.sendTransaction({ value: 1, gas: PROXY_FORWARD_GAS })) }) }) } diff --git a/test/contracts/kernel/kernel_funds.js b/test/contracts/kernel/kernel_funds.js index de80d198e..e6e4065fb 100644 --- a/test/contracts/kernel/kernel_funds.js +++ b/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 From d5bdcb0781a7b60ca4d175a15d5dc1a685a3673a Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Tue, 3 Sep 2019 12:30:20 +0200 Subject: [PATCH 9/9] chore: pin solidity-coverage to v0.6.2 (#552) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 36cceadc2..514f2aa19 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "ethereumjs-abi": "^0.6.5", "ganache-cli": "^6.4.2", "mocha-lcov-reporter": "^1.3.0", - "solidity-coverage": "^0.6.2", + "solidity-coverage": "0.6.2", "solium": "^1.2.3", "truffle": "4.1.14", "truffle-bytecode-manager": "^1.1.1",