Merge the develop branch to the master branch, preparation to v5.5.0-rc0

Author: akolotov

contracts/ERC677BridgeToken.sol

@@ -16,8 +16,6 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna
 
     address internal bridgeContractAddr;
 
-    event ContractFallbackCallFailed(address from, address to, uint256 value);
-
     constructor(string _name, string _symbol, uint8 _decimals) public DetailedERC20(_name, _symbol, _decimals) {
         // solhint-disable-previous-line no-empty-blocks
     }
@@ -67,10 +65,17 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna
         return true;
     }
 
+    /**
+     * @dev Internal function that calls onTokenTransfer callback on the receiver after the successful transfer.
+     * Since it is not present in the original ERC677 standard, the callback is only called on the bridge contract,
+     * in order to simplify UX. In other cases, this token complies with the ERC677/ERC20 standard.
+     * @param _from tokens sender address.
+     * @param _to tokens receiver address.
+     * @param _value amount of sent tokens.
+     */
     function callAfterTransfer(address _from, address _to, uint256 _value) internal {
-        if (AddressUtils.isContract(_to) && !contractFallback(_from, _to, _value, new bytes(0))) {
-            require(!isBridge(_to));
-            emit ContractFallbackCallFailed(_from, _to, _value);
+        if (isBridge(_to)) {
+            require(contractFallback(_from, _to, _value, new bytes(0)));
         }
     }
 
@@ -97,7 +102,12 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna
         revert();
     }
 
