From 69c207c1831ead05b2256d4f7aafae44c599c87d Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Wed, 14 Oct 2020 22:44:39 +0300 Subject: [PATCH 01/14] Add extra check on internal call gaslimit (#502) --- .../arbitrary_message/MessageProcessor.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol b/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol index acda93dba..14cc1eeff 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol @@ -194,6 +194,19 @@ contract MessageProcessor is EternalStorage { setMessageSender(_sender); setMessageId(_messageId); setMessageSourceChainId(_sourceChainId); + + // After EIP-150, max gas cost allowed to be passed to the internal call is equal to the 63/64 of total gas left. + // In reallity, min(gasLimit, 63/64 * gasleft()) will be used as the call gas limit. + // Imagine a situation, when message requires 10000000 gas to be executed successfully. + // Also suppose, that at this point, gasleft() is equal to 10158000, so the callee will receive ~ 10158000 * 63 / 64 = 9999300 gas. + // That amount of gas is not enough, so the call will fail. At the same time, + // even if the callee failed the bridge contract still has ~ 158000 gas to + // finish its execution and it will be enough. The internal call fails but + // only because the oracle provides incorrect gas limit for the transaction + // This check is needed here in order to force contract to pass exactly the requested amount of gas. + // Avoiding it may leed to the unwanted message failure in some extreme cases. + require((gasleft() * 63) / 64 > _gas); + bool status = _contract.call.gas(_gas)(_data); setMessageSender(address(0)); setMessageId(bytes32(0)); From 3c18c12d39c0a8932f59aeb9c8d60dc7b2c63737 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Wed, 14 Oct 2020 22:58:34 +0300 Subject: [PATCH 02/14] Add missing zero address checks (#503) --- .../upgradeable_contracts/BridgeValidators.sol | 3 +-- .../OtherSideBridgeStorage.sol | 1 + contracts/upgradeable_contracts/Ownable.sol | 6 +++--- .../upgradeable_contracts/RewardableValidators.sol | 3 +-- .../amb_erc20_to_native/BasicAMBErc20ToNative.sol | 3 +-- .../amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol | 2 +- .../amb_native_to_erc20/BasicAMBNativeToErc20.sol | 3 +-- .../arbitrary_message/BasicAMB.sol | 2 +- .../erc20_to_erc20/BasicForeignBridgeErcToErc.sol | 3 +-- .../erc20_to_erc20/HomeBridgeErcToErc.sol | 4 ++-- .../erc20_to_native/ForeignBridgeErcToNative.sol | 4 +--- .../erc20_to_native/HomeBridgeErcToNative.sol | 3 +-- .../ForeignMultiAMBErc20ToErc677.sol | 3 +-- .../HomeMultiAMBErc20ToErc677.sol | 3 +-- .../native_to_erc20/ForeignBridgeNativeToErc.sol | 3 +-- .../native_to_erc20/HomeBridgeNativeToErc.sol | 3 +-- .../AMBErc677ToErc677Behavior.test.js | 12 ++++++++++++ test/arbitrary_message/foreign_bridge.test.js | 11 +++++++++++ test/arbitrary_message/home_bridge.test.js | 11 +++++++++++ test/native_to_erc/foreign_bridge_test.js | 13 +++++++++++++ 20 files changed, 66 insertions(+), 30 deletions(-) diff --git a/contracts/upgradeable_contracts/BridgeValidators.sol b/contracts/upgradeable_contracts/BridgeValidators.sol index 1951b2ada..7f2de18a6 100644 --- a/contracts/upgradeable_contracts/BridgeValidators.sol +++ b/contracts/upgradeable_contracts/BridgeValidators.sol @@ -9,8 +9,7 @@ contract BridgeValidators is BaseBridgeValidators { returns (bool) { require(!isInitialized()); - require(_owner != address(0)); - setOwner(_owner); + _setOwner(_owner); require(_requiredSignatures != 0); require(_initialValidators.length >= _requiredSignatures); diff --git a/contracts/upgradeable_contracts/OtherSideBridgeStorage.sol b/contracts/upgradeable_contracts/OtherSideBridgeStorage.sol index 023ae3950..aa2a5bea8 100644 --- a/contracts/upgradeable_contracts/OtherSideBridgeStorage.sol +++ b/contracts/upgradeable_contracts/OtherSideBridgeStorage.sol @@ -6,6 +6,7 @@ contract OtherSideBridgeStorage is EternalStorage { bytes32 internal constant BRIDGE_CONTRACT = 0x71483949fe7a14d16644d63320f24d10cf1d60abecc30cc677a340e82b699dd2; // keccak256(abi.encodePacked("bridgeOnOtherSide")) function _setBridgeContractOnOtherSide(address _bridgeContract) internal { + require(_bridgeContract != address(0)); addressStorage[BRIDGE_CONTRACT] = _bridgeContract; } diff --git a/contracts/upgradeable_contracts/Ownable.sol b/contracts/upgradeable_contracts/Ownable.sol index b9846b628..0d14f5b2d 100644 --- a/contracts/upgradeable_contracts/Ownable.sol +++ b/contracts/upgradeable_contracts/Ownable.sol @@ -55,14 +55,14 @@ contract Ownable is EternalStorage { * @param newOwner the address to transfer ownership to. */ function transferOwnership(address newOwner) external onlyOwner { - require(newOwner != address(0)); - setOwner(newOwner); + _setOwner(newOwner); } /** * @dev Sets a new owner address */ - function setOwner(address newOwner) internal { + function _setOwner(address newOwner) internal { + require(newOwner != address(0)); emit OwnershipTransferred(owner(), newOwner); addressStorage[OWNER] = newOwner; } diff --git a/contracts/upgradeable_contracts/RewardableValidators.sol b/contracts/upgradeable_contracts/RewardableValidators.sol index c1e82d974..bcb866927 100644 --- a/contracts/upgradeable_contracts/RewardableValidators.sol +++ b/contracts/upgradeable_contracts/RewardableValidators.sol @@ -10,8 +10,7 @@ contract RewardableValidators is BaseBridgeValidators { address _owner ) external onlyRelevantSender returns (bool) { require(!isInitialized()); - require(_owner != address(0)); - setOwner(_owner); + _setOwner(_owner); require(_requiredSignatures != 0); require(_initialValidators.length >= _requiredSignatures); require(_initialValidators.length == _initialRewards.length); diff --git a/contracts/upgradeable_contracts/amb_erc20_to_native/BasicAMBErc20ToNative.sol b/contracts/upgradeable_contracts/amb_erc20_to_native/BasicAMBErc20ToNative.sol index 2554c5293..58b435c86 100644 --- a/contracts/upgradeable_contracts/amb_erc20_to_native/BasicAMBErc20ToNative.sol +++ b/contracts/upgradeable_contracts/amb_erc20_to_native/BasicAMBErc20ToNative.sol @@ -33,7 +33,6 @@ contract BasicAMBErc20ToNative is Initializable, Upgradeable, Claimable, Version address _owner ) internal { require(!isInitialized()); - require(_owner != address(0)); _setBridgeContract(_bridgeContract); _setMediatorContractOnOtherSide(_mediatorContract); @@ -41,7 +40,7 @@ contract BasicAMBErc20ToNative is Initializable, Upgradeable, Claimable, Version _setLimits(_dailyLimitMaxPerTxMinPerTxArray); _setExecutionLimits(_executionDailyLimitExecutionMaxPerTxArray); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); } /** diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol index 3ddc69905..c28c4aa3a 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol @@ -44,7 +44,7 @@ contract BasicAMBErc677ToErc677 is _setExecutionLimits(_executionDailyLimitExecutionMaxPerTxArray); _setRequestGasLimit(_requestGasLimit); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); setInitialize(); return isInitialized(); diff --git a/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol b/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol index e7e67bb86..6399855b5 100644 --- a/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol +++ b/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol @@ -43,7 +43,6 @@ contract BasicAMBNativeToErc20 is address _feeManager ) internal { require(!isInitialized()); - require(_owner != address(0)); _setBridgeContract(_bridgeContract); _setMediatorContractOnOtherSide(_mediatorContract); @@ -52,7 +51,7 @@ contract BasicAMBNativeToErc20 is _setExecutionLimits(_executionDailyLimitExecutionMaxPerTxArray); _setDecimalShift(_decimalShift); _setFeeManagerContract(_feeManager); - setOwner(_owner); + _setOwner(_owner); } /** diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol index 21ad38fb0..1f7e2684b 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol @@ -39,7 +39,7 @@ contract BasicAMB is BasicBridge, VersionableAMB { uintStorage[MAX_GAS_PER_TX] = _maxGasPerTx; _setGasPrice(_gasPrice); _setRequiredBlockConfirmations(_requiredBlockConfirmations); - setOwner(_owner); + _setOwner(_owner); setInitialize(); return isInitialized(); diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol b/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol index 3475a0006..4cb76a464 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol @@ -16,7 +16,6 @@ contract BasicForeignBridgeErcToErc is BasicForeignBridge { ) internal { require(!isInitialized()); require(AddressUtils.isContract(_validatorContract)); - require(_owner != address(0)); addressStorage[VALIDATOR_CONTRACT] = _validatorContract; setErc20token(_erc20token); @@ -26,7 +25,7 @@ contract BasicForeignBridgeErcToErc is BasicForeignBridge { _setLimits(_dailyLimitMaxPerTxMinPerTxArray); _setExecutionLimits(_homeDailyLimitHomeMaxPerTxArray); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); setInitialize(); } diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol index 3f8626f58..c6517d447 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol @@ -109,7 +109,7 @@ contract HomeBridgeErcToErc is ) internal { require(!isInitialized()); require(AddressUtils.isContract(_validatorContract)); - require(_owner != address(0)); + addressStorage[VALIDATOR_CONTRACT] = _validatorContract; uintStorage[DEPLOYED_AT_BLOCK] = block.number; _setLimits(_dailyLimitMaxPerTxMinPerTxArray); @@ -117,7 +117,7 @@ contract HomeBridgeErcToErc is _setRequiredBlockConfirmations(_requiredBlockConfirmations); _setExecutionLimits(_foreignDailyLimitForeignMaxPerTxArray); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); setErc677token(_erc677token); } diff --git a/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol index 9a8262111..3d5c8a9ee 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol @@ -19,8 +19,6 @@ contract ForeignBridgeErcToNative is BasicForeignBridge, ERC20Bridge, OtherSideB ) external onlyRelevantSender returns (bool) { require(!isInitialized()); require(AddressUtils.isContract(_validatorContract)); - require(_owner != address(0)); - require(_bridgeOnOtherSide != address(0)); addressStorage[VALIDATOR_CONTRACT] = _validatorContract; setErc20token(_erc20token); @@ -30,7 +28,7 @@ contract ForeignBridgeErcToNative is BasicForeignBridge, ERC20Bridge, OtherSideB _setLimits(_dailyLimitMaxPerTxMinPerTxArray); _setExecutionLimits(_homeDailyLimitHomeMaxPerTxArray); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); _setBridgeContractOnOtherSide(_bridgeOnOtherSide); setInitialize(); diff --git a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol index 46415e0d3..8f032fc4b 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol @@ -132,7 +132,6 @@ contract HomeBridgeErcToNative is require(!isInitialized()); require(AddressUtils.isContract(_validatorContract)); require(_blockReward == address(0) || AddressUtils.isContract(_blockReward)); - require(_owner != address(0)); addressStorage[VALIDATOR_CONTRACT] = _validatorContract; uintStorage[DEPLOYED_AT_BLOCK] = block.number; @@ -142,7 +141,7 @@ contract HomeBridgeErcToNative is addressStorage[BLOCK_REWARD_CONTRACT] = _blockReward; _setExecutionLimits(_foreignDailyLimitForeignMaxPerTxArray); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); } function onExecuteAffirmation(address _recipient, uint256 _value, bytes32 txHash) internal returns (bool) { diff --git a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol index 71e37e89e..ff59e25e0 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol @@ -35,14 +35,13 @@ contract ForeignMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677 { address _owner ) external onlyRelevantSender returns (bool) { require(!isInitialized()); - require(_owner != address(0)); _setBridgeContract(_bridgeContract); _setMediatorContractOnOtherSide(_mediatorContract); _setLimits(address(0), _dailyLimitMaxPerTxMinPerTxArray); _setExecutionLimits(address(0), _executionDailyLimitExecutionMaxPerTxArray); _setRequestGasLimit(_requestGasLimit); - setOwner(_owner); + _setOwner(_owner); setInitialize(); diff --git a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol index 163c745bb..69789dcc2 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol @@ -42,14 +42,13 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager uint256[2] _fees // [ 0 = homeToForeignFee, 1 = foreignToHomeFee ] ) external onlyRelevantSender returns (bool) { require(!isInitialized()); - require(_owner != address(0)); _setBridgeContract(_bridgeContract); _setMediatorContractOnOtherSide(_mediatorContract); _setLimits(address(0), _dailyLimitMaxPerTxMinPerTxArray); _setExecutionLimits(address(0), _executionDailyLimitExecutionMaxPerTxArray); _setRequestGasLimit(_requestGasLimit); - setOwner(_owner); + _setOwner(_owner); _setTokenImage(_tokenImage); if (_rewardAddreses.length > 0) { _setRewardAddressList(_rewardAddreses); diff --git a/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol index b388ead0a..d9872bd8e 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol @@ -89,7 +89,6 @@ contract ForeignBridgeNativeToErc is ) internal { require(!isInitialized()); require(AddressUtils.isContract(_validatorContract)); - require(_owner != address(0)); addressStorage[VALIDATOR_CONTRACT] = _validatorContract; setErc677token(_erc677token); @@ -99,7 +98,7 @@ contract ForeignBridgeNativeToErc is _setRequiredBlockConfirmations(_requiredBlockConfirmations); _setExecutionLimits(_homeDailyLimitHomeMaxPerTxArray); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); _setBridgeContractOnOtherSide(_bridgeOnOtherSide); } diff --git a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol index e257fdfb6..9ff454850 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol @@ -94,7 +94,6 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicHomeBridge, RewardableHom ) internal { require(!isInitialized()); require(AddressUtils.isContract(_validatorContract)); - require(_owner != address(0)); addressStorage[VALIDATOR_CONTRACT] = _validatorContract; uintStorage[DEPLOYED_AT_BLOCK] = block.number; @@ -103,7 +102,7 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicHomeBridge, RewardableHom _setRequiredBlockConfirmations(_requiredBlockConfirmations); _setExecutionLimits(_foreignDailyLimitForeignMaxPerTxArray); _setDecimalShift(_decimalShift); - setOwner(_owner); + _setOwner(_owner); } function onSignaturesCollected(bytes _message) internal { diff --git a/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js b/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js index b92290e98..3559e603d 100644 --- a/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js +++ b/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js @@ -144,6 +144,18 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou owner ).should.be.rejected + // not valid owner + await contract.initialize( + bridgeContract.address, + mediatorContract.address, + erc677Token.address, + [dailyLimit, maxPerTx, minPerTx], + [executionDailyLimit, executionMaxPerTx], + maxGasPerTx, + decimalShiftZero, + ZERO_ADDRESS + ).should.be.rejected + const { logs } = await contract.initialize( bridgeContract.address, mediatorContract.address, diff --git a/test/arbitrary_message/foreign_bridge.test.js b/test/arbitrary_message/foreign_bridge.test.js index b490002d0..a3415678a 100644 --- a/test/arbitrary_message/foreign_bridge.test.js +++ b/test/arbitrary_message/foreign_bridge.test.js @@ -150,6 +150,17 @@ contract('ForeignAMB', async accounts => { await foreignBridge .initialize(FOREIGN_CHAIN_ID_HEX, HOME_CHAIN_ID_HEX, validatorContract.address, oneEther, gasPrice, 0, owner) .should.be.rejectedWith(ERROR_MSG) + await foreignBridge + .initialize( + FOREIGN_CHAIN_ID_HEX, + HOME_CHAIN_ID_HEX, + validatorContract.address, + oneEther, + gasPrice, + requiredBlockConfirmations, + ZERO_ADDRESS + ) + .should.be.rejectedWith(ERROR_MSG) await foreignBridge.initialize( FOREIGN_CHAIN_ID_HEX, HOME_CHAIN_ID_HEX, diff --git a/test/arbitrary_message/home_bridge.test.js b/test/arbitrary_message/home_bridge.test.js index 87e9af167..e330c8c3f 100644 --- a/test/arbitrary_message/home_bridge.test.js +++ b/test/arbitrary_message/home_bridge.test.js @@ -130,6 +130,17 @@ contract('HomeAMB', async accounts => { await homeBridge .initialize(HOME_CHAIN_ID_HEX, FOREIGN_CHAIN_ID_HEX, validatorContract.address, oneEther, gasPrice, 0, owner) .should.be.rejectedWith(ERROR_MSG) + await homeBridge + .initialize( + HOME_CHAIN_ID_HEX, + FOREIGN_CHAIN_ID_HEX, + validatorContract.address, + oneEther, + 0, + requiredBlockConfirmations, + ZERO_ADDRESS + ) + .should.be.rejectedWith(ERROR_MSG) await homeBridge.initialize( HOME_CHAIN_ID_HEX, FOREIGN_CHAIN_ID_HEX, diff --git a/test/native_to_erc/foreign_bridge_test.js b/test/native_to_erc/foreign_bridge_test.js index 6cafac935..b17191875 100644 --- a/test/native_to_erc/foreign_bridge_test.js +++ b/test/native_to_erc/foreign_bridge_test.js @@ -155,6 +155,19 @@ contract('ForeignBridge', async accounts => { otherSideBridgeAddress ).should.be.rejected + // not valid otherSideBridgeAddress + await foreignBridge.initialize( + validatorContract.address, + token.address, + [oneEther, halfEther, minPerTx], + gasPrice, + requireBlockConfirmations, + [homeDailyLimit, homeMaxPerTx], + owner, + '9', + ZERO_ADDRESS + ).should.be.rejected + const { logs } = await foreignBridge.initialize( validatorContract.address, token.address, From 82e1864a5130cdf80a0f2a2062169fa92cb4f9d3 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 01:48:55 +0300 Subject: [PATCH 03/14] Correct handling of rewards addresses count (#525) --- .../upgradeable_contracts/BaseMediatorFeeManager.sol | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol b/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol index 7e0f6eb7b..09922b7b6 100644 --- a/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol +++ b/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol @@ -82,7 +82,7 @@ contract BaseMediatorFeeManager is Ownable { function addRewardAccount(address _account) external onlyOwner { require(isValidAccount(_account)); require(!isRewardAccount(_account)); - require(rewardAccounts.length.add(1) < MAX_REWARD_ACCOUNTS); + require(rewardAccounts.length < MAX_REWARD_ACCOUNTS); rewardAccounts.push(_account); } @@ -154,6 +154,12 @@ contract BaseMediatorFeeManager is Ownable { */ function distributeFee(uint256 _fee) internal { uint256 numOfAccounts = rewardAccountsCount(); + if (numOfAccounts == 0) { + // In case there are no reward accounts defined, no actual fee distribution will happen. + // Funds will be kept locked on the contract until some of the reward accounts will be added. + // After it, locked funds ca be distributed by a call to onTokenTransfer() of this contract, which can be done by anyone. + return; + } uint256 feePerAccount = _fee.div(numOfAccounts); uint256 randomAccountIndex; uint256 diff = _fee.sub(feePerAccount.mul(numOfAccounts)); From 910cacdc374625c44a12863c61d52a64077ba76a Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 01:59:18 +0300 Subject: [PATCH 04/14] Remove unused gasPrice fields (#527) --- contracts/libraries/ArbitraryMessage.sol | 32 +-- contracts/mocks/MessageTest.sol | 14 +- .../arbitrary_message/BasicForeignAMB.sol | 10 +- .../arbitrary_message/BasicHomeAMB.sol | 11 +- .../arbitrary_message/MessageProcessor.sol | 3 +- test/libraries/arbitraryMessage.test.js | 200 ++---------------- 6 files changed, 39 insertions(+), 231 deletions(-) diff --git a/contracts/libraries/ArbitraryMessage.sol b/contracts/libraries/ArbitraryMessage.sol index e2b8bb4a6..846eb12bd 100644 --- a/contracts/libraries/ArbitraryMessage.sol +++ b/contracts/libraries/ArbitraryMessage.sol @@ -11,11 +11,9 @@ library ArbitraryMessage { * offset 104 : 4 bytes :: uint32 - gasLimit * offset 108 : 1 bytes :: uint8 - source chain id length (X) * offset 109 : 1 bytes :: uint8 - destination chain id length (Y) - * offset 110 : 1 bytes :: bytes1 - dataType - * (optional) 111 : 32 bytes :: uint256 - gasPrice - * (optional) 111 : 1 bytes :: bytes1 - gasPriceSpeed - * offset 111/143/112 : X bytes :: bytes - source chain id - * offset 111/143/112 + X : Y bytes :: bytes - destination chain id + * offset 110 : 1 bytes :: uint8 - dataType + * offset 111 : X bytes :: bytes - source chain id + * offset 111 + X : Y bytes :: bytes - destination chain id * NOTE: when message structure is changed, make sure that MESSAGE_PACKING_VERSION from VersionableAMB is updated as well * NOTE: assembly code uses calldatacopy, make sure that message is passed as the first argument in the calldata @@ -29,9 +27,8 @@ library ArbitraryMessage { address sender, address executor, uint32 gasLimit, - bytes1 dataType, + uint8 dataType, uint256[2] chainIds, - uint256 gasPrice, bytes memory data ) { @@ -50,23 +47,16 @@ library ArbitraryMessage { executor := shr(96, blob) gasLimit := and(shr(64, blob), 0xffffffff) + dataType := byte(26, blob) + if gt(dataType, 0) { + // for now, only 0 datatype is supported - regular AMB calls + // other dataType values are kept reserved for future use + revert(0, 0) + } + // load source chain id length let chainIdLength := byte(24, blob) - dataType := and(shl(208, blob), 0xFF00000000000000000000000000000000000000000000000000000000000000) - switch dataType - case 0x0000000000000000000000000000000000000000000000000000000000000000 { - gasPrice := 0 - } - case 0x0100000000000000000000000000000000000000000000000000000000000000 { - gasPrice := mload(add(_data, 111)) // 32 - srcdataptr := add(srcdataptr, 32) - } - case 0x0200000000000000000000000000000000000000000000000000000000000000 { - gasPrice := 0 - srcdataptr := add(srcdataptr, 1) - } - // at this moment srcdataptr points to sourceChainId // mask for sourceChainId diff --git a/contracts/mocks/MessageTest.sol b/contracts/mocks/MessageTest.sol index 8c77eb983..e96783a0c 100644 --- a/contracts/mocks/MessageTest.sol +++ b/contracts/mocks/MessageTest.sol @@ -11,15 +11,12 @@ contract MessageTest { address sender, address executor, uint32 gasLimit, - bytes1 dataType, + uint8 dataType, uint256[2] chainIds, - uint256 gasPrice, bytes memory data ) { - (messageId, sender, executor, gasLimit, dataType, chainIds, gasPrice, data) = ArbitraryMessage.unpackData( - _data - ); + (messageId, sender, executor, gasLimit, dataType, chainIds, data) = ArbitraryMessage.unpackData(_data); } function unpackDataWithExtraParams( @@ -33,15 +30,12 @@ contract MessageTest { address sender, address executor, uint32 gasLimit, - bytes1 dataType, + uint8 dataType, uint256[2] chainIds, - uint256 gasPrice, bytes memory data ) { - (messageId, sender, executor, gasLimit, dataType, chainIds, gasPrice, data) = ArbitraryMessage.unpackData( - _data - ); + (messageId, sender, executor, gasLimit, dataType, chainIds, data) = ArbitraryMessage.unpackData(_data); } } diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicForeignAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicForeignAMB.sol index c77135f44..485a41d3f 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/BasicForeignAMB.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/BasicForeignAMB.sol @@ -19,19 +19,17 @@ contract BasicForeignAMB is BasicAMB, MessageRelay, MessageDelivery { address sender; address executor; uint32 gasLimit; - bytes1 dataType; + uint8 dataType; uint256[2] memory chainIds; - uint256 gasPrice; bytes memory data; - (messageId, sender, executor, gasLimit, dataType, chainIds, gasPrice, data) = ArbitraryMessage.unpackData( - _data - ); + (messageId, sender, executor, gasLimit, dataType, chainIds, data) = ArbitraryMessage.unpackData(_data); + require(_isMessageVersionValid(messageId)); require(_isDestinationChainIdValid(chainIds[1])); require(!relayedMessages(messageId)); setRelayedMessages(messageId, true); - processMessage(sender, executor, messageId, gasLimit, dataType, gasPrice, chainIds[0], data); + processMessage(sender, executor, messageId, gasLimit, dataType, chainIds[0], data); } /** diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol index 990e1c7c0..de22a3dec 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol @@ -46,16 +46,15 @@ contract BasicHomeAMB is BasicAMB, MessageDelivery { address sender; address executor; uint32 gasLimit; - bytes1 dataType; + uint8 dataType; uint256[2] memory chainIds; - uint256 gasPrice; bytes memory data; - (messageId, sender, executor, gasLimit, dataType, chainIds, gasPrice, data) = ArbitraryMessage.unpackData( - _message - ); + + (messageId, sender, executor, gasLimit, dataType, chainIds, data) = ArbitraryMessage.unpackData(_message); + require(_isMessageVersionValid(messageId)); require(_isDestinationChainIdValid(chainIds[1])); - processMessage(sender, executor, messageId, gasLimit, dataType, gasPrice, chainIds[0], data); + processMessage(sender, executor, messageId, gasLimit, dataType, chainIds[0], data); } function submitSignature(bytes signature, bytes message) external onlyValidator { diff --git a/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol b/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol index 14cc1eeff..34b196f5a 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/MessageProcessor.sol @@ -158,8 +158,7 @@ contract MessageProcessor is EternalStorage { address _executor, bytes32 _messageId, uint256 _gasLimit, - bytes1, /* dataType */ - uint256, /* gasPrice */ + uint8, /* dataType */ uint256 _sourceChainId, bytes memory _data ) internal { diff --git a/test/libraries/arbitraryMessage.test.js b/test/libraries/arbitraryMessage.test.js index 88d767501..2e06ea98c 100644 --- a/test/libraries/arbitraryMessage.test.js +++ b/test/libraries/arbitraryMessage.test.js @@ -1,10 +1,11 @@ const MessageTest = artifacts.require('MessageTest.sol') +require('../setup') const { expect } = require('chai') contract('ArbitraryMessage.sol', async () => { describe('unpackData', () => { - it('unpack dataType 0x00', async () => { + it('unpack dataType 0', async () => { // given const msgSender = '0x003667154bb32E42bb9e1E6532F19d187fa0082e' const msgContractAddress = '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955' @@ -12,8 +13,7 @@ contract('ArbitraryMessage.sol', async () => { const msgSrcChainId = '1122' const msgDstChainId = '3344' const msgGasLimit = '1535604485' - const msgGasPrice = '0' - const msgDataType = '0x00' + const msgDataType = '0' const msgData = '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' // event message generated by // requireToPassMessage(address _contract, bytes _data, uint256 _gas) @@ -33,19 +33,18 @@ contract('ArbitraryMessage.sol', async () => { const result = await messageTest.unpackData(message) // then - const { messageId, chainIds, sender, executor, gasLimit, dataType, gasPrice, data } = result + const { messageId, chainIds, sender, executor, gasLimit, dataType, data } = result expect(messageId).to.be.equal(msgId) expect(chainIds[0].toString(16)).to.be.equal(msgSrcChainId) expect(chainIds[1].toString(16)).to.be.equal(msgDstChainId) expect(sender).to.be.equal(msgSender) expect(executor).to.be.equal(msgContractAddress) expect(gasLimit.toString()).to.be.equal(msgGasLimit) - expect(dataType).to.be.equal(msgDataType) - expect(gasPrice.toString()).to.be.equal(msgGasPrice) + expect(dataType).to.be.bignumber.equal(msgDataType) expect(data).to.be.equal(msgData) }) - it('unpack dataType 0x00 with different chain ids', async () => { + it('unpack dataType 0 with different chain ids', async () => { // given const msgSender = '0x003667154bb32E42bb9e1E6532F19d187fa0082e' const msgContractAddress = '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955' @@ -64,8 +63,7 @@ contract('ArbitraryMessage.sol', async () => { ['aabbccddeeff', '112233445566'] ] const msgGasLimit = '1535604485' - const msgGasPrice = '0' - const msgDataType = '0x00' + const msgDataType = '0' const msgData = '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' // event message generated by // requireToPassMessage(address _contract, bytes _data, uint256 _gas) @@ -84,105 +82,21 @@ contract('ArbitraryMessage.sol', async () => { const result = await messageTest.unpackData(message) // then - const { messageId, chainIds, sender, executor, gasLimit, dataType, gasPrice, data } = result + const { messageId, chainIds, sender, executor, gasLimit, dataType, data } = result expect(messageId).to.be.equal(msgId) expect(chainIds[0].toString(16).padStart(msgSrcChainId.length, '0')).to.be.equal(msgSrcChainId) expect(chainIds[1].toString(16).padStart(msgDstChainId.length, '0')).to.be.equal(msgDstChainId) expect(sender).to.be.equal(msgSender) expect(executor).to.be.equal(msgContractAddress) expect(gasLimit.toString()).to.be.equal(msgGasLimit) - expect(dataType).to.be.equal(msgDataType) - expect(gasPrice.toString()).to.be.equal(msgGasPrice) + expect(dataType).to.be.bignumber.equal(msgDataType) expect(data).to.be.equal(msgData) } }) - - it('unpack dataType 0x01', async () => { - // given - const msgSender = '0xc95e32f5b21AEA8107aA2c645636E909489031e8' - const msgContractAddress = '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955' - const msgId = '0x27aa676e59e4feaafcfde2b88c6002b756795c1260762512ba26ff28fdaf0f64' - const msgSrcChainId = '1122' - const msgDstChainId = '3344' - const msgGasLimit = '1535604485' - const msgGasPrice = '6000000000' - const msgDataType = '0x01' - const msgData = '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' - // event message generated by - // requireToPassMessage(address _contract, bytes _data, uint256 _gas, uint256 _gasPrice) - const message = - `${msgId}c95e32f5b21aea8107aa2c645636e909489031e8` + // sender - 'f4bef13f9f4f2b203faf0c3cbbaabe1afe056955' + // contractAddress - '5b877705' + // gasLimit - '02' + // source chain id length - '02' + // destination chain id length - '01' + // dataType - '0000000000000000000000000000000000000000000000000000000165a0bc00' + // gasPrice - '1122' + // source chain id - '3344' + // destination chain id - 'b1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' // data - - // when - const messageTest = await MessageTest.new() - const result = await messageTest.unpackData(message) - - // then - const { messageId, chainIds, sender, executor, gasLimit, dataType, gasPrice, data } = result - expect(messageId).to.be.equal(msgId) - expect(chainIds[0].toString(16)).to.be.equal(msgSrcChainId) - expect(chainIds[1].toString(16)).to.be.equal(msgDstChainId) - expect(sender).to.be.equal(msgSender) - expect(executor).to.be.equal(msgContractAddress) - expect(gasLimit.toString()).to.be.equal(msgGasLimit) - expect(dataType).to.be.equal(msgDataType) - expect(gasPrice.toString()).to.be.equal(msgGasPrice) - expect(data).to.be.equal(msgData) - }) - - it('unpack dataType 0x02', async () => { - // given - const msgSender = '0xe0241f385c58c88911531DDFe3B5f0fc729BdaEe' - const msgContractAddress = '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955' - const msgId = '0xbdceda9d8c94838aca10c687da1411a07b1390e88239c0638cb9cc264219cc10' - const msgSrcChainId = '1122' - const msgDstChainId = '3344' - const msgGasLimit = '1535604485' - const msgGasPrice = '0' - const msgDataType = '0x02' - const msgData = '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' - // event message generated by - // requireToPassMessage(address _contract, bytes _data, uint256 _gas, bytes1 _oracleGasPriceSpeed) - const message = - `${msgId}e0241f385c58c88911531ddfe3b5f0fc729bdaee` + // sender - 'f4bef13f9f4f2b203faf0c3cbbaabe1afe056955' + // contractAddress - '5b877705' + // gasLimit - '02' + // source chain id length - '02' + // destination chain id length - '02' + // dataType - '10' + // gasPriceSpeed - '1122' + // source chain id - '3344' + // destination chain id - 'b1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' // data - - // when - const messageTest = await MessageTest.new() - const result = await messageTest.unpackData(message) - - // then - const { messageId, chainIds, sender, executor, gasLimit, dataType, gasPrice, data } = result - expect(messageId).to.be.equal(msgId) - expect(chainIds[0].toString(16)).to.be.equal(msgSrcChainId) - expect(chainIds[1].toString(16)).to.be.equal(msgDstChainId) - expect(sender).to.be.equal(msgSender) - expect(executor).to.be.equal(msgContractAddress) - expect(gasLimit.toString()).to.be.equal(msgGasLimit) - expect(dataType).to.be.equal(msgDataType) - expect(gasPrice.toString()).to.be.equal(msgGasPrice) - expect(data).to.be.equal(msgData) - }) }) + describe('unpackData with signatures parameters', () => { - it('unpack dataType 0x00', async () => { + it('unpack dataType 0', async () => { // given const msgSender = '0x003667154bb32E42bb9e1E6532F19d187fa0082e' const msgContractAddress = '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955' @@ -190,8 +104,7 @@ contract('ArbitraryMessage.sol', async () => { const msgSrcChainId = '1122' const msgDstChainId = '3344' const msgGasLimit = '1535604485' - const msgGasPrice = '0' - const msgDataType = '0x00' + const msgDataType = '0' const msgData = '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' // event message generated by // requireToPassMessage(address _contract, bytes _data, uint256 _gas) @@ -211,99 +124,14 @@ contract('ArbitraryMessage.sol', async () => { const result = await messageTest.unpackDataWithExtraParams(message, '0x') // then - const { messageId, chainIds, sender, executor, gasLimit, dataType, gasPrice, data } = result - expect(messageId).to.be.equal(msgId) - expect(chainIds[0].toString(16)).to.be.equal(msgSrcChainId) - expect(chainIds[1].toString(16)).to.be.equal(msgDstChainId) - expect(sender).to.be.equal(msgSender) - expect(executor).to.be.equal(msgContractAddress) - expect(gasLimit.toString()).to.be.equal(msgGasLimit) - expect(dataType).to.be.equal(msgDataType) - expect(gasPrice.toString()).to.be.equal(msgGasPrice) - expect(data).to.be.equal(msgData) - }) - - it('unpack dataType 0x01', async () => { - // given - const msgSender = '0xc95e32f5b21AEA8107aA2c645636E909489031e8' - const msgContractAddress = '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955' - const msgId = '0x27aa676e59e4feaafcfde2b88c6002b756795c1260762512ba26ff28fdaf0f64' - const msgSrcChainId = '1122' - const msgDstChainId = '3344' - const msgGasLimit = '1535604485' - const msgGasPrice = '6000000000' - const msgDataType = '0x01' - const msgData = '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' - // event message generated by - // requireToPassMessage(address _contract, bytes _data, uint256 _gas, uint256 _gasPrice) - const message = - `${msgId}c95e32f5b21aea8107aa2c645636e909489031e8` + // sender - 'f4bef13f9f4f2b203faf0c3cbbaabe1afe056955' + // contractAddress - '5b877705' + // gasLimit - '02' + // source chain id length - '02' + // destination chain id length - '01' + // dataType - '0000000000000000000000000000000000000000000000000000000165a0bc00' + // gasPrice - '1122' + // source chain id - '3344' + // destination chain id - 'b1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' // data - - // when - const messageTest = await MessageTest.new() - const result = await messageTest.unpackDataWithExtraParams(message, '0x') - - // then - const { messageId, chainIds, sender, executor, gasLimit, dataType, gasPrice, data } = result - expect(messageId).to.be.equal(msgId) - expect(chainIds[0].toString(16)).to.be.equal(msgSrcChainId) - expect(chainIds[1].toString(16)).to.be.equal(msgDstChainId) - expect(sender).to.be.equal(msgSender) - expect(executor).to.be.equal(msgContractAddress) - expect(gasLimit.toString()).to.be.equal(msgGasLimit) - expect(dataType).to.be.equal(msgDataType) - expect(gasPrice.toString()).to.be.equal(msgGasPrice) - expect(data).to.be.equal(msgData) - }) - - it('unpack dataType 0x02', async () => { - // given - const msgSender = '0xe0241f385c58c88911531DDFe3B5f0fc729BdaEe' - const msgContractAddress = '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955' - const msgId = '0xbdceda9d8c94838aca10c687da1411a07b1390e88239c0638cb9cc264219cc10' - const msgSrcChainId = '1122' - const msgDstChainId = '3344' - const msgGasLimit = '1535604485' - const msgGasPrice = '0' - const msgDataType = '0x02' - const msgData = '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' - // event message generated by - // requireToPassMessage(address _contract, bytes _data, uint256 _gas, bytes1 _oracleGasPriceSpeed) - const message = - `${msgId}e0241f385c58c88911531ddfe3b5f0fc729bdaee` + // sender - 'f4bef13f9f4f2b203faf0c3cbbaabe1afe056955' + // contractAddress - '5b877705' + // gasLimit - '02' + // source chain id length - '02' + // destination chain id length - '02' + // dataType - '10' + // gasPriceSpeed - '1122' + // source chain id - '3344' + // destination chain id - 'b1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03' // data - - // when - const messageTest = await MessageTest.new() - const result = await messageTest.unpackDataWithExtraParams(message, '0x') - - // then - const { messageId, chainIds, sender, executor, gasLimit, dataType, gasPrice, data } = result + const { messageId, chainIds, sender, executor, gasLimit, dataType, data } = result expect(messageId).to.be.equal(msgId) expect(chainIds[0].toString(16)).to.be.equal(msgSrcChainId) expect(chainIds[1].toString(16)).to.be.equal(msgDstChainId) expect(sender).to.be.equal(msgSender) expect(executor).to.be.equal(msgContractAddress) expect(gasLimit.toString()).to.be.equal(msgGasLimit) - expect(dataType).to.be.equal(msgDataType) - expect(gasPrice.toString()).to.be.equal(msgGasPrice) + expect(dataType).to.be.bignumber.equal(msgDataType) expect(data).to.be.equal(msgData) }) }) From d51a081f65741c31a363ec1c28cb8baca0f7c54f Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 02:04:08 +0300 Subject: [PATCH 05/14] Add extra constraints on ECDSA signature parameters (#528) --- contracts/PermittableToken.sol | 3 +++ contracts/libraries/Message.sol | 3 +++ 2 files changed, 6 insertions(+) diff --git a/contracts/PermittableToken.sol b/contracts/PermittableToken.sol index 1ea35fa9e..c297cf254 100644 --- a/contracts/PermittableToken.sol +++ b/contracts/PermittableToken.sol @@ -116,6 +116,9 @@ contract PermittableToken is ERC677BridgeToken { require(_spender != address(0)); require(_expiry == 0 || _now() <= _expiry); + require(_v == 27 || _v == 28); + require(uint256(_s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0); + bytes32 digest = keccak256( abi.encodePacked( "\x19\x01", diff --git a/contracts/libraries/Message.sol b/contracts/libraries/Message.sol index 4bc37151a..d674a3673 100644 --- a/contracts/libraries/Message.sol +++ b/contracts/libraries/Message.sol @@ -83,6 +83,9 @@ library Message { s := mload(add(signature, 0x40)) v := mload(add(signature, 0x60)) } + require(uint8(v) == 27 || uint8(v) == 28); + require(uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0); + return ecrecover(hashMessage(message, isAMBMessage), uint8(v), r, s); } From d94ad55f52b9e583d6011260d6a07a2d5d72aa97 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 02:06:20 +0300 Subject: [PATCH 06/14] Add missing input fee type validation where needed (#529) --- contracts/upgradeable_contracts/BaseFeeManager.sol | 7 ++++++- contracts/upgradeable_contracts/FeeTypes.sol | 9 +++++++++ contracts/upgradeable_contracts/RewardableBridge.sol | 4 ++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/contracts/upgradeable_contracts/BaseFeeManager.sol b/contracts/upgradeable_contracts/BaseFeeManager.sol index 7af5b2497..bb24897e8 100644 --- a/contracts/upgradeable_contracts/BaseFeeManager.sol +++ b/contracts/upgradeable_contracts/BaseFeeManager.sol @@ -16,7 +16,12 @@ contract BaseFeeManager is EternalStorage, FeeTypes { bytes32 internal constant HOME_FEE_STORAGE_KEY = 0xc3781f3cec62d28f56efe98358f59c2105504b194242dbcb2cc0806850c306e7; // keccak256(abi.encodePacked("homeFee")) bytes32 internal constant FOREIGN_FEE_STORAGE_KEY = 0x68c305f6c823f4d2fa4140f9cf28d32a1faccf9b8081ff1c2de11cf32c733efc; // keccak256(abi.encodePacked("foreignFee")) - function calculateFee(uint256 _value, bool _recover, bytes32 _feeType) public view returns (uint256) { + function calculateFee(uint256 _value, bool _recover, bytes32 _feeType) + public + view + validFeeType(_feeType) + returns (uint256) + { uint256 fee = _feeType == HOME_FEE ? getHomeFee() : getForeignFee(); if (!_recover) { return _value.mul(fee).div(MAX_FEE); diff --git a/contracts/upgradeable_contracts/FeeTypes.sol b/contracts/upgradeable_contracts/FeeTypes.sol index 7018c9ccb..81b780543 100644 --- a/contracts/upgradeable_contracts/FeeTypes.sol +++ b/contracts/upgradeable_contracts/FeeTypes.sol @@ -3,4 +3,13 @@ pragma solidity 0.4.24; contract FeeTypes { bytes32 internal constant HOME_FEE = 0x89d93e5e92f7e37e490c25f0e50f7f4aad7cc94b308a566553280967be38bcf1; // keccak256(abi.encodePacked("home-fee")) bytes32 internal constant FOREIGN_FEE = 0xdeb7f3adca07d6d1f708c1774389db532a2b2f18fd05a62b957e4089f4696ed5; // keccak256(abi.encodePacked("foreign-fee")) + + /** + * @dev Throws if given fee type is unknown. + */ + modifier validFeeType(bytes32 _feeType) { + require(_feeType == HOME_FEE || _feeType == FOREIGN_FEE); + /* solcov ignore next */ + _; + } } diff --git a/contracts/upgradeable_contracts/RewardableBridge.sol b/contracts/upgradeable_contracts/RewardableBridge.sol index 48a349128..00a4cc403 100644 --- a/contracts/upgradeable_contracts/RewardableBridge.sol +++ b/contracts/upgradeable_contracts/RewardableBridge.sol @@ -18,7 +18,7 @@ contract RewardableBridge is Ownable, FeeTypes { bytes4 internal constant DISTRIBUTE_FEE_FROM_SIGNATURES = 0x59d78464; // distributeFeeFromSignatures(uint256) bytes4 internal constant DISTRIBUTE_FEE_FROM_AFFIRMATION = 0x054d46ec; // distributeFeeFromAffirmation(uint256) - function _getFee(bytes32 _feeType) internal view returns (uint256) { + function _getFee(bytes32 _feeType) internal view validFeeType(_feeType) returns (uint256) { uint256 fee; address feeManager = feeManagerContract(); bytes4 method = _feeType == HOME_FEE ? GET_HOME_FEE : GET_FOREIGN_FEE; @@ -61,7 +61,7 @@ contract RewardableBridge is Ownable, FeeTypes { addressStorage[FEE_MANAGER_CONTRACT] = _feeManager; } - function _setFee(address _feeManager, uint256 _fee, bytes32 _feeType) internal { + function _setFee(address _feeManager, uint256 _fee, bytes32 _feeType) internal validFeeType(_feeType) { bytes4 method = _feeType == HOME_FEE ? SET_HOME_FEE : SET_FOREIGN_FEE; require(_feeManager.delegatecall(abi.encodeWithSelector(method, _fee))); } From f70426c8418ca8a96e1081e3ac9d5b97fb5f80fa Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 11:36:17 +0300 Subject: [PATCH 07/14] Update ERC677 to call onTokenTransfer only for the bridge contract (#530) --- contracts/ERC677BridgeToken.sol | 15 +++++++---- .../foreign_mediator.test.js | 15 +++++------ .../foreign_mediator.test.js | 26 ++++++++----------- .../home_mediator.test.js | 3 ++- test/poa20_test.js | 23 +++------------- 5 files changed, 33 insertions(+), 49 deletions(-) diff --git a/contracts/ERC677BridgeToken.sol b/contracts/ERC677BridgeToken.sol index 4fa0454e5..e78f806ed 100644 --- a/contracts/ERC677BridgeToken.sol +++ b/contracts/ERC677BridgeToken.sol @@ -16,8 +16,6 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna address internal bridgeContractAddr; - event ContractFallbackCallFailed(address from, address to, uint256 value); - constructor(string _name, string _symbol, uint8 _decimals) public DetailedERC20(_name, _symbol, _decimals) { // solhint-disable-previous-line no-empty-blocks } @@ -67,10 +65,17 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna return true; } + /** + * @dev Internal function that calls onTokenTransfer callback on the receiver after the successful transfer. + * Since it is not present in the original ERC677 standard, the callback is only called on the bridge contract, + * in order to simplify UX. In other cases, this token complies with the ERC677/ERC20 standard. + * @param _from tokens sender address. + * @param _to tokens receiver address. + * @param _value amount of sent tokens. + */ function callAfterTransfer(address _from, address _to, uint256 _value) internal { - if (AddressUtils.isContract(_to) && !contractFallback(_from, _to, _value, new bytes(0))) { - require(!isBridge(_to)); - emit ContractFallbackCallFailed(_from, _to, _value); + if (isBridge(_to)) { + require(contractFallback(_from, _to, _value, new bytes(0))); } } diff --git a/test/amb_erc20_to_native/foreign_mediator.test.js b/test/amb_erc20_to_native/foreign_mediator.test.js index 4ce57f500..52d505267 100644 --- a/test/amb_erc20_to_native/foreign_mediator.test.js +++ b/test/amb_erc20_to_native/foreign_mediator.test.js @@ -38,7 +38,6 @@ contract('ForeignAMBErc20ToNative', async accounts => { await ambBridgeContract.setMaxGasPerTx(maxGasPerTx) otherSideMediator = await HomeAMBErc20ToNative.new() token = await ERC677BridgeToken.new('TEST', 'TST', 18) - await token.setBridgeContract(contract.address) }) describe('initialize', () => { @@ -373,8 +372,8 @@ contract('ForeignAMBErc20ToNative', async accounts => { describe('handleBridgedTokens', () => { it('should unlock tokens on message from amb', async () => { - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(twoEthers) expect(await contract.mediatorBalance()).to.be.bignumber.equal(twoEthers) @@ -429,8 +428,8 @@ contract('ForeignAMBErc20ToNative', async accounts => { owner, token.address ).should.be.fulfilled - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(twoEthers) expect(await contract.mediatorBalance()).to.be.bignumber.equal(twoEthers) @@ -462,8 +461,8 @@ contract('ForeignAMBErc20ToNative', async accounts => { }) it('should revert when out of execution limits on message from amb', async () => { - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(twoEthers) expect(await contract.mediatorBalance()).to.be.bignumber.equal(twoEthers) @@ -515,7 +514,7 @@ contract('ForeignAMBErc20ToNative', async accounts => { }) it('should be a failed transaction', async () => { - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled // Given const data = await contract.contract.methods.handleBridgedTokens(user, value.toString()).encodeABI() diff --git a/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js b/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js index 585b2a470..5f81b75e4 100644 --- a/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js +++ b/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js @@ -44,10 +44,6 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { }) const sendFunctions = [ - async function simpleTransfer() { - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled - return user - }, async function emptyAlternativeReceiver() { await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled return user @@ -221,7 +217,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { await token.mint(user, oneEther).should.be.fulfilled expect(await token.balanceOf(user)).to.be.bignumber.equal(oneEther) - await token.transfer(contract.address, oneEther, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, oneEther, '0x', { from: user }).should.be.fulfilled expect(await token.balanceOf(user)).to.be.bignumber.equal(ZERO) expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(oneEther) @@ -329,7 +325,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { await contract.setExecutionDailyLimit(token.address, ether('5'), { from: owner }).should.be.rejected await contract.setExecutionMaxPerTx(token.address, ether('1.5'), { from: owner }).should.be.rejected - await token.transfer(contract.address, value, { from: user }) + await token.transferAndCall(contract.address, value, '0x', { from: user }) await contract.setDailyLimit(token.address, ether('5'), { from: owner }).should.be.fulfilled await contract.setMaxPerTx(token.address, ether('1.5'), { from: owner }).should.be.fulfilled @@ -513,7 +509,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { token = await ERC677BridgeToken.new('TEST', 'TST', decimals) await token.mint(user, value.mul(f1).div(f2)).should.be.fulfilled - await token.transfer(contract.address, value.mul(f1).div(f2), { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value.mul(f1).div(f2), '0x', { from: user }).should.be.fulfilled expect(await contract.dailyLimit(token.address)).to.be.bignumber.equal(dailyLimit.mul(f1).div(f2)) expect(await contract.maxPerTx(token.address)).to.be.bignumber.equal(maxPerTx.mul(f1).div(f2)) @@ -530,7 +526,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { it(`should initialize limits according to decimals = 0`, async () => { token = await ERC677BridgeToken.new('TEST', 'TST', 0) await token.mint(user, '1').should.be.fulfilled - await token.transfer(contract.address, '1', { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, '1', '0x', { from: user }).should.be.fulfilled expect(await contract.dailyLimit(token.address)).to.be.bignumber.equal('10000') expect(await contract.maxPerTx(token.address)).to.be.bignumber.equal('100') @@ -542,8 +538,8 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { describe('handleBridgedTokens', () => { it('should unlock tokens on message from amb', async () => { - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(twoEthers) expect(await contract.mediatorBalance(token.address)).to.be.bignumber.equal(twoEthers) @@ -605,8 +601,8 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { }) it('should not allow to operate when global shutdown is enabled', async () => { - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled const data = await contract.contract.methods .handleBridgedTokens(token.address, user, value.toString()) @@ -662,7 +658,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { }) it('should be a failed transaction', async () => { - await token.transfer(contract.address, value, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, value, '0x', { from: user }).should.be.fulfilled // Given const data = await contract.contract.methods .handleBridgedTokens(token.address, user, value.toString()) @@ -857,7 +853,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { expect(await contract.mediatorBalance(token.address)).to.be.bignumber.equal(ZERO) expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(twoEthers) - await token.transfer(contract.address, halfEther, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, halfEther, '0x', { from: user }).should.be.fulfilled await contract.setDailyLimit(token.address, ether('5')).should.be.fulfilled await contract.setMaxPerTx(token.address, ether('2')).should.be.fulfilled @@ -884,7 +880,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { expect(await contract.mediatorBalance(token.address)).to.be.bignumber.equal(ZERO) expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(twoEthers) - await token.transfer(contract.address, halfEther, { from: user }).should.be.fulfilled + await token.transferAndCall(contract.address, halfEther, '0x', { from: user }).should.be.fulfilled expect(await contract.mediatorBalance(token.address)).to.be.bignumber.equal(halfEther) expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(twoEthers.add(halfEther)) diff --git a/test/multi_amb_erc20_to_erc677/home_mediator.test.js b/test/multi_amb_erc20_to_erc677/home_mediator.test.js index 2e7d4e950..c1125332e 100644 --- a/test/multi_amb_erc20_to_erc677/home_mediator.test.js +++ b/test/multi_amb_erc20_to_erc677/home_mediator.test.js @@ -97,7 +97,8 @@ contract('HomeMultiAMBErc20ToErc677', async accounts => { async function bridgeToken(token, value = oneEther, forceFail = false) { await token.mint(user, value).should.be.fulfilled - const { receipt } = await token.transfer(otherSideMediator.address, value, { from: user }).should.be.fulfilled + const { receipt } = await token.transferAndCall(otherSideMediator.address, value, '0x', { from: user }).should.be + .fulfilled const encodedData = strip0x( web3.eth.abi.decodeParameters( ['bytes'], diff --git a/test/poa20_test.js b/test/poa20_test.js index c59fcb88d..69a1f1ad5 100644 --- a/test/poa20_test.js +++ b/test/poa20_test.js @@ -309,23 +309,6 @@ function testERC677BridgeToken(accounts, rewardable, permittable, createToken) { }) }) - it('sends tokens to contract that does not contains onTokenTransfer method', async () => { - await addBridge(token, homeErcToErcContract.address).should.be.fulfilled - await token.mint(user, oneEther, { from: owner }).should.be.fulfilled - - const result = await token.transfer(validatorContract.address, minPerTx, { from: user }).should.be.fulfilled - expectEventInLogs(result.logs, 'Transfer', { - from: user, - to: validatorContract.address, - value: minPerTx - }) - expectEventInLogs(result.logs, 'ContractFallbackCallFailed', { - from: user, - to: validatorContract.address, - value: minPerTx - }) - }) - it('fail to send tokens to bridge contract out of limits', async () => { const lessThanMin = ether('0.0001') await token.mint(user, oneEther, { from: owner }).should.be.fulfilled @@ -588,7 +571,7 @@ function testERC677BridgeToken(accounts, rewardable, permittable, createToken) { }) }) describe('#transfer', async () => { - it('if transfer called on contract, onTokenTransfer is also invoked', async () => { + it('if transfer called on contract, onTokenTransfer is not invoked', async () => { const receiver = await ERC677ReceiverTest.new() expect(await receiver.from()).to.be.equal(ZERO_ADDRESS) expect(await receiver.value()).to.be.bignumber.equal(ZERO) @@ -600,8 +583,8 @@ function testERC677BridgeToken(accounts, rewardable, permittable, createToken) { expect(await token.balanceOf(receiver.address)).to.be.bignumber.equal('1') expect(await token.balanceOf(user)).to.be.bignumber.equal(ZERO) - expect(await receiver.from()).to.be.equal(user) - expect(await receiver.value()).to.be.bignumber.equal('1') + expect(await receiver.from()).to.be.equal(ZERO_ADDRESS) + expect(await receiver.value()).to.be.bignumber.equal(ZERO) expect(await receiver.data()).to.be.equal(null) expect(logs[0].event).to.be.equal('Transfer') }) From 2def0318bba2fc4dba61b8c3e03df183364da96a Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 20:27:12 +0300 Subject: [PATCH 08/14] Add missing fix mediator balance (#510) --- contracts/ERC677BridgeToken.sol | 7 +- .../IBurnableMintableERC677Token.sol | 2 +- .../upgradeable_contracts/BasicBridge.sol | 4 - contracts/upgradeable_contracts/Claimable.sol | 51 ++--- .../InterestReceiver.sol | 3 +- .../MediatorBalanceStorage.sol | 27 +++ .../ForeignAMBErc20ToNative.sol | 30 +-- .../HomeAMBErc20ToNative.sol | 11 +- .../BasicAMBErc677ToErc677.sol | 4 - .../ForeignAMBErc677ToErc677.sol | 43 +++- .../ForeignStakeTokenMediator.sol | 10 + .../HomeAMBErc677ToErc677.sol | 11 + .../HomeStakeTokenMediator.sol | 11 + .../BasicAMBNativeToErc20.sol | 9 - .../ForeignAMBNativeToErc20.sol | 11 + .../HomeAMBNativeToErc20.sol | 36 +--- .../amb_native_to_erc20/callflows.md | 3 + .../BasicForeignBridgeErcToErc.sol | 10 +- .../erc20_to_erc20/HomeBridgeErcToErc.sol | 5 + .../ForeignBridgeErcToNative.sol | 10 +- .../erc20_to_native/HomeBridgeErcToNative.sol | 14 ++ .../BasicMultiAMBErc20ToErc677.sol | 5 +- .../ForeignMultiAMBErc20ToErc677.sol | 6 +- .../ForeignBridgeNativeToErc.sol | 16 ++ .../native_to_erc20/HomeBridgeNativeToErc.sol | 12 ++ .../foreign_mediator.test.js | 1 + .../amb_erc20_to_native/home_mediator.test.js | 2 + .../foreign_bridge.test.js | 202 +++++++++++++++++- .../amb_native_to_erc20/home_mediator.test.js | 2 + .../foreign_mediator.test.js | 1 + test/native_to_erc/home_bridge_test.js | 9 +- .../foreign_mediator.test.js | 25 +++ 32 files changed, 475 insertions(+), 118 deletions(-) create mode 100644 contracts/upgradeable_contracts/MediatorBalanceStorage.sol diff --git a/contracts/ERC677BridgeToken.sol b/contracts/ERC677BridgeToken.sol index e78f806ed..8110c4102 100644 --- a/contracts/ERC677BridgeToken.sol +++ b/contracts/ERC677BridgeToken.sol @@ -102,7 +102,12 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna revert(); } - function claimTokens(address _token, address _to) public onlyOwner validAddress(_to) { + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimTokens(address _token, address _to) external onlyOwner { claimValues(_token, _to); } diff --git a/contracts/interfaces/IBurnableMintableERC677Token.sol b/contracts/interfaces/IBurnableMintableERC677Token.sol index f244abc91..f00fa363a 100644 --- a/contracts/interfaces/IBurnableMintableERC677Token.sol +++ b/contracts/interfaces/IBurnableMintableERC677Token.sol @@ -4,5 +4,5 @@ import "../interfaces/ERC677.sol"; contract IBurnableMintableERC677Token is ERC677 { function mint(address _to, uint256 _amount) public returns (bool); function burn(uint256 _value) public; - function claimTokens(address _token, address _to) public; + function claimTokens(address _token, address _to) external; } diff --git a/contracts/upgradeable_contracts/BasicBridge.sol b/contracts/upgradeable_contracts/BasicBridge.sol index e49e9bc35..58bb093d9 100644 --- a/contracts/upgradeable_contracts/BasicBridge.sol +++ b/contracts/upgradeable_contracts/BasicBridge.sol @@ -50,10 +50,6 @@ contract BasicBridge is return uintStorage[REQUIRED_BLOCK_CONFIRMATIONS]; } - function claimTokens(address _token, address _to) public onlyIfUpgradeabilityOwner validAddress(_to) { - claimValues(_token, _to); - } - /** * @dev Internal function for updating fallback gas price value. * @param _gasPrice new value for the gas price, zero gas price is allowed. diff --git a/contracts/upgradeable_contracts/Claimable.sol b/contracts/upgradeable_contracts/Claimable.sol index 1bf14b58f..9e7685fca 100644 --- a/contracts/upgradeable_contracts/Claimable.sol +++ b/contracts/upgradeable_contracts/Claimable.sol @@ -1,18 +1,31 @@ pragma solidity 0.4.24; -import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol"; import "../libraries/Address.sol"; +import "../libraries/SafeERC20.sol"; +/** + * @title Claimable + * @dev Implementation of the claiming utils that can be useful for withdrawing accidentally sent tokens that are not used in bridge operations. + */ contract Claimable { - bytes4 internal constant TRANSFER = 0xa9059cbb; // transfer(address,uint256) + using SafeERC20 for address; + /** + * Throws if a given address is equal to address(0) + */ modifier validAddress(address _to) { require(_to != address(0)); /* solcov ignore next */ _; } - function claimValues(address _token, address _to) internal { + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. + * Caller should additionally check that the claimed token is not a part of bridge operations (i.e. that token != erc20token()). + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimValues(address _token, address _to) internal validAddress(_to) { if (_token == address(0)) { claimNativeCoins(_to); } else { @@ -20,35 +33,23 @@ contract Claimable { } } + /** + * @dev Internal function for withdrawing all native coins from the contract. + * @param _to address of the coins receiver. + */ function claimNativeCoins(address _to) internal { uint256 value = address(this).balance; Address.safeSendValue(_to, value); } + /** + * @dev Internal function for withdrawing all tokens of ssome particular ERC20 contract from this contract. + * @param _token address of the claimed ERC20 token. + * @param _to address of the tokens receiver. + */ function claimErc20Tokens(address _token, address _to) internal { ERC20Basic token = ERC20Basic(_token); uint256 balance = token.balanceOf(this); - safeTransfer(_token, _to, balance); - } - - function safeTransfer(address _token, address _to, uint256 _value) internal { - bytes memory returnData; - bool returnDataResult; - bytes memory callData = abi.encodeWithSelector(TRANSFER, _to, _value); - assembly { - let result := call(gas, _token, 0x0, add(callData, 0x20), mload(callData), 0, 32) - returnData := mload(0) - returnDataResult := mload(0) - - switch result - case 0 { - revert(0, 0) - } - } - - // Return data is optional - if (returnData.length > 0) { - require(returnDataResult); - } + _token.safeTransfer(_to, balance); } } diff --git a/contracts/upgradeable_contracts/InterestReceiver.sol b/contracts/upgradeable_contracts/InterestReceiver.sol index 25844e46d..e87216d2a 100644 --- a/contracts/upgradeable_contracts/InterestReceiver.sol +++ b/contracts/upgradeable_contracts/InterestReceiver.sol @@ -100,7 +100,8 @@ contract InterestReceiver is ERC677Receiver, Ownable, Claimable, TokenSwapper { * @param _token address of claimed token, address(0) for native * @param _to address of tokens receiver */ - function claimTokens(address _token, address _to) external onlyOwner validAddress(_to) { + function claimTokens(address _token, address _to) external onlyOwner { + // Only tokens other than CHAI/DAI can be claimed from this contract. require(_token != address(chaiToken()) && _token != address(daiToken())); claimValues(_token, _to); } diff --git a/contracts/upgradeable_contracts/MediatorBalanceStorage.sol b/contracts/upgradeable_contracts/MediatorBalanceStorage.sol new file mode 100644 index 000000000..f9bf792c9 --- /dev/null +++ b/contracts/upgradeable_contracts/MediatorBalanceStorage.sol @@ -0,0 +1,27 @@ +pragma solidity 0.4.24; + +import "../upgradeability/EternalStorage.sol"; + +/** + * @title MediatorBalanceStorage + * @dev Storage helpers for the mediator balance tracking. + */ +contract MediatorBalanceStorage is EternalStorage { + bytes32 internal constant MEDIATOR_BALANCE = 0x3db340e280667ee926fa8c51e8f9fcf88a0ff221a66d84d63b4778127d97d139; // keccak256(abi.encodePacked("mediatorBalance")) + + /** + * @dev Tells the expected mediator balance. + * @return the current expected mediator balance. + */ + function mediatorBalance() public view returns (uint256) { + return uintStorage[MEDIATOR_BALANCE]; + } + + /** + * @dev Sets the expected mediator balance of the contract. + * @param _balance the new expected mediator balance value. + */ + function _setMediatorBalance(uint256 _balance) internal { + uintStorage[MEDIATOR_BALANCE] = _balance; + } +} diff --git a/contracts/upgradeable_contracts/amb_erc20_to_native/ForeignAMBErc20ToNative.sol b/contracts/upgradeable_contracts/amb_erc20_to_native/ForeignAMBErc20ToNative.sol index ee9290eec..f2815b877 100644 --- a/contracts/upgradeable_contracts/amb_erc20_to_native/ForeignAMBErc20ToNative.sol +++ b/contracts/upgradeable_contracts/amb_erc20_to_native/ForeignAMBErc20ToNative.sol @@ -4,17 +4,16 @@ import "./BasicAMBErc20ToNative.sol"; import "../BaseERC677Bridge.sol"; import "../ReentrancyGuard.sol"; import "../../libraries/SafeERC20.sol"; +import "../MediatorBalanceStorage.sol"; /** * @title ForeignAMBErc20ToNative * @dev Foreign mediator implementation for erc20-to-native bridge intended to work on top of AMB bridge. * It is design to be used as implementation contract of EternalStorageProxy contract. */ -contract ForeignAMBErc20ToNative is BasicAMBErc20ToNative, ReentrancyGuard, BaseERC677Bridge { +contract ForeignAMBErc20ToNative is BasicAMBErc20ToNative, ReentrancyGuard, BaseERC677Bridge, MediatorBalanceStorage { using SafeERC20 for ERC677; - bytes32 internal constant MEDIATOR_BALANCE = 0x3db340e280667ee926fa8c51e8f9fcf88a0ff221a66d84d63b4778127d97d139; // keccak256(abi.encodePacked("mediatorBalance")) - /** * @dev Stores the initial parameters of the mediator. * @param _bridgeContract the address of the AMB bridge contract. @@ -109,25 +108,18 @@ contract ForeignAMBErc20ToNative is BasicAMBErc20ToNative, ReentrancyGuard, Base * @param _token address of the token, if it is not provided, native tokens will be transferred. * @param _to address that will receive the locked tokens on this contract. */ - function claimTokens(address _token, address _to) public onlyIfUpgradeabilityOwner validAddress(_to) { + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // Since bridged tokens are locked at this contract, it is not allowed to claim them with the use of claimTokens function require(_token != address(_erc677token())); claimValues(_token, _to); } /** - * @dev Tells the token balance of the contract. - * @return the current tracked token balance of the contract. - */ - function mediatorBalance() public view returns (uint256) { - return uintStorage[MEDIATOR_BALANCE]; - } - - /** - * @dev Allows to send to the other network the amount of locked native tokens that can be forced into the contract + * @dev Allows to send to the other network the amount of locked tokens that can be forced into the contract * without the invocation of the required methods. - * @param _receiver the address that will receive the tokens on the other network + * @param _receiver the address that will receive the native coins on the other network */ - function fixMediatorBalance(address _receiver) public onlyIfUpgradeabilityOwner { + function fixMediatorBalance(address _receiver) external onlyIfUpgradeabilityOwner validAddress(_receiver) { uint256 balance = _erc677token().balanceOf(address(this)); uint256 expectedBalance = mediatorBalance(); require(balance > expectedBalance); @@ -192,12 +184,4 @@ contract ForeignAMBErc20ToNative is BasicAMBErc20ToNative, ReentrancyGuard, Base function bridgeContractOnOtherSide() internal view returns (address) { return mediatorContractOnOtherSide(); } - - /** - * @dev Sets the updated token balance of the contract. - * @param _balance the new token balance of the contract. - */ - function _setMediatorBalance(uint256 _balance) internal { - uintStorage[MEDIATOR_BALANCE] = _balance; - } } diff --git a/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol b/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol index a0a1837ce..6b180f8ea 100644 --- a/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol +++ b/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol @@ -202,12 +202,13 @@ contract HomeAMBErc20ToNative is BasicAMBErc20ToNative, BlockRewardBridge, HomeF } /** - * @dev Allows to transfer any locked token on this contract that is not part of the bridge operations. - * Native tokens are not allowed to be claimed. - * @param _token address of the token. + * @dev Allows to transfer any locked tokens or native coins on this contract. + * @param _token address of the token, address(0) for native coins. * @param _to address that will receive the locked tokens on this contract. */ - function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner validAddress(_to) { + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // In case native coins were forced into this contract by using a selfdestruct opcode, + // they should be handled by a call to fixMediatorBalance, instead of using a claimTokens function. require(_token != address(0)); claimValues(_token, _to); } @@ -217,7 +218,7 @@ contract HomeAMBErc20ToNative is BasicAMBErc20ToNative, BlockRewardBridge, HomeF * without the invocation of the required methods. * @param _receiver the address that will receive the tokens on the other network */ - function fixMediatorBalance(address _receiver) external onlyIfUpgradeabilityOwner { + function fixMediatorBalance(address _receiver) external onlyIfUpgradeabilityOwner validAddress(_receiver) { uint256 balance = address(this).balance; uint256 available = maxAvailablePerTx(); if (balance > available) { diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol index c28c4aa3a..0182cf119 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol @@ -146,8 +146,4 @@ contract BasicAMBErc677ToErc677 is passMessage(recipient, recipient, valueToUnlock); } } - - function claimTokens(address _token, address _to) public onlyIfUpgradeabilityOwner validAddress(_to) { - claimValues(_token, _to); - } } diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignAMBErc677ToErc677.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignAMBErc677ToErc677.sol index d65c29eb4..0231f6e0a 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignAMBErc677ToErc677.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignAMBErc677ToErc677.sol @@ -2,13 +2,14 @@ pragma solidity 0.4.24; import "./BasicAMBErc677ToErc677.sol"; import "../../libraries/SafeERC20.sol"; +import "../MediatorBalanceStorage.sol"; /** * @title ForeignAMBErc677ToErc677 * @dev Foreign side implementation for erc677-to-erc677 mediator intended to work on top of AMB bridge. * It is designed to be used as an implementation contract of EternalStorageProxy contract. */ -contract ForeignAMBErc677ToErc677 is BasicAMBErc677ToErc677 { +contract ForeignAMBErc677ToErc677 is BasicAMBErc677ToErc677, MediatorBalanceStorage { using SafeERC20 for ERC677; /** @@ -19,7 +20,10 @@ contract ForeignAMBErc677ToErc677 is BasicAMBErc677ToErc677 { function executeActionOnBridgedTokens(address _recipient, uint256 _value) internal { uint256 value = _unshiftValue(_value); bytes32 _messageId = messageId(); + + _setMediatorBalance(mediatorBalance().sub(value)); erc677token().safeTransfer(_recipient, value); + emit TokensBridged(_recipient, value, _messageId); } @@ -44,6 +48,26 @@ contract ForeignAMBErc677ToErc677 is BasicAMBErc677ToErc677 { bridgeSpecificActionsOnTokenTransfer(token, msg.sender, _value, abi.encodePacked(_receiver)); } + /** + * @dev Allows to send to the other network the amount of locked tokens that can be forced into the contract + * without the invocation of the required methods. + * @param _receiver the address that will receive the tokens on the other network + */ + function fixMediatorBalance(address _receiver) external onlyIfUpgradeabilityOwner validAddress(_receiver) { + uint256 balance = _erc677token().balanceOf(address(this)); + uint256 expectedBalance = mediatorBalance(); + require(balance > expectedBalance); + uint256 diff = balance - expectedBalance; + uint256 available = maxAvailablePerTx(); + require(available > 0); + if (diff > available) { + diff = available; + } + addTotalSpentPerDay(getCurrentDay(), diff); + _setMediatorBalance(expectedBalance.add(diff)); + passMessage(_receiver, _receiver, diff); + } + /** * @dev Executes action on deposit of bridged tokens * @param _from address of tokens sender @@ -57,11 +81,28 @@ contract ForeignAMBErc677ToErc677 is BasicAMBErc677ToErc677 { bytes _data ) internal { if (!lock()) { + _setMediatorBalance(mediatorBalance().add(_value)); passMessage(_from, chooseReceiver(_from, _data), _value); } } + /** + * @dev Unlock back the amount of tokens that were bridged to the other network but failed. + * @param _recipient address that will receive the tokens + * @param _value amount of tokens to be received + */ function executeActionOnFixedTokens(address _recipient, uint256 _value) internal { + _setMediatorBalance(mediatorBalance().sub(_value)); erc677token().safeTransfer(_recipient, _value); } + + /** + * @dev Allows to transfer any locked token on this contract that is not part of the bridge operations. + * @param _token address of the token, if it is not provided, native tokens will be transferred. + * @param _to address that will receive the locked tokens on this contract. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + require(_token != address(_erc677token())); + claimValues(_token, _to); + } } diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignStakeTokenMediator.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignStakeTokenMediator.sol index 7a68ce403..3f54ce054 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignStakeTokenMediator.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/ForeignStakeTokenMediator.sol @@ -64,4 +64,14 @@ contract ForeignStakeTokenMediator is BasicStakeTokenMediator { token.transfer(_recipient, _value); } } + + /** + * @dev Allows to transfer any locked token on this contract other than stake token. + * @param _token address of the token, if it is not provided, native tokens will be transferred. + * @param _to address that will receive the locked tokens on this contract. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + require(_token != address(_erc677token())); + claimValues(_token, _to); + } } diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeAMBErc677ToErc677.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeAMBErc677ToErc677.sol index ddac4b2c2..adc9c79d9 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeAMBErc677ToErc677.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeAMBErc677ToErc677.sol @@ -35,6 +35,17 @@ contract HomeAMBErc677ToErc677 is BasicAMBErc677ToErc677 { } } + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // For home side of the bridge, tokens are not locked at the contract, they are minted and burned instead. + // So, its is safe to allow claiming of any tokens. Native coins are allowed as well. + claimValues(_token, _to); + } + function executeActionOnFixedTokens(address _recipient, uint256 _value) internal { IBurnableMintableERC677Token(erc677token()).mint(_recipient, _value); } diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeStakeTokenMediator.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeStakeTokenMediator.sol index 3026b13fb..86cd090a7 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeStakeTokenMediator.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/HomeStakeTokenMediator.sol @@ -154,6 +154,17 @@ contract HomeStakeTokenMediator is BasicStakeTokenMediator, HomeStakeTokenFeeMan } } + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // For home side of the bridge, tokens are not locked at the contract, they are minted and burned instead. + // So, its is safe to allow claiming of any tokens. Native coins are allowed as well. + claimValues(_token, _to); + } + /** * @dev Executes action on relayed request to fix the failed transfer of tokens * @param _recipient address of tokens receiver diff --git a/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol b/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol index 6399855b5..e7b3fc5d8 100644 --- a/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol +++ b/contracts/upgradeable_contracts/amb_native_to_erc20/BasicAMBNativeToErc20.sol @@ -81,13 +81,4 @@ contract BasicAMBNativeToErc20 is ) internal { revert(); } - - /** - * @dev Allows to transfer any locked token on this contract that is not part of the bridge operations. - * @param _token address of the token, if it is not provided, native tokens will be transferred. - * @param _to address that will receive the locked tokens on this contract. - */ - function claimTokens(address _token, address _to) public onlyIfUpgradeabilityOwner validAddress(_to) { - claimValues(_token, _to); - } } diff --git a/contracts/upgradeable_contracts/amb_native_to_erc20/ForeignAMBNativeToErc20.sol b/contracts/upgradeable_contracts/amb_native_to_erc20/ForeignAMBNativeToErc20.sol index 63a64b5fc..4cf4a6026 100644 --- a/contracts/upgradeable_contracts/amb_native_to_erc20/ForeignAMBNativeToErc20.sol +++ b/contracts/upgradeable_contracts/amb_native_to_erc20/ForeignAMBNativeToErc20.sol @@ -157,6 +157,17 @@ contract ForeignAMBNativeToErc20 is BasicAMBNativeToErc20, ReentrancyGuard, Base IBurnableMintableERC677Token(erc677token()).mint(_feeManager, _fee); } + /** + * @dev Allows to transfer any locked token on this contract that is not part of the bridge operations. + * @param _token address of the token, if it is not provided, native tokens will be transferred. + * @param _to address that will receive the locked tokens on this contract. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // For foreign side of the bridge, tokens are not locked at the contract, they are minted and burned instead. + // So, its is safe to allow claiming of any tokens. Native coins are allowed as well. + claimValues(_token, _to); + } + /** * @dev Allows to transfer any locked token on the ERC677 token contract. * @param _token address of the token, if it is not provided, native tokens will be transferred. diff --git a/contracts/upgradeable_contracts/amb_native_to_erc20/HomeAMBNativeToErc20.sol b/contracts/upgradeable_contracts/amb_native_to_erc20/HomeAMBNativeToErc20.sol index b0e35e4e5..6a98ef272 100644 --- a/contracts/upgradeable_contracts/amb_native_to_erc20/HomeAMBNativeToErc20.sol +++ b/contracts/upgradeable_contracts/amb_native_to_erc20/HomeAMBNativeToErc20.sol @@ -1,5 +1,6 @@ pragma solidity 0.4.24; +import "../MediatorBalanceStorage.sol"; import "./BasicAMBNativeToErc20.sol"; /** @@ -7,9 +8,7 @@ import "./BasicAMBNativeToErc20.sol"; * @dev Home mediator implementation for native-to-erc20 bridge intended to work on top of AMB bridge. * It is design to be used as implementation contract of EternalStorageProxy contract. */ -contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20 { - bytes32 internal constant MEDIATOR_BALANCE = 0x3db340e280667ee926fa8c51e8f9fcf88a0ff221a66d84d63b4778127d97d139; // keccak256(abi.encodePacked("mediatorBalance")) - +contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20, MediatorBalanceStorage { /** * @dev Stores the initial parameters of the mediator. * @param _bridgeContract the address of the AMB bridge contract. @@ -73,7 +72,7 @@ contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20 { require(msg.value > 0); require(withinLimit(msg.value)); addTotalSpentPerDay(getCurrentDay(), msg.value); - setMediatorBalance(mediatorBalance().add(msg.value)); + _setMediatorBalance(mediatorBalance().add(msg.value)); passMessage(msg.sender, _receiver, msg.value); } @@ -85,7 +84,7 @@ contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20 { */ function executeActionOnBridgedTokens(address _receiver, uint256 _value) internal { uint256 valueToTransfer = _shiftValue(_value); - setMediatorBalance(mediatorBalance().sub(valueToTransfer)); + _setMediatorBalance(mediatorBalance().sub(valueToTransfer)); bytes32 _messageId = messageId(); IMediatorFeeManager feeManager = feeManagerContract(); @@ -107,7 +106,7 @@ contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20 { * @param _value amount of native tokens to be received */ function executeActionOnFixedTokens(address _receiver, uint256 _value) internal { - setMediatorBalance(mediatorBalance().sub(_value)); + _setMediatorBalance(mediatorBalance().sub(_value)); Address.safeSendValue(_receiver, _value); } @@ -120,31 +119,16 @@ contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20 { Address.safeSendValue(_feeManager, _fee); } - /** - * @dev Tells the native balance of the contract. - * @return the current tracked native balance of the contract. - */ - function mediatorBalance() public view returns (uint256) { - return uintStorage[MEDIATOR_BALANCE]; - } - - /** - * @dev Sets the updated native balance of the contract. - * @param _balance the new native balance of the contract. - */ - function setMediatorBalance(uint256 _balance) internal { - uintStorage[MEDIATOR_BALANCE] = _balance; - } - /** * @dev Allows to transfer any locked token on this contract that is not part of the bridge operations. * Native tokens are not allowed to be claimed. * @param _token address of the token. * @param _to address that will receive the locked tokens on this contract. */ - function claimTokens(address _token, address _to) public { + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // Since bridged coins are locked at this contract, it is not allowed to claim them with the use of claimTokens function require(_token != address(0)); - super.claimTokens(_token, _to); + claimValues(_token, _to); } /** @@ -152,7 +136,7 @@ contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20 { * without the invocation of the required methods. * @param _receiver the address that will receive the tokens on the other network */ - function fixMediatorBalance(address _receiver) public onlyIfUpgradeabilityOwner { + function fixMediatorBalance(address _receiver) external onlyIfUpgradeabilityOwner validAddress(_receiver) { uint256 balance = address(this).balance; uint256 expectedBalance = mediatorBalance(); require(balance > expectedBalance); @@ -163,7 +147,7 @@ contract HomeAMBNativeToErc20 is BasicAMBNativeToErc20 { diff = available; } addTotalSpentPerDay(getCurrentDay(), diff); - setMediatorBalance(expectedBalance.add(diff)); + _setMediatorBalance(expectedBalance.add(diff)); passMessage(_receiver, _receiver, diff); } } diff --git a/contracts/upgradeable_contracts/amb_native_to_erc20/callflows.md b/contracts/upgradeable_contracts/amb_native_to_erc20/callflows.md index 7d4b07537..73d1843d7 100644 --- a/contracts/upgradeable_contracts/amb_native_to_erc20/callflows.md +++ b/contracts/upgradeable_contracts/amb_native_to_erc20/callflows.md @@ -123,6 +123,7 @@ BasicHomeAMB::executeAffirmation >>Mediator ........TokenBridgeMediator::handleBridgedTokens ..........HomeAMBNativeToErc20::executeActionOnBridgedTokens +............MediatorBalanceStorage::_setMediatorBalance ............RewardableMediator::distributeFee ..............HomeAMBNativeToErc20::onFeeDistribution ................Address::safeSendValue @@ -214,6 +215,7 @@ BasicHomeAMB::executeAffirmation ..........TokenBridgeMediator::messageHashRecipient ..........TokenBridgeMediator::messageHashValue ..........HomeAMBNativeToErc20::executeActionOnFixedTokens +............MediatorBalanceStorage::_setMediatorBalance ............Address::safeSendValue ..........emit FailedMessageFixed >>Bridge @@ -308,6 +310,7 @@ The mediator on the Home side has the `relayTokens` method which is payable. The >>Mediator HomeAMBNativeToErc20::relayTokens ..HomeAMBNativeToErc20::nativeTransfer +....MediatorBalanceStorage::_setMediatorBalance ....TokenBridgeMediator::passMessage ......TokenBridgeMediator::setMessageHashValue ......TokenBridgeMediator::setMessageHashRecipient diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol b/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol index 4cb76a464..83d3a4190 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/BasicForeignBridgeErcToErc.sol @@ -33,9 +33,15 @@ contract BasicForeignBridgeErcToErc is BasicForeignBridge { return 0xba4690f5; // bytes4(keccak256(abi.encodePacked("erc-to-erc-core"))) } - function claimTokens(address _token, address _to) public { + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. Bridged token cannot be withdrawn by this function. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // Since bridged tokens are locked at this contract, it is not allowed to claim them with the use of claimTokens function require(_token != address(erc20token())); - super.claimTokens(_token, _to); + claimValues(_token, _to); } function onExecuteMessage( diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol index c6517d447..32525cef0 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol @@ -121,6 +121,11 @@ contract HomeBridgeErcToErc is setErc677token(_erc677token); } + /** + * @dev Withdraws erc20 tokens or native coins from the token contract. It is required since the bridge contract is the owner of the token contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ function claimTokensFromErc677(address _token, address _to) external onlyIfUpgradeabilityOwner { IBurnableMintableERC677Token(erc677token()).claimTokens(_token, _to); } diff --git a/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol index 3d5c8a9ee..4ed11dab2 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol @@ -39,11 +39,17 @@ contract ForeignBridgeErcToNative is BasicForeignBridge, ERC20Bridge, OtherSideB return 0x18762d46; // bytes4(keccak256(abi.encodePacked("erc-to-native-core"))) } - function claimTokens(address _token, address _to) public { + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // Since bridged tokens are locked at this contract, it is not allowed to claim them with the use of claimTokens function require(_token != address(erc20token())); // Chai token is not claimable if investing into Chai is enabled require(_token != address(chaiToken()) || !isChaiTokenEnabled()); - super.claimTokens(_token, _to); + claimValues(_token, _to); } function onExecuteMessage( diff --git a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol index 8f032fc4b..48fd7626d 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol @@ -119,6 +119,20 @@ contract HomeBridgeErcToNative is _setBlockRewardContract(_blockReward); } + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // Since native coins are being minted by the blockReward contract and burned by sending them to the address(0), + // they are not locked at the contract during the normal operation. However, they can be still forced into this contract + // by using a selfdestruct opcode, or by using this contract address as a coinbase account. + // In this case it is necessary to allow claiming native coins back. + // Any other erc20 token can be safely claimed as well. + claimValues(_token, _to); + } + function _initialize( address _validatorContract, uint256[3] _dailyLimitMaxPerTxMinPerTxArray, // [ 0 = _dailyLimit, 1 = _maxPerTx, 2 = _minPerTx ] diff --git a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiAMBErc20ToErc677.sol b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiAMBErc20ToErc677.sol index 7c73dbf8c..b84577a99 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiAMBErc20ToErc677.sol @@ -75,8 +75,9 @@ contract BasicMultiAMBErc20ToErc677 is * @param _token address of claimed token, address(0) for native * @param _to address of tokens receiver */ - function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner validAddress(_to) { - require(_token == address(0) || !isTokenRegistered(_token)); // native coins or token not registered + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // Only unregistered tokens and native coins are allowed to be claimed with the use of this function + require(_token == address(0) || !isTokenRegistered(_token)); claimValues(_token, _to); } diff --git a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol index ff59e25e0..ec179c728 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/ForeignMultiAMBErc20ToErc677.sol @@ -200,7 +200,11 @@ contract ForeignMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677 { * @param _token address of the token contract. * @param _receiver the address that will receive the tokens on the other network. */ - function fixMediatorBalance(address _token, address _receiver) public onlyIfUpgradeabilityOwner { + function fixMediatorBalance(address _token, address _receiver) + external + onlyIfUpgradeabilityOwner + validAddress(_receiver) + { require(isTokenRegistered(_token)); uint256 balance = ERC677(_token).balanceOf(address(this)); uint256 expectedBalance = mediatorBalance(_token); diff --git a/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol index d9872bd8e..3fd0444d3 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol @@ -72,10 +72,26 @@ contract ForeignBridgeNativeToErc is return 0x92a8d7fe; // bytes4(keccak256(abi.encodePacked("native-to-erc-core"))) } + /** + * @dev Withdraws erc20 tokens or native coins from the token contract. It is required since the bridge contract is the owner of the token contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ function claimTokensFromErc677(address _token, address _to) external onlyIfUpgradeabilityOwner { IBurnableMintableERC677Token(erc677token()).claimTokens(_token, _to); } + /** + * @dev Withdraws the erc20 tokens or native coins from this contract. + * @param _token address of the claimed token or address(0) for native coins. + * @param _to address of the tokens/coins receiver. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // For foreign side of the bridge, tokens are not locked at the contract, they are minted and burned instead. + // So, its is safe to allow claiming of any tokens. Native coins are allowed as well. + claimValues(_token, _to); + } + function _initialize( address _validatorContract, address _erc677token, diff --git a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol index 9ff454850..86d11bac6 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol @@ -83,6 +83,18 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicHomeBridge, RewardableHom return 0x92a8d7fe; // bytes4(keccak256(abi.encodePacked("native-to-erc-core"))) } + /** + * @dev Allows to transfer any locked token on this contract that is not part of the bridge operations. + * Native tokens are not allowed to be claimed. + * @param _token address of the token. + * @param _to address that will receive the locked tokens on this contract. + */ + function claimTokens(address _token, address _to) external onlyIfUpgradeabilityOwner { + // Since bridged coins are locked at this contract, it is not allowed to claim them with the use of claimTokens function + require(_token != address(0)); + claimValues(_token, _to); + } + function _initialize( address _validatorContract, uint256[3] _dailyLimitMaxPerTxMinPerTxArray, // [ 0 = _dailyLimit, 1 = _maxPerTx, 2 = _minPerTx ] diff --git a/test/amb_erc20_to_native/foreign_mediator.test.js b/test/amb_erc20_to_native/foreign_mediator.test.js index 52d505267..4b23551e3 100644 --- a/test/amb_erc20_to_native/foreign_mediator.test.js +++ b/test/amb_erc20_to_native/foreign_mediator.test.js @@ -751,6 +751,7 @@ contract('ForeignAMBErc20ToNative', async accounts => { expect(events.length).to.be.equal(1) await contract.fixMediatorBalance(owner, { from: user }).should.be.rejected + await contract.fixMediatorBalance(ZERO_ADDRESS, { from: owner }).should.be.rejected await contract.fixMediatorBalance(owner, { from: owner }).should.be.fulfilled await contract.fixMediatorBalance(owner, { from: owner }).should.be.rejected diff --git a/test/amb_erc20_to_native/home_mediator.test.js b/test/amb_erc20_to_native/home_mediator.test.js index 69b08120c..f6a611d53 100644 --- a/test/amb_erc20_to_native/home_mediator.test.js +++ b/test/amb_erc20_to_native/home_mediator.test.js @@ -969,6 +969,8 @@ contract('HomeAMBErc20ToNative', async accounts => { // only owner can call the method await contract.fixMediatorBalance(user, { from: user }).should.be.rejected + await contract.fixMediatorBalance(ZERO_ADDRESS, { from: owner }).should.be.rejected + await contract.fixMediatorBalance(user, { from: owner }).should.be.fulfilled expect(await contract.totalBurntCoins()).to.be.bignumber.equal(ether('0.1')) diff --git a/test/amb_erc677_to_erc677/foreign_bridge.test.js b/test/amb_erc677_to_erc677/foreign_bridge.test.js index 1f77db1a9..d8b71eeff 100644 --- a/test/amb_erc677_to_erc677/foreign_bridge.test.js +++ b/test/amb_erc677_to_erc677/foreign_bridge.test.js @@ -10,7 +10,7 @@ const { expect } = require('chai') const { shouldBehaveLikeBasicAMBErc677ToErc677 } = require('./AMBErc677ToErc677Behavior.test') const { ether } = require('../helpers/helpers') const { getEvents, strip0x } = require('../helpers/helpers') -const { ERROR_MSG, toBN } = require('../setup') +const { ERROR_MSG, toBN, ZERO_ADDRESS } = require('../setup') const ZERO = toBN(0) const halfEther = ether('0.5') @@ -98,6 +98,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { expect(events.length).to.be.equal(1) expect(events[0].returnValues.encodedData.includes(strip0x(user).toLowerCase())).to.be.equal(true) expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(halfEther) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(halfEther) }) it('should be able to specify a different receiver', async () => { // Given @@ -126,6 +127,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { expect(events.length).to.be.equal(1) expect(events[0].returnValues.encodedData.includes(strip0x(user2).toLowerCase())).to.be.equal(true) expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(halfEther) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(halfEther) }) }) describe('handleBridgedTokens', () => { @@ -146,8 +148,9 @@ contract('ForeignAMBErc677ToErc677', async accounts => { decimalShiftZero, owner ).should.be.fulfilled - await erc677Token.mint(foreignBridge.address, twoEthers, { from: owner }).should.be.fulfilled - await erc677Token.transferOwnership(foreignBridge.address) + await erc677Token.mint(user, twoEthers, { from: owner }).should.be.fulfilled + await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled + await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled }) it('should transfer locked tokens on message from amb', async () => { // Given @@ -156,6 +159,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { const initialEvents = await getEvents(erc677Token, { event: 'Transfer' }) expect(initialEvents.length).to.be.equal(0) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) // can't be called by user @@ -184,6 +188,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { // Then expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(oneEther) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(oneEther) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(oneEther) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(oneEther) const TokensBridgedEvent = await getEvents(foreignBridge, { event: 'TokensBridged' }) @@ -208,14 +213,16 @@ contract('ForeignAMBErc677ToErc677', async accounts => { decimalShift, owner ).should.be.fulfilled - await erc677Token.mint(foreignBridge.address, twoEthers, { from: owner }).should.be.fulfilled - await erc677Token.transferOwnership(foreignBridge.address) + await erc677Token.mint(user, twoEthers, { from: owner }).should.be.fulfilled + await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled + await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled const currentDay = await foreignBridge.getCurrentDay() expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(ZERO) const initialEvents = await getEvents(erc677Token, { event: 'Transfer' }) expect(initialEvents.length).to.be.equal(0) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) const valueOnForeign = toBN('1000') @@ -242,6 +249,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { // Then expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(valueOnHome) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers.sub(valueOnForeign)) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers.sub(valueOnForeign)) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(valueOnForeign) const TokensBridgedEvent = await getEvents(foreignBridge, { event: 'TokensBridged' }) @@ -258,6 +266,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { const initialEvents = await getEvents(erc677Token, { event: 'Transfer' }) expect(initialEvents.length).to.be.equal(0) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) const outOfLimitValueData = await foreignBridge.contract.methods @@ -278,6 +287,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { // Then expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(ZERO) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) expect(await foreignBridge.outOfLimitAmount()).to.be.bignumber.equal(twoEthers) @@ -291,4 +301,186 @@ contract('ForeignAMBErc677ToErc677', async accounts => { expect(TokensBridgedEvent.length).to.be.equal(0) }) }) + + describe('fixFailedMessage', () => { + it('should update mediatorBalance ', async () => { + ambBridgeContract = await AMBMock.new() + await ambBridgeContract.setMaxGasPerTx(maxGasPerTx) + mediatorContract = await HomeAMBErc677ToErc677.new() + erc677Token = await ERC677BridgeToken.new('test', 'TST', 18) + await erc677Token.mint(user, twoEthers, { from: owner }).should.be.fulfilled + + foreignBridge = await ForeignAMBErc677ToErc677.new() + + await foreignBridge.initialize( + ambBridgeContract.address, + mediatorContract.address, + erc677Token.address, + [dailyLimit, maxPerTx, minPerTx], + [executionDailyLimit, executionMaxPerTx], + maxGasPerTx, + decimalShiftZero, + owner + ).should.be.fulfilled + await erc677Token.transferOwnership(foreignBridge.address) + + expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(twoEthers) + expect(await erc677Token.totalSupply()).to.be.bignumber.equal(twoEthers) + + // User transfer tokens + await erc677Token.transferAndCall(foreignBridge.address, oneEther, '0x', { from: user }).should.be.fulfilled + + expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(oneEther) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(oneEther) + + const events = await getEvents(ambBridgeContract, { event: 'MockedEvent' }) + expect(events.length).to.be.equal(1) + const transferMessageId = events[0].returnValues.messageId + // Given + expect(await foreignBridge.messageFixed(transferMessageId)).to.be.equal(false) + + // When + const fixData = await foreignBridge.contract.methods.fixFailedMessage(transferMessageId).encodeABI() + await ambBridgeContract.executeMessageCall( + foreignBridge.address, + mediatorContract.address, + fixData, + exampleMessageId, + 1000000 + ).should.be.fulfilled + + // Then + expect(await ambBridgeContract.messageCallStatus(exampleMessageId)).to.be.equal(true) + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(ZERO) + expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(twoEthers) + expect(await erc677Token.totalSupply()).to.be.bignumber.equal(twoEthers) + expect(await foreignBridge.messageFixed(transferMessageId)).to.be.equal(true) + }) + }) + + describe('fixMediatorBalance', () => { + let currentDay + beforeEach(async () => { + const storageProxy = await EternalStorageProxy.new() + foreignBridge = await ForeignAMBErc677ToErc677.new() + await storageProxy.upgradeTo('1', foreignBridge.address).should.be.fulfilled + foreignBridge = await ForeignAMBErc677ToErc677.at(storageProxy.address) + + erc677Token = await ERC677BridgeToken.new('test', 'TST', 18) + await erc677Token.mint(user, twoEthers, { from: owner }).should.be.fulfilled + await erc677Token.mint(foreignBridge.address, twoEthers, { from: owner }).should.be.fulfilled + + ambBridgeContract = await AMBMock.new() + await ambBridgeContract.setMaxGasPerTx(maxGasPerTx) + await foreignBridge.initialize( + ambBridgeContract.address, + mediatorContract.address, + erc677Token.address, + [dailyLimit, maxPerTx, minPerTx], + [executionDailyLimit, executionMaxPerTx], + maxGasPerTx, + decimalShiftZero, + owner + ).should.be.fulfilled + + currentDay = await foreignBridge.getCurrentDay() + expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(ZERO) + const initialEvents = await getEvents(ambBridgeContract, { event: 'MockedEvent' }) + expect(initialEvents.length).to.be.equal(0) + }) + + it('should allow to fix extra mediator balance', async () => { + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(ZERO) + expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) + + await erc677Token.transferAndCall(foreignBridge.address, halfEther, '0x', { from: user }).should.be.fulfilled + await foreignBridge.setDailyLimit(ether('3')).should.be.fulfilled + await foreignBridge.setMaxPerTx(twoEthers).should.be.fulfilled + + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(halfEther) + expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers.add(halfEther)) + expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(halfEther) + let events = await getEvents(ambBridgeContract, { event: 'MockedEvent' }) + expect(events.length).to.be.equal(1) + + await foreignBridge.fixMediatorBalance(owner, { from: user }).should.be.rejected + await foreignBridge.fixMediatorBalance(ZERO_ADDRESS, { from: owner }).should.be.rejected + await foreignBridge.fixMediatorBalance(owner, { from: owner }).should.be.fulfilled + await foreignBridge.fixMediatorBalance(owner, { from: owner }).should.be.rejected + + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers.add(halfEther)) + expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers.add(halfEther)) + + expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(twoEthers.add(halfEther)) + events = await getEvents(ambBridgeContract, { event: 'MockedEvent' }) + expect(events.length).to.be.equal(2) + }) + + it('should allow to fix extra mediator balance with respect to limits', async () => { + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(ZERO) + expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) + + await erc677Token.transferAndCall(foreignBridge.address, halfEther, '0x', { from: user }).should.be.fulfilled + await foreignBridge.setMinPerTx('1').should.be.fulfilled + await foreignBridge.setMaxPerTx(halfEther).should.be.fulfilled + await foreignBridge.setDailyLimit(ether('1.25')).should.be.fulfilled + + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(halfEther) + expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers.add(halfEther)) + expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(halfEther) + let events = await getEvents(ambBridgeContract, { event: 'MockedEvent' }) + expect(events.length).to.be.equal(1) + + await foreignBridge.fixMediatorBalance(owner, { from: user }).should.be.rejected + // should fix 0.5 ether + await foreignBridge.fixMediatorBalance(owner, { from: owner }).should.be.fulfilled + + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(oneEther) + expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(oneEther) + + // should fix 0.25 ether + await foreignBridge.fixMediatorBalance(owner, { from: owner }).should.be.fulfilled + // no remaining daily quota + await foreignBridge.fixMediatorBalance(owner, { from: owner }).should.be.rejected + + await foreignBridge.setDailyLimit(oneEther).should.be.fulfilled + + // no remaining daily quota + await foreignBridge.fixMediatorBalance(owner, { from: owner }).should.be.rejected + + expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(ether('1.25')) + expect(await foreignBridge.totalSpentPerDay(currentDay)).to.be.bignumber.equal(ether('1.25')) + + expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers.add(halfEther)) + events = await getEvents(ambBridgeContract, { event: 'MockedEvent' }) + expect(events.length).to.be.equal(3) + }) + }) + + describe('claimTokens', () => { + it('should not allow to claim bridged token', async () => { + const storageProxy = await EternalStorageProxy.new() + foreignBridge = await ForeignAMBErc677ToErc677.new() + await storageProxy.upgradeTo('1', foreignBridge.address).should.be.fulfilled + foreignBridge = await ForeignAMBErc677ToErc677.at(storageProxy.address) + erc677Token = await ERC677BridgeToken.new('test', 'TST', 18) + await erc677Token.mint(foreignBridge.address, twoEthers, { from: owner }).should.be.fulfilled + + ambBridgeContract = await AMBMock.new() + await ambBridgeContract.setMaxGasPerTx(maxGasPerTx) + await foreignBridge.initialize( + ambBridgeContract.address, + mediatorContract.address, + erc677Token.address, + [dailyLimit, maxPerTx, minPerTx], + [executionDailyLimit, executionMaxPerTx], + maxGasPerTx, + decimalShiftZero, + owner + ).should.be.fulfilled + + await foreignBridge.claimTokens(erc677Token.address, accounts[3], { from: accounts[3] }).should.be.rejected + await foreignBridge.claimTokens(erc677Token.address, accounts[3], { from: owner }).should.be.rejected + }) + }) }) diff --git a/test/amb_native_to_erc20/home_mediator.test.js b/test/amb_native_to_erc20/home_mediator.test.js index a77b779e2..f8f765e7d 100644 --- a/test/amb_native_to_erc20/home_mediator.test.js +++ b/test/amb_native_to_erc20/home_mediator.test.js @@ -1442,6 +1442,8 @@ contract('HomeAMBNativeToErc20', async accounts => { // only owner can call the method await contract.fixMediatorBalance(user, { from: user }).should.be.rejectedWith(ERROR_MSG) + await contract.fixMediatorBalance(ZERO_ADDRESS, { from: owner }).should.be.rejected + await contract.fixMediatorBalance(user, { from: owner }).should.be.fulfilled // imbalance was already fixed diff --git a/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js b/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js index 5f81b75e4..67109e612 100644 --- a/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js +++ b/test/multi_amb_erc20_to_erc677/foreign_mediator.test.js @@ -865,6 +865,7 @@ contract('ForeignMultiAMBErc20ToErc677', async accounts => { await contract.fixMediatorBalance(token.address, owner, { from: user }).should.be.rejected await contract.fixMediatorBalance(ZERO_ADDRESS, owner, { from: owner }).should.be.rejected + await contract.fixMediatorBalance(token.address, ZERO_ADDRESS, { from: owner }).should.be.rejected await contract.fixMediatorBalance(token.address, owner, { from: owner }).should.be.fulfilled await contract.fixMediatorBalance(token.address, owner, { from: owner }).should.be.rejected diff --git a/test/native_to_erc/home_bridge_test.js b/test/native_to_erc/home_bridge_test.js index 4a8d7d8e5..b70d331f2 100644 --- a/test/native_to_erc/home_bridge_test.js +++ b/test/native_to_erc/home_bridge_test.js @@ -1083,7 +1083,7 @@ contract('HomeBridge', async accounts => { expect(await tokenMock.balanceOf(homeBridge.address)).to.be.bignumber.equal(ZERO) expect(await tokenMock.balanceOf(accounts[3])).to.be.bignumber.equal(halfEther) }) - it('should work for native coins', async () => { + it('should not work for native coins', async () => { const storageProxy = await EternalStorageProxy.new() const data = homeContract.contract.methods .initialize( @@ -1099,14 +1099,11 @@ contract('HomeBridge', async accounts => { await storageProxy.upgradeToAndCall('1', homeContract.address, data).should.be.fulfilled const homeBridge = await HomeBridge.at(storageProxy.address) - const balanceBefore = toBN(await web3.eth.getBalance(accounts[3])) - await homeBridge.sendTransaction({ from: accounts[2], value: halfEther }).should.be.fulfilled expect(toBN(await web3.eth.getBalance(homeBridge.address))).to.be.bignumber.equal(halfEther) - await homeBridge.claimTokens(ZERO_ADDRESS, accounts[3], { from: owner }).should.be.fulfilled - expect(toBN(await web3.eth.getBalance(homeBridge.address))).to.be.bignumber.equal(ZERO) - expect(toBN(await web3.eth.getBalance(accounts[3]))).to.be.bignumber.equal(balanceBefore.add(halfEther)) + await homeBridge.claimTokens(ZERO_ADDRESS, accounts[3], { from: accounts[3] }).should.be.rejected + await homeBridge.claimTokens(ZERO_ADDRESS, accounts[3], { from: owner }).should.be.rejected }) }) diff --git a/test/stake_token_mediators/foreign_mediator.test.js b/test/stake_token_mediators/foreign_mediator.test.js index c0fdd6836..abe72037d 100644 --- a/test/stake_token_mediators/foreign_mediator.test.js +++ b/test/stake_token_mediators/foreign_mediator.test.js @@ -1,5 +1,6 @@ const ForeignStakeTokenMediator = artifacts.require('ForeignStakeTokenMediator.sol') const HomeStakeTokenMediator = artifacts.require('HomeStakeTokenMediator.sol') +const EternalStorageProxy = artifacts.require('EternalStorageProxy.sol') const ERC677BridgeToken = artifacts.require('ERC677BridgeToken.sol') const AMBMock = artifacts.require('AMBMock.sol') @@ -263,4 +264,28 @@ contract('ForeignStakeTokenMediator', async accounts => { await token.transferAndCall(foreignMediator.address, halfEther, user, { from: user }).should.be.fulfilled }) }) + + describe('claimTokens', () => { + it('should not allow to claim bridged token', async () => { + const storageProxy = await EternalStorageProxy.new() + await storageProxy.upgradeTo('1', foreignMediator.address).should.be.fulfilled + foreignMediator = await ForeignStakeTokenMediator.at(storageProxy.address) + token = await ERC677BridgeToken.new('test', 'TST', 18) + await token.mint(foreignMediator.address, twoEthers, { from: owner }).should.be.fulfilled + + await foreignMediator.initialize( + foreignBridge.address, + homeMediator.address, + token.address, + [dailyLimit, maxPerTx, minPerTx], + [executionDailyLimit, executionMaxPerTx], + maxGasPerTx, + decimalShiftZero, + owner + ).should.be.fulfilled + + await foreignMediator.claimTokens(token.address, accounts[3], { from: accounts[3] }).should.be.rejected + await foreignMediator.claimTokens(token.address, accounts[3], { from: owner }).should.be.rejected + }) + }) }) From d5fc8ab8d5ec5ea34828570c79bb7b8d039421f1 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 22:09:37 +0300 Subject: [PATCH 09/14] Add missing checks and return values (#535) --- .../BaseMediatorFeeManager.sol | 3 ++ .../InterestReceiver.sol | 3 ++ .../HomeMultiAMBErc20ToErc677.sol | 1 + .../foreign_mediator.test.js | 9 ++++++ .../amb_native_to_erc20/home_mediator.test.js | 4 +++ test/erc_to_native/foreign_bridge.test.js | 1 + .../home_mediator.test.js | 28 +++++++++++++++++++ 7 files changed, 49 insertions(+) diff --git a/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol b/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol index 09922b7b6..ced8c4559 100644 --- a/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol +++ b/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol @@ -2,6 +2,7 @@ pragma solidity 0.4.24; import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; import "openzeppelin-solidity/contracts/math/SafeMath.sol"; +import "openzeppelin-solidity/contracts/AddressUtils.sol"; /** * @title BaseMediatorFeeManager @@ -32,8 +33,10 @@ contract BaseMediatorFeeManager is Ownable { * @param _owner address of the owner of the fee manager contract. * @param _fee the fee percentage amount. * @param _rewardAccountList list of addresses that will receive the fee rewards. + * @param _mediatorContract address of the mediator contract used together with this fee manager. */ constructor(address _owner, uint256 _fee, address[] _rewardAccountList, address _mediatorContract) public { + require(AddressUtils.isContract(_mediatorContract)); require(_rewardAccountList.length > 0 && _rewardAccountList.length <= MAX_REWARD_ACCOUNTS); _transferOwnership(_owner); _setFee(_fee); diff --git a/contracts/upgradeable_contracts/InterestReceiver.sol b/contracts/upgradeable_contracts/InterestReceiver.sol index e87216d2a..c881447d2 100644 --- a/contracts/upgradeable_contracts/InterestReceiver.sol +++ b/contracts/upgradeable_contracts/InterestReceiver.sol @@ -29,6 +29,7 @@ contract InterestReceiver is ERC677Receiver, Ownable, Claimable, TokenSwapper { */ constructor(address _owner, address _bridgeContract, address _receiverInXDai) public { require(AddressUtils.isContract(_bridgeContract)); + require(_receiverInXDai != address(0)); _transferOwnership(_owner); bridgeContract = _bridgeContract; receiverInXDai = _receiverInXDai; @@ -49,6 +50,7 @@ contract InterestReceiver is ERC677Receiver, Ownable, Claimable, TokenSwapper { * @param _receiverInXDai address of new receiver account in the xDai chain */ function setReceiverInXDai(address _receiverInXDai) external onlyOwner { + require(_receiverInXDai != address(0)); receiverInXDai = _receiverInXDai; } @@ -93,6 +95,7 @@ contract InterestReceiver is ERC677Receiver, Ownable, Claimable, TokenSwapper { daiToken().approve(address(bridgeContract), 0); emit RelayTokensFailed(receiverInXDai, finalDaiBalance); } + return true; } /** diff --git a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol index 69789dcc2..e42d5820c 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol @@ -98,6 +98,7 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager ) external onlyMediator { string memory name = _name; string memory symbol = _symbol; + require(bytes(name).length > 0 || bytes(symbol).length > 0); if (bytes(name).length == 0) { name = symbol; } else if (bytes(symbol).length == 0) { diff --git a/test/amb_native_to_erc20/foreign_mediator.test.js b/test/amb_native_to_erc20/foreign_mediator.test.js index 0641373d8..9235b7ea2 100644 --- a/test/amb_native_to_erc20/foreign_mediator.test.js +++ b/test/amb_native_to_erc20/foreign_mediator.test.js @@ -601,6 +601,15 @@ contract('ForeignAMBNativeToErc20', async accounts => { mediator.address, token.address ).should.be.rejectedWith(ERROR_MSG) + // invalid mediator contract address + await ForeignFeeManagerAMBNativeToErc20.new( + owner, + fee, + rewardAccountList, + ZERO_ADDRESS, + token.address + ).should.be.rejectedWith(ERROR_MSG) + // invalid token address await ForeignFeeManagerAMBNativeToErc20.new( owner, fee, diff --git a/test/amb_native_to_erc20/home_mediator.test.js b/test/amb_native_to_erc20/home_mediator.test.js index f8f765e7d..3b48e64ce 100644 --- a/test/amb_native_to_erc20/home_mediator.test.js +++ b/test/amb_native_to_erc20/home_mediator.test.js @@ -1120,6 +1120,10 @@ contract('HomeAMBNativeToErc20', async accounts => { [...rewardAccountList, mediator.address], mediator.address ).should.be.rejectedWith(ERROR_MSG) + // invalid mediator contract + await HomeFeeManagerAMBNativeToErc20.new(owner, fee, rewardAccountList, ZERO_ADDRESS).should.be.rejectedWith( + ERROR_MSG + ) await HomeFeeManagerAMBNativeToErc20.new(owner, fee, rewardAccountList, mediator.address) }) }) diff --git a/test/erc_to_native/foreign_bridge.test.js b/test/erc_to_native/foreign_bridge.test.js index a5b4f4dfd..20a3645f8 100644 --- a/test/erc_to_native/foreign_bridge.test.js +++ b/test/erc_to_native/foreign_bridge.test.js @@ -1598,6 +1598,7 @@ contract('ForeignBridge_ERC20_to_Native', async accounts => { }) it('should update bridge contract address', async () => { + await interestRecipient.setReceiverInXDai(ZERO_ADDRESS, { from: receiverOwner }).should.be.rejected await interestRecipient.setReceiverInXDai(accounts[8], { from: receiverOwner }).should.be.fulfilled expect(await interestRecipient.receiverInXDai()).to.be.equal(accounts[8]) }) diff --git a/test/multi_amb_erc20_to_erc677/home_mediator.test.js b/test/multi_amb_erc20_to_erc677/home_mediator.test.js index c1125332e..a8af30d9a 100644 --- a/test/multi_amb_erc20_to_erc677/home_mediator.test.js +++ b/test/multi_amb_erc20_to_erc677/home_mediator.test.js @@ -419,6 +419,34 @@ contract('HomeMultiAMBErc20ToErc677', async accounts => { expect(await homeToken.decimals()).to.be.bignumber.equal('18') }) + it('should not register new token with empty name and empty symbol', async () => { + const data1 = await contract.contract.methods + .deployAndHandleBridgedTokens(accounts[0], '', '', 18, user, oneEther.toString(10)) + .encodeABI() + await ambBridgeContract.executeMessageCall( + contract.address, + otherSideMediator.address, + data1, + exampleMessageId, + 2000000 + ).should.be.fulfilled + + expect(await ambBridgeContract.messageCallStatus(exampleMessageId)).to.be.equal(false) + + const data2 = await contract.contract.methods + .deployAndHandleBridgedTokens(accounts[1], 'TEST', '', 18, user, oneEther.toString(10)) + .encodeABI() + await ambBridgeContract.executeMessageCall( + contract.address, + otherSideMediator.address, + data2, + otherMessageId, + 2000000 + ).should.be.fulfilled + + expect(await ambBridgeContract.messageCallStatus(otherMessageId)).to.be.equal(true) + }) + for (const decimals of [3, 18, 20]) { it(`should initialize limits according to decimals = ${decimals}`, async () => { const f1 = toBN(`1${'0'.repeat(decimals)}`) From 16b8bbbed0a6160ae12dc2566a5c40bd8c2686ad Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 15 Oct 2020 22:17:56 +0300 Subject: [PATCH 10/14] Improve documentation in several methods (#536) --- contracts/ERC677BridgeTokenRewardable.sol | 18 +++++++++++++++++ contracts/PermittableToken.sol | 2 ++ contracts/libraries/Message.sol | 20 ------------------- contracts/libraries/TokenReader.sol | 2 +- .../BaseMediatorFeeManager.sol | 2 +- .../arbitrary_message/BasicAMB.sol | 5 +++++ .../BasicMultiTokenBridge.sol | 3 +++ 7 files changed, 30 insertions(+), 22 deletions(-) diff --git a/contracts/ERC677BridgeTokenRewardable.sol b/contracts/ERC677BridgeTokenRewardable.sol index ee4fd12be..42338c36b 100644 --- a/contracts/ERC677BridgeTokenRewardable.sol +++ b/contracts/ERC677BridgeTokenRewardable.sol @@ -13,11 +13,29 @@ contract ERC677BridgeTokenRewardable is ERC677MultiBridgeToken { // solhint-disable-previous-line no-empty-blocks } + /** + * @dev Updates the address of the used block reward contract. + * Only the token owner can call this method. + * Even though this function is inteded only for the initialization purpose, + * it is still possible to change the already used block reward contract. + * In this case users of the old contract won't lose their accumulated rewards, + * they can proceed with the withdrawal by calling the old block reward contract directly. + * @param _blockRewardContract address of the new block reward contract. + */ function setBlockRewardContract(address _blockRewardContract) external onlyOwner { require(AddressUtils.isContract(_blockRewardContract)); blockRewardContract = _blockRewardContract; } + /** + * @dev Updates the address of the used staking contract. + * Only the token owner can call this method. + * Even though this function is inteded only for the initialization purpose, + * it is still possible to change the already used staking contract. + * In this case users of the old staking contract won't lose their tokens, + * they can proceed with the withdrawal by calling the old staking contract directly. + * @param _stakingContract address of the new staking contract. + */ function setStakingContract(address _stakingContract) external onlyOwner { require(AddressUtils.isContract(_stakingContract)); require(balanceOf(_stakingContract) == 0); diff --git a/contracts/PermittableToken.sol b/contracts/PermittableToken.sol index c297cf254..2f00c8664 100644 --- a/contracts/PermittableToken.sol +++ b/contracts/PermittableToken.sol @@ -98,6 +98,8 @@ contract PermittableToken is ERC677BridgeToken { /// @param _nonce The nonce taken from `nonces(_holder)` public getter. /// @param _expiry The allowance expiration date (unix timestamp in UTC). /// Can be zero for no expiration. Forced to zero if `_allowed` is `false`. + /// Note that timestamps are not precise, malicious miner/validator can manipulate them to some extend. + /// Assume that there can be a 900 seconds time delta between the desired timestamp and the actual expiration. /// @param _allowed True to enable unlimited allowance for the spender by the holder. False to disable. /// @param _v A final byte of signature (ECDSA component). /// @param _r The first 32 bytes of signature (ECDSA component). diff --git a/contracts/libraries/Message.sol b/contracts/libraries/Message.sol index d674a3673..4e43e36a4 100644 --- a/contracts/libraries/Message.sol +++ b/contracts/libraries/Message.sol @@ -2,26 +2,6 @@ pragma solidity 0.4.24; import "../interfaces/IBridgeValidators.sol"; library Message { - // function uintToString(uint256 inputValue) internal pure returns (string) { - // // figure out the length of the resulting string - // uint256 length = 0; - // uint256 currentValue = inputValue; - // do { - // length++; - // currentValue /= 10; - // } while (currentValue != 0); - // // allocate enough memory - // bytes memory result = new bytes(length); - // // construct the string backwards - // uint256 i = length - 1; - // currentValue = inputValue; - // do { - // result[i--] = byte(48 + currentValue % 10); - // currentValue /= 10; - // } while (currentValue != 0); - // return string(result); - // } - function addressArrayContains(address[] array, address value) internal pure returns (bool) { for (uint256 i = 0; i < array.length; i++) { if (array[i] == value) { diff --git a/contracts/libraries/TokenReader.sol b/contracts/libraries/TokenReader.sol index d6f94c79f..9c919a7fc 100644 --- a/contracts/libraries/TokenReader.sol +++ b/contracts/libraries/TokenReader.sol @@ -73,7 +73,7 @@ library TokenReader { ptr := mload(0x40) mstore(ptr, 0x95d89b4100000000000000000000000000000000000000000000000000000000) // symbol() if iszero(staticcall(gas, _token, ptr, 4, ptr, 32)) { - mstore(ptr, 0xf76f8d7800000000000000000000000000000000000000000000000000000000) // SYMBOl() + mstore(ptr, 0xf76f8d7800000000000000000000000000000000000000000000000000000000) // SYMBOL() staticcall(gas, _token, ptr, 4, ptr, 32) pop } diff --git a/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol b/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol index ced8c4559..f15b4f00d 100644 --- a/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol +++ b/contracts/upgradeable_contracts/BaseMediatorFeeManager.sol @@ -32,7 +32,7 @@ contract BaseMediatorFeeManager is Ownable { * @dev Stores the initial parameters of the fee manager. * @param _owner address of the owner of the fee manager contract. * @param _fee the fee percentage amount. - * @param _rewardAccountList list of addresses that will receive the fee rewards. + * @param _rewardAccountList list of unique addresses that will receive the fee rewards. * @param _mediatorContract address of the mediator contract used together with this fee manager. */ constructor(address _owner, uint256 _fee, address[] _rewardAccountList, address _mediatorContract) public { diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol index 1f7e2684b..342a359cd 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/BasicAMB.sol @@ -106,6 +106,11 @@ contract BasicAMB is BasicBridge, VersionableAMB { function _setChainIds(uint256 _sourceChainId, uint256 _destinationChainId) internal { require(_sourceChainId > 0 && _destinationChainId > 0); require(_sourceChainId != _destinationChainId); + + // Length fields are needed further when encoding the message. + // Chain ids are compressed, so that leading zero bytes are not preserved. + // In order to save some gas during calls to MessageDelivery.c, + // lengths of chain ids are precalculated and being saved in the storage. uint256 sourceChainIdLength = 0; uint256 destinationChainIdLength = 0; uint256 mask = 0xff; diff --git a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiTokenBridge.sol b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiTokenBridge.sol index a051088d8..91ffc85c4 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiTokenBridge.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiTokenBridge.sol @@ -276,6 +276,9 @@ contract BasicMultiTokenBridge is EternalStorage, Ownable { // e.g. minPerTx(address(0)) == 10 ** 14, _decimals == 3. _minPerTx happens to be 0, which is not allowed. // in this case, limits are raised to the default values if (_minPerTx == 0) { + // Numbers 1, 100, 10000 are chosen in a semi-random way, + // so that any token with small decimals can still be bridged in some amounts. + // It is possible to override limits for the particular token later if needed. _minPerTx = 1; if (_maxPerTx <= _minPerTx) { _maxPerTx = 100; From 62e722e9e707204b0eb87e1b9690237b9f33e436 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Fri, 16 Oct 2020 09:46:49 +0300 Subject: [PATCH 11/14] Fix tests by using transferAndCall for foreign side in erc20-to-erc677 on top of AMB (#537) --- test/amb_erc677_to_erc677/foreign_bridge.test.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/amb_erc677_to_erc677/foreign_bridge.test.js b/test/amb_erc677_to_erc677/foreign_bridge.test.js index d8b71eeff..847609cce 100644 --- a/test/amb_erc677_to_erc677/foreign_bridge.test.js +++ b/test/amb_erc677_to_erc677/foreign_bridge.test.js @@ -149,15 +149,13 @@ contract('ForeignAMBErc677ToErc677', async accounts => { owner ).should.be.fulfilled await erc677Token.mint(user, twoEthers, { from: owner }).should.be.fulfilled - await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled - await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled + await erc677Token.transferAndCall(foreignBridge.address, oneEther, '0x', { from: user }).should.be.fulfilled + await erc677Token.transferAndCall(foreignBridge.address, oneEther, '0x', { from: user }).should.be.fulfilled }) it('should transfer locked tokens on message from amb', async () => { // Given const currentDay = await foreignBridge.getCurrentDay() expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(ZERO) - const initialEvents = await getEvents(erc677Token, { event: 'Transfer' }) - expect(initialEvents.length).to.be.equal(0) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) @@ -214,13 +212,11 @@ contract('ForeignAMBErc677ToErc677', async accounts => { owner ).should.be.fulfilled await erc677Token.mint(user, twoEthers, { from: owner }).should.be.fulfilled - await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled - await erc677Token.transfer(foreignBridge.address, oneEther, { from: user }).should.be.fulfilled + await erc677Token.transferAndCall(foreignBridge.address, oneEther, '0x', { from: user }).should.be.fulfilled + await erc677Token.transferAndCall(foreignBridge.address, oneEther, '0x', { from: user }).should.be.fulfilled const currentDay = await foreignBridge.getCurrentDay() expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(ZERO) - const initialEvents = await getEvents(erc677Token, { event: 'Transfer' }) - expect(initialEvents.length).to.be.equal(0) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) @@ -263,8 +259,6 @@ contract('ForeignAMBErc677ToErc677', async accounts => { // Given const currentDay = await foreignBridge.getCurrentDay() expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(ZERO) - const initialEvents = await getEvents(erc677Token, { event: 'Transfer' }) - expect(initialEvents.length).to.be.equal(0) expect(await erc677Token.balanceOf(foreignBridge.address)).to.be.bignumber.equal(twoEthers) expect(await foreignBridge.mediatorBalance()).to.be.bignumber.equal(twoEthers) expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) From 65fa34f562168d6981de4efe9eec0d55e52198c8 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Fri, 16 Oct 2020 13:09:54 +0300 Subject: [PATCH 12/14] Add an expirations reset after unlimited approval (#533) --- contracts/PermittableToken.sol | 20 ++++++++++++ test/poa20_test.js | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/contracts/PermittableToken.sol b/contracts/PermittableToken.sol index 2f00c8664..b2469b3c8 100644 --- a/contracts/PermittableToken.sol +++ b/contracts/PermittableToken.sol @@ -67,6 +67,26 @@ contract PermittableToken is ERC677BridgeToken { return true; } + /// @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender. + /// @param _to The address which will spend the funds. + /// @param _value The amount of tokens to be spent. + function approve(address _to, uint256 _value) public returns (bool result) { + result = super.approve(_to, _value); + if (_value == uint256(-1)) { + delete expirations[msg.sender][_to]; + } + } + + /// @dev Atomically increases the allowance granted to spender by the caller. + /// @param _to The address which will spend the funds. + /// @param _addedValue The amount of tokens to increase the allowance by. + function increaseAllowance(address _to, uint256 _addedValue) public returns (bool result) { + result = super.increaseAllowance(_to, _addedValue); + if (allowed[msg.sender][_to] == uint256(-1)) { + delete expirations[msg.sender][_to]; + } + } + /// @dev An alias for `transfer` function. /// @param _to The address of the recipient. /// @param _amount The value to transfer. diff --git a/test/poa20_test.js b/test/poa20_test.js index 69a1f1ad5..906ff510f 100644 --- a/test/poa20_test.js +++ b/test/poa20_test.js @@ -631,6 +631,8 @@ function testERC677BridgeToken(accounts, rewardable, permittable, createToken) { // Mint some extra tokens for the `holder` await token.mint(holder, '10000', { from: owner }).should.be.fulfilled ;(await token.balanceOf.call(holder)).should.be.bignumber.equal(new BN('10000')) + web3.eth.accounts.wallet.add(privateKey) + await web3.eth.sendTransaction({ from: owner, to: holder, value: oneEther }) }) it('should permit', async () => { // Holder signs the `permit` params with their privateKey @@ -750,6 +752,61 @@ function testERC677BridgeToken(accounts, rewardable, permittable, createToken) { await token.setNow(901).should.be.fulfilled await token.transferFrom(holder, accounts[9], '4000', { from: spender }).should.be.rejectedWith(ERROR_MSG) }) + it('should reset expiration on subsequent approve', async () => { + expiry = 900 + const signature = permitSign( + { + name: await token.name.call(), + version: await token.version.call(), + chainId: '100', + verifyingContract: token.address + }, + { + holder, + spender, + nonce, + expiry, + allowed + }, + privateKey + ) + await token.setNow(800).should.be.fulfilled + await token.permit(holder, spender, nonce, expiry, allowed, signature.v, signature.r, signature.s).should.be + .fulfilled + ;(await token.expirations.call(holder, spender)).should.be.bignumber.equal(new BN(expiry)) + const data = await token.contract.methods.approve(spender, -1).encodeABI() + await web3.eth.sendTransaction({ from: holder, to: token.address, data, gas: 100000 }).should.be.fulfilled + ;(await token.expirations.call(holder, spender)).should.be.bignumber.equal(ZERO) + }) + it('should reset expiration on subsequent increaseAllowance', async () => { + expiry = 900 + const signature = permitSign( + { + name: await token.name.call(), + version: await token.version.call(), + chainId: '100', + verifyingContract: token.address + }, + { + holder, + spender, + nonce, + expiry, + allowed + }, + privateKey + ) + await token.setNow(800).should.be.fulfilled + await token.permit(holder, spender, nonce, expiry, allowed, signature.v, signature.r, signature.s).should.be + .fulfilled + ;(await token.expirations.call(holder, spender)).should.be.bignumber.equal(new BN(expiry)) + let data = await token.contract.methods.approve(spender, -2).encodeABI() + await web3.eth.sendTransaction({ from: holder, to: token.address, data, gas: 100000 }).should.be.fulfilled + ;(await token.expirations.call(holder, spender)).should.be.bignumber.equal(new BN(expiry)) + data = await token.contract.methods.increaseAllowance(spender, 1).encodeABI() + await web3.eth.sendTransaction({ from: holder, to: token.address, data, gas: 100000 }).should.be.fulfilled + ;(await token.expirations.call(holder, spender)).should.be.bignumber.equal(ZERO) + }) it('should disallow unlimited allowance', async () => { expiry = 900 await token.setNow(800).should.be.fulfilled From ac39624d1aae705aba3130b753bb46f93a4fca72 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Fri, 16 Oct 2020 19:13:22 +0300 Subject: [PATCH 13/14] Refactor BlockReward.reward() function (#534) --- .../amb_erc20_to_native/BlockReward.sol | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/contracts/upgradeable_contracts/amb_erc20_to_native/BlockReward.sol b/contracts/upgradeable_contracts/amb_erc20_to_native/BlockReward.sol index b79a6d152..4f8d7980c 100644 --- a/contracts/upgradeable_contracts/amb_erc20_to_native/BlockReward.sol +++ b/contracts/upgradeable_contracts/amb_erc20_to_native/BlockReward.sol @@ -75,32 +75,42 @@ contract BlockReward is EternalStorage { /** * @dev Special method that is called by the system, in order to get accounts and values for minting new coins in upcoming block. - * Can be called on by a system-reserved address. + * Can be called only by a system-reserved address. * @param benefactors list of block reward receivers, should be empty. * @param kind list of reward types for addresses in benefactors list, should be empty. * @return tuple of addresses list and values list of the same length that describes where and how much new coins should be minted. */ function reward(address[] benefactors, uint16[] kind) external onlySystem returns (address[], uint256[]) { + // As these contracts were intended to work on top of the forked Quorum client, + // the arguments of this function will depend on the particular client code. + // For simplicity, and since Quorum does not have any block rewards, + // it was decided to keep argument arrays empty. + // However, in the original OpenEthereum blockReward implementation, + // first argument should contain some reward receiver addresses (i.e. miner of a block, uncle blocks miners, etc.), + // second argument describes the reward types of the benefactors. require(benefactors.length == 0); require(kind.length == 0); uint256 extraLength = extraReceiversLength(); + // Lists with extra receivers and their rewards. Extra receivers are generated by the bridge/mediator contracts, + // and they are not passed in the benefactors array. address[] memory receivers = new address[](extraLength); uint256[] memory rewards = new uint256[](extraLength); uint256 i; + uint256 sumOfRewards = 0; + uint256 sumOfBridgeAmounts = 0; + for (i = 0; i < extraLength; i++) { address extraAddress = extraReceiverByIndex(i); uint256 extraAmount = extraReceiverAmount(extraAddress); _setExtraReceiverAmount(0, extraAddress); receivers[i] = extraAddress; rewards[i] = extraAmount; - } - - for (i = 0; i < extraLength; i++) { - _setMinted(rewards[i], receivers[i]); + _setMinted(extraAmount, extraAddress); + sumOfRewards += extraAmount; } for (i = 0; i < bridgesAllowedLength; i++) { @@ -110,9 +120,12 @@ contract BlockReward is EternalStorage { if (bridgeAmountForBlock > 0) { _setBridgeAmount(0, bridgeAddress); _addMintedTotallyByBridge(bridgeAmountForBlock, bridgeAddress); + sumOfBridgeAmounts += bridgeAmountForBlock; } } + require(sumOfRewards == sumOfBridgeAmounts); + _clearExtraReceivers(); return (receivers, rewards); From 13f8ea6ed78a773274a60e4b43cfdea1c54bfd62 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Mon, 19 Oct 2020 16:06:55 +0300 Subject: [PATCH 14/14] Bump package version to 5.5.0-rc0 (#540) --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 50d2644b0..186895a4b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "tokenbridge-contracts", - "version": "5.4.1", + "version": "5.5.0-rc0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 3c0fd8520..02eba3ff2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tokenbridge-contracts", - "version": "5.4.1", + "version": "5.5.0-rc0", "description": "Bridge", "main": "index.js", "scripts": {