Change r, s, v arrays to bytes

README.md

@@ -54,9 +54,9 @@ There are multiple implementations of the Gnosis Safe contract with different me
 #### GnosisSafePersonalEdition.sol
 This version is targeted at users that control all keys owning a safe. The transaction hash can be signed with the private keys that manage the safe. 
 
-Once the required number of confirmations is available `execAndPayTransaction` can be called with the sending confirmation signatures. This method will pay the submitter of the transaction for the transaction fees after the Safe transaction has been executed.
+Once the required number of confirmations is available `execTransactionAndPaySubmitter` can be called with the sending confirmation signatures. This method will pay the submitter of the transaction for the transaction fees after the Safe transaction has been executed.
 
-`execAndPayTransaction` expects all confirmations sorted by owner address. This is required to easily validate no confirmation duplicates exist.
+`execTransactionAndPaySubmitter` expects all confirmations sorted by owner address. This is required to easily validate no confirmation duplicates exist.
 
 #### GnosisSafeTeamEdition.sol
 This version is targeted at teams where each owner is a different user. Each owner has to confirm a transaction by using `confirmTransaction`. Once the required number of owners has confirmed, the transaction can be executed via `execTransactionIfApproved`. If the sender of `execTransactionIfApproved` is an owner it is not necessary to confirm the transaction before. Furthermore this version doesn't store the nonce in the contract but for each transaction a nonce needs to be specified.
@@ -70,12 +70,10 @@ Assuming we have 2 owners in a 2 out of 2 multisig configuration:
 
 `0x1` and `0x2` are confirming by signing a message.
 
-The Safe transaction parameters used for `execTransaction` have to be set like the following:
-* `v = [v_0x1, v_0x2]`
-* `r = [r_0x1, r_0x2]`
-* `s = [s_0x1, s_0x2]`
+The signatures bytes used for `execTransaction` have to be build like the following:
+* `bytes = 0x{r_0x1}{s_0x1}{v_0x1}{r_0x2}{s_0x2}{v_0x2}`
 
-`v`, `r` and `s` are the signature parameters for the signed confirmation messages. Position `0` in `v` represents `0x1`'s signature part and corresponds to position `0` in `r` and `s`.
+`v`, `r` and `s` are the signature parameters for the signed confirmation messages. All values are hex encoded. `r` and `s` are padded to 32 bytes and `v` is padded to 8 bytes.
 
 ### Modules
 Modules allow to execute transactions from the Safe without the requirement of multiple signatures. For this Modules that have been added to a Safe can use the `execTransactionFromModule` function. Modules define their own requirements for execution. Modules need to implement their own replay protection.

contracts/GnosisSafePersonalEdition.sol

@@ -2,13 +2,14 @@ pragma solidity 0.4.24;
 import "./interfaces/ERC20Token.sol";
 import "./GnosisSafe.sol";
 import "./MasterCopy.sol";
+import "./SignatureValidator.sol";
 
 
 /// @title Gnosis Safe Personal Edition - A multisignature wallet with support for confirmations using signed messages based on ERC191.
 /// @author Stefan George - <[email protected]>
 /// @author Richard Meissner - <[email protected]>
 /// @author Ricardo Guilherme Schmidt - (Status Research & Development GmbH) - Gas Token Payment
-contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe {
+contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe, SignatureValidator {
 
     string public constant NAME = "Gnosis Safe Personal Edition";
     string public constant VERSION = "0.0.1";
@@ -17,7 +18,8 @@ contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe {
 
     uint256 public nonce;
 
-    /// @dev Allows to execute a Safe transaction confirmed by required number of owners.
+    /// @dev Allows to execute a Safe transaction confirmed by required number of owners and then pays the account that submitted the transaction.
+    ///      Note: The fees are always transfered, even if the user transaction fails. 
     /// @param to Destination address of Safe transaction.
     /// @param value Ether value of Safe transaction.
     /// @param data Data payload of Safe transaction.
@@ -26,10 +28,8 @@ contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe {
     /// @param dataGas Gas costs for data used to trigger the safe transaction and to pay the payment transfer
     /// @param gasPrice Gas price that should be used for the payment calculation.
     /// @param gasToken Token address (or 0 if ETH) that is used for the payment.
-    /// @param v Array of signature V values sorted by owner addresses.
-    /// @param r Array of signature R values sorted by owner addresses.
-    /// @param s Array of signature S values sorted by owner addresses.
-    function execAndPayTransaction(
+    /// @param signatures Packed signature data ({bytes32 r}{bytes32 s}{uint8 v})
+    function execTransactionAndPaySubmitter(
         address to, 
         uint256 value, 
         bytes data, 
@@ -38,19 +38,19 @@ contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe {
         uint256 dataGas,
         uint256 gasPrice,
         address gasToken,
-        uint8[] v, 
-        bytes32[] r, 
-        bytes32[] s
+        bytes signatures
     )
         public
+        returns (bool success)
     {
         uint256 startGas = gasleft();
         bytes32 txHash = getTransactionHash(to, value, data, operation, safeTxGas, dataGas, gasPrice, gasToken, nonce);
-        checkHash(txHash, v, r, s);
+        checkHash(txHash, signatures);
         // Increase nonce and execute transaction.
         nonce++;
         require(gasleft() >= safeTxGas, "Not enough gas to execute safe transaction");
-        if (!execute(to, value, data, operation, safeTxGas)) {
+        success = execute(to, value, data, operation, safeTxGas);
+        if (!success) {
             emit ExecutionFailed(txHash);
         }
 
@@ -73,7 +73,7 @@ contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe {
     ///      1.) The method can only be called from the safe itself
     ///      2.) The response is returned with a revert
     ///      When estimating set `from` to the address of the safe.
-    ///      Since the `estimateGas` function includes refunds, call this method to get an estimated of the costs that are deducted from the safe with `execAndPayTransaction`
+    ///      Since the `estimateGas` function includes refunds, call this method to get an estimated of the costs that are deducted from the safe with `execTransactionAndPaySubmitter`
     /// @param to Destination address of Safe transaction.
     /// @param value Ether value of Safe transaction.
     /// @param data Data payload of Safe transaction.
@@ -92,7 +92,7 @@ contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe {
         revert(string(abi.encodePacked(requiredGas)));
     }
 
-    function checkHash(bytes32 hash, uint8[] v, bytes32[] r, bytes32[] s)
+    function checkHash(bytes32 txHash, bytes signatures)
         internal
         view
     {
@@ -102,7 +102,7 @@ contract GnosisSafePersonalEdition is MasterCopy, GnosisSafe {
         uint256 i;
         // Validate threshold is reached.
         for (i = 0; i < threshold; i++) {
-            currentOwner = ecrecover(hash, v[i], r[i], s[i]);
+            currentOwner = recoverKey(txHash, signatures, i);
             require(owners[currentOwner] != 0, "Signature not provided by owner");
             require(currentOwner > lastOwner, "Signatures are not ordered by owner address");
             lastOwner = currentOwner;

contracts/SignatureValidator.sol

@@ -0,0 +1,53 @@
+pragma solidity 0.4.24;
+
+
+/// @title SignatureValidator - recovers a sender from a signature 
+/// @author Ricardo Guilherme Schmidt (Status Research & Development GmbH) 
+/// @author Richard Meissner - <[email protected]>
+contract SignatureValidator {
+
+    /// @dev Recovers address who signed the message 
+    /// @param txHash operation ethereum signed message hash
+    /// @param messageSignature message `txHash` signature
+    /// @param pos which signature to read
+    function recoverKey (
+        bytes32 txHash, 
+        bytes messageSignature,
+        uint256 pos
+    )
+        pure
+        public
+        returns (address) 
+    {
+        uint8 v;
+        bytes32 r;
+        bytes32 s;
+        (v, r, s) = signatureSplit(messageSignature, pos);
+        return ecrecover(txHash, v, r, s);
+    }
+
+    /// @dev divides bytes signature into `uint8 v, bytes32 r, bytes32 s`
+    /// @param pos which signature to read
+    /// @param signatures concatenated rsv signatures
+    function signatureSplit(bytes signatures, uint256 pos)
+        pure
+        public
+        returns (uint8 v, bytes32 r, bytes32 s)
+    {
+        // The signature format is a compact form of:
+        //   {bytes32 r}{bytes32 s}{uint8 v}
+        // Compact means, uint8 is not padded to 32 bytes.
+        // solium-disable-next-line security/no-inline-assembly
+        assembly {
+            let signaturePos := mul(0x41, pos)
+            r := mload(add(signatures, add(signaturePos, 0x20)))
+            s := mload(add(signatures, add(signaturePos, 0x40)))
+            // Here we are loading the last 32 bytes, including 31 bytes
+            // of 's'. There is no 'mload8' to do this.
+            //
+            // 'byte' is not working due to the Solidity parser, so lets
+            // use the second best option, 'and'
+            v := and(mload(add(signatures, add(signaturePos, 0x41))), 0xff)
+        }
+    }
+}

contracts/modules/StateChannelModule.sol

@@ -1,12 +1,13 @@
 pragma solidity 0.4.24;
 import "../Module.sol";
 import "../OwnerManager.sol";
+import "../SignatureValidator.sol";
 
 
 /// @title Gnosis Safe State Module - A module that allows interaction with statechannels.
 /// @author Stefan George - <[email protected]>
 /// @author Richard Meissner - <[email protected]>
-contract StateChannelModule is Module {
+contract StateChannelModule is Module, SignatureValidator {
 
     string public constant NAME = "State Channel Module";
     string public constant VERSION = "0.0.1";
@@ -27,30 +28,26 @@ contract StateChannelModule is Module {
     /// @param data Data payload of Safe transaction.
     /// @param operation Operation type of Safe transaction.
     /// @param nonce Nonce used for this Safe transaction.
-    /// @param v Array of signature V values sorted by owner addresses.
-    /// @param r Array of signature R values sorted by owner addresses.
-    /// @param s Array of signature S values sorted by owner addresses.
+    /// @param signatures Packed signature data ({bytes32 r}{bytes32 s}{uint8 v})
     function execTransaction(
         address to, 
         uint256 value, 
         bytes data, 
         Enum.Operation operation, 
         uint256 nonce,
-        uint8[] v, 
-        bytes32[] r, 
-        bytes32[] s
+        bytes signatures
     )
         public
     {
         bytes32 transactionHash = getTransactionHash(to, value, data, operation, nonce);
         require(isExecuted[transactionHash] == 0, "Transaction already executed");
-        checkHash(transactionHash, v, r, s);
+        checkHash(transactionHash, signatures);
         // Mark as executed and execute transaction.
         isExecuted[transactionHash] = 1;
         require(manager.execTransactionFromModule(to, value, data, operation), "Could not execute transaction");
     }
 
-    function checkHash(bytes32 transactionHash, uint8[] v, bytes32[] r, bytes32[] s)
+    function checkHash(bytes32 transactionHash, bytes signatures)
         internal
         view
     {
@@ -61,7 +58,7 @@ contract StateChannelModule is Module {
         uint8 threshold = OwnerManager(manager).getThreshold();
         // Validate threshold is reached.
         for (i = 0; i < threshold; i++) {
-            currentOwner = ecrecover(transactionHash, v[i], r[i], s[i]);
+            currentOwner = recoverKey(transactionHash, signatures, i);
             require(OwnerManager(manager).isOwner(currentOwner), "Signature not provided by owner");
             require(currentOwner > lastOwner, "Signatures are not ordered by owner address");
             lastOwner = currentOwner;

test/dailyLimitModule.js

@@ -89,9 +89,9 @@ contract('DailyLimitModule', function(accounts) {
         let sigs = utils.signTransaction(lw, [lw.accounts[0], lw.accounts[1]], transactionHash)
 
         utils.logGasUsage(
-            'execAndPayTransaction change daily limit',
-            await gnosisSafe.execAndPayTransaction(
-                dailyLimitModule.address, 0, data, CALL, 100000, 0, web3.toWei(100, 'gwei'), 0, sigs.sigV, sigs.sigR, sigs.sigS
+            'execTransactionAndPaySubmitter change daily limit',
+            await gnosisSafe.execTransactionAndPaySubmitter(
+                dailyLimitModule.address, 0, data, CALL, 100000, 0, web3.toWei(100, 'gwei'), 0, sigs
             )
         )
         dailyLimit = await dailyLimitModule.dailyLimits(0)
@@ -127,7 +127,7 @@ contract('DailyLimitModule', function(accounts) {
         let nonce = await gnosisSafe.nonce()
         transactionHash = await gnosisSafe.getTransactionHash(dailyLimitModule.address, 0, data, CALL, 100000, 0, 0, 0, nonce)
         let sigs = utils.signTransaction(lw, [lw.accounts[0], lw.accounts[1]], transactionHash)
-        await gnosisSafe.execAndPayTransaction(dailyLimitModule.address, 0, data, CALL, 100000, 0, 0, 0, sigs.sigV, sigs.sigR, sigs.sigS)
+        await gnosisSafe.execTransactionAndPaySubmitter(dailyLimitModule.address, 0, data, CALL, 100000, 0, 0, 0, sigs)
 
         // Withdrawal should fail as there are no tokens
         assert.equal(await testToken.balances(gnosisSafe.address), 0);

test/multiSend.js

@@ -66,8 +66,8 @@ contract('MultiSend', function(accounts) {
         let sigs = utils.signTransaction(lw, [lw.accounts[0]], transactionHash)
         utils.logGasUsage(
             'execTransaction send multiple transactions',
-            await gnosisSafe.execAndPayTransaction(
-                multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, sigs.sigV, sigs.sigR, sigs.sigS
+            await gnosisSafe.execTransactionAndPaySubmitter(
+                multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, sigs
             )
         )
         assert.equal(await web3.eth.getBalance(gnosisSafe.address).toNumber(), 0)
@@ -88,8 +88,8 @@ contract('MultiSend', function(accounts) {
         let transactionHash = await gnosisSafe.getTransactionHash(multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, nonce)
         let sigs = utils.signTransaction(lw, [lw.accounts[0]], transactionHash)
         utils.checkTxEvent(
-            await gnosisSafe.execAndPayTransaction(
-                multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, sigs.sigV, sigs.sigR, sigs.sigS
+            await gnosisSafe.execTransactionAndPaySubmitter(
+                multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, sigs
             ),
             'ExecutionFailed', gnosisSafe.address, true, 'execTransaction send multiple transactions'
         )
@@ -112,8 +112,8 @@ contract('MultiSend', function(accounts) {
         let transactionHash = await gnosisSafe.getTransactionHash(multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, nonce)
         let sigs = utils.signTransaction(lw, [lw.accounts[0]], transactionHash)
         utils.checkTxEvent(
-            await gnosisSafe.execAndPayTransaction(
-                multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, sigs.sigV, sigs.sigR, sigs.sigS
+            await gnosisSafe.execTransactionAndPaySubmitter(
+                multiSend.address, 0, data, DELEGATECALL, 1000000, 0, 0, 0, sigs
             ),
             'ExecutionFailed', gnosisSafe.address, true, 'execTransaction send multiple transactions'
         )

test/safeMethodNaming.js

@@ -30,17 +30,12 @@ contract('GnosisSafeEditions', function(accounts) {
     it('check method naming of personal safe', async () => {
         let functions = getSortedFunctions(GnosisSafePersonal.abi)
         console.log(functions)
-        assert.equal('execAndPayTransaction', functions[0].name)
+        assert.equal('execTransactionAndPaySubmitter', functions[0].name)
     })
     it('check method naming of team safe', async () => {
         let functions = getSortedFunctions(GnosisSafeTeam.abi)
         console.log(functions)
         assert.equal('approveTransactionWithParameters', functions[0].name)
         assert.equal('execTransactionIfApproved', functions[1].name)
     })
-    it('check method naming of sate channel module', async () => {
-        let functions = getSortedFunctions(StateChannelModule.abi)
-        console.log(functions)
-        assert.equal('execTransaction', functions[0].name)
-    })
 });

test/stateChannelModule.js

@@ -27,7 +27,7 @@ contract('StateChannelModule', function(accounts) {
         // Execute paying transaction
         // We add the minGasEstimate and an additional 10k to the estimate to ensure that there is enough gas for the safe transaction
         let tx = stateChannelModule.execTransaction(
-            to, value, data, operation, nonce, sigs.sigV, sigs.sigR, sigs.sigS, {from: executor}
+            to, value, data, operation, nonce, sigs, {from: executor}
         )
 
         let res

test/utils.js

@@ -90,21 +90,13 @@ async function createLightwallet() {
 }
 
 function signTransaction(lw, signers, transactionHash) {
-    let sigV = []
-    let sigR = []
-    let sigS = []
+    let signatureBytes = "0x"
     signers.sort()
     for (var i=0; i<signers.length; i++) {
         let sig = lightwallet.signing.signMsgHash(lw.keystore, lw.passwords, transactionHash, signers[i])
-        sigV.push(sig.v)
-        sigR.push('0x' + sig.r.toString('hex'))
-        sigS.push('0x' + sig.s.toString('hex'))
-    }
-    return {
-        sigV: sigV,
-        sigR: sigR,
-        sigS: sigS
+        signatureBytes += sig.r.toString('hex') + sig.s.toString('hex') + sig.v.toString(16)
     }
+    return signatureBytes
 }
 
 async function assertRejects(q, msg) {

test/utilsPersonalSafe.js

@@ -8,9 +8,9 @@ let estimateDataGas = function(safe, to, value, data, operation, txGasEstimate,
     // numbers < 256 are 192 -> 31 * 4 + 68
     // numbers < 65k are 256 -> 30 * 4 + 2 * 68
     // For signature array length and dataGasEstimate we already calculated the 0 bytes so we just add 64 for each non-zero byte
-    let signatureCost = 3 * (64 + 64) + signatureCount * (192 + 2176 + 2176) // array count (3 -> r, s, v) * (array pointer + array length) + signature count * (v, r, s)
-    let payload = safe.contract.execAndPayTransaction.getData(
-        to, value, data, operation, txGasEstimate, 0, GAS_PRICE, gasToken, [], [], []
+    let signatureCost = signatureCount * (68 + 2176 + 2176) // array count (3 -> r, s, v) * signature count
+    let payload = safe.contract.execTransactionAndPaySubmitter.getData(
+        to, value, data, operation, txGasEstimate, 0, GAS_PRICE, gasToken, "0x"
     )
     let dataGasEstimate = utils.estimateDataGasCosts(payload) + signatureCost
     if (dataGasEstimate > 65536) {
@@ -51,20 +51,20 @@ let executeTransaction = async function(lw, safe, subject, accounts, to, value,
     // Confirm transaction with signed messages
     let sigs = utils.signTransaction(lw, accounts, transactionHash)
 
-    let payload = safe.contract.execAndPayTransaction.getData(
-        to, value, data, operation, txGasEstimate, dataGasEstimate, gasPrice, txGasToken, sigs.sigV, sigs.sigR, sigs.sigS
+    let payload = safe.contract.execTransactionAndPaySubmitter.getData(
+        to, value, data, operation, txGasEstimate, dataGasEstimate, gasPrice, txGasToken, sigs
     )
     console.log("    Data costs: " + utils.estimateDataGasCosts(payload))
 
     // Estimate gas of paying transaction
-    let estimate = await safe.execAndPayTransaction.estimateGas(
-        to, value, data, operation, txGasEstimate, dataGasEstimate, gasPrice, txGasToken, sigs.sigV, sigs.sigR, sigs.sigS
+    let estimate = await safe.execTransactionAndPaySubmitter.estimateGas(
+        to, value, data, operation, txGasEstimate, dataGasEstimate, gasPrice, txGasToken, sigs
     )
 
     // Execute paying transaction
     // We add the txGasEstimate and an additional 10k to the estimate to ensure that there is enough gas for the safe transaction
-    let tx = await safe.execAndPayTransaction(
-        to, value, data, operation, txGasEstimate, dataGasEstimate, gasPrice, txGasToken, sigs.sigV, sigs.sigR, sigs.sigS, {from: executor, gas: estimate + txGasEstimate + 10000}
+    let tx = await safe.execTransactionAndPaySubmitter(
+        to, value, data, operation, txGasEstimate, dataGasEstimate, gasPrice, txGasToken, sigs, {from: executor, gas: estimate + txGasEstimate + 10000}
     )
     let events = utils.checkTxEvent(tx, 'ExecutionFailed', safe.address, txFailed, subject)
     if (txFailed) {

test/whitelistModule.js

@@ -69,8 +69,8 @@ contract('WhitelistModule', function(accounts) {
         let sigs = utils.signTransaction(lw, [lw.accounts[0], lw.accounts[1]], transactionHash)
         utils.logGasUsage(
             'execTransaction add account to whitelist',
-            await gnosisSafe.execAndPayTransaction(
-                whitelistModule.address, 0, data, CALL, 100000, 0, 0, 0, sigs.sigV, sigs.sigR, sigs.sigS
+            await gnosisSafe.execTransactionAndPaySubmitter(
+                whitelistModule.address, 0, data, CALL, 100000, 0, 0, 0, sigs
             )
         )
         assert.equal(await whitelistModule.isWhitelisted(accounts[1]), true)
@@ -81,8 +81,8 @@ contract('WhitelistModule', function(accounts) {
         sigs = utils.signTransaction(lw, [lw.accounts[0], lw.accounts[1]], transactionHash)
         utils.logGasUsage(
             'execTransaction remove account from whitelist',
-            await gnosisSafe.execAndPayTransaction(
-                whitelistModule.address, 0, data, CALL, 100000, 0, 0, 0, sigs.sigV, sigs.sigR, sigs.sigS
+            await gnosisSafe.execTransactionAndPaySubmitter(
+                whitelistModule.address, 0, data, CALL, 100000, 0, 0, 0, sigs
             )
         )
         assert.equal(await whitelistModule.isWhitelisted(accounts[1]), false)