diff --git a/contracts/apps/AppStorage.sol b/contracts/apps/AppStorage.sol index ed0cec69b..8c258b4cb 100644 --- a/contracts/apps/AppStorage.sol +++ b/contracts/apps/AppStorage.sol @@ -15,11 +15,9 @@ contract AppStorage { * Hardcoded constants to save gas * bytes32 internal constant KERNEL_POSITION = keccak256("aragonOS.appStorage.kernel"); * bytes32 internal constant APP_ID_POSITION = keccak256("aragonOS.appStorage.appId"); - * bytes32 internal constant VOLATILE_SENDER_POSITION = keccak256("aragonOS.appStorage.volatile.sender"); */ bytes32 internal constant KERNEL_POSITION = 0x4172f0f7d2289153072b0a6ca36959e0cbe2efc3afe50fc81636caa96338137b; bytes32 internal constant APP_ID_POSITION = 0xd625496217aa6a3453eecb9c3489dc5a53e6c67b444329ea2b2cbc9ff547639b; - bytes32 internal constant VOLATILE_SENDER_POSITION = 0xd6486d5aa3dac4242db35dd7559408452252cf8050988dbc66956eaa315379ce; function kernel() public view returns (IKernel) { return IKernel(KERNEL_POSITION.getStorageAddress()); @@ -29,10 +27,6 @@ contract AppStorage { return APP_ID_POSITION.getStorageBytes32(); } - function volatileStorageSender() public view returns (address) { - return VOLATILE_SENDER_POSITION.getStorageAddress(); - } - function setKernel(IKernel _kernel) internal { KERNEL_POSITION.setStorageAddress(address(_kernel)); } @@ -40,8 +34,4 @@ contract AppStorage { function setAppId(bytes32 _appId) internal { APP_ID_POSITION.setStorageBytes32(_appId); } - - function setVolatileStorageSender(address _sender) internal { - VOLATILE_SENDER_POSITION.setStorageAddress(_sender); - } } diff --git a/contracts/common/MemoryHelpers.sol b/contracts/common/MemoryHelpers.sol new file mode 100644 index 000000000..98ecd6872 --- /dev/null +++ b/contracts/common/MemoryHelpers.sol @@ -0,0 +1,49 @@ +pragma solidity ^0.4.24; + + +library MemoryHelpers { + + function append(bytes memory self, address addr) internal pure returns (bytes memory) { + // alloc required encoded data size + uint256 dataSize = self.length; + uint256 appendedDataSize = dataSize + 32; + bytes memory appendedData = new bytes(appendedDataSize); + + // copy data + uint256 inputPointer; + uint256 outputPointer; + assembly { + inputPointer := add(self, 0x20) + outputPointer := add(appendedData, 0x20) + } + memcpy(outputPointer, inputPointer, dataSize); + + // append address + assembly { + let signerPointer := add(add(appendedData, 0x20), dataSize) + mstore(signerPointer, addr) + } + + return appendedData; + } + + // From https://github.com/Arachnid/solidity-stringutils/blob/master/src/strings.sol + function memcpy(uint256 dest, uint256 src, uint256 len) private pure { + // Copy word-length chunks while possible + for(; len >= 32; len -= 32) { + assembly { + mstore(dest, mload(src)) + } + dest += 32; + src += 32; + } + + // Copy remaining bytes + uint256 mask = 256 ** (32 - len) - 1; + assembly { + let srcpart := and(mload(src), not(mask)) + let destpart := and(mload(dest), mask) + mstore(dest, or(destpart, srcpart)) + } + } +} diff --git a/contracts/kernel/IKernel.sol b/contracts/kernel/IKernel.sol index e1a2b40e5..95b7d3955 100644 --- a/contracts/kernel/IKernel.sol +++ b/contracts/kernel/IKernel.sol @@ -5,6 +5,7 @@ pragma solidity ^0.4.24; import "../acl/IACL.sol"; +import "../relayer/IRelayer.sol"; import "../common/IVaultRecoverable.sol"; @@ -16,6 +17,7 @@ interface IKernelEvents { // This should be an interface, but interfaces can't inherit yet :( contract IKernel is IKernelEvents, IVaultRecoverable { function acl() public view returns (IACL); + function relayer() public view returns (IRelayer); function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool); function setApp(bytes32 namespace, bytes32 appId, address app) public; diff --git a/contracts/kernel/Kernel.sol b/contracts/kernel/Kernel.sol index 1fc919055..eba6a716a 100644 --- a/contracts/kernel/Kernel.sol +++ b/contracts/kernel/Kernel.sol @@ -5,6 +5,7 @@ import "./KernelConstants.sol"; import "./KernelStorage.sol"; import "../acl/IACL.sol"; import "../acl/ACLSyntaxSugar.sol"; +import "../relayer/IRelayer.sol"; import "../common/ConversionHelpers.sol"; import "../common/IsContract.sol"; import "../common/Petrifiable.sol"; @@ -169,6 +170,7 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant function APP_ADDR_NAMESPACE() external pure returns (bytes32) { return KERNEL_APP_ADDR_NAMESPACE; } function KERNEL_APP_ID() external pure returns (bytes32) { return KERNEL_CORE_APP_ID; } function DEFAULT_ACL_APP_ID() external pure returns (bytes32) { return KERNEL_DEFAULT_ACL_APP_ID; } + function DEFAULT_RELAYER_APP_ID() external pure returns (bytes32) { return KERNEL_DEFAULT_RELAYER_APP_ID; } /* solium-enable function-order, mixedcase */ /** @@ -197,6 +199,14 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant return IACL(getApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_ACL_APP_ID)); } + /** + * @dev Get the installed Relayer app + * @return Relayer app + */ + function relayer() public view returns (IRelayer) { + return IRelayer(getApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_RELAYER_APP_ID)); + } + /** * @dev Function called by apps to check ACL on kernel or to check permission status * @param _who Sender of the original call diff --git a/contracts/kernel/KernelConstants.sol b/contracts/kernel/KernelConstants.sol index 77816a74c..462b35881 100644 --- a/contracts/kernel/KernelConstants.sol +++ b/contracts/kernel/KernelConstants.sol @@ -10,10 +10,12 @@ contract KernelAppIds { bytes32 internal constant KERNEL_CORE_APP_ID = apmNamehash("kernel"); bytes32 internal constant KERNEL_DEFAULT_ACL_APP_ID = apmNamehash("acl"); bytes32 internal constant KERNEL_DEFAULT_VAULT_APP_ID = apmNamehash("vault"); + bytes32 internal constant KERNEL_DEFAULT_VAULT_APP_ID = apmNamehash("relayer"); */ bytes32 internal constant KERNEL_CORE_APP_ID = 0x3b4bf6bf3ad5000ecf0f989d5befde585c6860fea3e574a4fab4c49d1c177d9c; bytes32 internal constant KERNEL_DEFAULT_ACL_APP_ID = 0xe3262375f45a6e2026b7e7b18c2b807434f2508fe1a2a3dfb493c7df8f4aad6a; bytes32 internal constant KERNEL_DEFAULT_VAULT_APP_ID = 0x7e852e0fcfce6551c13800f1e7476f982525c2b5277ba14b24339c68416336d1; + bytes32 internal constant KERNEL_DEFAULT_RELAYER_APP_ID = 0x7641595d1a2007abf0fe95c31d0b7a822954acbf6fb0cbe3bd1161d9dec9e1d3; } diff --git a/contracts/relayer/IRelayer.sol b/contracts/relayer/IRelayer.sol new file mode 100644 index 000000000..1a4aaa869 --- /dev/null +++ b/contracts/relayer/IRelayer.sol @@ -0,0 +1,6 @@ +pragma solidity ^0.4.24; + + +contract IRelayer { + function relay(address from, address to, uint256 nonce, bytes calldata, bytes signature) external; +} diff --git a/contracts/relayer/RelayedAragonApp.sol b/contracts/relayer/RelayedAragonApp.sol index bf0d4f138..4ccba686e 100644 --- a/contracts/relayer/RelayedAragonApp.sol +++ b/contracts/relayer/RelayedAragonApp.sol @@ -1,34 +1,25 @@ pragma solidity ^0.4.24; - +import "./IRelayer.sol"; import "../apps/AragonApp.sol"; -interface IRelayedAragonApp { - function exec(address from, bytes calldata) external; -} +contract RelayedAragonApp is AragonApp { -contract RelayedAragonApp is IRelayedAragonApp, AragonApp { - bytes32 public constant RELAYER_ROLE = keccak256("RELAYER_ROLE"); + function sender() internal view returns (address) { + address relayer = address(_relayer()); + if (msg.sender != relayer) return msg.sender; - function exec(address from, bytes calldata) external auth(RELAYER_ROLE) { - setVolatileStorageSender(from); - bool success = address(this).call(calldata); - if (!success) revertForwardingError(); - setVolatileStorageSender(address(0)); + address signer = _decodeSigner(); + return signer != address(0) ? signer : relayer; } - function sender() internal view returns (address) { - if (msg.sender != address(this)) return msg.sender; - address volatileSender = volatileStorageSender(); - return volatileSender != address(0) ? volatileSender : address(this); + function _decodeSigner() internal returns (address signer) { + bytes memory calldata = msg.data; + assembly { signer := mload(add(calldata, calldatasize)) } } - function revertForwardingError() private { - assembly { - let ptr := mload(0x40) - returndatacopy(ptr, 0, returndatasize) - revert(ptr, returndatasize) - } + function _relayer() internal returns (IRelayer) { + return kernel().relayer(); } } diff --git a/contracts/relayer/Relayer.sol b/contracts/relayer/Relayer.sol index c55107f2a..ced51ea78 100644 --- a/contracts/relayer/Relayer.sol +++ b/contracts/relayer/Relayer.sol @@ -1,13 +1,16 @@ pragma solidity ^0.4.24; +import "./IRelayer.sol"; import "./RelayedAragonApp.sol"; import "../lib/sig/ECDSA.sol"; import "../apps/AragonApp.sol"; +import "../common/MemoryHelpers.sol"; import "../common/DepositableStorage.sol"; -contract Relayer is AragonApp, DepositableStorage { +contract Relayer is IRelayer, AragonApp, DepositableStorage { using ECDSA for bytes32; + using MemoryHelpers for bytes; bytes32 public constant ALLOW_OFF_CHAIN_SERVICE_ROLE = keccak256("ALLOW_OFF_CHAIN_SERVICE_ROLE"); bytes32 public constant DISALLOW_OFF_CHAIN_SERVICE_ROLE = keccak256("DISALLOW_OFF_CHAIN_SERVICE_ROLE"); @@ -21,7 +24,7 @@ contract Relayer is AragonApp, DepositableStorage { event ServiceAllowed(address indexed service); event ServiceDisallowed(address indexed service); - event TransactionRelayed(address indexed from, address indexed to, uint256 nonce, bytes calldata); + event TransactionRelayed(address from, address to, uint256 nonce, bytes calldata); mapping (address => bool) internal allowedServices; mapping (address => uint256) internal lastUsedNonce; @@ -48,8 +51,9 @@ contract Relayer is AragonApp, DepositableStorage { assertValidTransaction(from, nonce, calldata, signature); lastUsedNonce[from] = nonce; - IRelayedAragonApp(to).exec(from, calldata); + relayCall(from, to, calldata); emit TransactionRelayed(from, to, nonce, calldata); + forwardReturnedData(); } function allowService(address service) external authP(ALLOW_OFF_CHAIN_SERVICE_ROLE, arr(service)) { @@ -84,4 +88,24 @@ contract Relayer is AragonApp, DepositableStorage { function messageHash(bytes calldata, uint256 nonce) internal pure returns (bytes32) { return keccak256(abi.encodePacked(keccak256(calldata), nonce)); } + + function relayCall(address from, address to, bytes calldata) private { + bytes memory encodedSignerCalldata = calldata.append(from); + assembly { + let success := call(gas, to, 0, add(encodedSignerCalldata, 0x20), mload(encodedSignerCalldata), 0, 0) + switch success case 0 { + let ptr := mload(0x40) + returndatacopy(ptr, 0, returndatasize) + revert(ptr, returndatasize) + } + } + } + + function forwardReturnedData() private { + assembly { + let ptr := mload(0x40) + returndatacopy(ptr, 0, returndatasize) + return(ptr, returndatasize) + } + } } diff --git a/contracts/test/mocks/common/KeccakConstants.sol b/contracts/test/mocks/common/KeccakConstants.sol index 6cc9a5bb5..2a279db9a 100644 --- a/contracts/test/mocks/common/KeccakConstants.sol +++ b/contracts/test/mocks/common/KeccakConstants.sol @@ -22,6 +22,7 @@ contract KeccakConstants { bytes32 public constant KERNEL_APP_ID = keccak256(abi.encodePacked(APM_NODE, keccak256("kernel"))); bytes32 public constant DEFAULT_ACL_APP_ID = keccak256(abi.encodePacked(APM_NODE, keccak256("acl"))); bytes32 public constant DEFAULT_VAULT_APP_ID = keccak256(abi.encodePacked(APM_NODE, keccak256("vault"))); + bytes32 public constant DEFAULT_RELAYER_APP_ID = keccak256(abi.encodePacked(APM_NODE, keccak256("relayer"))); // ACL bytes32 public constant CREATE_PERMISSIONS_ROLE = keccak256(abi.encodePacked("CREATE_PERMISSIONS_ROLE")); diff --git a/contracts/test/mocks/kernel/KernelConstantsMock.sol b/contracts/test/mocks/kernel/KernelConstantsMock.sol index 47c634848..74f38c365 100644 --- a/contracts/test/mocks/kernel/KernelConstantsMock.sol +++ b/contracts/test/mocks/kernel/KernelConstantsMock.sol @@ -12,4 +12,5 @@ contract KernelConstantsMock is Kernel { function getKernelAppId() external pure returns (bytes32) { return KERNEL_CORE_APP_ID; } function getDefaultACLAppId() external pure returns (bytes32) { return KERNEL_DEFAULT_ACL_APP_ID; } function getDefaultVaultAppId() external pure returns (bytes32) { return KERNEL_DEFAULT_VAULT_APP_ID; } + function getDefaultRelayerAppId() external pure returns (bytes32) { return KERNEL_DEFAULT_RELAYER_APP_ID; } } diff --git a/test/contracts/common/keccak_constants.js b/test/contracts/common/keccak_constants.js index 972bb9b03..8bcd14e32 100644 --- a/test/contracts/common/keccak_constants.js +++ b/test/contracts/common/keccak_constants.js @@ -27,6 +27,7 @@ contract('Constants', () => { assert.equal(await kernelConstants.getKernelAppId(), await keccakConstants.KERNEL_APP_ID(), "kernel app id doesn't match") assert.equal(await kernelConstants.getDefaultACLAppId(), await keccakConstants.DEFAULT_ACL_APP_ID(), "default ACL id doesn't match") assert.equal(await kernelConstants.getDefaultVaultAppId(), await keccakConstants.DEFAULT_VAULT_APP_ID(), "default vault id doesn't match") + assert.equal(await kernelConstants.getDefaultRelayerAppId(), await keccakConstants.DEFAULT_RELAYER_APP_ID(), "default relayer id doesn't match") assert.equal(await kernelConstants.getKernelCoreNamespace(), await keccakConstants.KERNEL_CORE_NAMESPACE(), "core namespace doesn't match") assert.equal(await kernelConstants.getKernelAppBasesNamespace(), await keccakConstants.KERNEL_APP_BASES_NAMESPACE(), "base namespace doesn't match") assert.equal(await kernelConstants.getKernelAppAddrNamespace(), await keccakConstants.KERNEL_APP_ADDR_NAMESPACE(), "app namespace doesn't match") diff --git a/test/contracts/relayer/relayer.js b/test/contracts/relayer/relayer.js index 789f8ea8e..7766b2b3d 100644 --- a/test/contracts/relayer/relayer.js +++ b/test/contracts/relayer/relayer.js @@ -1,4 +1,5 @@ const { sha3, soliditySha3 } = require('web3-utils') +const { assertRevert } = require('../../helpers/assertThrow') const { getEventArgument, getNewProxyAddress } = require('../../helpers/events') const ACL = artifacts.require('ACL') @@ -7,10 +8,10 @@ const Relayer = artifacts.require('Relayer') const DAOFactory = artifacts.require('DAOFactory') const SampleApp = artifacts.require('RelayedAppMock') -contract('VolatileRelayedApp', ([_, root, sender, vault, offChainRelayerService]) => { - let daoFactory, dao, acl, app, relayer, relayedTx, nonce = 1 +contract('RelayedApp', ([_, root, member, anyone, vault, offChainRelayerService]) => { + let daoFactory, dao, acl, app, relayer, relayedTx, nonce = 0 let kernelBase, aclBase, sampleAppBase, relayerBase - let WRITING_ROLE, APP_MANAGER_ROLE, RELAYER_ROLE, ALLOW_OFF_CHAIN_SERVICE_ROLE + let WRITING_ROLE, APP_MANAGER_ROLE, ALLOW_OFF_CHAIN_SERVICE_ROLE, RELAYER_APP_ID before('deploy base implementations', async () => { aclBase = await ACL.new() @@ -20,9 +21,9 @@ contract('VolatileRelayedApp', ([_, root, sender, vault, offChainRelayerService] daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, '0x0') }) - before('load roles', async () => { + before('load constants', async () => { + RELAYER_APP_ID = await kernelBase.DEFAULT_RELAYER_APP_ID() WRITING_ROLE = await sampleAppBase.WRITING_ROLE() - RELAYER_ROLE = await sampleAppBase.RELAYER_ROLE() APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() ALLOW_OFF_CHAIN_SERVICE_ROLE = await relayerBase.ALLOW_OFF_CHAIN_SERVICE_ROLE() }) @@ -36,7 +37,7 @@ contract('VolatileRelayedApp', ([_, root, sender, vault, offChainRelayerService] }) before('create relayer instance', async () => { - const receipt = await dao.newAppInstance('0x11111', relayerBase.address, '0x', false, { from: root }) + const receipt = await dao.newAppInstance(RELAYER_APP_ID, relayerBase.address, '0x', true, { from: root }) relayer = Relayer.at(getNewProxyAddress(receipt)) await relayer.initialize() @@ -51,32 +52,48 @@ contract('VolatileRelayedApp', ([_, root, sender, vault, offChainRelayerService] app = SampleApp.at(getNewProxyAddress(receipt)) await app.initialize() - await acl.createPermission(sender, app.address, WRITING_ROLE, root, { from: root }) - await acl.createPermission(relayer.address, app.address, RELAYER_ROLE, root, { from: root }) + await acl.createPermission(member, app.address, WRITING_ROLE, root, { from: root }) }) - beforeEach('relay transaction', async () => { - const calldata = app.contract.write.getData(10) - const messageHash = soliditySha3(sha3(calldata), nonce) - const signature = web3.eth.sign(sender, messageHash) + beforeEach('increment nonce', () => nonce++) - relayedTx = await relayer.relay(sender, app.address, nonce, calldata, signature, { from: offChainRelayerService }) - nonce++ - }) + context('when the sender is authorized', () => { + const sender = member + + beforeEach('relay transaction', async () => { + const calldata = app.contract.write.getData(10) + const messageHash = soliditySha3(sha3(calldata), nonce) + const signature = web3.eth.sign(sender, messageHash) + + relayedTx = await relayer.relay(sender, app.address, nonce, calldata, signature, { from: offChainRelayerService }) + }) + + it('relays transactions to app', async () => { + assert.equal((await app.read()).toString(), 10, 'app value does not match') + }) + + it('overloads a transaction with ~34k of gas', async () => { + const { receipt: { cumulativeGasUsed: relayedGasUsed } } = relayedTx + const { receipt: { cumulativeGasUsed: nonRelayerGasUsed } } = await app.write(10, { from: sender }) + + const gasOverload = relayedGasUsed - nonRelayerGasUsed + console.log('relayedGasUsed:', relayedGasUsed) + console.log('nonRelayerGasUsed:', nonRelayerGasUsed) + console.log('gasOverload:', gasOverload) - it('relays transactions to app', async () => { - assert.equal((await app.read()).toString(), 10, 'app value does not match') + assert.isBelow(gasOverload, 34000, 'relayed txs gas overload is higher than 34k') + }) }) - it('overloads a transaction with ~78k of gas', async () => { - const { receipt: { cumulativeGasUsed: relayedGasUsed } } = relayedTx - const { receipt: { cumulativeGasUsed: nonRelayerGasUsed } } = await app.write(10, { from: sender }) + context('when the sender is not authorized', () => { + const sender = anyone - const gasOverload = relayedGasUsed - nonRelayerGasUsed - console.log('relayedGasUsed:', relayedGasUsed) - console.log('nonRelayerGasUsed:', nonRelayerGasUsed) - console.log('gasOverload:', gasOverload) + it('reverts', async () => { + const calldata = app.contract.write.getData(10) + const messageHash = soliditySha3(sha3(calldata), nonce) + const signature = web3.eth.sign(sender, messageHash) - assert.isBelow(gasOverload, 78000, 'relayed txs gas overload is higher than 78k') + await assertRevert(relayer.relay(sender, app.address, nonce, calldata, signature, { from: offChainRelayerService }), 'APP_AUTH_FAIL') + }) }) })