diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol index eda7ca5a7..d7291859d 100644 --- a/contracts/common/DepositableDelegateProxy.sol +++ b/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); } } 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/contracts/test/helpers/ProxyTarget.sol b/contracts/test/helpers/ProxyTarget.sol new file mode 100644 index 000000000..177fb65c9 --- /dev/null +++ b/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(); + } +} \ 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/package.json b/package.json index 31f20a3bb..fe7fcd5d3 100644 --- a/package.json +++ b/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", 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 626bc67de..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 @@ -338,4 +339,4 @@ contract('Recovery to vault', ([permissionsRoot]) => { await recoverEth({ target: kernel, vault }) }) }) -}) +}) \ No newline at end of file diff --git a/test/contracts/common/depositable_delegate_proxy.js b/test/contracts/common/depositable_delegate_proxy.js new file mode 100644 index 000000000..c05231b3a --- /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 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() + }) + }) +}) \ No newline at end of file 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 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`