From 2b51dcf0c4937bfa299a675be90c610e19fd43ed Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Mon, 19 Oct 2020 23:14:12 +0300 Subject: [PATCH 1/8] Avoid doublespending for transfers above limits (#509) --- .../BaseOverdrawManagement.sol | 59 +++++--- .../upgradeable_contracts/BasicHomeBridge.sol | 25 +++- .../HomeOverdrawManagement.sol | 64 +++++++++ .../OverdrawManagement.sol | 41 ------ .../BasicAMBErc677ToErc677.sol | 21 ++- .../erc20_to_erc20/HomeBridgeErcToErc.sol | 44 ++++-- .../erc20_to_native/HomeBridgeErcToNative.sol | 44 ++++-- .../native_to_erc20/HomeBridgeNativeToErc.sol | 37 ++++- .../AMBErc677ToErc677Behavior.test.js | 24 ++-- .../foreign_bridge.test.js | 6 +- test/amb_erc677_to_erc677/home_bridge.test.js | 6 +- test/erc_to_erc/home_bridge.test.js | 136 +++++++++++++----- test/erc_to_native/home_bridge.test.js | 133 ++++++++++++----- 13 files changed, 447 insertions(+), 193 deletions(-) create mode 100644 contracts/upgradeable_contracts/HomeOverdrawManagement.sol delete mode 100644 contracts/upgradeable_contracts/OverdrawManagement.sol diff --git a/contracts/upgradeable_contracts/BaseOverdrawManagement.sol b/contracts/upgradeable_contracts/BaseOverdrawManagement.sol index b7339369b..1a39b723d 100644 --- a/contracts/upgradeable_contracts/BaseOverdrawManagement.sol +++ b/contracts/upgradeable_contracts/BaseOverdrawManagement.sol @@ -2,42 +2,63 @@ pragma solidity 0.4.24; import "../upgradeability/EternalStorage.sol"; +/** + * @title BaseOverdrawManagement + * @dev This contract implements basic functionality for tracking execution bridge operations that are out of limits. + */ contract BaseOverdrawManagement is EternalStorage { - event AmountLimitExceeded(address recipient, uint256 value, bytes32 transactionHash); - event AssetAboveLimitsFixed(bytes32 indexed transactionHash, uint256 value, uint256 remaining); + event MediatorAmountLimitExceeded(address recipient, uint256 value, bytes32 indexed messageId); + event AmountLimitExceeded(address recipient, uint256 value, bytes32 indexed transactionHash, bytes32 messageId); + event AssetAboveLimitsFixed(bytes32 indexed messageId, uint256 value, uint256 remaining); bytes32 internal constant OUT_OF_LIMIT_AMOUNT = 0x145286dc85799b6fb9fe322391ba2d95683077b2adf34dd576dedc437e537ba7; // keccak256(abi.encodePacked("outOfLimitAmount")) + /** + * @dev Total amount coins/tokens that were bridged from the other side and are out of execution limits. + * @return total amount of all bridge operations above limits. + */ function outOfLimitAmount() public view returns (uint256) { return uintStorage[OUT_OF_LIMIT_AMOUNT]; } - function fixedAssets(bytes32 _txHash) public view returns (bool) { - return boolStorage[keccak256(abi.encodePacked("fixedAssets", _txHash))]; - } - + /** + * @dev Internal function for updating a total amount that is out of execution limits. + * @param _value new value for the total amount of bridge operations above limits. + */ function setOutOfLimitAmount(uint256 _value) internal { uintStorage[OUT_OF_LIMIT_AMOUNT] = _value; } - function txAboveLimits(bytes32 _txHash) internal view returns (address recipient, uint256 value) { - recipient = addressStorage[keccak256(abi.encodePacked("txOutOfLimitRecipient", _txHash))]; - value = uintStorage[keccak256(abi.encodePacked("txOutOfLimitValue", _txHash))]; - } - - function setTxAboveLimits(address _recipient, uint256 _value, bytes32 _txHash) internal { - addressStorage[keccak256(abi.encodePacked("txOutOfLimitRecipient", _txHash))] = _recipient; - setTxAboveLimitsValue(_value, _txHash); + /** + * @dev Internal function for retrieving information about out-of-limits bridge operation. + * @param _messageId id of the message that cause above-limits error. + * @return (address of the receiver, amount of coins/tokens in the bridge operation) + */ + function txAboveLimits(bytes32 _messageId) internal view returns (address recipient, uint256 value) { + recipient = addressStorage[keccak256(abi.encodePacked("txOutOfLimitRecipient", _messageId))]; + value = uintStorage[keccak256(abi.encodePacked("txOutOfLimitValue", _messageId))]; } - function setTxAboveLimitsValue(uint256 _value, bytes32 _txHash) internal { - uintStorage[keccak256(abi.encodePacked("txOutOfLimitValue", _txHash))] = _value; + /** + * @dev Internal function for updating information about tbe out-of-limits bridge operation. + * @param _recipient receiver specified in the bridge operation. + * @param _value amount of coins/tokens inside the bridge operation. + * @param _messageId id of the message that cause above-limits error. + */ + function setTxAboveLimits(address _recipient, uint256 _value, bytes32 _messageId) internal { + addressStorage[keccak256(abi.encodePacked("txOutOfLimitRecipient", _messageId))] = _recipient; + setTxAboveLimitsValue(_value, _messageId); } - function setFixedAssets(bytes32 _txHash) internal { - boolStorage[keccak256(abi.encodePacked("fixedAssets", _txHash))] = true; + /** + * @dev Internal function for updating information about the remaining value of out-of-limits bridge operation. + * @param _value amount of coins/tokens inside the bridge operation. + * @param _messageId id of the message that cause above-limits error. + */ + function setTxAboveLimitsValue(uint256 _value, bytes32 _messageId) internal { + uintStorage[keccak256(abi.encodePacked("txOutOfLimitValue", _messageId))] = _value; } /* solcov ignore next */ - function fixAssetsAboveLimits(bytes32 txHash, bool unlockOnForeign, uint256 valueToUnlock) external; + function fixAssetsAboveLimits(bytes32 messageId, bool unlockOnForeign, uint256 valueToUnlock) external; } diff --git a/contracts/upgradeable_contracts/BasicHomeBridge.sol b/contracts/upgradeable_contracts/BasicHomeBridge.sol index 8620acaef..0fc4d2b8c 100644 --- a/contracts/upgradeable_contracts/BasicHomeBridge.sol +++ b/contracts/upgradeable_contracts/BasicHomeBridge.sol @@ -8,6 +8,10 @@ import "../libraries/Message.sol"; import "./BasicBridge.sol"; import "./BasicTokenBridge.sol"; +/** + * @title BasicHomeBridge + * @dev This contract implements common functionality for all vanilla bridge modes on the Home side. + */ contract BasicHomeBridge is EternalStorage, Validatable, BasicBridge, BasicTokenBridge { using SafeMath for uint256; @@ -21,9 +25,16 @@ contract BasicHomeBridge is EternalStorage, Validatable, BasicBridge, BasicToken uint256 NumberOfCollectedSignatures ); + /** + * @dev Executes a message affirmation for some Foreign side event. + * Can be called only by a current bridge validator. + * @param recipient tokens/coins of receiver address, where the assets should be unlocked/minted. + * @param value amount of assets to unlock/mint. + * @param transactionHash reference event transaction hash on the Foreign side of the bridge. + */ function executeAffirmation(address recipient, uint256 value, bytes32 transactionHash) external onlyValidator { + bytes32 hashMsg = keccak256(abi.encodePacked(recipient, value, transactionHash)); if (withinExecutionLimit(value)) { - bytes32 hashMsg = keccak256(abi.encodePacked(recipient, value, transactionHash)); bytes32 hashSender = keccak256(abi.encodePacked(msg.sender, hashMsg)); // Duplicated affirmations require(!affirmationsSigned(hashSender)); @@ -43,12 +54,12 @@ contract BasicHomeBridge is EternalStorage, Validatable, BasicBridge, BasicToken // it will couse funds lock on the home side of the bridge setNumAffirmationsSigned(hashMsg, markAsProcessed(signed)); if (value > 0) { - require(onExecuteAffirmation(recipient, value, transactionHash)); + require(onExecuteAffirmation(recipient, value, transactionHash, hashMsg)); } emit AffirmationCompleted(recipient, value, transactionHash); } } else { - onFailedAffirmation(recipient, value, transactionHash); + onFailedAffirmation(recipient, value, transactionHash, hashMsg); } } @@ -92,7 +103,10 @@ contract BasicHomeBridge is EternalStorage, Validatable, BasicBridge, BasicToken } /* solcov ignore next */ - function onExecuteAffirmation(address, uint256, bytes32) internal returns (bool); + function onExecuteAffirmation(address, uint256, bytes32, bytes32) internal returns (bool); + + /* solcov ignore next */ + function onFailedAffirmation(address, uint256, bytes32, bytes32) internal; /* solcov ignore next */ function onSignaturesCollected(bytes) internal; @@ -153,7 +167,4 @@ contract BasicHomeBridge is EternalStorage, Validatable, BasicBridge, BasicToken function requiredMessageLength() public pure returns (uint256) { return Message.requiredMessageLength(); } - - /* solcov ignore next */ - function onFailedAffirmation(address, uint256, bytes32) internal; } diff --git a/contracts/upgradeable_contracts/HomeOverdrawManagement.sol b/contracts/upgradeable_contracts/HomeOverdrawManagement.sol new file mode 100644 index 000000000..8fc3e2767 --- /dev/null +++ b/contracts/upgradeable_contracts/HomeOverdrawManagement.sol @@ -0,0 +1,64 @@ +pragma solidity 0.4.24; + +import "openzeppelin-solidity/contracts/math/SafeMath.sol"; +import "./Upgradeable.sol"; +import "./RewardableBridge.sol"; +import "./BasicHomeBridge.sol"; +import "./BaseOverdrawManagement.sol"; + +/** + * @title HomeOverdrawManagement + * @dev This contract implements functionality for recovering from out-of-limits executions in Home side vanilla bridges. + */ +contract HomeOverdrawManagement is BaseOverdrawManagement, RewardableBridge, Upgradeable, BasicHomeBridge { + using SafeMath for uint256; + + /** + * @dev Fixes locked tokens, that were out of execution limits during the call to executeAffirmation. + * @param hashMsg reference for bridge operation that was out of execution limits. + * @param unlockOnForeign true if fixed tokens should be unlocked to the other side of the bridge. + * @param valueToUnlock unlocked amount of tokens, should be less than txAboveLimitsValue. + * Should be less than maxPerTx(), if tokens need to be unlocked on the other side. + */ + function fixAssetsAboveLimits(bytes32 hashMsg, bool unlockOnForeign, uint256 valueToUnlock) + external + onlyIfUpgradeabilityOwner + { + uint256 signed = numAffirmationsSigned(hashMsg); + require(!isAlreadyProcessed(signed)); + (address recipient, uint256 value) = txAboveLimits(hashMsg); + require(recipient != address(0) && value > 0 && value >= valueToUnlock); + setOutOfLimitAmount(outOfLimitAmount().sub(valueToUnlock)); + uint256 pendingValue = value.sub(valueToUnlock); + setTxAboveLimitsValue(pendingValue, hashMsg); + emit AssetAboveLimitsFixed(hashMsg, valueToUnlock, pendingValue); + if (unlockOnForeign) { + require(valueToUnlock <= maxPerTx()); + address feeManager = feeManagerContract(); + uint256 eventValue = valueToUnlock; + if (feeManager != address(0)) { + uint256 fee = calculateFee(valueToUnlock, false, feeManager, HOME_FEE); + eventValue = valueToUnlock.sub(fee); + } + emit UserRequestForSignature(recipient, eventValue); + } + } + + /** + * @dev Internal function for clearing above limits markers for some failed transfer. + * Useful when transfer is being reprocessed on a new day or after limits were updated. + * It is required that fixAssetsAboveLimits was not called on the failed transfer before prior to this function. + * @param _hashMsg hash of the message, works as a unique indentifier. + * @param _value transferred amount of tokens/coins in the fixed message. + */ + function _clearAboveLimitsMarker(bytes32 _hashMsg, uint256 _value) internal { + (address aboveLimitsRecipient, uint256 aboveLimitsValue) = txAboveLimits(_hashMsg); + // check if transfer was marked as out of limits + if (aboveLimitsRecipient != address(0)) { + // revert if a given transaction hash was already processed by the call to fixAssetsAboveLimits + require(aboveLimitsValue == _value); + setTxAboveLimits(address(0), 0, _hashMsg); + setOutOfLimitAmount(outOfLimitAmount().sub(_value)); + } + } +} diff --git a/contracts/upgradeable_contracts/OverdrawManagement.sol b/contracts/upgradeable_contracts/OverdrawManagement.sol deleted file mode 100644 index 9c01b53be..000000000 --- a/contracts/upgradeable_contracts/OverdrawManagement.sol +++ /dev/null @@ -1,41 +0,0 @@ -pragma solidity 0.4.24; - -import "openzeppelin-solidity/contracts/math/SafeMath.sol"; -import "./Upgradeable.sol"; -import "./RewardableBridge.sol"; -import "./BasicTokenBridge.sol"; -import "./BaseOverdrawManagement.sol"; - -contract OverdrawManagement is BaseOverdrawManagement, RewardableBridge, Upgradeable, BasicTokenBridge { - using SafeMath for uint256; - - event UserRequestForSignature(address recipient, uint256 value); - - function fixAssetsAboveLimits(bytes32 txHash, bool unlockOnForeign, uint256 valueToUnlock) - external - onlyIfUpgradeabilityOwner - { - require(!fixedAssets(txHash)); - require(valueToUnlock <= maxPerTx()); - address recipient; - uint256 value; - (recipient, value) = txAboveLimits(txHash); - require(recipient != address(0) && value > 0 && value >= valueToUnlock); - setOutOfLimitAmount(outOfLimitAmount().sub(valueToUnlock)); - uint256 pendingValue = value.sub(valueToUnlock); - setTxAboveLimitsValue(pendingValue, txHash); - emit AssetAboveLimitsFixed(txHash, valueToUnlock, pendingValue); - if (pendingValue == 0) { - setFixedAssets(txHash); - } - if (unlockOnForeign) { - address feeManager = feeManagerContract(); - uint256 eventValue = valueToUnlock; - if (feeManager != address(0)) { - uint256 fee = calculateFee(valueToUnlock, false, feeManager, HOME_FEE); - eventValue = valueToUnlock.sub(fee); - } - emit UserRequestForSignature(recipient, eventValue); - } - } -} diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol index 0182cf119..83f0ebdb1 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol @@ -116,33 +116,28 @@ contract BasicAMBErc677ToErc677 is require(recipient == address(0) && value == 0); setOutOfLimitAmount(outOfLimitAmount().add(_value)); setTxAboveLimits(_recipient, _value, _messageId); - emit AmountLimitExceeded(_recipient, _value, _messageId); + emit MediatorAmountLimitExceeded(_recipient, _value, _messageId); } /** * @dev Fixes locked tokens, that were out of execution limits during the call to handleBridgedTokens * @param messageId reference for bridge operation that was out of execution limits - * @param unlockOnForeign true if fixed tokens should be unlocked to the other side of the bridge - * @param valueToUnlock unlocked amount of tokens, should be less than maxPerTx() and saved txAboveLimitsValue + * @param unlockOnOtherSide true if fixed tokens should be unlocked to the other side of the bridge + * @param valueToUnlock unlocked amount of tokens, should be less than saved txAboveLimitsValue. + * Should be less than maxPerTx(), if tokens need to be unlocked on the other side. */ - function fixAssetsAboveLimits(bytes32 messageId, bool unlockOnForeign, uint256 valueToUnlock) + function fixAssetsAboveLimits(bytes32 messageId, bool unlockOnOtherSide, uint256 valueToUnlock) external onlyIfUpgradeabilityOwner { - require(!fixedAssets(messageId)); - require(valueToUnlock <= maxPerTx()); - address recipient; - uint256 value; - (recipient, value) = txAboveLimits(messageId); + (address recipient, uint256 value) = txAboveLimits(messageId); require(recipient != address(0) && value > 0 && value >= valueToUnlock); setOutOfLimitAmount(outOfLimitAmount().sub(valueToUnlock)); uint256 pendingValue = value.sub(valueToUnlock); setTxAboveLimitsValue(pendingValue, messageId); emit AssetAboveLimitsFixed(messageId, valueToUnlock, pendingValue); - if (pendingValue == 0) { - setFixedAssets(messageId); - } - if (unlockOnForeign) { + if (unlockOnOtherSide) { + require(valueToUnlock <= maxPerTx()); passMessage(recipient, recipient, valueToUnlock); } } diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol index 32525cef0..0ae71855a 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol @@ -4,15 +4,20 @@ import "../../libraries/Message.sol"; import "../../upgradeability/EternalStorage.sol"; import "../../interfaces/IBurnableMintableERC677Token.sol"; import "../BasicHomeBridge.sol"; -import "../OverdrawManagement.sol"; +import "../HomeOverdrawManagement.sol"; import "./RewardableHomeBridgeErcToErc.sol"; import "../ERC677BridgeForBurnableMintableToken.sol"; +/** + * @title HomeBridgeErcToErc + * @dev This contract Home side logic for the erc-to-erc vanilla bridge mode. + * It is designed to be used as an implementation contract of EternalStorageProxy contract. + */ contract HomeBridgeErcToErc is EternalStorage, BasicHomeBridge, ERC677BridgeForBurnableMintableToken, - OverdrawManagement, + HomeOverdrawManagement, RewardableHomeBridgeErcToErc { function initialize( @@ -134,13 +139,26 @@ contract HomeBridgeErcToErc is return 0xba4690f5; // bytes4(keccak256(abi.encodePacked("erc-to-erc-core"))) } - function onExecuteAffirmation(address _recipient, uint256 _value, bytes32 txHash) internal returns (bool) { + /** + * @dev Internal callback to be called on successfull message execution. + * Should be called only after enough affirmations from the validators are already collected. + * @param _recipient address of the receiver where the new tokens should be minted. + * @param _value amount of tokens to mint. + * @param _txHash reference transaction hash on the Foreign side of the bridge which cause this operation. + * @param _hashMsg unique identifier of the particular bridge operation. + * @return true, if execution completed successfully. + */ + function onExecuteAffirmation(address _recipient, uint256 _value, bytes32 _txHash, bytes32 _hashMsg) + internal + returns (bool) + { + _clearAboveLimitsMarker(_hashMsg, _value); addTotalExecutedPerDay(getCurrentDay(), _value); uint256 valueToMint = _shiftValue(_value); address feeManager = feeManagerContract(); if (feeManager != address(0)) { uint256 fee = calculateFee(valueToMint, false, feeManager, FOREIGN_FEE); - distributeFeeFromAffirmation(fee, feeManager, txHash); + distributeFeeFromAffirmation(fee, feeManager, _txHash); valueToMint = valueToMint.sub(fee); } return IBurnableMintableERC677Token(erc677token()).mint(_recipient, valueToMint); @@ -169,13 +187,19 @@ contract HomeBridgeErcToErc is } } - function onFailedAffirmation(address _recipient, uint256 _value, bytes32 _txHash) internal { - address recipient; - uint256 value; - (recipient, value) = txAboveLimits(_txHash); + /** + * @dev Internal callback to be called on failed message execution due to the out-of-limits error. + * This function saves the bridge operation information for further processing. + * @param _recipient address of the receiver where the new tokens should be minted. + * @param _value amount of tokens to mint. + * @param _txHash reference transaction hash on the Foreign side of the bridge which cause this operation. + * @param _hashMsg unique identifier of the particular bridge operation. + */ + function onFailedAffirmation(address _recipient, uint256 _value, bytes32 _txHash, bytes32 _hashMsg) internal { + (address recipient, uint256 value) = txAboveLimits(_hashMsg); require(recipient == address(0) && value == 0); setOutOfLimitAmount(outOfLimitAmount().add(_value)); - setTxAboveLimits(_recipient, _value, _txHash); - emit AmountLimitExceeded(_recipient, _value, _txHash); + setTxAboveLimits(_recipient, _value, _hashMsg); + emit AmountLimitExceeded(_recipient, _value, _txHash, _hashMsg); } } diff --git a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol index 48fd7626d..108cc81be 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol @@ -4,14 +4,19 @@ import "../../libraries/Message.sol"; import "../../upgradeability/EternalStorage.sol"; import "../../interfaces/IBlockReward.sol"; import "../BasicHomeBridge.sol"; -import "../OverdrawManagement.sol"; +import "../HomeOverdrawManagement.sol"; import "./RewardableHomeBridgeErcToNative.sol"; import "../BlockRewardBridge.sol"; +/** + * @title HomeBridgeErcToNative + * @dev This contract Home side logic for the erc-to-native vanilla bridge mode. + * It is designed to be used as an implementation contract of EternalStorageProxy contract. + */ contract HomeBridgeErcToNative is EternalStorage, BasicHomeBridge, - OverdrawManagement, + HomeOverdrawManagement, RewardableHomeBridgeErcToNative, BlockRewardBridge { @@ -158,7 +163,20 @@ contract HomeBridgeErcToNative is _setOwner(_owner); } - function onExecuteAffirmation(address _recipient, uint256 _value, bytes32 txHash) internal returns (bool) { + /** + * @dev Internal callback to be called on successfull message execution. + * Should be called only after enough affirmations from the validators are already collected. + * @param _recipient address of the receiver where the new coins should be minted. + * @param _value amount of coins to mint. + * @param _txHash reference transaction hash on the Foreign side of the bridge which cause this operation. + * @param _hashMsg unique identifier of the particular bridge operation. + * @return true, if execution completed successfully. + */ + function onExecuteAffirmation(address _recipient, uint256 _value, bytes32 _txHash, bytes32 _hashMsg) + internal + returns (bool) + { + _clearAboveLimitsMarker(_hashMsg, _value); addTotalExecutedPerDay(getCurrentDay(), _value); IBlockReward blockReward = blockRewardContract(); require(blockReward != address(0)); @@ -166,7 +184,7 @@ contract HomeBridgeErcToNative is address feeManager = feeManagerContract(); if (feeManager != address(0)) { uint256 fee = calculateFee(valueToMint, false, feeManager, FOREIGN_FEE); - distributeFeeFromAffirmation(fee, feeManager, txHash); + distributeFeeFromAffirmation(fee, feeManager, _txHash); valueToMint = valueToMint.sub(fee); } blockReward.addExtraReceiver(valueToMint, _recipient); @@ -190,13 +208,19 @@ contract HomeBridgeErcToNative is uintStorage[TOTAL_BURNT_COINS] = _amount; } - function onFailedAffirmation(address _recipient, uint256 _value, bytes32 _txHash) internal { - address recipient; - uint256 value; - (recipient, value) = txAboveLimits(_txHash); + /** + * @dev Internal callback to be called on failed message execution due to the out-of-limits error. + * This function saves the bridge operation information for further processing. + * @param _recipient address of the receiver where the new coins should be minted. + * @param _value amount of coins to mint. + * @param _txHash reference transaction hash on the Foreign side of the bridge which cause this operation. + * @param _hashMsg unique identifier of the particular bridge operation. + */ + function onFailedAffirmation(address _recipient, uint256 _value, bytes32 _txHash, bytes32 _hashMsg) internal { + (address recipient, uint256 value) = txAboveLimits(_hashMsg); require(recipient == address(0) && value == 0); setOutOfLimitAmount(outOfLimitAmount().add(_value)); - setTxAboveLimits(_recipient, _value, _txHash); - emit AmountLimitExceeded(_recipient, _value, _txHash); + setTxAboveLimits(_recipient, _value, _hashMsg); + emit AmountLimitExceeded(_recipient, _value, _txHash, _hashMsg); } } diff --git a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol index 86d11bac6..0e4293827 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol @@ -6,6 +6,11 @@ import "../BasicHomeBridge.sol"; import "./RewardableHomeBridgeNativeToErc.sol"; import "../../libraries/Address.sol"; +/** + * @title HomeBridgeNativeToErc + * @dev This contract Home side logic for the native-to-erc vanilla bridge mode. + * It is designed to be used as an implementation contract of EternalStorageProxy contract. + */ contract HomeBridgeNativeToErc is EternalStorage, BasicHomeBridge, RewardableHomeBridgeNativeToErc { function() public payable { require(msg.data.length == 0); @@ -132,14 +137,27 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicHomeBridge, RewardableHom } } - function onExecuteAffirmation(address _recipient, uint256 _value, bytes32 txHash) internal returns (bool) { + /** + * @dev Internal callback to be called on successfull message execution. + * Should be called only after enough affirmations from the validators are already collected. + * @param _recipient address of the receiver where the new coins should be unlocked. + * @param _value amount of coins to unlock. + * @param _txHash reference transaction hash on the Foreign side of the bridge which cause this operation. + * @param _hashMsg unique identifier of the particular bridge operation. + * Not used in this bridge mode, but required for interface unification with other bridge modes. + * @return true, if execution completed successfully. + */ + function onExecuteAffirmation(address _recipient, uint256 _value, bytes32 _txHash, bytes32 _hashMsg) + internal + returns (bool) + { addTotalExecutedPerDay(getCurrentDay(), _value); uint256 valueToTransfer = _shiftValue(_value); address feeManager = feeManagerContract(); if (feeManager != address(0)) { uint256 fee = calculateFee(valueToTransfer, false, feeManager, FOREIGN_FEE); - distributeFeeFromAffirmation(fee, feeManager, txHash); + distributeFeeFromAffirmation(fee, feeManager, _txHash); valueToTransfer = valueToTransfer.sub(fee); } @@ -147,11 +165,16 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicHomeBridge, RewardableHom return true; } - function onFailedAffirmation( - address, /*_recipient*/ - uint256, /*_value*/ - bytes32 /*_txHash*/ - ) internal { + /** + * @dev Internal callback to be called on failed message execution due to the out-of-limits error. + * This function saves the bridge operation information for further processing. + * @param _recipient address of the receiver where the new coins should be unlocked. + * @param _value amount of coins to unlock. + * @param _txHash reference transaction hash on the Foreign side of the bridge which cause this operation. + * @param _hashMsg unique identifier of the particular bridge operation. + */ + function onFailedAffirmation(address _recipient, uint256 _value, bytes32 _txHash, bytes32 _hashMsg) internal { + // solhint-disable-previous-line no-unused-vars revert(); } diff --git a/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js b/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js index 3559e603d..1b9e9318e 100644 --- a/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js +++ b/test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js @@ -391,21 +391,21 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou 1000000 ).should.be.fulfilled - const outOfLimitEvent = await getEvents(contract, { event: 'AmountLimitExceeded' }) + const outOfLimitEvent = await getEvents(contract, { event: 'MediatorAmountLimitExceeded' }) expect(outOfLimitEvent.length).to.be.equal(1) expect(outOfLimitEvent[0].returnValues.recipient).to.be.equal(user) expect(outOfLimitEvent[0].returnValues.value).to.be.equal(twoEthers.toString()) - expect(outOfLimitEvent[0].returnValues.transactionHash).to.be.equal(exampleMessageId) + expect(outOfLimitEvent[0].returnValues.messageId).to.be.equal(exampleMessageId) }) it('Should revert if value to unlock is bigger than max per transaction', async function() { - await contract.fixAssetsAboveLimits(exampleMessageId, false, twoEthers).should.be.rejectedWith(ERROR_MSG) + await contract.fixAssetsAboveLimits(exampleMessageId, true, twoEthers).should.be.rejectedWith(ERROR_MSG) }) it('Should allow to partially reduce outOfLimitAmount and not emit amb event', async function() { const { logs } = await contract.fixAssetsAboveLimits(exampleMessageId, false, halfEther).should.be.fulfilled logs.length.should.be.equal(1) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: halfEther, remaining: ether('1.5') }) @@ -418,7 +418,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logsSecondTx.length.should.be.equal(1) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: halfEther, remaining: oneEther }) @@ -431,7 +431,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logs.length.should.be.equal(1) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: halfEther, remaining: ether('1.5') }) @@ -444,7 +444,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logsSecondTx.length.should.be.equal(1) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: halfEther, remaining: oneEther }) @@ -457,7 +457,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logs.length.should.be.equal(1) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: halfEther, remaining: ether('1.5') }) @@ -467,7 +467,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logsSecondTx.length.should.be.equal(1) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: halfEther, remaining: oneEther }) @@ -477,7 +477,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logsThirdTx.length.should.be.equal(1) expectEventInLogs(logsThirdTx, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: halfEther, remaining: halfEther }) @@ -489,7 +489,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logs.length.should.be.equal(1) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: oneEther, remaining: oneEther }) @@ -499,7 +499,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou logsSecondTx.length.should.be.equal(1) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash: exampleMessageId, + messageId: exampleMessageId, value: oneEther, remaining: ZERO }) diff --git a/test/amb_erc677_to_erc677/foreign_bridge.test.js b/test/amb_erc677_to_erc677/foreign_bridge.test.js index 847609cce..12cfeee42 100644 --- a/test/amb_erc677_to_erc677/foreign_bridge.test.js +++ b/test/amb_erc677_to_erc677/foreign_bridge.test.js @@ -255,7 +255,7 @@ contract('ForeignAMBErc677ToErc677', async accounts => { expect(TokensBridgedEvent[0].returnValues.messageId).to.be.equal(exampleMessageId) }) } - it('should emit AmountLimitExceeded and not transfer tokens when out of execution limits', async () => { + it('should emit MediatorAmountLimitExceeded and not transfer tokens when out of execution limits', async () => { // Given const currentDay = await foreignBridge.getCurrentDay() expect(await foreignBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(ZERO) @@ -285,11 +285,11 @@ contract('ForeignAMBErc677ToErc677', async accounts => { expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) expect(await foreignBridge.outOfLimitAmount()).to.be.bignumber.equal(twoEthers) - const outOfLimitEvent = await getEvents(foreignBridge, { event: 'AmountLimitExceeded' }) + const outOfLimitEvent = await getEvents(foreignBridge, { event: 'MediatorAmountLimitExceeded' }) expect(outOfLimitEvent.length).to.be.equal(1) expect(outOfLimitEvent[0].returnValues.recipient).to.be.equal(user) expect(outOfLimitEvent[0].returnValues.value).to.be.equal(twoEthers.toString()) - expect(outOfLimitEvent[0].returnValues.transactionHash).to.be.equal(exampleMessageId) + expect(outOfLimitEvent[0].returnValues.messageId).to.be.equal(exampleMessageId) const TokensBridgedEvent = await getEvents(foreignBridge, { event: 'TokensBridged' }) expect(TokensBridgedEvent.length).to.be.equal(0) diff --git a/test/amb_erc677_to_erc677/home_bridge.test.js b/test/amb_erc677_to_erc677/home_bridge.test.js index 38a92c23a..7799867a6 100644 --- a/test/amb_erc677_to_erc677/home_bridge.test.js +++ b/test/amb_erc677_to_erc677/home_bridge.test.js @@ -261,7 +261,7 @@ contract('HomeAMBErc677ToErc677', async accounts => { expect(TokensBridgedEvent[0].returnValues.messageId).to.be.equal(exampleMessageId) }) } - it('should emit AmountLimitExceeded and not mint tokens when out of execution limits', async () => { + it('should emit MediatorAmountLimitExceeded and not mint tokens when out of execution limits', async () => { // Given const currentDay = await homeBridge.getCurrentDay() expect(await homeBridge.totalExecutedPerDay(currentDay)).to.be.bignumber.equal(ZERO) @@ -292,11 +292,11 @@ contract('HomeAMBErc677ToErc677', async accounts => { expect(await erc677Token.balanceOf(user)).to.be.bignumber.equal(ZERO) expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(twoEthers) - const outOfLimitEvent = await getEvents(homeBridge, { event: 'AmountLimitExceeded' }) + const outOfLimitEvent = await getEvents(homeBridge, { event: 'MediatorAmountLimitExceeded' }) expect(outOfLimitEvent.length).to.be.equal(1) expect(outOfLimitEvent[0].returnValues.recipient).to.be.equal(user) expect(outOfLimitEvent[0].returnValues.value).to.be.equal(twoEthers.toString()) - expect(outOfLimitEvent[0].returnValues.transactionHash).to.be.equal(exampleMessageId) + expect(outOfLimitEvent[0].returnValues.messageId).to.be.equal(exampleMessageId) const TokensBridgedEvent = await getEvents(homeBridge, { event: 'TokensBridged' }) expect(TokensBridgedEvent.length).to.be.equal(0) diff --git a/test/erc_to_erc/home_bridge.test.js b/test/erc_to_erc/home_bridge.test.js index eeba3f733..cf6714d36 100644 --- a/test/erc_to_erc/home_bridge.test.js +++ b/test/erc_to_erc/home_bridge.test.js @@ -600,12 +600,13 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { transactionHash }) + // should fail for the same message await homeBridge .executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }) .should.be.rejectedWith(ERROR_MSG) - await homeBridge - .executeAffirmation(accounts[6], value, transactionHash, { from: authorities[0] }) - .should.be.rejectedWith(ERROR_MSG) + // should succeed for different transfer in the same message + await homeBridge.executeAffirmation(accounts[6], value, transactionHash, { from: authorities[0] }).should.be + .fulfilled }) it('should not allow execute affirmation over daily foreign limit', async () => { const recipient = accounts[5] @@ -869,6 +870,7 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { const storageProxy = await EternalStorageProxy.new().should.be.fulfilled await storageProxy.upgradeTo('1', homeBridgeImpl.address).should.be.fulfilled homeBridge = await HomeBridge.at(storageProxy.address) + token = await ERC677BridgeToken.new('TEST', 'TST', 18) await homeBridge.initialize( validatorContract.address, [oneEther, halfEther, minPerTx], @@ -879,6 +881,7 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { owner, decimalShiftZero ).should.be.fulfilled + await token.transferOwnership(homeBridge.address).should.be.fulfilled }) it('Should revert if value to unlock is bigger than max per transaction', async () => { const recipient = accounts[5] @@ -889,11 +892,13 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - await homeBridge.fixAssetsAboveLimits(transactionHash, false, value).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId, true, value).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId, false, value).should.be.fulfilled }) it('Should allow to partially reduce outOfLimitAmount and not emit UserRequestForSignature', async () => { const recipient = accounts[5] @@ -904,26 +909,27 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be.fulfilled logs.length.should.be.equal(1) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(halfEther) - const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be + const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be .fulfilled logsSecondTx.length.should.be.equal(1) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: ZERO }) @@ -938,15 +944,16 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.fulfilled logs.length.should.be.equal(2) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) @@ -957,12 +964,12 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(halfEther) - const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be + const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be .fulfilled logsSecondTx.length.should.be.equal(2) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: ZERO }) @@ -982,15 +989,16 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.fulfilled logs.length.should.be.equal(2) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) @@ -1001,12 +1009,12 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(halfEther) - const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, quarterEther).should - .be.fulfilled + const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(messageId, true, quarterEther).should.be + .fulfilled logsSecondTx.length.should.be.equal(2) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: quarterEther, remaining: quarterEther }) @@ -1017,11 +1025,11 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(quarterEther) - await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.rejectedWith(ERROR_MSG) - const { logs: logsThirdTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, quarterEther).should.be + await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.rejectedWith(ERROR_MSG) + const { logs: logsThirdTx } = await homeBridge.fixAssetsAboveLimits(messageId, true, quarterEther).should.be .fulfilled expectEventInLogs(logsThirdTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: quarterEther, remaining: ZERO }) @@ -1038,36 +1046,39 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' const transactionHash2 = '0x35d3818e50234655f6aebb2a1cfbf30f59568d8a4ec72066fac5a25dbe7b8121' - await homeBridge.executeAffirmation(recipient, value, transactionHash, { + const { logs: logs1 } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be.fulfilled - await homeBridge.executeAffirmation(recipient, value, transactionHash2, { + const { logs: logs2 } = await homeBridge.executeAffirmation(recipient, value, transactionHash2, { from: authorities[0] }).should.be.fulfilled + const messageId1 = logs1[0].args.messageId + const messageId2 = logs2[0].args.messageId + const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value.add(value)) - await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.fulfilled - await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId1, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId1, false, halfEther).should.be.fulfilled const newOutOfLimitAmount = await homeBridge.outOfLimitAmount() newOutOfLimitAmount.should.be.bignumber.equal(value) - await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId1, false, halfEther).should.be.rejectedWith(ERROR_MSG) - await homeBridge.fixAssetsAboveLimits(transactionHash2, false, halfEther).should.be.fulfilled - await homeBridge.fixAssetsAboveLimits(transactionHash2, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId2, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId2, false, halfEther).should.be.fulfilled expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(ZERO) - await homeBridge.fixAssetsAboveLimits(transactionHash2, false, halfEther).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId2, false, halfEther).should.be.rejectedWith(ERROR_MSG) }) it('Should fail if txHash didnt increase out of limit amount', async () => { const recipient = accounts[5] const value = oneEther const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' - const invalidTxHash = '0x35d3818e50234655f6aebb2a1cfbf30f59568d8a4ec72066fac5a25dbe7b8121' + const invalidMessageId = '0x35d3818e50234655f6aebb2a1cfbf30f59568d8a4ec72066fac5a25dbe7b8121' const { logs: affirmationLogs } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] @@ -1075,7 +1086,7 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') - await homeBridge.fixAssetsAboveLimits(invalidTxHash, true, halfEther).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(invalidMessageId, true, halfEther).should.be.rejectedWith(ERROR_MSG) }) it('Should fail if not called by proxyOwner', async () => { const recipient = accounts[5] @@ -1087,11 +1098,12 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId await homeBridge - .fixAssetsAboveLimits(transactionHash, true, halfEther, { from: recipient }) + .fixAssetsAboveLimits(messageId, true, halfEther, { from: recipient }) .should.be.rejectedWith(ERROR_MSG) - await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther, { from: owner }).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther, { from: owner }).should.be.fulfilled }) it('Should emit UserRequestForSignature with value reduced by fee', async () => { const recipient = accounts[5] @@ -1115,15 +1127,16 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { value, transactionHash }) + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.fulfilled logs.length.should.be.equal(2) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) @@ -1132,6 +1145,61 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { value: finalValue }) }) + + it('should not allow to fix assets after they were already processed', async () => { + const recipient = accounts[5] + const value = oneEther + const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' + const { logs: affirmationLogs } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { + from: authorities[0] + }).should.be.fulfilled + + affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId + + await homeBridge.setExecutionDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setExecutionMaxPerTx(value, { from: owner }).should.be.fulfilled + await homeBridge.setDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setMaxPerTx(value, { from: owner }).should.be.fulfilled + + await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be + .fulfilled + + const outOfLimitAmount = await homeBridge.outOfLimitAmount() + outOfLimitAmount.should.be.bignumber.equal(ZERO) + + await homeBridge.fixAssetsAboveLimits(messageId, false, value).should.be.rejectedWith(ERROR_MSG) + }) + + it('should not allow to executeAffirmation after assets were fixed', async () => { + const recipient = accounts[5] + const value = oneEther + const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' + const { logs: affirmationLogs } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { + from: authorities[0] + }).should.be.fulfilled + + affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId + + const outOfLimitAmount = await homeBridge.outOfLimitAmount() + outOfLimitAmount.should.be.bignumber.equal(value) + + await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be.fulfilled + + await homeBridge.setExecutionDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setExecutionMaxPerTx(value, { from: owner }).should.be.fulfilled + await homeBridge.setDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setMaxPerTx(value, { from: owner }).should.be.fulfilled + + await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be + .rejected + + await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be.fulfilled + + await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be + .rejected + }) }) describe('#claimTokens', () => { it('should be able to call claimTokens on tokenAddress', async () => { diff --git a/test/erc_to_native/home_bridge.test.js b/test/erc_to_native/home_bridge.test.js index 8382e342f..8f3180fde 100644 --- a/test/erc_to_native/home_bridge.test.js +++ b/test/erc_to_native/home_bridge.test.js @@ -1135,12 +1135,13 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { transactionHash }) + // should fail for the same message await homeBridge .executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }) .should.be.rejectedWith(ERROR_MSG) - await homeBridge - .executeAffirmation(accounts[6], value, transactionHash, { from: authorities[0] }) - .should.be.rejectedWith(ERROR_MSG) + // should succeed for different transfer in the same message + await homeBridge.executeAffirmation(accounts[6], value, transactionHash, { from: authorities[0] }).should.be + .fulfilled }) it('should not allow execute affirmation over daily foreign limit', async () => { await blockRewardContract.sendTransaction({ @@ -1414,11 +1415,13 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - await homeBridge.fixAssetsAboveLimits(transactionHash, false, value).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId, true, value).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId, false, value).should.be.fulfilled }) it('Should allow to partially reduce outOfLimitAmount and not emit UserRequestForSignature', async () => { const recipient = accounts[5] @@ -1429,26 +1432,27 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be.fulfilled logs.length.should.be.equal(1) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(halfEther) - const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be + const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be .fulfilled logsSecondTx.length.should.be.equal(1) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: ZERO }) @@ -1463,15 +1467,16 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.fulfilled logs.length.should.be.equal(2) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) @@ -1482,12 +1487,12 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(halfEther) - const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be + const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be .fulfilled logsSecondTx.length.should.be.equal(2) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: ZERO }) @@ -1507,15 +1512,16 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value) - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.fulfilled logs.length.should.be.equal(2) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) @@ -1526,12 +1532,12 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(halfEther) - const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, quarterEther).should - .be.fulfilled + const { logs: logsSecondTx } = await homeBridge.fixAssetsAboveLimits(messageId, true, quarterEther).should.be + .fulfilled logsSecondTx.length.should.be.equal(2) expectEventInLogs(logsSecondTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: quarterEther, remaining: quarterEther }) @@ -1542,11 +1548,11 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(quarterEther) - await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.rejectedWith(ERROR_MSG) - const { logs: logsThirdTx } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, quarterEther).should.be + await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.rejectedWith(ERROR_MSG) + const { logs: logsThirdTx } = await homeBridge.fixAssetsAboveLimits(messageId, true, quarterEther).should.be .fulfilled expectEventInLogs(logsThirdTx, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: quarterEther, remaining: ZERO }) @@ -1563,36 +1569,38 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' const transactionHash2 = '0x35d3818e50234655f6aebb2a1cfbf30f59568d8a4ec72066fac5a25dbe7b8121' - await homeBridge.executeAffirmation(recipient, value, transactionHash, { + const { logs: logs1 } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be.fulfilled - await homeBridge.executeAffirmation(recipient, value, transactionHash2, { + const { logs: logs2 } = await homeBridge.executeAffirmation(recipient, value, transactionHash2, { from: authorities[0] }).should.be.fulfilled + const messageId1 = logs1[0].args.messageId + const messageId2 = logs2[0].args.messageId const outOfLimitAmount = await homeBridge.outOfLimitAmount() outOfLimitAmount.should.be.bignumber.equal(value.add(value)) - await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.fulfilled - await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId1, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId1, false, halfEther).should.be.fulfilled const newOutOfLimitAmount = await homeBridge.outOfLimitAmount() newOutOfLimitAmount.should.be.bignumber.equal(value) - await homeBridge.fixAssetsAboveLimits(transactionHash, false, halfEther).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId1, false, halfEther).should.be.rejectedWith(ERROR_MSG) - await homeBridge.fixAssetsAboveLimits(transactionHash2, false, halfEther).should.be.fulfilled - await homeBridge.fixAssetsAboveLimits(transactionHash2, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId2, false, halfEther).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId2, false, halfEther).should.be.fulfilled expect(await homeBridge.outOfLimitAmount()).to.be.bignumber.equal(ZERO) - await homeBridge.fixAssetsAboveLimits(transactionHash2, false, halfEther).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(messageId2, false, halfEther).should.be.rejectedWith(ERROR_MSG) }) it('Should fail if txHash didnt increase out of limit amount', async () => { const recipient = accounts[5] const value = oneEther const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' - const invalidTxHash = '0x35d3818e50234655f6aebb2a1cfbf30f59568d8a4ec72066fac5a25dbe7b8121' + const invalidMessageId = '0x35d3818e50234655f6aebb2a1cfbf30f59568d8a4ec72066fac5a25dbe7b8121' const { logs: affirmationLogs } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] @@ -1600,7 +1608,7 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') - await homeBridge.fixAssetsAboveLimits(invalidTxHash, true, halfEther).should.be.rejectedWith(ERROR_MSG) + await homeBridge.fixAssetsAboveLimits(invalidMessageId, true, halfEther).should.be.rejectedWith(ERROR_MSG) }) it('Should fail if not called by proxyOwner', async () => { const recipient = accounts[5] @@ -1612,11 +1620,12 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { }).should.be.fulfilled affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId await homeBridge - .fixAssetsAboveLimits(transactionHash, true, halfEther, { from: recipient }) + .fixAssetsAboveLimits(messageId, true, halfEther, { from: recipient }) .should.be.rejectedWith(ERROR_MSG) - await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther, { from: owner }).should.be.fulfilled + await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther, { from: owner }).should.be.fulfilled }) it('Should emit UserRequestForSignature with value reduced by fee', async () => { // Initialize @@ -1671,13 +1680,14 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { value, transactionHash }) + const messageId = affirmationLogs[0].args.messageId - const { logs } = await homeBridge.fixAssetsAboveLimits(transactionHash, true, halfEther).should.be.fulfilled + const { logs } = await homeBridge.fixAssetsAboveLimits(messageId, true, halfEther).should.be.fulfilled // Then logs.length.should.be.equal(2) expectEventInLogs(logs, 'AssetAboveLimitsFixed', { - transactionHash, + messageId, value: halfEther, remaining: halfEther }) @@ -1686,6 +1696,61 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { value: finalValue }) }) + + it('should not allow to fix assets after they were already processed', async () => { + const recipient = accounts[5] + const value = oneEther + const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' + const { logs: affirmationLogs } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { + from: authorities[0] + }).should.be.fulfilled + + affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId + + await homeBridge.setExecutionDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setExecutionMaxPerTx(value, { from: owner }).should.be.fulfilled + await homeBridge.setDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setMaxPerTx(value, { from: owner }).should.be.fulfilled + + await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be + .fulfilled + + const outOfLimitAmount = await homeBridge.outOfLimitAmount() + outOfLimitAmount.should.be.bignumber.equal(ZERO) + + await homeBridge.fixAssetsAboveLimits(messageId, false, value).should.be.rejectedWith(ERROR_MSG) + }) + + it('should not allow to executeAffirmation after assets were fixed', async () => { + const recipient = accounts[5] + const value = oneEther + const transactionHash = '0x806335163828a8eda675cff9c84fa6e6c7cf06bb44cc6ec832e42fe789d01415' + const { logs: affirmationLogs } = await homeBridge.executeAffirmation(recipient, value, transactionHash, { + from: authorities[0] + }).should.be.fulfilled + + affirmationLogs[0].event.should.be.equal('AmountLimitExceeded') + const messageId = affirmationLogs[0].args.messageId + + const outOfLimitAmount = await homeBridge.outOfLimitAmount() + outOfLimitAmount.should.be.bignumber.equal(value) + + await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be.fulfilled + + await homeBridge.setExecutionDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setExecutionMaxPerTx(value, { from: owner }).should.be.fulfilled + await homeBridge.setDailyLimit(ether('2'), { from: owner }).should.be.fulfilled + await homeBridge.setMaxPerTx(value, { from: owner }).should.be.fulfilled + + await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be + .rejected + + await homeBridge.fixAssetsAboveLimits(messageId, false, halfEther).should.be.fulfilled + + await homeBridge.executeAffirmation(recipient, value, transactionHash, { from: authorities[0] }).should.be + .rejected + }) }) describe('#feeManager', async () => { let homeBridge From 4e59f30655673116354fc20846fc8127dc164d68 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Tue, 20 Oct 2020 12:19:11 +0300 Subject: [PATCH 2/8] Disable fee collection if sender is a reward address (#538) --- .../HomeMultiAMBErc20ToErc677.sol | 14 ++++++++++---- .../home_mediator.test.js | 13 +++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) 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 e42d5820c..14d8ac0cd 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol @@ -249,10 +249,16 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager if (!lock()) { bytes32 _messageId = messageId(); uint256 valueToBridge = _value; - uint256 fee = _distributeFee(HOME_TO_FOREIGN_FEE, _token, valueToBridge); - if (fee > 0) { - emit FeeDistributed(fee, _token, _messageId); - valueToBridge = valueToBridge.sub(fee); + // Next line disables fee collection in case sender is one of the reward addresses. + // It is needed to allow a 100% withdrawal of tokens from the home side. + // If fees are not disabled for reward receivers, small fraction of tokens will always + // be redistributed between the same set of reward addresses, which is not the desired behaviour. + if (!isRewardAddress(_from)) { + uint256 fee = _distributeFee(HOME_TO_FOREIGN_FEE, _token, valueToBridge); + if (fee > 0) { + valueToBridge = valueToBridge.sub(fee); + emit FeeDistributed(fee, _token, _messageId); + } } IBurnableMintableERC677Token(_token).burn(valueToBridge); passMessage(_token, _from, chooseReceiver(_from, _data), valueToBridge); 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 a8af30d9a..9a4881983 100644 --- a/test/multi_amb_erc20_to_erc677/home_mediator.test.js +++ b/test/multi_amb_erc20_to_erc677/home_mediator.test.js @@ -1339,6 +1339,19 @@ contract('HomeMultiAMBErc20ToErc677', async accounts => { const feeEvents = await getEvents(contract, { event: 'FeeDistributed' }) expect(feeEvents.length).to.be.equal(2) }) + + it('should not collect and distribute fee if sender is a reward address', async () => { + await token.transfer(owner, value, { from: user }).should.be.fulfilled + + expect(await contract.totalSpentPerDay(token.address, currentDay)).to.be.bignumber.equal(ZERO) + await token.transfer(contract.address, value, { from: owner }).should.be.fulfilled + expect(await contract.totalSpentPerDay(token.address, currentDay)).to.be.bignumber.equal(value) + expect(await token.balanceOf(contract.address)).to.be.bignumber.equal(ZERO) + expect(await token.balanceOf(owner)).to.be.bignumber.equal(ZERO) + + const feeEvents = await getEvents(contract, { event: 'FeeDistributed' }) + expect(feeEvents.length).to.be.equal(0) + }) }) }) }) From 2d9efe29377ec73bf646b23142446ed7893cb75a Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 22 Oct 2020 00:24:08 +0300 Subject: [PATCH 3/8] Fix behaviour for the unset fee manager contract (#539) --- .../RewardableBridge.sol | 36 ++++++++----------- .../HomeBridgeErcToErcPOSDAO.sol | 13 +++---- .../RewardableHomeBridgeErcToNative.sol | 12 +++---- test/erc_to_erc/home_bridge.test.js | 18 ++++++++++ test/erc_to_native/home_bridge.test.js | 9 +++++ test/native_to_erc/foreign_bridge_test.js | 19 ++++++++++ test/native_to_erc/home_bridge_test.js | 17 +++++++++ 7 files changed, 87 insertions(+), 37 deletions(-) diff --git a/contracts/upgradeable_contracts/RewardableBridge.sol b/contracts/upgradeable_contracts/RewardableBridge.sol index 00a4cc403..4650e6c0c 100644 --- a/contracts/upgradeable_contracts/RewardableBridge.sol +++ b/contracts/upgradeable_contracts/RewardableBridge.sol @@ -18,38 +18,30 @@ 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 validFeeType(_feeType) returns (uint256) { - uint256 fee; + function _getFee(bytes32 _feeType) internal view validFeeType(_feeType) returns (uint256 fee) { address feeManager = feeManagerContract(); bytes4 method = _feeType == HOME_FEE ? GET_HOME_FEE : GET_FOREIGN_FEE; bytes memory callData = abi.encodeWithSelector(method); assembly { let result := callcode(gas, feeManager, 0x0, add(callData, 0x20), mload(callData), 0, 32) - fee := mload(0) - switch result - case 0 { - revert(0, 0) - } + if and(eq(returndatasize, 32), result) { + fee := mload(0) + } } - return fee; } - function getFeeManagerMode() external view returns (bytes4) { - bytes4 mode; + function getFeeManagerMode() external view returns (bytes4 mode) { bytes memory callData = abi.encodeWithSelector(GET_FEE_MANAGER_MODE); address feeManager = feeManagerContract(); assembly { let result := callcode(gas, feeManager, 0x0, add(callData, 0x20), mload(callData), 0, 4) - mode := mload(0) - switch result - case 0 { - revert(0, 0) - } + if and(eq(returndatasize, 32), result) { + mode := mload(0) + } } - return mode; } function feeManagerContract() public view returns (address) { @@ -69,20 +61,20 @@ contract RewardableBridge is Ownable, FeeTypes { function calculateFee(uint256 _value, bool _recover, address _impl, bytes32 _feeType) internal view - returns (uint256) + returns (uint256 fee) { - uint256 fee; bytes memory callData = abi.encodeWithSelector(CALCULATE_FEE, _value, _recover, _feeType); assembly { let result := callcode(gas, _impl, 0x0, add(callData, 0x20), mload(callData), 0, 32) - fee := mload(0) - switch result - case 0 { + switch and(eq(returndatasize, 32), result) + case 1 { + fee := mload(0) + } + default { revert(0, 0) } } - return fee; } function distributeFeeFromSignatures(uint256 _fee, address _feeManager, bytes32 _txHash) internal { diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErcPOSDAO.sol b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErcPOSDAO.sol index ee4604139..1aaee4fc8 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErcPOSDAO.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErcPOSDAO.sol @@ -37,22 +37,17 @@ contract HomeBridgeErcToErcPOSDAO is HomeBridgeErcToErc { return isInitialized(); } - function blockRewardContract() public view returns (address) { - address blockReward; + function blockRewardContract() public view returns (address blockReward) { address feeManager = feeManagerContract(); bytes memory callData = abi.encodeWithSelector(BLOCK_REWARD_CONTRACT_SELECTOR); assembly { let result := callcode(gas, feeManager, 0x0, add(callData, 0x20), mload(callData), 0, 32) - blockReward := mload(0) - switch result - case 0 { - revert(0, 0) - } + if and(eq(returndatasize, 32), result) { + blockReward := mload(0) + } } - - return blockReward; } function setBlockRewardContract(address _blockReward) external onlyOwner { diff --git a/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol index fb8ebc612..03a36cdf8 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol @@ -21,19 +21,19 @@ contract RewardableHomeBridgeErcToNative is RewardableBridge { return _getFee(FOREIGN_FEE); } - function getAmountToBurn(uint256 _value) public view returns (uint256) { - uint256 amount; + function getAmountToBurn(uint256 _value) public view returns (uint256 amount) { bytes memory callData = abi.encodeWithSelector(GET_AMOUNT_TO_BURN, _value); address feeManager = feeManagerContract(); assembly { let result := callcode(gas, feeManager, 0x0, add(callData, 0x20), mload(callData), 0, 32) - amount := mload(0) - switch result - case 0 { + switch and(eq(returndatasize, 32), result) + case 1 { + amount := mload(0) + } + default { revert(0, 0) } } - return amount; } } diff --git a/test/erc_to_erc/home_bridge.test.js b/test/erc_to_erc/home_bridge.test.js index cf6714d36..a5050f917 100644 --- a/test/erc_to_erc/home_bridge.test.js +++ b/test/erc_to_erc/home_bridge.test.js @@ -1440,6 +1440,24 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { expect(await homeBridge.getHomeFee()).to.be.bignumber.equals(newHomeFee) expect(await homeBridge.getForeignFee()).to.be.bignumber.equals(newForeignFee) }) + it('should return zero parameters for zero fee manager', async () => { + // When + await homeBridge.initialize( + validatorContract.address, + [oneEther, halfEther, minPerTx], + gasPrice, + requireBlockConfirmations, + token.address, + [foreignDailyLimit, foreignMaxPerTx], + owner, + decimalShiftZero + ).should.be.fulfilled + + // Then + expect(await homeBridge.getFeeManagerMode()).to.be.equal('0x00000000') + expect(await homeBridge.getHomeFee()).to.be.bignumber.equal(ZERO) + expect(await homeBridge.getForeignFee()).to.be.bignumber.equal(ZERO) + }) it('should be able to get fee manager mode', async () => { // Given const feeManager = await FeeManagerErcToErcPOSDAO.new() diff --git a/test/erc_to_native/home_bridge.test.js b/test/erc_to_native/home_bridge.test.js index 8f3180fde..d2243d2e0 100644 --- a/test/erc_to_native/home_bridge.test.js +++ b/test/erc_to_native/home_bridge.test.js @@ -1810,6 +1810,15 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { const bridgeForeignFee = await homeBridge.getForeignFee() bridgeForeignFee.should.be.bignumber.equal(foreignFee) }) + it('should return zero parameters for zero fee manager', async () => { + // When + await homeBridge.setFeeManagerContract(ZERO_ADDRESS, { from: owner }).should.be.fulfilled + + // Then + expect(await homeBridge.getFeeManagerMode()).to.be.equal('0x00000000') + expect(await homeBridge.getHomeFee()).to.be.bignumber.equal(ZERO) + expect(await homeBridge.getForeignFee()).to.be.bignumber.equal(ZERO) + }) it('should be able to get fee manager mode', async () => { // Given const feeManager = await FeeManagerErcToNative.new() diff --git a/test/native_to_erc/foreign_bridge_test.js b/test/native_to_erc/foreign_bridge_test.js index b17191875..78259f155 100644 --- a/test/native_to_erc/foreign_bridge_test.js +++ b/test/native_to_erc/foreign_bridge_test.js @@ -1185,6 +1185,25 @@ contract('ForeignBridge', async accounts => { expect(await foreignBridge.getHomeFee()).to.be.bignumber.equals(newHomeFee) }) + it('should return zero parameters for zero fee manager', async () => { + // When + await foreignBridge.initialize( + validatorContract.address, + token.address, + [oneEther, halfEther, minPerTx], + gasPrice, + requireBlockConfirmations, + [homeDailyLimit, homeMaxPerTx], + owner, + decimalShiftZero, + otherSideBridgeAddress + ) + + // Then + expect(await foreignBridge.getFeeManagerMode()).to.be.equal('0x00000000') + expect(await foreignBridge.getHomeFee()).to.be.bignumber.equal(ZERO) + }) + it('should be able to get fee manager mode', async () => { // Given const feeManager = await FeeManagerNativeToErc.new() diff --git a/test/native_to_erc/home_bridge_test.js b/test/native_to_erc/home_bridge_test.js index b70d331f2..9d052e59a 100644 --- a/test/native_to_erc/home_bridge_test.js +++ b/test/native_to_erc/home_bridge_test.js @@ -1290,6 +1290,23 @@ contract('HomeBridge', async accounts => { // Then expect(await homeBridge.getForeignFee()).to.be.bignumber.equals(newForeignFee) }) + it('should return zero parameters for zero fee manager', async () => { + // When + await homeBridge.initialize( + validatorContract.address, + [oneEther, halfEther, minPerTx], + gasPrice, + requireBlockConfirmations, + [foreignDailyLimit, foreignMaxPerTx], + owner, + decimalShiftZero + ).should.be.fulfilled + + // Then + expect(await homeBridge.getFeeManagerMode()).to.be.equal('0x00000000') + expect(await homeBridge.getHomeFee()).to.be.bignumber.equal(ZERO) + expect(await homeBridge.getForeignFee()).to.be.bignumber.equal(ZERO) + }) it('should be able to get fee manager mode', async () => { // Given const feeManager = await FeeManagerNativeToErc.new() From 816c4b1f2393252271ed30ba293443b6684ff582 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Sat, 31 Oct 2020 00:01:36 +0300 Subject: [PATCH 4/8] Warning for admins about fee value changing on working bridge (#545) --- .../upgradeable_contracts/BaseFeeManager.sol | 7 +++ .../RewardableBridge.sol | 62 +++++++++++++++++-- .../erc20_to_erc20/HomeBridgeErcToErc.sol | 11 ++-- .../RewardableHomeBridgeErcToErc.sol | 12 ++++ .../erc20_to_native/HomeBridgeErcToNative.sol | 11 ++-- .../RewardableHomeBridgeErcToNative.sol | 12 ++++ .../ForeignBridgeNativeToErc.sol | 2 +- .../native_to_erc20/HomeBridgeNativeToErc.sol | 15 +++-- .../RewardableForeignBridgeNativeToErc.sol | 18 ++++++ .../RewardableHomeBridgeNativeToErc.sol | 20 ++++-- 10 files changed, 143 insertions(+), 27 deletions(-) diff --git a/contracts/upgradeable_contracts/BaseFeeManager.sol b/contracts/upgradeable_contracts/BaseFeeManager.sol index bb24897e8..252803663 100644 --- a/contracts/upgradeable_contracts/BaseFeeManager.sol +++ b/contracts/upgradeable_contracts/BaseFeeManager.sol @@ -16,6 +16,13 @@ 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")) + /** + * @dev Calculated the amount of fee for the particular bridge operation. + * @param _value bridged amount of tokens/coins for which fee amount is calculated. + * @param _recover true, if the fee was already subtracted from the given _value and needs to be restored. + * @param _feeType type of the fee, should be either HOME_FEE of FOREIGN_FEE. + * @return calculated fee amount. + */ function calculateFee(uint256 _value, bool _recover, bytes32 _feeType) public view diff --git a/contracts/upgradeable_contracts/RewardableBridge.sol b/contracts/upgradeable_contracts/RewardableBridge.sol index 4650e6c0c..107467938 100644 --- a/contracts/upgradeable_contracts/RewardableBridge.sol +++ b/contracts/upgradeable_contracts/RewardableBridge.sol @@ -4,6 +4,10 @@ import "openzeppelin-solidity/contracts/AddressUtils.sol"; import "./Ownable.sol"; import "./FeeTypes.sol"; +/** + * @title RewardableBridge + * @dev Common functionality for fee management logic delegation to the separate fee management contract. + */ contract RewardableBridge is Ownable, FeeTypes { event FeeDistributedFromAffirmation(uint256 feeAmount, bytes32 indexed transactionHash); event FeeDistributedFromSignatures(uint256 feeAmount, bytes32 indexed transactionHash); @@ -18,6 +22,11 @@ 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) + /** + * @dev Internal function for reading the fee value from the fee manager. + * @param _feeType type of the fee, should be either HOME_FEE of FOREIGN_FEE. + * @return retrieved fee percentage. + */ function _getFee(bytes32 _feeType) internal view validFeeType(_feeType) returns (uint256 fee) { address feeManager = feeManagerContract(); bytes4 method = _feeType == HOME_FEE ? GET_HOME_FEE : GET_FOREIGN_FEE; @@ -32,6 +41,10 @@ contract RewardableBridge is Ownable, FeeTypes { } } + /** + * @dev Retrieves the mode of the used fee manager. + * @return manager mode identifier, or zero bytes otherwise. + */ function getFeeManagerMode() external view returns (bytes4 mode) { bytes memory callData = abi.encodeWithSelector(GET_FEE_MANAGER_MODE); address feeManager = feeManagerContract(); @@ -44,20 +57,45 @@ contract RewardableBridge is Ownable, FeeTypes { } } + /** + * @dev Retrieves the address of the fee manager contract used. + * @return address of the fee manager contract. + */ function feeManagerContract() public view returns (address) { return addressStorage[FEE_MANAGER_CONTRACT]; } + /** + * @dev Updates the address of the used fee manager contract. + * Only contract owner can call this method. + * If during this operation, home fee is changed, it is highly recommended to stop the bridge operations first. + * Otherwise, pending signature requests can become a reason for imbalance between two bridge sides. + * @param _feeManager address of the new fee manager contract, or zero address to disable fee collection. + */ function setFeeManagerContract(address _feeManager) external onlyOwner { require(_feeManager == address(0) || AddressUtils.isContract(_feeManager)); addressStorage[FEE_MANAGER_CONTRACT] = _feeManager; } + /** + * @dev Internal function for setting the fee value by using the fee manager. + * @param _feeManager address of the fee manager contract. + * @param _fee new value for fee percentage amount. + * @param _feeType type of the fee, should be either HOME_FEE of FOREIGN_FEE. + */ 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))); } + /** + * @dev Calculates the exact fee amount by using the fee manager. + * @param _value transferred value for which fee should be calculated. + * @param _recover true, if the fee was already subtracted from the given _value and needs to be restored. + * @param _impl address of the fee manager contract. + * @param _feeType type of the fee, should be either HOME_FEE of FOREIGN_FEE. + * @return calculated fee amount. + */ function calculateFee(uint256 _value, bool _recover, address _impl, bytes32 _feeType) internal view @@ -77,13 +115,29 @@ contract RewardableBridge is Ownable, FeeTypes { } } + /** + * @dev Internal function for distributing the fee for collecting sufficient amount of signatures. + * @param _fee amount of fee to distribute. + * @param _feeManager address of the fee manager contract. + * @param _txHash reference transaction hash where the original bridge request happened. + */ function distributeFeeFromSignatures(uint256 _fee, address _feeManager, bytes32 _txHash) internal { - require(_feeManager.delegatecall(abi.encodeWithSelector(DISTRIBUTE_FEE_FROM_SIGNATURES, _fee))); - emit FeeDistributedFromSignatures(_fee, _txHash); + if (_fee > 0) { + require(_feeManager.delegatecall(abi.encodeWithSelector(DISTRIBUTE_FEE_FROM_SIGNATURES, _fee))); + emit FeeDistributedFromSignatures(_fee, _txHash); + } } + /** + * @dev Internal function for distributing the fee for collecting sufficient amount of affirmations. + * @param _fee amount of fee to distribute. + * @param _feeManager address of the fee manager contract. + * @param _txHash reference transaction hash where the original bridge request happened. + */ function distributeFeeFromAffirmation(uint256 _fee, address _feeManager, bytes32 _txHash) internal { - require(_feeManager.delegatecall(abi.encodeWithSelector(DISTRIBUTE_FEE_FROM_AFFIRMATION, _fee))); - emit FeeDistributedFromAffirmation(_fee, _txHash); + if (_fee > 0) { + require(_feeManager.delegatecall(abi.encodeWithSelector(DISTRIBUTE_FEE_FROM_AFFIRMATION, _fee))); + emit FeeDistributedFromAffirmation(_fee, _txHash); + } } } diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol index 0ae71855a..34517a677 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol @@ -174,14 +174,15 @@ contract HomeBridgeErcToErc is emit UserRequestForSignature(_from, valueToTransfer); } + /** + * @dev Internal function to be called when enough signatures are collected. + * Distributed the fee for collecting signatures. + * @param _message encoded message signed by the validators. + */ function onSignaturesCollected(bytes _message) internal { address feeManager = feeManagerContract(); if (feeManager != address(0)) { - address recipient; - uint256 amount; - bytes32 txHash; - address contractAddress; - (recipient, amount, txHash, contractAddress) = Message.parseMessage(_message); + (, uint256 amount, bytes32 txHash, ) = Message.parseMessage(_message); uint256 fee = calculateFee(amount, true, feeManager, HOME_FEE); distributeFeeFromSignatures(fee, feeManager, txHash); } diff --git a/contracts/upgradeable_contracts/erc20_to_erc20/RewardableHomeBridgeErcToErc.sol b/contracts/upgradeable_contracts/erc20_to_erc20/RewardableHomeBridgeErcToErc.sol index 816226902..02a2ca0d7 100644 --- a/contracts/upgradeable_contracts/erc20_to_erc20/RewardableHomeBridgeErcToErc.sol +++ b/contracts/upgradeable_contracts/erc20_to_erc20/RewardableHomeBridgeErcToErc.sol @@ -3,10 +3,22 @@ pragma solidity 0.4.24; import "../RewardableBridge.sol"; contract RewardableHomeBridgeErcToErc is RewardableBridge { + /** + * @dev Updates the fee percentage for home->foreign bridge operations. + * Only owner is allowed to call this method. + * If during this operation, home fee is changed, it is highly recommended to stop the bridge operations first. + * Otherwise, pending signature requests can become a reason for imbalance between two bridge sides. + * @param _fee new value for fee percentage. + */ function setHomeFee(uint256 _fee) external onlyOwner { _setFee(feeManagerContract(), _fee, HOME_FEE); } + /** + * @dev Updates the fee percentage for foreign->home bridge operations. + * Only owner is allowed to call this method. + * @param _fee new value for fee percentage. + */ function setForeignFee(uint256 _fee) external onlyOwner { _setFee(feeManagerContract(), _fee, FOREIGN_FEE); } diff --git a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol index 108cc81be..664b49069 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/HomeBridgeErcToNative.sol @@ -191,14 +191,15 @@ contract HomeBridgeErcToNative is return true; } + /** + * @dev Internal function to be called when enough signatures are collected. + * Distributed the fee for collecting signatures. + * @param _message encoded message signed by the validators. + */ function onSignaturesCollected(bytes _message) internal { address feeManager = feeManagerContract(); if (feeManager != address(0)) { - address recipient; - uint256 amount; - bytes32 txHash; - address contractAddress; - (recipient, amount, txHash, contractAddress) = Message.parseMessage(_message); + (, uint256 amount, bytes32 txHash, ) = Message.parseMessage(_message); uint256 fee = calculateFee(amount, true, feeManager, HOME_FEE); distributeFeeFromSignatures(fee, feeManager, txHash); } diff --git a/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol b/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol index 03a36cdf8..85a393433 100644 --- a/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol +++ b/contracts/upgradeable_contracts/erc20_to_native/RewardableHomeBridgeErcToNative.sol @@ -5,10 +5,22 @@ import "../RewardableBridge.sol"; contract RewardableHomeBridgeErcToNative is RewardableBridge { bytes4 internal constant GET_AMOUNT_TO_BURN = 0x916850e9; // getAmountToBurn(uint256) + /** + * @dev Updates the fee percentage for home->foreign bridge operations. + * Only owner is allowed to call this method. + * If during this operation, home fee is changed, it is highly recommended to stop the bridge operations first. + * Otherwise, pending signature requests can become a reason for imbalance between two bridge sides. + * @param _fee new value for fee percentage. + */ function setHomeFee(uint256 _fee) external onlyOwner { _setFee(feeManagerContract(), _fee, HOME_FEE); } + /** + * @dev Updates the fee percentage for foreign->home bridge operations. + * Only owner is allowed to call this method. + * @param _fee new value for fee percentage. + */ function setForeignFee(uint256 _fee) external onlyOwner { _setFee(feeManagerContract(), _fee, FOREIGN_FEE); } diff --git a/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol index 3fd0444d3..357b3bb44 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol @@ -124,7 +124,7 @@ contract ForeignBridgeNativeToErc is address feeManager = feeManagerContract(); if (feeManager != address(0)) { uint256 fee = calculateFee(valueToMint, false, feeManager, HOME_FEE); - if (fee != 0) { + if (fee > 0) { distributeFeeFromSignatures(fee, feeManager, _txHash); valueToMint = valueToMint.sub(fee); } diff --git a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol index 0e4293827..88d81f2d5 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol @@ -122,18 +122,17 @@ contract HomeBridgeNativeToErc is EternalStorage, BasicHomeBridge, RewardableHom _setOwner(_owner); } + /** + * @dev Internal function to be called when enough signatures are collected. + * Distributed the fee for collecting signatures. + * @param _message encoded message signed by the validators. + */ function onSignaturesCollected(bytes _message) internal { address feeManager = feeManagerContract(); if (feeManager != address(0)) { - address recipient; - uint256 amount; - bytes32 txHash; - address contractAddress; - (recipient, amount, txHash, contractAddress) = Message.parseMessage(_message); + (, uint256 amount, bytes32 txHash, ) = Message.parseMessage(_message); uint256 fee = calculateFee(amount, true, feeManager, HOME_FEE); - if (fee != 0) { - distributeFeeFromSignatures(fee, feeManager, txHash); - } + distributeFeeFromSignatures(fee, feeManager, txHash); } } diff --git a/contracts/upgradeable_contracts/native_to_erc20/RewardableForeignBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/RewardableForeignBridgeNativeToErc.sol index 964264218..836d7720c 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/RewardableForeignBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/RewardableForeignBridgeNativeToErc.sol @@ -3,6 +3,13 @@ pragma solidity 0.4.24; import "../RewardableBridge.sol"; contract RewardableForeignBridgeNativeToErc is RewardableBridge { + /** + * @dev Updates the fee percentage for home->foreign bridge operations. + * Only owner is allowed to call this method. + * If during this operation, home fee is changed, it is highly recommended to stop the bridge operations first. + * Otherwise, pending signature requests can become a reason for imbalance between two bridge sides. + * @param _fee new value for fee percentage. + */ function setHomeFee(uint256 _fee) external onlyOwner { _setFee(feeManagerContract(), _fee, HOME_FEE); } @@ -10,4 +17,15 @@ contract RewardableForeignBridgeNativeToErc is RewardableBridge { function getHomeFee() public view returns (uint256) { return _getFee(HOME_FEE); } + + /** + * @dev Internal function for distributing the fee for collecting sufficient amount of signatures. + * @param _fee amount of fee to distribute. + * @param _feeManager address of the fee manager contract. + * @param _txHash reference transaction hash where the original bridge request happened. + */ + function distributeFeeFromSignatures(uint256 _fee, address _feeManager, bytes32 _txHash) internal { + require(_feeManager.delegatecall(abi.encodeWithSelector(DISTRIBUTE_FEE_FROM_SIGNATURES, _fee))); + emit FeeDistributedFromSignatures(_fee, _txHash); + } } diff --git a/contracts/upgradeable_contracts/native_to_erc20/RewardableHomeBridgeNativeToErc.sol b/contracts/upgradeable_contracts/native_to_erc20/RewardableHomeBridgeNativeToErc.sol index 14cb0e420..e84d80bd6 100644 --- a/contracts/upgradeable_contracts/native_to_erc20/RewardableHomeBridgeNativeToErc.sol +++ b/contracts/upgradeable_contracts/native_to_erc20/RewardableHomeBridgeNativeToErc.sol @@ -3,14 +3,26 @@ pragma solidity 0.4.24; import "../RewardableBridge.sol"; contract RewardableHomeBridgeNativeToErc is RewardableBridge { - function setForeignFee(uint256 _fee) external onlyOwner { - _setFee(feeManagerContract(), _fee, FOREIGN_FEE); - } - + /** + * @dev Updates the fee percentage for home->foreign bridge operations. + * Only owner is allowed to call this method. + * If during this operation, home fee is changed, it is highly recommended to stop the bridge operations first. + * Otherwise, pending signature requests can become a reason for imbalance between two bridge sides. + * @param _fee new value for fee percentage. + */ function setHomeFee(uint256 _fee) external onlyOwner { _setFee(feeManagerContract(), _fee, HOME_FEE); } + /** + * @dev Updates the fee percentage for foreign->home bridge operations. + * Only owner is allowed to call this method. + * @param _fee new value for fee percentage. + */ + function setForeignFee(uint256 _fee) external onlyOwner { + _setFee(feeManagerContract(), _fee, FOREIGN_FEE); + } + function getForeignFee() public view returns (uint256) { return _getFee(FOREIGN_FEE); } From 1748f94757c07ce99d13d99a0a5d1d738b292354 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Sat, 31 Oct 2020 20:03:26 +0300 Subject: [PATCH 5/8] Support manual lane in AMB contracts (#546) --- contracts/interfaces/IAMB.sol | 1 + contracts/mocks/Box.sol | 6 +++++ .../arbitrary_message/BasicHomeAMB.sol | 12 ++++++++++ .../arbitrary_message/MessageDelivery.sol | 24 ++++++++++++++++--- .../arbitrary_message/VersionableAMB.sol | 2 +- test/arbitrary_message/home_bridge.test.js | 19 +++++++++++++-- 6 files changed, 58 insertions(+), 6 deletions(-) diff --git a/contracts/interfaces/IAMB.sol b/contracts/interfaces/IAMB.sol index a809b34f5..50b38a9ef 100644 --- a/contracts/interfaces/IAMB.sol +++ b/contracts/interfaces/IAMB.sol @@ -11,6 +11,7 @@ interface IAMB { function failedMessageReceiver(bytes32 _messageId) external view returns (address); function failedMessageSender(bytes32 _messageId) external view returns (address); function requireToPassMessage(address _contract, bytes _data, uint256 _gas) external returns (bytes32); + function requireToConfirmMessage(address _contract, bytes _data, uint256 _gas) external returns (bytes32); function sourceChainId() external view returns (uint256); function destinationChainId() external view returns (uint256); } diff --git a/contracts/mocks/Box.sol b/contracts/mocks/Box.sol index 4da684e50..069212338 100644 --- a/contracts/mocks/Box.sol +++ b/contracts/mocks/Box.sol @@ -45,4 +45,10 @@ contract Box { bytes memory encodedData = abi.encodeWithSelector(methodSelector, _i); IAMB(_bridge).requireToPassMessage(_executor, encodedData, 141647); } + + function setValueOnOtherNetworkUsingManualLane(uint256 _i, address _bridge, address _executor) public { + bytes4 methodSelector = this.setValue.selector; + bytes memory encodedData = abi.encodeWithSelector(methodSelector, _i); + IAMB(_bridge).requireToConfirmMessage(_executor, encodedData, 141647); + } } diff --git a/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol index de22a3dec..15b481b65 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/BasicHomeAMB.sol @@ -15,6 +15,8 @@ contract BasicHomeAMB is BasicAMB, MessageDelivery { uint256 NumberOfCollectedSignatures ); + uint256 internal constant SEND_TO_MANUAL_LANE = 0xf0; + function executeAffirmation(bytes message) external onlyValidator { bytes32 hashMsg = keccak256(abi.encodePacked(message)); bytes32 hashSender = keccak256(abi.encodePacked(msg.sender, hashMsg)); @@ -37,6 +39,16 @@ contract BasicHomeAMB is BasicAMB, MessageDelivery { } } + /** + * @dev Requests message relay to the opposite network, message is sent to the manual lane. + * @param _contract executor address on the other side. + * @param _data calldata passed to the executor on the other side. + * @param _gas gas limit used on the other network for executing a message. + */ + function requireToConfirmMessage(address _contract, bytes _data, uint256 _gas) external returns (bytes32) { + return _sendMessage(_contract, _data, _gas, SEND_TO_MANUAL_LANE); + } + /** * Parses given message, processes a call inside it * @param _message relayed message diff --git a/contracts/upgradeable_contracts/arbitrary_message/MessageDelivery.sol b/contracts/upgradeable_contracts/arbitrary_message/MessageDelivery.sol index c5394eecb..73b5555b2 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/MessageDelivery.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/MessageDelivery.sol @@ -9,6 +9,8 @@ import "../../libraries/Bytes.sol"; contract MessageDelivery is BasicAMB, MessageProcessor { using SafeMath for uint256; + uint256 internal constant SEND_TO_ORACLE_DRIVEN_LANE = 0x00; + /** * @dev Requests message relay to the opposite network * @param _contract executor address on the other side @@ -16,13 +18,24 @@ contract MessageDelivery is BasicAMB, MessageProcessor { * @param _gas gas limit used on the other network for executing a message */ function requireToPassMessage(address _contract, bytes _data, uint256 _gas) public returns (bytes32) { + return _sendMessage(_contract, _data, _gas, SEND_TO_ORACLE_DRIVEN_LANE); + } + + /** + * @dev Initiates sending of an AMB message to the opposite network + * @param _contract executor address on the other side + * @param _data calldata passed to the executor on the other side + * @param _gas gas limit used on the other network for executing a message + * @param _dataType AMB message dataType to be included as a part of the header + */ + function _sendMessage(address _contract, bytes _data, uint256 _gas, uint256 _dataType) public returns (bytes32) { // it is not allowed to pass messages while other messages are processed require(messageId() == bytes32(0)); require(_gas >= getMinimumGasUsage(_data) && _gas <= maxGasPerTx()); bytes32 _messageId; - bytes memory header = _packHeader(_contract, _gas); + bytes memory header = _packHeader(_contract, _gas, _dataType); _setNonce(_nonce() + 1); assembly { @@ -50,8 +63,13 @@ contract MessageDelivery is BasicAMB, MessageProcessor { * @dev Packs message header into a single bytes blob * @param _contract executor address on the other side * @param _gas gas limit used on the other network for executing a message + * @param _dataType AMB message dataType to be included as a part of the header */ - function _packHeader(address _contract, uint256 _gas) internal view returns (bytes memory header) { + function _packHeader(address _contract, uint256 _gas, uint256 _dataType) + internal + view + returns (bytes memory header) + { uint256 srcChainId = sourceChainId(); uint256 srcChainIdLength = _sourceChainIdLength(); uint256 dstChainId = destinationChainId(); @@ -73,7 +91,7 @@ contract MessageDelivery is BasicAMB, MessageProcessor { mstore(ptr, dstChainId) mstore(sub(ptr, dstChainIdLength), srcChainId) - mstore(add(header, 79), 0x00) + mstore(add(header, 79), _dataType) mstore(add(header, 78), dstChainIdLength) mstore(add(header, 77), srcChainIdLength) mstore(add(header, 76), _gas) diff --git a/contracts/upgradeable_contracts/arbitrary_message/VersionableAMB.sol b/contracts/upgradeable_contracts/arbitrary_message/VersionableAMB.sol index 6546aea19..0e692dcb1 100644 --- a/contracts/upgradeable_contracts/arbitrary_message/VersionableAMB.sol +++ b/contracts/upgradeable_contracts/arbitrary_message/VersionableAMB.sol @@ -17,6 +17,6 @@ contract VersionableAMB is VersionableBridge { * @return (major, minor, patch) version triple */ function getBridgeInterfacesVersion() external pure returns (uint64 major, uint64 minor, uint64 patch) { - return (5, 3, 0); + return (5, 4, 0); } } diff --git a/test/arbitrary_message/home_bridge.test.js b/test/arbitrary_message/home_bridge.test.js index e330c8c3f..92923cbff 100644 --- a/test/arbitrary_message/home_bridge.test.js +++ b/test/arbitrary_message/home_bridge.test.js @@ -258,7 +258,7 @@ contract('HomeAMB', async accounts => { expect(await proxy.upgradeabilityOwner()).to.be.equal(newOwner) }) }) - describe('requireToPassMessage', () => { + describe('requireToPassMessage & requireToConfirmMessage', () => { let homeBridge let bridgeId beforeEach(async () => { @@ -289,7 +289,22 @@ contract('HomeAMB', async accounts => { ) tx.receipt.logs.length.should.be.equal(1) - expect(tx.receipt.logs[0].args.messageId).to.include(`${bridgeId}0000000000000000`) + const { messageId, encodedData } = tx.receipt.logs[0].args + expect(messageId).to.include(`${bridgeId}0000000000000000`) + expect(encodedData.substr(2 + (32 + 20 + 20 + 4 + 1 + 1) * 2, 2)).to.be.equal('00') + }) + it('call requireToConfirmMessage(address, bytes, uint256)', async () => { + const tx = await homeBridge.requireToConfirmMessage( + '0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955', + '0xb1591967aed668a4b27645ff40c444892d91bf5951b382995d4d4f6ee3a2ce03', + 1535604485, + { from: accounts[3] } + ) + + tx.receipt.logs.length.should.be.equal(1) + const { messageId, encodedData } = tx.receipt.logs[0].args + expect(messageId).to.include(`${bridgeId}0000000000000000`) + expect(encodedData.substr(2 + (32 + 20 + 20 + 4 + 1 + 1) * 2, 2)).to.be.equal('f0') }) it('call requireToPassMessage(address, bytes, uint256) should fail', async () => { // Should fail because gas < minimumGasUsage From 09d228fa72566edd5e569a697205718494f3dd31 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Thu, 5 Nov 2020 18:22:56 +0300 Subject: [PATCH 6/8] Omnibridge usage of manual/oracle-driven lanes (#547) --- contracts/mocks/AMBMock.sol | 10 +- .../HomeAMBErc20ToNative.sol | 8 +- .../BasicMultiAMBErc20ToErc677.sol | 2 +- .../HomeMultiAMBErc20ToErc677.sol | 52 ++++--- .../MultiTokenForwardingRules.sol | 130 ++++++++++++++++++ .../home_mediator.test.js | 56 ++++++++ 6 files changed, 232 insertions(+), 26 deletions(-) create mode 100644 contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/MultiTokenForwardingRules.sol diff --git a/contracts/mocks/AMBMock.sol b/contracts/mocks/AMBMock.sol index 9fe0c5426..33ba903aa 100644 --- a/contracts/mocks/AMBMock.sol +++ b/contracts/mocks/AMBMock.sol @@ -42,6 +42,14 @@ contract AMBMock { } function requireToPassMessage(address _contract, bytes _data, uint256 _gas) external returns (bytes32) { + return _sendMessage(_contract, _data, _gas, 0x00); + } + + function requireToConfirmMessage(address _contract, bytes _data, uint256 _gas) external returns (bytes32) { + return _sendMessage(_contract, _data, _gas, 0xf0); + } + + function _sendMessage(address _contract, bytes _data, uint256 _gas, uint256 _dataType) internal returns (bytes32) { require(messageId == bytes32(0)); bytes32 bridgeId = keccak256(abi.encodePacked(uint16(1337), address(this))) & 0x00000000ffffffffffffffffffffffffffffffffffffffff0000000000000000; @@ -55,7 +63,7 @@ contract AMBMock { uint32(_gas), uint8(2), uint8(2), - uint8(0x00), + uint8(_dataType), uint16(1337), uint16(1338), _data diff --git a/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol b/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol index 6b180f8ea..a8461d446 100644 --- a/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol +++ b/contracts/upgradeable_contracts/amb_erc20_to_native/HomeAMBErc20ToNative.sol @@ -61,8 +61,8 @@ contract HomeAMBErc20ToNative is BasicAMBErc20ToNative, BlockRewardBridge, HomeF * @param _decimalShift number of decimals shift required to adjust the amount of tokens bridged. * @param _owner address of the owner of the mediator contract * @param _blockReward address of the block reward contract - * @param _rewardAddreses list of reward addresses, between whom fees will be distributed - * @param _fees array with initial fees for both bridge firections + * @param _rewardAddresses list of reward addresses, between whom fees will be distributed + * @param _fees array with initial fees for both bridge directions * [ 0 = homeToForeignFee, 1 = foreignToHomeFee ] */ function rewardableInitialize( @@ -74,10 +74,10 @@ contract HomeAMBErc20ToNative is BasicAMBErc20ToNative, BlockRewardBridge, HomeF int256 _decimalShift, address _owner, address _blockReward, - address[] _rewardAddreses, + address[] _rewardAddresses, uint256[2] _fees // [ 0 = homeToForeignFee, 1 = foreignToHomeFee ] ) external returns (bool) { - _setRewardAddressList(_rewardAddreses); + _setRewardAddressList(_rewardAddresses); _setFee(HOME_TO_FOREIGN_FEE, _fees[0]); _setFee(FOREIGN_TO_HOME_FEE, _fees[1]); return 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 b84577a99..1af3cdbfc 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/BasicMultiAMBErc20ToErc677.sol @@ -58,7 +58,7 @@ contract BasicMultiAMBErc20ToErc677 is * @return patch value of the version */ function getBridgeInterfacesVersion() external pure returns (uint64 major, uint64 minor, uint64 patch) { - return (1, 1, 1); + return (1, 2, 0); } /** 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 14d8ac0cd..d64ab1a08 100644 --- a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/HomeMultiAMBErc20ToErc677.sol @@ -4,13 +4,18 @@ import "./BasicMultiAMBErc20ToErc677.sol"; import "./TokenProxy.sol"; import "./HomeFeeManagerMultiAMBErc20ToErc677.sol"; import "../../interfaces/IBurnableMintableERC677Token.sol"; +import "./MultiTokenForwardingRules.sol"; /** * @title HomeMultiAMBErc20ToErc677 * @dev Home side implementation for multi-erc20-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 HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManagerMultiAMBErc20ToErc677 { +contract HomeMultiAMBErc20ToErc677 is + BasicMultiAMBErc20ToErc677, + HomeFeeManagerMultiAMBErc20ToErc677, + MultiTokenForwardingRules +{ bytes32 internal constant TOKEN_IMAGE_CONTRACT = 0x20b8ca26cc94f39fab299954184cf3a9bd04f69543e4f454fab299f015b8130f; // keccak256(abi.encodePacked("tokenImageContract")) event NewTokenRegistered(address indexed foreignToken, address indexed homeToken); @@ -26,8 +31,8 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager * @param _requestGasLimit the gas limit for the message execution. * @param _owner address of the owner of the mediator contract. * @param _tokenImage address of the PermittableToken contract that will be used for deploying of new tokens. - * @param _rewardAddreses list of reward addresses, between whom fees will be distributed. - * @param _fees array with initial fees for both bridge firections. + * @param _rewardAddresses list of reward addresses, between whom fees will be distributed. + * @param _fees array with initial fees for both bridge directions. * [ 0 = homeToForeignFee, 1 = foreignToHomeFee ] */ function initialize( @@ -38,7 +43,7 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager uint256 _requestGasLimit, address _owner, address _tokenImage, - address[] _rewardAddreses, + address[] _rewardAddresses, uint256[2] _fees // [ 0 = homeToForeignFee, 1 = foreignToHomeFee ] ) external onlyRelevantSender returns (bool) { require(!isInitialized()); @@ -50,8 +55,8 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager _setRequestGasLimit(_requestGasLimit); _setOwner(_owner); _setTokenImage(_tokenImage); - if (_rewardAddreses.length > 0) { - _setRewardAddressList(_rewardAddreses); + if (_rewardAddresses.length > 0) { + _setRewardAddressList(_rewardAddresses); } _setFee(HOME_TO_FOREIGN_FEE, address(0), _fees[0]); _setFee(FOREIGN_TO_HOME_FEE, address(0), _fees[1]); @@ -242,26 +247,26 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager * @dev Executes action on withdrawal of bridged tokens * @param _token address of token contract * @param _from address of tokens sender - * @param _value requsted amount of bridged tokens + * @param _value requested amount of bridged tokens * @param _data alternative receiver, if specified */ function bridgeSpecificActionsOnTokenTransfer(ERC677 _token, address _from, uint256 _value, bytes _data) internal { if (!lock()) { - bytes32 _messageId = messageId(); uint256 valueToBridge = _value; + uint256 fee = 0; // Next line disables fee collection in case sender is one of the reward addresses. // It is needed to allow a 100% withdrawal of tokens from the home side. // If fees are not disabled for reward receivers, small fraction of tokens will always // be redistributed between the same set of reward addresses, which is not the desired behaviour. if (!isRewardAddress(_from)) { - uint256 fee = _distributeFee(HOME_TO_FOREIGN_FEE, _token, valueToBridge); - if (fee > 0) { - valueToBridge = valueToBridge.sub(fee); - emit FeeDistributed(fee, _token, _messageId); - } + fee = _distributeFee(HOME_TO_FOREIGN_FEE, _token, valueToBridge); + valueToBridge = valueToBridge.sub(fee); } IBurnableMintableERC677Token(_token).burn(valueToBridge); - passMessage(_token, _from, chooseReceiver(_from, _data), valueToBridge); + bytes32 _messageId = passMessage(_token, _from, chooseReceiver(_from, _data), valueToBridge); + if (fee > 0) { + emit FeeDistributed(fee, _token, _messageId); + } } } @@ -273,20 +278,27 @@ contract HomeMultiAMBErc20ToErc677 is BasicMultiAMBErc20ToErc677, HomeFeeManager * @param _from address of sender, if bridge operation fails, tokens will be returned to this address * @param _receiver address of receiver on the other side, will eventually receive bridged tokens * @param _value bridged amount of tokens + * @return id of the created and passed message */ - function passMessage(ERC677 _token, address _from, address _receiver, uint256 _value) internal { + function passMessage(ERC677 _token, address _from, address _receiver, uint256 _value) internal returns (bytes32) { bytes4 methodSelector = this.handleBridgedTokens.selector; address foreignToken = foreignTokenAddress(_token); bytes memory data = abi.encodeWithSelector(methodSelector, foreignToken, _receiver, _value); - bytes32 _messageId = bridgeContract().requireToPassMessage( - mediatorContractOnOtherSide(), - data, - requestGasLimit() - ); + address executor = mediatorContractOnOtherSide(); + uint256 gasLimit = requestGasLimit(); + IAMB bridge = bridgeContract(); + + // Address of the foreign token is used here for determining lane permissions. + // Such decision makes it possible to set rules for tokens that are not bridged yet. + bytes32 _messageId = destinationLane(foreignToken, _from, _receiver) >= 0 + ? bridge.requireToPassMessage(executor, data, gasLimit) + : bridge.requireToConfirmMessage(executor, data, gasLimit); setMessageToken(_messageId, _token); setMessageValue(_messageId, _value); setMessageRecipient(_messageId, _from); + + return _messageId; } } diff --git a/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/MultiTokenForwardingRules.sol b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/MultiTokenForwardingRules.sol new file mode 100644 index 000000000..ca90a971f --- /dev/null +++ b/contracts/upgradeable_contracts/multi_amb_erc20_to_erc677/MultiTokenForwardingRules.sol @@ -0,0 +1,130 @@ +pragma solidity 0.4.24; + +import "../../upgradeable_contracts/Ownable.sol"; + +/** + * @title MultiTokenForwardingRules + * @dev Multi token mediator functionality for managing destination AMB lanes permissions. + */ +contract MultiTokenForwardingRules is Ownable { + address internal constant ANY_ADDRESS = 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF; + + event ForwardingRuleUpdated(address token, address sender, address receiver, int256 lane); + + /** + * @dev Tells the destination lane for a particular bridge operation by checking several wildcard forwarding rules. + * @param _token address of the token contract on the foreign side of the bridge. + * @param _sender address of the tokens sender on the home side of the bridge. + * @param _receiver address of the tokens receiver on the foreign side of the bridge. + * @return destination lane identifier, where the message should be forwarded to. + * 1 - oracle-driven-lane should be used. + * 0 - default behaviour should be applied. + * -1 - manual lane should be used. + */ + function destinationLane(address _token, address _sender, address _receiver) public view returns (int256) { + int256 defaultLane = forwardingRule(_token, ANY_ADDRESS, ANY_ADDRESS); // specific token for all senders and receivers + int256 lane; + if (defaultLane < 0) { + lane = forwardingRule(_token, _sender, ANY_ADDRESS); // specific token for specific sender + if (lane != 0) return lane; + lane = forwardingRule(_token, ANY_ADDRESS, _receiver); // specific token for specific receiver + if (lane != 0) return lane; + return defaultLane; + } + lane = forwardingRule(ANY_ADDRESS, _sender, ANY_ADDRESS); // all tokens for specific sender + if (lane != 0) return lane; + return forwardingRule(ANY_ADDRESS, ANY_ADDRESS, _receiver); // all tokens for specific receiver + } + + /** + * Updates the forwarding rule for bridging specific token. + * Only owner can call this method. + * @param _token address of the token contract on the foreign side. + * @param _enable true, if bridge operations for a given token should be forwarded to the manual lane. + */ + function setTokenForwardingRule(address _token, bool _enable) external { + require(_token != ANY_ADDRESS); + _setForwardingRule(_token, ANY_ADDRESS, ANY_ADDRESS, _enable ? int256(-1) : int256(0)); + } + + /** + * Allows a particular address to send bridge requests to the oracle-driven lane for a particular token. + * Only owner can call this method. + * @param _token address of the token contract on the foreign side. + * @param _sender address of the tokens sender on the home side of the bridge. + * @param _enable true, if bridge operations for a given token and sender should be forwarded to the oracle-driven lane. + */ + function setSenderExceptionForTokenForwardingRule(address _token, address _sender, bool _enable) external { + require(_token != ANY_ADDRESS); + require(_sender != ANY_ADDRESS); + _setForwardingRule(_token, _sender, ANY_ADDRESS, _enable ? int256(1) : int256(0)); + } + + /** + * Allows a particular address to receive bridged tokens from the oracle-driven lane for a particular token. + * Only owner can call this method. + * @param _token address of the token contract on the foreign side. + * @param _receiver address of the tokens receiver on the foreign side of the bridge. + * @param _enable true, if bridge operations for a given token and receiver should be forwarded to the oracle-driven lane. + */ + function setReceiverExceptionForTokenForwardingRule(address _token, address _receiver, bool _enable) external { + require(_token != ANY_ADDRESS); + require(_receiver != ANY_ADDRESS); + _setForwardingRule(_token, ANY_ADDRESS, _receiver, _enable ? int256(1) : int256(0)); + } + + /** + * Updates the forwarding rule for the specific sender. + * Only owner can call this method. + * @param _sender address of the tokens sender on the home side. + * @param _enable true, if all bridge operations from a given sender should be forwarded to the manual lane. + */ + function setSenderForwardingRule(address _sender, bool _enable) external { + require(_sender != ANY_ADDRESS); + _setForwardingRule(ANY_ADDRESS, _sender, ANY_ADDRESS, _enable ? int256(-1) : int256(0)); + } + + /** + * Updates the forwarding rule for the specific receiver. + * Only owner can call this method. + * @param _receiver address of the tokens receiver on the foreign side. + * @param _enable true, if all bridge operations to a given receiver should be forwarded to the manual lane. + */ + function setReceiverForwardingRule(address _receiver, bool _enable) external { + require(_receiver != ANY_ADDRESS); + _setForwardingRule(ANY_ADDRESS, ANY_ADDRESS, _receiver, _enable ? int256(-1) : int256(0)); + } + + /** + * @dev Tells forwarding rule set up for a particular bridge operation. + * @param _token address of the token contract on the foreign side of the bridge. + * @param _sender address of the tokens sender on the home side of the bridge. + * @param _receiver address of the tokens receiver on the foreign side of the bridge. + * @return preferred destination lane for the particular bridge operation. + */ + function forwardingRule(address _token, address _sender, address _receiver) public view returns (int256) { + return intStorage[keccak256(abi.encodePacked("forwardTo", _token, _sender, _receiver))]; + } + + /** + * @dev Internal function for updating the preferred destination lane for the specific wildcard pattern. + * Only owner can call this method. + * Examples: + * _setForwardingRule(tokenA, ANY_ADDRESS, ANY_ADDRESS, -1) - forward all operations on tokenA to the manual lane + * _setForwardingRule(tokenA, Alice, ANY_ADDRESS, 1) - allow Alice to use the oracle-driven lane for bridging tokenA + * _setForwardingRule(tokenA, ANY_ADDRESS, Bob, 1) - forward all tokenA bridge operations, where Bob is the receiver, to the oracle-driven lane + * _setForwardingRule(ANY_ADDRESS, Mallory, ANY_ADDRESS, -1) - forward all bridge operations from Mallory to the manual lane + * @param _token address of the token contract on the foreign side of the bridge. + * @param _sender address of the tokens sender on the home side of the bridge. + * @param _receiver address of the tokens receiver on the foreign side of the bridge. + * @param _lane preferred destination lane for the particular sender. + * 1 - forward to the oracle-driven lane. + * 0 - behaviour is unset, proceed by checking other less-specific rules. + * -1 - manual lane should be used. + */ + function _setForwardingRule(address _token, address _sender, address _receiver, int256 _lane) internal onlyOwner { + intStorage[keccak256(abi.encodePacked("forwardTo", _token, _sender, _receiver))] = _lane; + + emit ForwardingRuleUpdated(_token, _sender, _receiver, _lane); + } +} 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 9a4881983..84ebfdab0 100644 --- a/test/multi_amb_erc20_to_erc677/home_mediator.test.js +++ b/test/multi_amb_erc20_to_erc677/home_mediator.test.js @@ -995,6 +995,62 @@ contract('HomeMultiAMBErc20ToErc677', async accounts => { }) } }) + + describe('oracle driven lane permissions', () => { + it('should allow to set/update lane permissions', async () => { + expect(await contract.destinationLane(token.address, user, user2)).to.be.bignumber.equal('0') + + await contract.setTokenForwardingRule(token.address, true, { from: user }).should.be.rejected + await contract.setTokenForwardingRule(token.address, true, { from: owner }).should.be.fulfilled + + expect(await contract.destinationLane(token.address, user, user2)).to.be.bignumber.equal('-1') + + await contract.setSenderExceptionForTokenForwardingRule(token.address, user, true, { from: user }).should.be + .rejected + await contract.setSenderExceptionForTokenForwardingRule(token.address, user, true, { from: owner }).should.be + .fulfilled + + expect(await contract.destinationLane(token.address, user, user2)).to.be.bignumber.equal('1') + expect(await contract.destinationLane(token.address, user2, user2)).to.be.bignumber.equal('-1') + + await contract.setSenderExceptionForTokenForwardingRule(token.address, user, false, { from: owner }).should.be + .fulfilled + await contract.setReceiverExceptionForTokenForwardingRule(token.address, user, true, { from: user }).should.be + .rejected + await contract.setReceiverExceptionForTokenForwardingRule(token.address, user, true, { from: owner }).should.be + .fulfilled + + expect(await contract.destinationLane(token.address, user, user)).to.be.bignumber.equal('1') + expect(await contract.destinationLane(token.address, user, user2)).to.be.bignumber.equal('-1') + + await contract.setTokenForwardingRule(token.address, false, { from: owner }).should.be.fulfilled + + expect(await contract.destinationLane(token.address, user2, user2)).to.be.bignumber.equal('0') + + await contract.setSenderForwardingRule(user2, true, { from: user }).should.be.rejected + await contract.setSenderForwardingRule(user2, true, { from: owner }).should.be.fulfilled + + expect(await contract.destinationLane(token.address, user2, user2)).to.be.bignumber.equal('-1') + + await contract.setReceiverForwardingRule(user2, true, { from: user }).should.be.rejected + await contract.setReceiverForwardingRule(user2, true, { from: owner }).should.be.fulfilled + + expect(await contract.destinationLane(token.address, user, user2)).to.be.bignumber.equal('-1') + }) + + it('should send a message to the manual lane', async () => { + homeToken = await bridgeToken(token) + + await homeToken.transferAndCall(contract.address, ether('0.1'), '0x', { from: user }).should.be.fulfilled + await contract.setTokenForwardingRule(token.address, true, { from: owner }).should.be.fulfilled + await homeToken.transferAndCall(contract.address, ether('0.1'), '0x', { from: user }).should.be.fulfilled + + const events = await getEvents(ambBridgeContract, { event: 'MockedEvent' }) + expect(events.length).to.be.equal(2) + expect(strip0x(events[0].returnValues.encodedData).slice(156, 158)).to.be.equal('00') + expect(strip0x(events[1].returnValues.encodedData).slice(156, 158)).to.be.equal('f0') + }) + }) }) describe('fees management', () => { From 1e2a17ab0e1816f619c152d77ecdc44531719716 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Fri, 6 Nov 2020 15:20:14 +0300 Subject: [PATCH 7/8] Bump contracts interfaces versions according to the already merged PRs (#548) --- contracts/ERC677BridgeToken.sol | 2 +- contracts/PermittableToken.sol | 5 ----- contracts/upgradeable_contracts/VersionableBridge.sol | 2 +- .../amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol | 2 +- .../amb_erc677_to_erc677/BasicStakeTokenMediator.sol | 2 +- 5 files changed, 4 insertions(+), 9 deletions(-) diff --git a/contracts/ERC677BridgeToken.sol b/contracts/ERC677BridgeToken.sol index 8110c4102..a8d063e4e 100644 --- a/contracts/ERC677BridgeToken.sol +++ b/contracts/ERC677BridgeToken.sol @@ -46,7 +46,7 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna } function getTokenInterfacesVersion() external pure returns (uint64 major, uint64 minor, uint64 patch) { - return (2, 2, 0); + return (2, 4, 0); } function superTransfer(address _to, uint256 _value) internal returns (bool) { diff --git a/contracts/PermittableToken.sol b/contracts/PermittableToken.sol index b2469b3c8..805e244ec 100644 --- a/contracts/PermittableToken.sol +++ b/contracts/PermittableToken.sol @@ -163,9 +163,4 @@ contract PermittableToken is ERC677BridgeToken { function _now() internal view returns (uint256) { return now; } - - /// @dev Version of the token contract. - function getTokenInterfacesVersion() external pure returns (uint64 major, uint64 minor, uint64 patch) { - return (2, 3, 0); - } } diff --git a/contracts/upgradeable_contracts/VersionableBridge.sol b/contracts/upgradeable_contracts/VersionableBridge.sol index 75f61937b..038099041 100644 --- a/contracts/upgradeable_contracts/VersionableBridge.sol +++ b/contracts/upgradeable_contracts/VersionableBridge.sol @@ -2,7 +2,7 @@ pragma solidity 0.4.24; contract VersionableBridge { function getBridgeInterfacesVersion() external pure returns (uint64 major, uint64 minor, uint64 patch) { - return (5, 1, 0); + return (5, 2, 0); } /* solcov ignore next */ diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol index 83f0ebdb1..75e7c321d 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol @@ -96,7 +96,7 @@ contract BasicAMBErc677ToErc677 is } function getBridgeInterfacesVersion() external pure returns (uint64 major, uint64 minor, uint64 patch) { - return (1, 2, 1); + return (1, 3, 0); } function getBridgeMode() external pure returns (bytes4 _data) { diff --git a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicStakeTokenMediator.sol b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicStakeTokenMediator.sol index d069a95a0..90b816d3d 100644 --- a/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicStakeTokenMediator.sol +++ b/contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicStakeTokenMediator.sol @@ -4,7 +4,7 @@ import "./BasicAMBErc677ToErc677.sol"; contract BasicStakeTokenMediator is BasicAMBErc677ToErc677 { function getBridgeInterfacesVersion() external pure returns (uint64 major, uint64 minor, uint64 patch) { - return (1, 1, 0); + return (1, 2, 0); } function getBridgeMode() external pure returns (bytes4 _data) { From d8a9f8c6ffc7a0eea316e37a364d428856be7ef4 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Sat, 7 Nov 2020 12:51:12 +0300 Subject: [PATCH 8/8] Bump package version to 5.5.0 (#549) --- 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 186895a4b..ebce43663 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "tokenbridge-contracts", - "version": "5.5.0-rc0", + "version": "5.5.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 02eba3ff2..833576755 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tokenbridge-contracts", - "version": "5.5.0-rc0", + "version": "5.5.0", "description": "Bridge", "main": "index.js", "scripts": {