From f006a6a2c64b4ae89c1dad927010ab632fb74fe6 Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Tue, 15 Oct 2019 15:49:19 +0200 Subject: [PATCH] Added events + required refactoring --- contracts/GnosisSafe.sol | 78 ++++++++++++++++++++++---- contracts/base/FallbackManager.sol | 5 ++ contracts/base/ModuleManager.sol | 6 ++ package.json | 2 +- test/createCall.js | 8 +-- test/gnosisSafeDeploymentViaCreate2.js | 2 +- test/multiSend.js | 10 ++-- test/utils/execution.js | 19 ++++--- 8 files changed, 102 insertions(+), 28 deletions(-) diff --git a/contracts/GnosisSafe.sol b/contracts/GnosisSafe.sol index a946eb041..8922bdce5 100644 --- a/contracts/GnosisSafe.sol +++ b/contracts/GnosisSafe.sol @@ -35,7 +35,21 @@ contract GnosisSafe //); bytes32 public constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca; - event ExecutionFailed(bytes32 txHash); + // TODO: what should be indexed (8 gas per data on non-indexed => 256 gas per word VS 375 gas indexed word) + event ApproveHash( + bytes32 indexed approvedHash, + address indexed owner + ); + event SignMsg( + bytes32 indexed msgHash + ); + event Execution( + bytes32 indexed txHash, bool indexed success, address indexed to, // topics + uint256 value, bytes data, Enum.Operation operation + ); + event ExecutionPayment( + address token, uint256 value + ); uint256 public nonce; bytes32 public domainSeparator; @@ -104,8 +118,32 @@ contract GnosisSafe bytes calldata signatures ) external - returns (bool success) + returns (bool) { + bytes32 txHash = checkTransaction( + to, value, data, operation, // Transaction info + safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, // Payment info + signatures + ); + require(gasleft() >= safeTxGas, "Not enough gas to execute safe transaction"); + return executeTransaction( + txHash, to, value, data, operation, // Transaction info + safeTxGas, baseGas, gasPrice, gasToken, refundReceiver // Payment info + ); + } + + function checkTransaction( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation, + uint256 safeTxGas, + uint256 baseGas, + uint256 gasPrice, + address gasToken, + address payable refundReceiver, + bytes memory signatures + ) private returns (bytes32 txHash) { bytes memory txHashData = encodeTransactionData( to, value, data, operation, // Transaction info safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, // Payment info @@ -113,16 +151,27 @@ contract GnosisSafe ); // Increase nonce and execute transaction. nonce++; - checkSignatures(keccak256(txHashData), txHashData, signatures, true); - require(gasleft() >= safeTxGas, "Not enough gas to execute safe transaction"); + txHash = keccak256(txHashData); + checkSignatures(txHash, txHashData, signatures, true); + } + + function executeTransaction( + bytes32 txHash, + address to, + uint256 value, + bytes memory data, + Enum.Operation operation, + uint256 safeTxGas, + uint256 baseGas, + uint256 gasPrice, + address gasToken, + address payable refundReceiver + ) private returns (bool success) { uint256 gasUsed = gasleft(); // If no safeTxGas has been set and the gasPrice is 0 we assume that all available gas can be used success = execute(to, value, data, operation, safeTxGas == 0 && gasPrice == 0 ? gasleft() : safeTxGas); gasUsed = gasUsed.sub(gasleft()); - if (!success) { - emit ExecutionFailed(keccak256(txHashData)); - } - + emit Execution(txHash, success, to, value, data, operation); // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls if (gasPrice > 0) { handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver); @@ -142,10 +191,14 @@ contract GnosisSafe address payable receiver = refundReceiver == address(0) ? tx.origin : refundReceiver; if (gasToken == address(0)) { // For ETH we will only adjust the gas price to not be higher than the actual used gas price + uint256 payment = gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice); // solium-disable-next-line security/no-send - require(receiver.send(gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice)), "Could not pay gas costs with ether"); + require(receiver.send(payment), "Could not pay gas costs with ether"); + emit ExecutionPayment(gasToken, payment); } else { - require(transferToken(gasToken, receiver, gasUsed.add(baseGas).mul(gasPrice)), "Could not pay gas costs with token"); + uint256 payment = gasUsed.add(baseGas).mul(gasPrice); + require(transferToken(gasToken, receiver, payment), "Could not pay gas costs with token"); + emit ExecutionPayment(gasToken, payment); } } @@ -258,6 +311,7 @@ contract GnosisSafe { require(owners[msg.sender] != address(0), "Only owners can approve a hash"); approvedHashes[msg.sender][hashToApprove] = 1; + emit ApproveHash(hashToApprove, msg.sender); } /** @@ -268,7 +322,9 @@ contract GnosisSafe external authorized { - signedMessages[getMessageHash(_data)] = 1; + bytes32 msgHash = getMessageHash(_data); + signedMessages[msgHash] = 1; + emit SignMsg(msgHash); } /** diff --git a/contracts/base/FallbackManager.sol b/contracts/base/FallbackManager.sol index 3fb794b1f..7bed7beba 100644 --- a/contracts/base/FallbackManager.sol +++ b/contracts/base/FallbackManager.sol @@ -6,6 +6,10 @@ import "../common/SelfAuthorized.sol"; /// @author Richard Meissner - contract FallbackManager is SelfAuthorized { + event IncomingTransaction( + address from, uint256 value, bool hasData + ); + // keccak256("fallback_manager.handler.address") bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5; @@ -32,6 +36,7 @@ contract FallbackManager is SelfAuthorized { external payable { + emit IncomingTransaction(msg.sender, msg.value, msg.data.length != 0); // Only calls without value and with data will be forwarded if (msg.value > 0 || msg.data.length == 0) return; bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT; diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 249e7e5ee..03be081f0 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -12,6 +12,11 @@ contract ModuleManager is SelfAuthorized, Executor { event EnabledModule(Module module); event DisabledModule(Module module); + // TODO: what should be indexed + event ExecutionFromModule( + bool indexed success, address indexed module, address indexed to, // topics + uint256 value, bytes data, Enum.Operation operation + ); address public constant SENTINEL_MODULES = address(0x1); @@ -72,6 +77,7 @@ contract ModuleManager is SelfAuthorized, Executor { require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "Method can only be called from an enabled module"); // Execute transaction without further confirmations. success = execute(to, value, data, operation, gasleft()); + emit ExecutionFromModule(success, msg.sender, to, value, data, operation); } /// @dev Returns array of modules. diff --git a/package.json b/package.json index cf89568c2..edb5920aa 100644 --- a/package.json +++ b/package.json @@ -18,7 +18,7 @@ "preversion": "npm run restore", "truffle-zos-merge": "node scripts/merge_zos.js", "restore": "rm -rf build && npx truffle compile && npm run truffle-zos-merge && npx tnt cB", - "install": "cd $INIT_CWD && npm explore truffle -- npm install solc@0.5.7" + "install": "cd $INIT_CWD && npm explore truffle -- npm install solc@0.5.11" }, "repository": { "type": "git", diff --git a/test/createCall.js b/test/createCall.js index 5aef0368e..ea8fdbc6a 100644 --- a/test/createCall.js +++ b/test/createCall.js @@ -60,7 +60,7 @@ contract('CreateCall', function(accounts) { let creationData = createCall.contract.performCreate.getData(0, compileContract.data) let testContract = utils.getParamFromTxEvent( - await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor), + await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }), 'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE with value' ) @@ -82,7 +82,7 @@ contract('CreateCall', function(accounts) { let creationData = createCall.contract.performCreate.getData(web3.toWei(1, 'ether'), compileContract.data) let testContract = utils.getParamFromTxEvent( - await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor), + await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }), 'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE' ) @@ -106,7 +106,7 @@ contract('CreateCall', function(accounts) { let creationData = createCall.contract.performCreate2.getData(0, compileContract.data, salt) let testContract = utils.getParamFromTxEvent( - await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor), + await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }), 'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE2' ) @@ -129,7 +129,7 @@ contract('CreateCall', function(accounts) { let creationData = createCall.contract.performCreate2.getData(web3.toWei(1, 'ether'), compileContract.data, salt) let testContract = utils.getParamFromTxEvent( - await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor), + await safeUtils.executeTransaction(lw, gnosisSafe, 'create test contract', [lw.accounts[0], lw.accounts[1]], createCall.address, 0, creationData, DELEGATECALL, executor, { extraGas: 15000 }), 'ContractCreation', 'newContract', gnosisSafe.address, TestContract, 'executeTransaction CREATE2 with value' ) diff --git a/test/gnosisSafeDeploymentViaCreate2.js b/test/gnosisSafeDeploymentViaCreate2.js index 22a90dbf4..68aad278a 100644 --- a/test/gnosisSafeDeploymentViaCreate2.js +++ b/test/gnosisSafeDeploymentViaCreate2.js @@ -75,7 +75,7 @@ contract('GnosisSafe via create2', function(accounts) { // Estimate safe creation costs let gnosisSafeData = await gnosisSafeMasterCopy.contract.setup.getData([lw.accounts[0], lw.accounts[1], lw.accounts[2]], 2, 0, "0x", 0, 0, 0, 0) let creationNonce = new Date().getTime() - let estimate = (await proxyFactory.createProxyWithNonce.estimateGas(gnosisSafeMasterCopy.address, gnosisSafeData, creationNonce)) + 9000 + let estimate = (await proxyFactory.createProxyWithNonce.estimateGas(gnosisSafeMasterCopy.address, gnosisSafeData, creationNonce)) + 14000 let creationData = await getCreationData(0, estimate * gasPrice, creationNonce) // User funds safe diff --git a/test/multiSend.js b/test/multiSend.js index ab3792142..768e67c02 100644 --- a/test/multiSend.js +++ b/test/multiSend.js @@ -91,12 +91,13 @@ contract('MultiSend', function(accounts) { let data = await multiSend.contract.multiSend.getData(nestedTransactionData) let transactionHash = await gnosisSafe.getTransactionHash(multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, nonce) let sigs = utils.signTransaction(lw, [lw.accounts[0]], transactionHash) - utils.checkTxEvent( + let event = utils.checkTxEvent( await gnosisSafe.execTransaction( multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, sigs ), - 'ExecutionFailed', gnosisSafe.address, true, 'execTransaction send multiple transactions' + 'Execution', gnosisSafe.address, true, 'execTransaction send multiple transactions' ) + assert.equal(false, event.args.success) }) it('single fail should fail all', async () => { @@ -115,12 +116,13 @@ contract('MultiSend', function(accounts) { let data = await multiSend.contract.multiSend.getData(nestedTransactionData) let transactionHash = await gnosisSafe.getTransactionHash(multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, nonce) let sigs = utils.signTransaction(lw, [lw.accounts[0]], transactionHash) - utils.checkTxEvent( + let event = utils.checkTxEvent( await gnosisSafe.execTransaction( multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, sigs ), - 'ExecutionFailed', gnosisSafe.address, true, 'execTransaction send multiple transactions' + 'Execution', gnosisSafe.address, true, 'execTransaction send multiple transactions' ) + assert.equal(false, event.args.success) assert.equal(await gnosisSafe.getThreshold(), 1) }) }) diff --git a/test/utils/execution.js b/test/utils/execution.js index 9a7372ada..2d6b56218 100644 --- a/test/utils/execution.js +++ b/test/utils/execution.js @@ -4,6 +4,8 @@ const BigNumber = require('bignumber.js'); const GAS_PRICE = web3.toWei(100, 'gwei') let baseGasValue = function(hexValue) { + // TODO: adjust for Istanbul hardfork (https://eips.ethereum.org/EIPS/eip-2028) + // TODO: this should include the event costs switch(hexValue) { case "0x": return 0 case "00": return 4 @@ -18,6 +20,7 @@ let baseGasValue = function(hexValue) { } let estimateBaseGas = function(safe, to, value, data, operation, txGasEstimate, gasToken, refundReceiver, signatureCount, nonce) { + // TODO: adjust for Istanbul hardfork (https://eips.ethereum.org/EIPS/eip-2028) // numbers < 256 are 192 -> 31 * 4 + 68 // numbers < 65k are 256 -> 30 * 4 + 2 * 68 // For signature array length and baseGasEstimate we already calculated the 0 bytes so we just add 64 for each non-zero byte @@ -26,7 +29,8 @@ let estimateBaseGas = function(safe, to, value, data, operation, txGasEstimate, to, value, data, operation, txGasEstimate, 0, GAS_PRICE, gasToken, refundReceiver, "0x" ) let baseGasEstimate = estimatebaseGasCosts(payload) + signatureCost + (nonce > 0 ? 5000 : 20000) + 1500 // 1500 -> hash generation costs - return baseGasEstimate + 32000; // Add aditional gas costs (e.g. base tx costs, transfer costs) + // TODO: reduce to 32k again and adjust basecost calc for events + return baseGasEstimate + 37000; // Add aditional gas costs (e.g. base tx costs, transfer costs) } let executeTransactionWithSigner = async function(signer, safe, subject, accounts, to, value, data, operation, executor, opts) { @@ -34,6 +38,7 @@ let executeTransactionWithSigner = async function(signer, safe, subject, account let txFailed = options.fails || false let txGasToken = options.gasToken || 0 let refundReceiver = options.refundReceiver || 0 + let extraGas = options.extraGas || 0 // Estimate safe transaction (need to be called with from set to the safe address) let txGasEstimate = 0 @@ -49,7 +54,7 @@ let executeTransactionWithSigner = async function(signer, safe, subject, account } let nonce = await safe.nonce() - let baseGasEstimate = estimateBaseGas(safe, to, value, data, operation, txGasEstimate, txGasToken, refundReceiver, accounts.length, nonce) + let baseGasEstimate = estimateBaseGas(safe, to, value, data, operation, txGasEstimate, txGasToken, refundReceiver, accounts.length, nonce) + extraGas console.log(" Base Gas estimate: " + baseGasEstimate) let gasPrice = GAS_PRICE @@ -86,11 +91,11 @@ let executeTransactionWithSigner = async function(signer, safe, subject, account let tx = await safe.execTransaction( to, value, data, operation, txGasEstimate, baseGasEstimate, gasPrice, txGasToken, refundReceiver, sigs, {from: executor, gas: estimate + txGasEstimate + 10000, gasPrice: options.txGasPrice || gasPrice} ) - let events = utils.checkTxEvent(tx, 'ExecutionFailed', safe.address, txFailed, subject) - if (txFailed) { - let transactionHash = await safe.getTransactionHash(to, value, data, operation, txGasEstimate, baseGasEstimate, gasPrice, txGasToken, refundReceiver, nonce) - assert.equal(transactionHash, events.args.txHash) - } + let event = utils.checkTxEvent(tx, 'Execution', safe.address, true, subject) + let transactionHash = await safe.getTransactionHash(to, value, data, operation, txGasEstimate, baseGasEstimate, gasPrice, txGasToken, refundReceiver, nonce) + assert.equal(transactionHash, event.args.txHash) + assert.equal(!txFailed, event.args.success) + // TODO: test all events params return tx }