From 20acfe0c0405ede7019b8116ed3bd31477a52567 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Thu, 18 Jan 2018 03:55:34 +0300 Subject: [PATCH] merge with latest (1cef159) parity-bridge state --- erc20/bridge/contracts/bridge.sol | 355 ++++++++++++++++++++---------- erc20/bridge/erc20.toml | 8 +- 2 files changed, 248 insertions(+), 115 deletions(-) diff --git a/erc20/bridge/contracts/bridge.sol b/erc20/bridge/contracts/bridge.sol index 1cbac9b..ff6d162 100644 --- a/erc20/bridge/contracts/bridge.sol +++ b/erc20/bridge/contracts/bridge.sol @@ -1,43 +1,59 @@ -pragma solidity ^0.4.15; +pragma solidity ^0.4.17; -library Authorities { - function contains (address[] self, address value) internal pure returns (bool) { - for (uint i = 0; i < self.length; i++) { - if (self[i] == value) { + +/// general helpers. +/// `internal` so they get compiled into contracts using them. +library Helpers { + /// returns whether `array` contains `value`. + function addressArrayContains(address[] array, address value) internal pure returns (bool) { + for (uint i = 0; i < array.length; i++) { + if (array[i] == value) { return true; } } return false; } -} -/// Library used only to test Signer library via rpc calls -library SignerTest { - function signer (bytes signature, bytes message) public pure returns (address) { - return Signer.signer(signature, message); + // returns the digits of `inputValue` as a string. + // example: `uintToString(12345678)` returns `"12345678"` + function uintToString(uint inputValue) internal pure returns (string) { + // figure out the length of the resulting string + uint length = 0; + uint currentValue = inputValue; + do { + length++; + currentValue /= 10; + } while (currentValue != 0); + // allocate enough memory + bytes memory result = new bytes(length); + // construct the string backwards + uint i = length - 1; + currentValue = inputValue; + do { + result[i--] = byte(48 + currentValue % 10); + currentValue /= 10; + } while (currentValue != 0); + return string(result); } } -library Utils { - function toString (uint256 v) internal pure returns (string str) { - // it is used only for small numbers - bytes memory reversed = new bytes(8); - uint i = 0; - while (v != 0) { - uint remainder = v % 10; - v = v / 10; - reversed[i++] = byte(48 + remainder); - } - bytes memory s = new bytes(i); - for (uint j = 0; j < i; j++) { - s[j] = reversed[i - j - 1]; - } - str = string(s); + +/// Library used only to test Helpers library via rpc calls +library HelpersTest { + function addressArrayContains(address[] array, address value) public pure returns (bool) { + return Helpers.addressArrayContains(array, value); + } + + function uintToString(uint256 inputValue) public pure returns (string str) { + return Helpers.uintToString(inputValue); } } -library Signer { - function signer (bytes signature, bytes message) internal pure returns (address) { + +// helpers for message signing. +// `internal` so they get compiled into contracts using them. +library MessageSigning { + function recoverAddressFromSignedMessage(bytes signature, bytes message) internal pure returns (address) { require(signature.length == 65); bytes32 r; bytes32 s; @@ -48,23 +64,100 @@ library Signer { s := mload(add(signature, 0x40)) v := mload(add(signature, 0x60)) } - return ecrecover(hash(message), uint8(v), r, s); + return ecrecover(hashMessage(message), uint8(v), r, s); } - function hash (bytes message) internal pure returns (bytes32) { + function hashMessage(bytes message) internal pure returns (bytes32) { bytes memory prefix = "\x19Ethereum Signed Message:\n"; - return keccak256(prefix, Utils.toString(message.length), message); + return keccak256(prefix, Helpers.uintToString(message.length), message); } } -contract HomeBridge { - using Authorities for address[]; +/// Library used only to test MessageSigning library via rpc calls +library MessageSigningTest { + function recoverAddressFromSignedMessage(bytes signature, bytes message) public pure returns (address) { + return MessageSigning.recoverAddressFromSignedMessage(signature, message); + } +} + + +library Message { + // layout of message :: bytes: + // offset 0: 32 bytes :: uint (little endian) - message length + // offset 32: 20 bytes :: address - recipient address + // offset 52: 32 bytes :: uint (little endian) - value + // offset 84: 32 bytes :: bytes32 - transaction hash + + // bytes 1 to 32 are 0 because message length is stored as little endian. + // mload always reads 32 bytes. + // so we can and have to start reading recipient at offset 20 instead of 32. + // if we were to read at 32 the address would contain part of value and be corrupted. + // when reading from offset 20 mload will read 12 zero bytes followed + // by the 20 recipient address bytes and correctly convert it into an address. + // this saves some storage/gas over the alternative solution + // which is padding address to 32 bytes and reading recipient at offset 32. + // for more details see discussion in: + // https://github.com/paritytech/parity-bridge/issues/61 + + function getRecipient(bytes message) internal pure returns (address) { + address recipient; + // solium-disable-next-line security/no-inline-assembly + assembly { + recipient := mload(add(message, 20)) + } + return recipient; + } + + function getValue(bytes message) internal pure returns (uint) { + uint value; + // solium-disable-next-line security/no-inline-assembly + assembly { + value := mload(add(message, 52)) + } + return value; + } + + function getTransactionHash(bytes message) internal pure returns (bytes32) { + bytes32 hash; + // solium-disable-next-line security/no-inline-assembly + assembly { + hash := mload(add(message, 84)) + } + return hash; + } +} + + +/// Library used only to test Message library via rpc calls +library MessageTest { + function getRecipient(bytes message) public pure returns (address) { + return Message.getRecipient(message); + } + + function getValue(bytes message) public pure returns (uint) { + return Message.getValue(message); + } + + function getTransactionHash(bytes message) public pure returns (bytes32) { + return Message.getTransactionHash(message); + } +} + + +contract HomeBridge { /// Number of authorities signatures required to withdraw the money. /// /// Must be lesser than number of authorities. uint public requiredSignatures; + /// The gas cost of calling `HomeBridge.withdraw`. + /// + /// Is subtracted from `value` on withdraw. + /// recipient pays the relaying authority for withdraw. + /// this shuts down attacks that exhaust authorities funds on home chain. + uint public estimatedGasCostOfWithdraw; + /// Contract authorities. address[] public authorities; @@ -78,27 +171,33 @@ contract HomeBridge { event Withdraw (address recipient, uint value); /// Multisig authority validation - modifier allAuthorities (uint8[] v, bytes32[] r, bytes32[] s, bytes message) { - var hash = Signer.hash(message); + modifier allAuthorities(uint8[] v, bytes32[] r, bytes32[] s, bytes message) { + var hash = MessageSigning.hashMessage(message); var used = new address[](requiredSignatures); require(requiredSignatures <= v.length); for (uint i = 0; i < requiredSignatures; i++) { var a = ecrecover(hash, v[i], r[i], s[i]); - require(authorities.contains(a)); - require(!used.contains(a)); + require(Helpers.addressArrayContains(authorities, a)); + require(!Helpers.addressArrayContains(used, a)); used[i] = a; } _; } /// Constructor. - function HomeBridge (uint n, address[] a) public { - require(n != 0); - require(n <= a.length); - requiredSignatures = n; - authorities = a; + function HomeBridge( + uint requiredSignaturesParam, + address[] authoritiesParam, + uint estimatedGasCostOfWithdrawParam + ) public + { + require(requiredSignaturesParam != 0); + require(requiredSignaturesParam <= authoritiesParam.length); + requiredSignatures = requiredSignaturesParam; + authorities = authoritiesParam; + estimatedGasCostOfWithdraw = estimatedGasCostOfWithdrawParam; } /// Should be used to deposit money. @@ -106,6 +205,18 @@ contract HomeBridge { Deposit(msg.sender, msg.value); } + /// to be called by authorities to check + /// whether they withdraw message should be relayed or whether it + /// is too low to cover the cost of calling withdraw and can be ignored + function isMessageValueSufficientToCoverRelay(bytes message) public view returns (bool) { + return Message.getValue(message) > getWithdrawRelayCost(); + } + + /// an upper bound to the cost of relaying a withdraw by calling HomeBridge.withdraw + function getWithdrawRelayCost() public view returns (uint) { + return estimatedGasCostOfWithdraw * tx.gasprice; + } + /// Used to withdraw money from the contract. /// /// message contains: @@ -113,44 +224,36 @@ contract HomeBridge { /// withdrawal value (uint) /// 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) { + /// 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) { require(message.length == 84); - address recipient; - uint value; - bytes32 hash; - // solium-disable-next-line security/no-inline-assembly - assembly { - // layout of message :: bytes: - // offset 0: 32 bytes :: uint (little endian) - message length - // offset 32: 20 bytes :: address - recipient address - // offset 52: 32 bytes :: uint (little endian) - value - // offset 84: 32 bytes :: bytes32 - transaction hash + address recipient = Message.getRecipient(message); + uint value = Message.getValue(message); + bytes32 hash = Message.getTransactionHash(message); - // we require above that message length == 84. - // bytes 1 to 32 are 0 because message length is stored as little endian. - // mload always reads 32 bytes. - // so we can and have to start reading recipient at offset 20 instead of 32. - // if we were to read at 32 the address would contain part of value and be corrupted. - // when reading from offset 20 mload will read 12 zero bytes followed - // by the 20 recipient address bytes and correctly convert it into an address. - // this saves some storage/gas over the alternative solution - // which is padding address to 32 bytes and reading recipient at offset 32. - // for more details see discussion in: - // https://github.com/paritytech/parity-bridge/issues/61 - recipient := mload(add(message, 20)) - value := mload(add(message, 52)) - hash := mload(add(message, 84)) - } - - // Duplicated withdraw + // The following two statements guard against reentry into this function. + // Duplicated withdraw or reentry. require(!withdraws[hash]); - - // Order of operations below is critical to avoid TheDAO-like bug + // Order of operations below is critical to avoid TheDAO-like re-entry bug withdraws[hash] = true; - recipient.transfer(value); - Withdraw(recipient, value); + + // this fails if `value` is not even enough to cover the relay cost. + // Authorities simply IGNORE withdraws where `value` can’t relay cost. + // Think of it as `value` getting burned entirely on the relay with no value left to pay out the recipient. + require(isMessageValueSufficientToCoverRelay(message)); + + uint estimatedWeiCostOfWithdraw = getWithdrawRelayCost(); + + // charge recipient for relay cost + uint valueRemainingAfterSubtractingCost = value - estimatedWeiCostOfWithdraw; + + // pay out recipient + recipient.transfer(valueRemainingAfterSubtractingCost); + + // refund relay cost to relaying authority + msg.sender.transfer(estimatedWeiCostOfWithdraw); + + Withdraw(recipient, valueRemainingAfterSubtractingCost); } } @@ -161,8 +264,6 @@ contract ERC20 { } contract ForeignBridge { - using Authorities for address[]; - struct SignaturesCollection { /// Signed message. bytes message; @@ -180,41 +281,51 @@ contract ForeignBridge { /// Contract authorities. address[] public authorities; + /// Ether balances + mapping (address => uint) public balances; + /// Pending deposits and authorities who confirmed them mapping (bytes32 => address[]) deposits; - /// List of authorities confirmed to set up ERC-20 token address - mapping (address => address[]) public token_address; - - /// Token to work with - ERC20 public erc20token; - - /// Event created when new token address is set up. - event TokenAddress(address token); - /// Pending signatures and authorities who confirmed them mapping (bytes32 => SignaturesCollection) signatures; + /// List of authorities confirmed to set up ERC-20 token address + mapping (address => address[]) tokenAddressAprrovals; + + /// Token to work with + ERC20 public erc20token; + /// Event created on money deposit. event Deposit(address recipient, uint value); /// Event created on money withdraw. event Withdraw(address recipient, uint value); + /// Event created on money transfer + event Transfer(address from, address to, uint value); + /// Collected signatures which should be relayed to home chain. event CollectedSignatures(address authority, bytes32 messageHash); + /// Event created when new token address is set up. + event TokenAddress(address token); + /// Constructor. - function ForeignBridge(uint n, address[] a) public { - require(n != 0); - require(n <= a.length); - requiredSignatures = n; - authorities = a; + function ForeignBridge( + uint requiredSignaturesParam, + address[] authoritiesParam + ) public + { + require(requiredSignaturesParam != 0); + require(requiredSignaturesParam <= authoritiesParam.length); + requiredSignatures = requiredSignaturesParam; + authorities = authoritiesParam; } /// Multisig authority validation - modifier onlyAuthority () { - require(authorities.contains(msg.sender)); + modifier onlyAuthority() { + require(Helpers.addressArrayContains(authorities, msg.sender)); _; } @@ -223,27 +334,32 @@ contract ForeignBridge { /// token address (address) function setTokenAddress (ERC20 token) public onlyAuthority() { // Protect duplicated request - require(!token_address[token].contains(msg.sender)); + require(!Helpers.addressArrayContains(tokenAddressAprrovals[token], msg.sender)); - token_address[token].push(msg.sender); + tokenAddressAprrovals[token].push(msg.sender); // TODO: this may cause troubles if requriedSignatures len is changed - if (token_address[token].length == requiredSignatures) { + if (tokenAddressAprrovals[token].length == requiredSignatures) { erc20token = ERC20(token); TokenAddress(token); } } - /// Used to deposit money to the contract. + /// Used to transfer tokens to the `recipient`. + /// The bridge contract must own enough tokens to release them for + /// recipients. Tokens must be transfered to the bridge contract BEFORE + /// the first deposit will be performed. /// /// 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() { + require(erc20token != address(0x0)); + // Protection from misbehaing authority - var hash = keccak256(recipient, value, transactionHash); + bytes32 hash = keccak256(recipient, value, transactionHash); // Duplicated deposits - require(!deposits[hash].contains(msg.sender)); + require(!Helpers.addressArrayContains(deposits[hash], msg.sender)); deposits[hash].push(msg.sender); // TODO: this may cause troubles if requriedSignatures len is changed @@ -253,15 +369,30 @@ contract ForeignBridge { } } - /// To swap token to money the account must allow this explicitly - /// in the token contract by calling approveAndCall with address + /// Used to transfer `value` of tokens from `_from`s balance on local + /// (`foreign`) chain to the same address (`_from`) on `home` chain. + /// Transfer of tokens within local (`foreign`) chain performed by usual + /// way through transfer method of the token contract. + /// In order to swap tokens to coins the owner (`_from`) must allow this + /// explicitly in the token contract by calling approveAndCall with address /// of the bridge account. - function receiveApproval(address _from, uint256 _value, address _tokenContract, bytes _msg) external returns(bool) { - require(msg.sender == address(erc20token)); - require(erc20token.allowance(_from, this) >= _value); + /// The method locks tokens and 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`, the recipient (`_from`) 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 the + /// recipient completing the transfer. + function receiveApproval(address _from, uint256 _value, ERC20 _tokenContract, bytes _msg) external returns(bool) { + require(erc20token != address(0x0)); + require(msg.sender == address(erc20token)); + require(erc20token.allowance(_from, this) >= _value); erc20token.transferFrom(_from, this, _value); Withdraw(_from, _value); - + return true; } @@ -273,16 +404,16 @@ 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); + require(MessageSigning.recoverAddressFromSignedMessage(signature, message) == msg.sender); // Valid withdraw message must have 84 bytes require(message.length == 84); - var hash = keccak256(message); + bytes32 hash = keccak256(message); // Duplicated signatures - require(!signatures[hash].signed.contains(msg.sender)); + require(!Helpers.addressArrayContains(signatures[hash].signed, msg.sender)); signatures[hash].message = message; signatures[hash].signed.push(msg.sender); signatures[hash].signatures.push(signature); @@ -294,12 +425,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; } } diff --git a/erc20/bridge/erc20.toml b/erc20/bridge/erc20.toml index 8e1b1bd..24104d6 100644 --- a/erc20/bridge/erc20.toml +++ b/erc20/bridge/erc20.toml @@ -1,3 +1,5 @@ +estimated_gas_cost_of_withdraw = 0 + [home] account = "0x842eb2142c5aa1260954f07aae39ddee1640c3a7" ipc = "/home/koal/parity/PoA_home/jsonrpc.ipc" @@ -27,6 +29,6 @@ required_signatures = 1 [transactions] home_deploy = { gas = 3000000, gas_price = 18000000000 } foreign_deploy = { gas = 3000000, gas_price = 18000000000 } -deposit_relay = { gas = 120000, gas_price = 18000000000 } -withdraw_confirm = { gas = 120000, gas_price = 18000000000 } -withdraw_relay = { gas = 300000, gas_price = 18000000000 } +deposit_relay = { gas = 1200000, gas_price = 18000000000 } +withdraw_confirm = { gas = 3000000, gas_price = 18000000000 } +withdraw_relay = { gas = 3000000, gas_price = 18000000000 }