From b7e4d431bc94a347a4a60c0302661ffde27b485e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 14:15:51 +0100 Subject: [PATCH 1/9] split ForeignBridge.transfer into transferHomeViaRelay and transferLocal resolve #55 --- contracts/bridge.sol | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/contracts/bridge.sol b/contracts/bridge.sol index 8708841..2833737 100644 --- a/contracts/bridge.sol +++ b/contracts/bridge.sol @@ -301,19 +301,35 @@ contract ForeignBridge { } } - /// Used to transfer money between accounts - function transfer (address recipient, uint value, bool externalTransfer) public { + /// Transfer `value` from `msg.sender`s local balance (on `foreign` chain) to `recipient` on `home` chain. + /// + /// immediately decreases `msg.sender`s local balance. + /// emits a `Withdraw` event which will be picked up by the bridge authorities. + /// bridge authorities will then sign off (by calling `submitSignature`) on a message containing `value`, + /// `recipient` and the `hash` of the transaction on `foreign` containing the `Withdraw` event. + /// once `requiredSignatures` are collected a `CollectedSignatures` event will be emitted. + /// an authority will pick up `CollectedSignatures` an call `HomeBridge.withdraw` + /// which transfers `value - relayCost` to `recipient` completing the transfer. + function transferHomeViaRelay(address recipient, uint value) public { require(balances[msg.sender] >= value); // fails if value == 0, or if there is an overflow require(balances[recipient] + value > balances[recipient]); balances[msg.sender] -= value; - if (externalTransfer) { - Withdraw(recipient, value); - } else { - balances[recipient] += value; - Transfer(msg.sender, recipient, value); - } + Withdraw(recipient, value); + } + + /// Transfer `value` to `recipient` on this `foreign` chain. + /// + /// does not affect `home` chain. does not do a relay. + function transferLocal(address recipient, uint value) public { + require(balances[msg.sender] >= value); + // fails if value == 0, or if there is an overflow + require(balances[recipient] + value > balances[recipient]); + + balances[msg.sender] -= value; + balances[recipient] += value; + Transfer(msg.sender, recipient, value); } /// Should be used as sync tool From 148ceb7f40217b97f853b3718821310e4b9150d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 14:16:25 +0100 Subject: [PATCH 2/9] adapt tests to split of ForeignBridge.transfer --- truffle/test/foreign.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/truffle/test/foreign.js b/truffle/test/foreign.js index d6d7fa4..5110a99 100644 --- a/truffle/test/foreign.js +++ b/truffle/test/foreign.js @@ -128,7 +128,7 @@ contract('ForeignBridge', function(accounts) { meta = instance; return meta.deposit(userAccount, value, hash, { from: authorities[0] }); }).then(function(result) { - return meta.transfer(userAccount2, value2, false, { from: userAccount }); + return meta.transferLocal(userAccount2, value2, { from: userAccount }); }).then(function(result) { assert.equal(1, result.logs.length, "Exactly one event should be created"); assert.equal("Transfer", result.logs[0].event, "Event name should be Transfer"); @@ -158,7 +158,7 @@ contract('ForeignBridge', function(accounts) { meta = instance; return meta.deposit(userAccount, value, hash, { from: authorities[0] }); }).then(function(result) { - return meta.transfer(userAccount2, value2, false, { from: userAccount }); + return meta.transferLocal(userAccount2, value2, { from: userAccount }); }).then(function(result) { assert(false, "Transfer should fail"); }, function(err) { @@ -178,7 +178,7 @@ contract('ForeignBridge', function(accounts) { meta = instance; return meta.deposit(userAccount, value, hash, { from: authorities[0] }); }).then(function(result) { - return meta.transfer(userAccount2, value2, false, { from: userAccount }); + return meta.transferLocal(userAccount2, value2, { from: userAccount }); }).then(function(result) { assert(false, "Transfer of value 0 should fail"); }, function (err) { @@ -201,7 +201,7 @@ contract('ForeignBridge', function(accounts) { meta.deposit(userAccount2, value2, hash, { from: authorities[0] }), ]) }).then(function(result) { - return meta.transfer(userAccount2, value, false, { from: userAccount }); + return meta.transferLocal(userAccount2, value, { from: userAccount }); }).then(function(result) { assert(false, "Transfer with overflow should fail"); }, function (err) { @@ -221,7 +221,7 @@ contract('ForeignBridge', function(accounts) { meta = instance; return meta.deposit(userAccount, value, hash, { from: authorities[0] }); }).then(function(result) { - return meta.transfer(userAccount2, value2, true, { from: userAccount }); + return meta.transferHomeViaRelay(userAccount2, value2, { from: userAccount }); }).then(function(result) { assert.equal(1, result.logs.length, "Exactly one event should be created"); assert.equal("Withdraw", result.logs[0].event, "Event name should be Withdraw"); From 48d78ba0e78090f25dcd5608bf6691858483180c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 14:20:48 +0100 Subject: [PATCH 3/9] bridge.sol: style consistency: remove space between func name and param list --- contracts/bridge.sol | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/contracts/bridge.sol b/contracts/bridge.sol index 2833737..cc0b325 100644 --- a/contracts/bridge.sol +++ b/contracts/bridge.sol @@ -2,7 +2,7 @@ pragma solidity ^0.4.17; library Authorities { - function contains (address[] self, address value) internal pure returns (bool) { + function contains(address[] self, address value) internal pure returns (bool) { for (uint i = 0; i < self.length; i++) { if (self[i] == value) { return true; @@ -15,14 +15,14 @@ library Authorities { /// Library used only to test Signer library via rpc calls library SignerTest { - function signer (bytes signature, bytes message) public pure returns (address) { + function signer(bytes signature, bytes message) public pure returns (address) { return Signer.signer(signature, message); } } library Utils { - function toString (uint256 inputValue) internal pure returns (string str) { + function toString(uint256 inputValue) internal pure returns (string str) { // it is used only for small numbers bytes memory reversed = new bytes(8); uint workingValue = inputValue; @@ -42,7 +42,7 @@ library Utils { library Signer { - function signer (bytes signature, bytes message) internal pure returns (address) { + function signer(bytes signature, bytes message) internal pure returns (address) { require(signature.length == 65); bytes32 r; bytes32 s; @@ -56,7 +56,7 @@ library Signer { return ecrecover(hash(message), uint8(v), r, s); } - function hash (bytes message) internal pure returns (bytes32) { + function hash(bytes message) internal pure returns (bytes32) { bytes memory prefix = "\x19Ethereum Signed Message:\n"; return keccak256(prefix, Utils.toString(message.length), message); } @@ -91,7 +91,7 @@ contract HomeBridge { event Withdraw (address recipient, uint value); /// Multisig authority validation - modifier allAuthorities (uint8[] v, bytes32[] r, bytes32[] s, bytes message) { + modifier allAuthorities(uint8[] v, bytes32[] r, bytes32[] s, bytes message) { var hash = Signer.hash(message); var used = new address[](requiredSignatures); @@ -107,7 +107,7 @@ contract HomeBridge { } /// Constructor. - function HomeBridge ( + function HomeBridge( uint requiredSignaturesParam, address[] authoritiesParam, uint estimatedGasCostOfWithdrawParam @@ -189,7 +189,7 @@ contract HomeBridge { /// foreign transaction hash (bytes32) // to avoid transaction duplication /// /// NOTE that anyone can call withdraw provided they have the message and required signatures! - function withdraw (uint8[] v, bytes32[] r, bytes32[] s, bytes message) public allAuthorities(v, r, s, message) { + function withdraw(uint8[] v, bytes32[] r, bytes32[] s, bytes message) public allAuthorities(v, r, s, message) { require(message.length == 84); address recipient = getRecipientFromMessage(message); uint value = getValueFromMessage(message); @@ -276,7 +276,7 @@ contract ForeignBridge { } /// Multisig authority validation - modifier onlyAuthority () { + modifier onlyAuthority() { require(authorities.contains(msg.sender)); _; } @@ -286,7 +286,7 @@ contract ForeignBridge { /// deposit recipient (bytes20) /// deposit value (uint) /// mainnet transaction hash (bytes32) // to avoid transaction duplication - function deposit (address recipient, uint value, bytes32 transactionHash) public onlyAuthority() { + function deposit(address recipient, uint value, bytes32 transactionHash) public onlyAuthority() { // Protection from misbehaing authority var hash = keccak256(recipient, value, transactionHash); @@ -340,7 +340,7 @@ contract ForeignBridge { /// withdrawal recipient (bytes20) /// withdrawal value (uint) /// foreign transaction hash (bytes32) // to avoid transaction duplication - function submitSignature (bytes signature, bytes message) public onlyAuthority() { + function submitSignature(bytes signature, bytes message) public onlyAuthority() { // Validate submited signatures require(Signer.signer(signature, message) == msg.sender); @@ -361,12 +361,12 @@ contract ForeignBridge { } /// Get signature - function signature (bytes32 hash, uint index) public view returns (bytes) { + function signature(bytes32 hash, uint index) public view returns (bytes) { return signatures[hash].signatures[index]; } /// Get message - function message (bytes32 hash) public view returns (bytes) { + function message(bytes32 hash) public view returns (bytes) { return signatures[hash].message; } } From ae1d32639fd4da1175c4775f76a8ea2efc8a3cb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 14:50:43 +0100 Subject: [PATCH 4/9] test/foreign: underscore -> camelcase --- truffle/test/foreign.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/truffle/test/foreign.js b/truffle/test/foreign.js index 5110a99..ad001ce 100644 --- a/truffle/test/foreign.js +++ b/truffle/test/foreign.js @@ -91,7 +91,7 @@ contract('ForeignBridge', function(accounts) { var requiredSignatures = 2; var authorities = [accounts[0], accounts[1], accounts[2]]; var userAccount = accounts[3]; - var invalid_value = web3.toWei(2, "ether"); + var invalidValue = web3.toWei(2, "ether"); var value = web3.toWei(1, "ether"); var hash = "0xe55bb43c36cdf79e23b4adc149cdded921f0d482e613c50c6540977c213bc408"; @@ -100,7 +100,7 @@ contract('ForeignBridge', function(accounts) { return meta.deposit(userAccount, value, hash, { from: authorities[0] }); }).then(function(result) { assert.equal(0, result.logs.length, "No event should be created yet"); - return meta.deposit(userAccount, invalid_value, hash, { from: authorities[1] }); + return meta.deposit(userAccount, invalidValue, hash, { from: authorities[1] }); }).then(function(result) { assert.equal(0, result.logs.length, "Misbehaving authority should be ignored"); return meta.deposit(userAccount, value, hash, { from: authorities[2] }) From 82f0836e75bd4ed37a51497c85f1e9e95e15cb9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 14:51:17 +0100 Subject: [PATCH 5/9] test/foreign: improve some variable names and test names --- truffle/test/foreign.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/truffle/test/foreign.js b/truffle/test/foreign.js index ad001ce..c07054c 100644 --- a/truffle/test/foreign.js +++ b/truffle/test/foreign.js @@ -115,33 +115,34 @@ contract('ForeignBridge', function(accounts) { }) }) - it("should allow user to transfer value internally", function() { + it("should allow user to transfer value locally", function() { var meta; var requiredSignatures = 1; var authorities = [accounts[0], accounts[1]]; var userAccount = accounts[2]; var userAccount2 = accounts[3]; - var value = web3.toWei(3, "ether"); - var value2 = web3.toWei(1, "ether"); + var user1InitialValue = web3.toWei(3, "ether"); + var transferedValue = web3.toWei(1, "ether"); var hash = "0xe55bb43c36cdf79e23b4adc149cdded921f0d482e613c50c6540977c213bc408"; return ForeignBridge.new(requiredSignatures, authorities).then(function(instance) { meta = instance; - return meta.deposit(userAccount, value, hash, { from: authorities[0] }); + // top up balance so we can transfer + return meta.deposit(userAccount, user1InitialValue, hash, { from: authorities[0] }); }).then(function(result) { - return meta.transferLocal(userAccount2, value2, { from: userAccount }); + return meta.transferLocal(userAccount2, transferedValue, { from: userAccount }); }).then(function(result) { assert.equal(1, result.logs.length, "Exactly one event should be created"); assert.equal("Transfer", result.logs[0].event, "Event name should be Transfer"); assert.equal(userAccount, result.logs[0].args.from, "Event from should be transaction sender"); assert.equal(userAccount2, result.logs[0].args.to, "Event from should be transaction recipient"); - assert.equal(value2, result.logs[0].args.value, "Event value should match transaction value"); + assert.equal(transferedValue, result.logs[0].args.value, "Event value should match transaction value"); return Promise.all([ meta.balances.call(userAccount), meta.balances.call(userAccount2) ]) }).then(function(result) { assert.equal(web3.toWei(2, "ether"), result[0]); - assert.equal(web3.toWei(1, "ether"), result[1]); + assert.equal(transferedValue, result[1]); }) }) From bda5e1a472b1b07f1d1e6a376be7328eb19ebd67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 14:52:00 +0100 Subject: [PATCH 6/9] test/foreign: various improvements and more coverage for transferHomeViaRelay --- truffle/test/foreign.js | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/truffle/test/foreign.js b/truffle/test/foreign.js index c07054c..7082069 100644 --- a/truffle/test/foreign.js +++ b/truffle/test/foreign.js @@ -146,43 +146,51 @@ contract('ForeignBridge', function(accounts) { }) }) - it("should not allow user to transfer value", function() { + it("should not allow user to transfer value they don't have either locally or to home", function() { var meta; var requiredSignatures = 1; var authorities = [accounts[0], accounts[1]]; var userAccount = accounts[2]; - var userAccount2 = accounts[3]; - var value = web3.toWei(3, "ether"); - var value2 = web3.toWei(4, "ether"); + var recipientAccount = accounts[3]; + var userValue = web3.toWei(3, "ether"); + var transferedValue = web3.toWei(4, "ether"); var hash = "0xe55bb43c36cdf79e23b4adc149cdded921f0d482e613c50c6540977c213bc408"; return ForeignBridge.new(requiredSignatures, authorities).then(function(instance) { meta = instance; - return meta.deposit(userAccount, value, hash, { from: authorities[0] }); + return meta.deposit(userAccount, userValue, hash, { from: authorities[0] }); }).then(function(result) { - return meta.transferLocal(userAccount2, value2, { from: userAccount }); + return meta.transferLocal(recipientAccount, transferedValue, { from: userAccount }); }).then(function(result) { - assert(false, "Transfer should fail"); + assert(false, "transferLocal should fail"); + }, function(err) { + return meta.transferHomeViaRelay(recipientAccount, transferedValue, { from: userAccount }); + }).then(function(result) { + assert(false, "transferHomeViaRelay should fail"); }, function(err) { }) }) - it("should fail to transfer 0 value", function() { + it("should fail to transfer 0 value both locally and to home", function() { var meta; var requiredSignatures = 1; var authorities = [accounts[0], accounts[1]]; var userAccount = accounts[2]; - var userAccount2 = accounts[3]; - var value = web3.toWei(3, "ether"); - var value2 = web3.toWei(0, "ether"); + var recipientAccount = accounts[3]; + var userValue = web3.toWei(3, "ether"); + var transferedValue = web3.toWei(0, "ether"); var hash = "0xe55bb43c36cdf79e23b4adc149cdded921f0d482e613c50c6540977c213bc408"; return ForeignBridge.new(requiredSignatures, authorities).then(function(instance) { meta = instance; - return meta.deposit(userAccount, value, hash, { from: authorities[0] }); + return meta.deposit(userAccount, userValue, hash, { from: authorities[0] }); }).then(function(result) { - return meta.transferLocal(userAccount2, value2, { from: userAccount }); + return meta.transferLocal(recipientAccount, transferedValue, { from: userAccount }); }).then(function(result) { - assert(false, "Transfer of value 0 should fail"); - }, function (err) { + assert(false, "transferLocal should fail"); + }, function(err) { + return meta.transferHomeViaRelay(recipientAccount, transferedValue, { from: userAccount }); + }).then(function(result) { + assert(false, "transferHomeViaRelay should fail"); + }, function(err) { }) }) @@ -220,6 +228,7 @@ contract('ForeignBridge', function(accounts) { var hash = "0xe55bb43c36cdf79e23b4adc149cdded921f0d482e613c50c6540977c213bc408"; return ForeignBridge.new(requiredSignatures, authorities).then(function(instance) { meta = instance; + // top up balance so we can transfer return meta.deposit(userAccount, value, hash, { from: authorities[0] }); }).then(function(result) { return meta.transferHomeViaRelay(userAccount2, value2, { from: userAccount }); From 14b42a14ced41daed9723756d4d6cbb9476452b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 15:00:30 +0100 Subject: [PATCH 7/9] test/foreign: should fail to transfer with value overflow both locally and to home --- truffle/test/foreign.js | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/truffle/test/foreign.js b/truffle/test/foreign.js index 7082069..e6b9513 100644 --- a/truffle/test/foreign.js +++ b/truffle/test/foreign.js @@ -194,26 +194,27 @@ contract('ForeignBridge', function(accounts) { }) }) - it("should fail to transfer with value overflow", function() { + it("should fail to transfer with value overflow both locally and to home", function() { var meta; var requiredSignatures = 1; var authorities = [accounts[0], accounts[1]]; var userAccount = accounts[2]; - var userAccount2 = accounts[3]; - var value = web3.toWei("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", "wei"); - var value2 = web3.toWei(1, "wei"); + var recipientAccount = accounts[3]; + var userValue = web3.toWei(3, "ether"); + var transferedvalue = web3.toWei("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", "wei"); var hash = "0xe55bb43c36cdf79e23b4adc149cdded921f0d482e613c50c6540977c213bc408"; return ForeignBridge.new(requiredSignatures, authorities).then(function(instance) { meta = instance; - return Promise.all([ - meta.deposit(userAccount, value, hash, { from: authorities[0] }), - meta.deposit(userAccount2, value2, hash, { from: authorities[0] }), - ]) + return meta.deposit(userAccount, userValue, hash, { from: authorities[0] }); + }).then(function(result) { + return meta.transferLocal(recipientAccount, transferedValue, { from: userAccount }); }).then(function(result) { - return meta.transferLocal(userAccount2, value, { from: userAccount }); + assert(false, "transferLocal should fail"); + }, function(err) { + return meta.transferHomeViaRelay(recipientAccount, transferedValue, { from: userAccount }); }).then(function(result) { - assert(false, "Transfer with overflow should fail"); - }, function (err) { + assert(false, "transferHomeViaRelay should fail"); + }, function(err) { }) }) From 3a483672c180b0130d5e2b721026256252125292 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 15:05:50 +0100 Subject: [PATCH 8/9] test/foreign: fix test "should not be possible to submit signature twice" increasing coverage --- truffle/test/foreign.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/truffle/test/foreign.js b/truffle/test/foreign.js index e6b9513..49f7dca 100644 --- a/truffle/test/foreign.js +++ b/truffle/test/foreign.js @@ -401,19 +401,21 @@ contract('ForeignBridge', function(accounts) { it("should not be possible to submit signature twice", function() { var meta; - var requiredSignatures = 0; + var requiredSignatures = 1; var authorities = [accounts[0], accounts[1]]; var message = "0x111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111"; + var signature; return ForeignBridge.new(requiredSignatures, authorities).then(function(instance) { meta = instance; return helpers.sign(authorities[0], message); }).then(function(result) { - return meta.submitSignature(result, message, { from: authorities[0] }); - }).then(function(result) { - return meta.submitSignature(result, message, { from: authorities[0] }); - }).then(function(result) { + signature = result; + return meta.submitSignature(signature, message, { from: authorities[0] }); + }).then(function(_) { + return meta.submitSignature(signature, message, { from: authorities[0] }); + }).then(function(_) { assert(false, "submitSignature should fail"); - }, function (err) { + }, function (_) { // nothing }) }) From ddc2e02a8c211180b97789421bfca0835a1d4bc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Kru=CC=88ger?= Date: Thu, 11 Jan 2018 15:15:33 +0100 Subject: [PATCH 9/9] test/foreign: increase coverage: add test "should not be possible to do same deposit twice for same authority" --- truffle/test/foreign.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/truffle/test/foreign.js b/truffle/test/foreign.js index 49f7dca..5b0d22d 100644 --- a/truffle/test/foreign.js +++ b/truffle/test/foreign.js @@ -86,6 +86,25 @@ contract('ForeignBridge', function(accounts) { }) }) + it("should not be possible to do same deposit twice for same authority", function() { + var meta; + var requiredSignatures = 1; + var authorities = [accounts[0], accounts[1]]; + var userAccount = accounts[2]; + var value = web3.toWei(1, "ether"); + var hash = "0xe55bb43c36cdf79e23b4adc149cdded921f0d482e613c50c6540977c213bc408"; + + return ForeignBridge.new(requiredSignatures, authorities).then(function(instance) { + meta = instance; + return meta.deposit(userAccount, value, hash, { from: authorities[0] }); + }).then(function(_) { + return meta.deposit(userAccount, value, hash, { from: authorities[0] }); + }).then(function(result) { + assert(false, "doing same deposit twice from same authority should fail"); + }, function(err) { + }) + }) + it("should ignore misbehaving authority when confirming deposit", function() { var meta; var requiredSignatures = 2;