From e4d9a3d5496ae4d100de7da2c0152bfcdcd8410f Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Tue, 15 Oct 2019 15:49:19 +0200 Subject: [PATCH 1/6] Added events + required refactoring --- contracts/GnosisSafe.sol | 78 ++++++++++++++++++++++---- contracts/base/FallbackManager.sol | 5 ++ contracts/base/ModuleManager.sol | 6 ++ test/createCall.js | 8 +-- test/gnosisSafeDeploymentViaCreate2.js | 2 +- test/multiSend.js | 10 ++-- test/utils/execution.js | 22 ++++++-- 7 files changed, 105 insertions(+), 26 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/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 e11620cf1..7f87776b9 100644 --- a/test/multiSend.js +++ b/test/multiSend.js @@ -131,12 +131,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 () => { @@ -155,12 +156,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 2ad62312c..66253180f 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 @@ -25,8 +28,14 @@ let estimateBaseGas = function(safe, to, value, data, operation, txGasEstimate, let payload = safe.contract.execTransaction.getData( to, value, data, operation, txGasEstimate, 0, GAS_PRICE, gasToken, refundReceiver, "0x" ) +<<<<<<< HEAD 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) +======= + let baseGasEstimate = estimatebaseGasCosts(payload) + signatureCost + (nonce > 0 ? 5000 : 20000) + 1500 // 1500 -> hash generation 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) +>>>>>>> f006a6a... Added events + required refactoring } let executeTransactionWithSigner = async function(signer, safe, subject, accounts, to, value, data, operation, executor, opts) { @@ -34,6 +43,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 +59,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 +96,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 } From 83fec2495ef71085a10a92dae6e81ea3cdd61b41 Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Thu, 24 Oct 2019 11:33:41 +0200 Subject: [PATCH 2/6] Optimize events for gas usage --- contracts/GnosisSafe.sol | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/contracts/GnosisSafe.sol b/contracts/GnosisSafe.sol index 8922bdce5..7174b81c0 100644 --- a/contracts/GnosisSafe.sol +++ b/contracts/GnosisSafe.sol @@ -18,7 +18,7 @@ contract GnosisSafe using SafeMath for uint256; string public constant NAME = "Gnosis Safe"; - string public constant VERSION = "1.0.0"; + string public constant VERSION = "1.1.0"; //keccak256( // "EIP712Domain(address verifyingContract)" @@ -43,12 +43,11 @@ contract GnosisSafe 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 ExecutionFailure( + bytes32 txHash, uint256 payment ); - event ExecutionPayment( - address token, uint256 value + event ExecutionSuccess( + bytes32 txHash, uint256 payment ); uint256 public nonce; @@ -171,11 +170,13 @@ contract GnosisSafe // 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()); - 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 + uint256 payment = 0; if (gasPrice > 0) { - handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver); + payment = handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver); } + if (success) emit ExecutionSuccess(txHash, payment); + else emit ExecutionFailure(txHash, payment); } function handlePayment( @@ -186,19 +187,18 @@ contract GnosisSafe address payable refundReceiver ) private + returns (uint256 payment) { // solium-disable-next-line security/no-tx-origin 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); + payment = gasUsed.add(baseGas).mul(gasPrice < tx.gasprice ? gasPrice : tx.gasprice); // solium-disable-next-line security/no-send require(receiver.send(payment), "Could not pay gas costs with ether"); - emit ExecutionPayment(gasToken, payment); } else { - uint256 payment = gasUsed.add(baseGas).mul(gasPrice); + payment = gasUsed.add(baseGas).mul(gasPrice); require(transferToken(gasToken, receiver, payment), "Could not pay gas costs with token"); - emit ExecutionPayment(gasToken, payment); } } From 7928b8e8fbd6aa0972ae80f144f2eedcf8264fa8 Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Thu, 24 Oct 2019 12:01:27 +0200 Subject: [PATCH 3/6] Fix tests --- contracts/base/FallbackManager.sol | 10 +++++----- test/gnosisSafeDeploymentViaCreate2.js | 2 +- test/gnosisSafeDeploymentViaTx.js | 2 +- test/gnosisSafeManagement.js | 2 +- test/gnosisSafeSignatureTypes.js | 2 +- test/gnosisSafeTransactionExecution.js | 2 +- test/multiSend.js | 8 ++++---- test/utils/execution.js | 24 +++++++++++++----------- 8 files changed, 27 insertions(+), 25 deletions(-) diff --git a/contracts/base/FallbackManager.sol b/contracts/base/FallbackManager.sol index 7bed7beba..365c4d4e4 100644 --- a/contracts/base/FallbackManager.sol +++ b/contracts/base/FallbackManager.sol @@ -6,9 +6,7 @@ import "../common/SelfAuthorized.sol"; /// @author Richard Meissner - contract FallbackManager is SelfAuthorized { - event IncomingTransaction( - address from, uint256 value, bool hasData - ); + event IncomingTransaction(address from, uint256 value); // keccak256("fallback_manager.handler.address") bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5; @@ -36,9 +34,11 @@ 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; + if (msg.value > 0 || msg.data.length == 0) { + emit IncomingTransaction(msg.sender, msg.value); + return; + } bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT; address handler; // solium-disable-next-line security/no-inline-assembly diff --git a/test/gnosisSafeDeploymentViaCreate2.js b/test/gnosisSafeDeploymentViaCreate2.js index 68aad278a..6fcd45fc8 100644 --- a/test/gnosisSafeDeploymentViaCreate2.js +++ b/test/gnosisSafeDeploymentViaCreate2.js @@ -9,7 +9,7 @@ const ProxyFactory = artifacts.require("./ProxyFactory.sol") const MockContract = artifacts.require('./MockContract.sol') const MockToken = artifacts.require('./Token.sol') -contract('GnosisSafe via create2', function(accounts) { +contract('GnosisSafe deployment via create2', function(accounts) { const CALL = 0 diff --git a/test/gnosisSafeDeploymentViaTx.js b/test/gnosisSafeDeploymentViaTx.js index d7112a98b..b2af28a22 100644 --- a/test/gnosisSafeDeploymentViaTx.js +++ b/test/gnosisSafeDeploymentViaTx.js @@ -9,7 +9,7 @@ const GnosisSafe = artifacts.require("./GnosisSafe.sol") const MockContract = artifacts.require('./MockContract.sol'); const MockToken = artifacts.require('./Token.sol'); -contract('GnosisSafe Trustless Deployment', function(accounts) { +contract('GnosisSafe trustless deployment', function(accounts) { const CALL = 0 diff --git a/test/gnosisSafeManagement.js b/test/gnosisSafeManagement.js index 791f164ce..d1b8f2ff2 100644 --- a/test/gnosisSafeManagement.js +++ b/test/gnosisSafeManagement.js @@ -7,7 +7,7 @@ const ProxyFactory = artifacts.require("./ProxyFactory.sol") const MockContract = artifacts.require('./MockContract.sol') const MockToken = artifacts.require('./Token.sol') -contract('GnosisSafe', function(accounts) { +contract('GnosisSafe owner and module management', function(accounts) { let gnosisSafe let gnosisSafeMasterCopy diff --git a/test/gnosisSafeSignatureTypes.js b/test/gnosisSafeSignatureTypes.js index 0150bb7e1..3c5138427 100644 --- a/test/gnosisSafeSignatureTypes.js +++ b/test/gnosisSafeSignatureTypes.js @@ -4,7 +4,7 @@ const GnosisSafe = artifacts.require("./GnosisSafe.sol") const ProxyFactory = artifacts.require("./ProxyFactory.sol") -contract('GnosisSafe Without Refund', function(accounts) { +contract('GnosisSafe without refund', function(accounts) { let gnosisSafe let executor = accounts[8] diff --git a/test/gnosisSafeTransactionExecution.js b/test/gnosisSafeTransactionExecution.js index 87792c2bf..8cb8a9816 100644 --- a/test/gnosisSafeTransactionExecution.js +++ b/test/gnosisSafeTransactionExecution.js @@ -7,7 +7,7 @@ const ProxyFactory = artifacts.require("./ProxyFactory.sol") const MockContract = artifacts.require('./MockContract.sol') const MockToken = artifacts.require('./Token.sol') -contract('GnosisSafe', function(accounts) { +contract('GnosisSafe with refunds', function(accounts) { let gnosisSafe let lw diff --git a/test/multiSend.js b/test/multiSend.js index 7f87776b9..fa6d495cb 100644 --- a/test/multiSend.js +++ b/test/multiSend.js @@ -135,9 +135,9 @@ contract('MultiSend', function(accounts) { await gnosisSafe.execTransaction( multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, sigs ), - 'Execution', gnosisSafe.address, true, 'execTransaction send multiple transactions' + 'ExecutionFailure', gnosisSafe.address, true, 'execTransaction send multiple transactions' ) - assert.equal(false, event.args.success) + assert.equal(0, event.args.payment) }) it('single fail should fail all', async () => { @@ -160,9 +160,9 @@ contract('MultiSend', function(accounts) { await gnosisSafe.execTransaction( multiSend.address, 0, data, DELEGATECALL, 0, 0, 0, 0, 0, sigs ), - 'Execution', gnosisSafe.address, true, 'execTransaction send multiple transactions' + 'ExecutionFailure', gnosisSafe.address, true, 'execTransaction send multiple transactions' ) - assert.equal(false, event.args.success) + assert.equal(0, event.args.payment) assert.equal(await gnosisSafe.getThreshold(), 1) }) }) diff --git a/test/utils/execution.js b/test/utils/execution.js index 66253180f..ccd3a68d9 100644 --- a/test/utils/execution.js +++ b/test/utils/execution.js @@ -28,14 +28,10 @@ let estimateBaseGas = function(safe, to, value, data, operation, txGasEstimate, let payload = safe.contract.execTransaction.getData( to, value, data, operation, txGasEstimate, 0, GAS_PRICE, gasToken, refundReceiver, "0x" ) -<<<<<<< HEAD - 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) -======= - let baseGasEstimate = estimatebaseGasCosts(payload) + signatureCost + (nonce > 0 ? 5000 : 20000) + 1500 // 1500 -> hash generation 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) ->>>>>>> f006a6a... Added events + required refactoring + let baseGasEstimate = estimateBaseGasCosts(payload) + signatureCost + (nonce > 0 ? 5000 : 20000) + baseGasEstimate += 1500 // 1500 -> hash generation costs + baseGasEstimate += 1000 // 1000 -> Event emission + return baseGasEstimate + 32000; // 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) { @@ -96,11 +92,17 @@ 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 event = utils.checkTxEvent(tx, 'Execution', safe.address, true, subject) + let eventName = (txFailed) ? 'ExecutionFailure' : 'ExecutionSuccess' + let event = utils.checkTxEvent(tx, eventName, 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 + if (txGasEstimate > 0) { + let maxPayment = (baseGasEstimate + txGasEstimate) * gasPrice + console.log(" User paid", event.args.payment.toNumber(), "after signing a maximum of", maxPayment) + assert.ok(maxPayment >= event.args.payment, "Should not pay more than signed") + } else { + console.log(" User paid", event.args.payment.toNumber()) + } return tx } From dbea5bb7973ead9a5ebad6727fe0d67abccce519 Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Thu, 24 Oct 2019 12:10:00 +0200 Subject: [PATCH 4/6] Optimize module execution events --- contracts/base/ModuleManager.sol | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 03be081f0..d4c6bf553 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -12,11 +12,8 @@ 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 - ); + event ExecutionFromModuleSuccess(address indexed module); + event ExecutionFromModuleFailure(address indexed module); address public constant SENTINEL_MODULES = address(0x1); @@ -77,7 +74,8 @@ 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); + if (success) emit ExecutionFromModuleSuccess(msg.sender); + else emit ExecutionFromModuleFailure(msg.sender); } /// @dev Returns array of modules. From 6457e764992ac5ade67e82acca47eee9359db361 Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Thu, 24 Oct 2019 12:11:43 +0200 Subject: [PATCH 5/6] remove todos --- contracts/GnosisSafe.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/GnosisSafe.sol b/contracts/GnosisSafe.sol index 7174b81c0..917a8ea70 100644 --- a/contracts/GnosisSafe.sol +++ b/contracts/GnosisSafe.sol @@ -35,7 +35,6 @@ contract GnosisSafe //); bytes32 public constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca; - // 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 From 7fb2f6e4f1d1be36fb0808709e5d8cd8c186625e Mon Sep 17 00:00:00 2001 From: Richard Meissner Date: Thu, 24 Oct 2019 14:37:29 +0200 Subject: [PATCH 6/6] Remove more todos --- test/utils/execution.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/utils/execution.js b/test/utils/execution.js index ccd3a68d9..bfbb2e96e 100644 --- a/test/utils/execution.js +++ b/test/utils/execution.js @@ -5,7 +5,6 @@ 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