-    function claimTokens(address _token, address _to) public onlyOwner validAddress(_to) {
+    /**
+     * @dev Withdraws the erc20 tokens or native coins from this contract.
+     * @param _token address of the claimed token or address(0) for native coins.
+     * @param _to address of the tokens/coins receiver.
+     */
+    function claimTokens(address _token, address _to) external onlyOwner {
         claimValues(_token, _to);
     }
 

contracts/ERC677BridgeTokenRewardable.sol

@@ -13,11 +13,29 @@ contract ERC677BridgeTokenRewardable is ERC677MultiBridgeToken {
         // solhint-disable-previous-line no-empty-blocks
     }
 
+    /**
+     * @dev Updates the address of the used block reward contract.
+     * Only the token owner can call this method. 
+     * Even though this function is inteded only for the initialization purpose,
+     * it is still possible to change the already used block reward contract.
+     * In this case users of the old contract won't lose their accumulated rewards,
+     * they can proceed with the withdrawal by calling the old block reward contract directly.
+     * @param _blockRewardContract address of the new block reward contract.
+     */
     function setBlockRewardContract(address _blockRewardContract) external onlyOwner {
         require(AddressUtils.isContract(_blockRewardContract));
         blockRewardContract = _blockRewardContract;
     }
 
+    /**
+     * @dev Updates the address of the used staking contract.
+     * Only the token owner can call this method. 
+     * Even though this function is inteded only for the initialization purpose,
+     * it is still possible to change the already used staking contract.
+     * In this case users of the old staking contract won't lose their tokens,
+     * they can proceed with the withdrawal by calling the old staking contract directly.
+     * @param _stakingContract address of the new staking contract.
+     */
     function setStakingContract(address _stakingContract) external onlyOwner {
         require(AddressUtils.isContract(_stakingContract));
         require(balanceOf(_stakingContract) == 0);

contracts/PermittableToken.sol

@@ -67,6 +67,26 @@ contract PermittableToken is ERC677BridgeToken {
         return true;
     }
 
+    /// @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
+    /// @param _to The address which will spend the funds.
+    /// @param _value The amount of tokens to be spent.
+    function approve(address _to, uint256 _value) public returns (bool result) {
+        result = super.approve(_to, _value);
+        if (_value == uint256(-1)) {
+            delete expirations[msg.sender][_to];
+        }
+    }
+
+    /// @dev Atomically increases the allowance granted to spender by the caller.
+    /// @param _to The address which will spend the funds.
+    /// @param _addedValue The amount of tokens to increase the allowance by.
+    function increaseAllowance(address _to, uint256 _addedValue) public returns (bool result) {
+        result = super.increaseAllowance(_to, _addedValue);
+        if (allowed[msg.sender][_to] == uint256(-1)) {
+            delete expirations[msg.sender][_to];
+        }
+    }
+
     /// @dev An alias for `transfer` function.
     /// @param _to The address of the recipient.
     /// @param _amount The value to transfer.
@@ -98,6 +118,8 @@ contract PermittableToken is ERC677BridgeToken {
     /// @param _nonce The nonce taken from `nonces(_holder)` public getter.
     /// @param _expiry The allowance expiration date (unix timestamp in UTC).
     /// Can be zero for no expiration. Forced to zero if `_allowed` is `false`.
+    /// Note that timestamps are not precise, malicious miner/validator can manipulate them to some extend.
+    /// Assume that there can be a 900 seconds time delta between the desired timestamp and the actual expiration.
     /// @param _allowed True to enable unlimited allowance for the spender by the holder. False to disable.
     /// @param _v A final byte of signature (ECDSA component).
     /// @param _r The first 32 bytes of signature (ECDSA component).
@@ -116,6 +138,9 @@ contract PermittableToken is ERC677BridgeToken {
         require(_spender != address(0));
         require(_expiry == 0 || _now() <= _expiry);
 
+        require(_v == 27 || _v == 28);
+        require(uint256(_s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0);
+
         bytes32 digest = keccak256(
             abi.encodePacked(
                 "\x19\x01",

contracts/interfaces/IBurnableMintableERC677Token.sol

@@ -4,5 +4,5 @@ import "../interfaces/ERC677.sol";
 contract IBurnableMintableERC677Token is ERC677 {
     function mint(address _to, uint256 _amount) public returns (bool);
     function burn(uint256 _value) public;
-    function claimTokens(address _token, address _to) public;
+    function claimTokens(address _token, address _to) external;
 }

contracts/libraries/ArbitraryMessage.sol

@@ -11,11 +11,9 @@ library ArbitraryMessage {
     * offset 104             : 4 bytes  :: uint32  - gasLimit
     * offset 108             : 1 bytes  :: uint8   - source chain id length (X)
     * offset 109             : 1 bytes  :: uint8   - destination chain id length (Y)
-    * offset 110             : 1 bytes  :: bytes1  - dataType
-    * (optional) 111         : 32 bytes :: uint256 - gasPrice
-    * (optional) 111         : 1 bytes  :: bytes1  - gasPriceSpeed
-    * offset 111/143/112     : X bytes  :: bytes   - source chain id
-    * offset 111/143/112 + X : Y bytes  :: bytes   - destination chain id
+    * offset 110             : 1 bytes  :: uint8   - dataType
+    * offset 111             : X bytes  :: bytes   - source chain id
+    * offset 111 + X         : Y bytes  :: bytes   - destination chain id
 
     * NOTE: when message structure is changed, make sure that MESSAGE_PACKING_VERSION from VersionableAMB is updated as well
     * NOTE: assembly code uses calldatacopy, make sure that message is passed as the first argument in the calldata
@@ -29,9 +27,8 @@ library ArbitraryMessage {
             address sender,
             address executor,
             uint32 gasLimit,
-            bytes1 dataType,
+            uint8 dataType,
             uint256[2] chainIds,
-            uint256 gasPrice,
             bytes memory data
         )
     {
@@ -50,23 +47,16 @@ library ArbitraryMessage {
             executor := shr(96, blob)
             gasLimit := and(shr(64, blob), 0xffffffff)
 
+            dataType := byte(26, blob)
+            if gt(dataType, 0) {
+                // for now, only 0 datatype is supported - regular AMB calls
+                // other dataType values are kept reserved for future use
+                revert(0, 0)
+            }
+
             // load source chain id length
             let chainIdLength := byte(24, blob)
 
-            dataType := and(shl(208, blob), 0xFF00000000000000000000000000000000000000000000000000000000000000)
-            switch dataType
-                case 0x0000000000000000000000000000000000000000000000000000000000000000 {
-                    gasPrice := 0
-                }
-                case 0x0100000000000000000000000000000000000000000000000000000000000000 {
-                    gasPrice := mload(add(_data, 111)) // 32
-                    srcdataptr := add(srcdataptr, 32)
-                }
-                case 0x0200000000000000000000000000000000000000000000000000000000000000 {
-                    gasPrice := 0
-                    srcdataptr := add(srcdataptr, 1)
-                }
-
             // at this moment srcdataptr points to sourceChainId
 
             // mask for sourceChainId

contracts/libraries/Message.sol

@@ -2,26 +2,6 @@ pragma solidity 0.4.24;
 import "../interfaces/IBridgeValidators.sol";
 
 library Message {
-    // function uintToString(uint256 inputValue) internal pure returns (string) {
-    //     // figure out the length of the resulting string
-    //     uint256 length = 0;
-    //     uint256 currentValue = inputValue;
-    //     do {
-    //         length++;
-    //         currentValue /= 10;
-    //     } while (currentValue != 0);
-    //     // allocate enough memory
-    //     bytes memory result = new bytes(length);
-    //     // construct the string backwards
-    //     uint256 i = length - 1;
-    //     currentValue = inputValue;
-    //     do {
-    //         result[i--] = byte(48 + currentValue % 10);
-    //         currentValue /= 10;
-    //     } while (currentValue != 0);
-    //     return string(result);
-    // }
-
     function addressArrayContains(address[] array, address value) internal pure returns (bool) {
         for (uint256 i = 0; i < array.length; i++) {
             if (array[i] == value) {
@@ -83,6 +63,9 @@ library Message {
             s := mload(add(signature, 0x40))
             v := mload(add(signature, 0x60))
         }
+        require(uint8(v) == 27 || uint8(v) == 28);
+        require(uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0);
+
         return ecrecover(hashMessage(message, isAMBMessage), uint8(v), r, s);
     }
 

contracts/libraries/TokenReader.sol

@@ -73,7 +73,7 @@ library TokenReader {
             ptr := mload(0x40)
             mstore(ptr, 0x95d89b4100000000000000000000000000000000000000000000000000000000) // symbol()
             if iszero(staticcall(gas, _token, ptr, 4, ptr, 32)) {
-                mstore(ptr, 0xf76f8d7800000000000000000000000000000000000000000000000000000000) // SYMBOl()
+                mstore(ptr, 0xf76f8d7800000000000000000000000000000000000000000000000000000000) // SYMBOL()
                 staticcall(gas, _token, ptr, 4, ptr, 32)
                 pop
             }

contracts/mocks/MessageTest.sol

@@ -11,15 +11,12 @@ contract MessageTest {
             address sender,
             address executor,
             uint32 gasLimit,
-            bytes1 dataType,
+            uint8 dataType,
             uint256[2] chainIds,
-            uint256 gasPrice,
             bytes memory data
         )
     {
-        (messageId, sender, executor, gasLimit, dataType, chainIds, gasPrice, data) = ArbitraryMessage.unpackData(
-            _data
-        );
+        (messageId, sender, executor, gasLimit, dataType, chainIds, data) = ArbitraryMessage.unpackData(_data);
     }
 
     function unpackDataWithExtraParams(
@@ -33,15 +30,12 @@ contract MessageTest {
             address sender,
             address executor,
             uint32 gasLimit,
-            bytes1 dataType,
+            uint8 dataType,
             uint256[2] chainIds,
-            uint256 gasPrice,
             bytes memory data
         )
     {
-        (messageId, sender, executor, gasLimit, dataType, chainIds, gasPrice, data) = ArbitraryMessage.unpackData(
-            _data
-        );
+        (messageId, sender, executor, gasLimit, dataType, chainIds, data) = ArbitraryMessage.unpackData(_data);
     }
 
 }

contracts/upgradeable_contracts/BaseFeeManager.sol

@@ -16,7 +16,12 @@ contract BaseFeeManager is EternalStorage, FeeTypes {
     bytes32 internal constant HOME_FEE_STORAGE_KEY = 0xc3781f3cec62d28f56efe98358f59c2105504b194242dbcb2cc0806850c306e7; // keccak256(abi.encodePacked("homeFee"))
     bytes32 internal constant FOREIGN_FEE_STORAGE_KEY = 0x68c305f6c823f4d2fa4140f9cf28d32a1faccf9b8081ff1c2de11cf32c733efc; // keccak256(abi.encodePacked("foreignFee"))
 
-    function calculateFee(uint256 _value, bool _recover, bytes32 _feeType) public view returns (uint256) {
+    function calculateFee(uint256 _value, bool _recover, bytes32 _feeType)
+        public
+        view
+        validFeeType(_feeType)
+        returns (uint256)
+    {
         uint256 fee = _feeType == HOME_FEE ? getHomeFee() : getForeignFee();
         if (!_recover) {
             return _value.mul(fee).div(MAX_FEE);

contracts/upgradeable_contracts/BaseMediatorFeeManager.sol

@@ -2,6 +2,7 @@ pragma solidity 0.4.24;
 
 import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
 import "openzeppelin-solidity/contracts/math/SafeMath.sol";
+import "openzeppelin-solidity/contracts/AddressUtils.sol";
 
 /**
 * @title BaseMediatorFeeManager
@@ -31,9 +32,11 @@ contract BaseMediatorFeeManager is Ownable {
     * @dev Stores the initial parameters of the fee manager.
     * @param _owner address of the owner of the fee manager contract.
     * @param _fee the fee percentage amount.
-    * @param _rewardAccountList list of addresses that will receive the fee rewards.
+    * @param _rewardAccountList list of unique addresses that will receive the fee rewards.
+    * @param _mediatorContract address of the mediator contract used together with this fee manager.
     */
     constructor(address _owner, uint256 _fee, address[] _rewardAccountList, address _mediatorContract) public {
+        require(AddressUtils.isContract(_mediatorContract));
         require(_rewardAccountList.length > 0 && _rewardAccountList.length <= MAX_REWARD_ACCOUNTS);
         _transferOwnership(_owner);
         _setFee(_fee);
@@ -82,7 +85,7 @@ contract BaseMediatorFeeManager is Ownable {
     function addRewardAccount(address _account) external onlyOwner {
         require(isValidAccount(_account));
         require(!isRewardAccount(_account));
-        require(rewardAccounts.length.add(1) < MAX_REWARD_ACCOUNTS);
+        require(rewardAccounts.length < MAX_REWARD_ACCOUNTS);
         rewardAccounts.push(_account);
     }
 
@@ -154,6 +157,12 @@ contract BaseMediatorFeeManager is Ownable {
     */
     function distributeFee(uint256 _fee) internal {
         uint256 numOfAccounts = rewardAccountsCount();
+        if (numOfAccounts == 0) {
+            // In case there are no reward accounts defined, no actual fee distribution will happen.
+            // Funds will be kept locked on the contract until some of the reward accounts will be added.
+            // After it, locked funds ca be distributed by a call to onTokenTransfer() of this contract, which can be done by anyone.
+            return;
+        }
         uint256 feePerAccount = _fee.div(numOfAccounts);
         uint256 randomAccountIndex;
         uint256 diff = _fee.sub(feePerAccount.mul(numOfAccounts));