Skip to content

Commit

Permalink
Merge pull request omni#81 from paritytech/snd-issue-55
Browse files Browse the repository at this point in the history 
issue 55 - split ForeignBridge.transfer into two functions and increase branch coverage
  • Loading branch information
@debris
debris authored Jan 11, 2018
2 parents 85ee965 + ddc2e02 commit 1e86ac2
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 63 deletions.
58 changes: 37 additions & 21 deletions contracts/bridge.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);

Expand All @@ -107,7 +107,7 @@ contract HomeBridge {
}

/// Constructor.
function HomeBridge (
function HomeBridge(
uint requiredSignaturesParam,
address[] authoritiesParam,
uint estimatedGasCostOfWithdrawParam
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -276,7 +276,7 @@ contract ForeignBridge {
}

/// Multisig authority validation
modifier onlyAuthority () {
modifier onlyAuthority() {
require(authorities.contains(msg.sender));
_;
}
Expand All @@ -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);

Expand All @@ -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
Expand All @@ -324,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);

Expand All @@ -345,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;
}
}
116 changes: 74 additions & 42 deletions truffle/test/foreign.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,31 @@ 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;
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";

Expand All @@ -100,7 +119,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] })
Expand All @@ -115,96 +134,106 @@ 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.transfer(userAccount2, value2, false, { 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]);
})
})

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.transfer(userAccount2, value2, false, { 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.transfer(userAccount2, value2, false, { 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) {
})
})

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.transfer(userAccount2, value, false, { from: userAccount });
return meta.transferLocal(recipientAccount, transferedValue, { from: userAccount });
}).then(function(result) {
assert(false, "Transfer with overflow 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) {
})
})

Expand All @@ -219,9 +248,10 @@ 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.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");
Expand Down Expand Up @@ -390,19 +420,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
})
})
Expand Down

0 comments on commit 1e86ac2

Please sign in to comment.