Merge the develop branch to the master branch, preparation to v6.0.0

This update for the `master` branch contains the changes made to address findings discovered during a security audit:
 * [Fix] Stricter preconditions for payInterest (omni#623)
 * [Fix] Fix offset in comments (omni#624)
 * [Fix] Use fixed lower call gas limit (omni#627)
 * [Fix] Separate XDaiForeignBridge contract with compound and GSN support (omni#626)
 * [Fix] Update GSN interface (omni#628)
 * [Fix] Block ERC20 selectors in AMB requests (omni#630)
 * [Other] Bump package and contracts interfaces version prior to 6.0.0 (omni#629)

contracts/gsn/BasePaymaster.sol

@@ -4,7 +4,6 @@ pragma experimental ABIEncoderV2;
 
 import "../upgradeable_contracts/Ownable.sol";
 
-import "./interfaces/GsnTypes.sol";
 import "./interfaces/IPaymaster.sol";
 import "./interfaces/IRelayHub.sol";
 import "./utils/GsnEip712Library.sol";
@@ -18,7 +17,7 @@ import "./forwarder/IForwarder.sol";
  */
 contract BasePaymaster is IPaymaster, Ownable {
     IRelayHub internal relayHub;
-    IForwarder public trustedForwarder;
+    address private _trustedForwarder;
 
     function getHubAddr() public view returns (address) {
         return address(relayHub);
@@ -27,45 +26,63 @@ contract BasePaymaster is IPaymaster, Ownable {
     //overhead of forwarder verify+signature, plus hub overhead.
     uint256 public constant FORWARDER_HUB_OVERHEAD = 50000;
 
-    //These parameters are documented in IPaymaster.GasLimits
+    //These parameters are documented in IPaymaster.GasAndDataLimits
     uint256 public constant PRE_RELAYED_CALL_GAS_LIMIT = 100000;
     uint256 public constant POST_RELAYED_CALL_GAS_LIMIT = 110000;
     uint256 public constant PAYMASTER_ACCEPTANCE_BUDGET = PRE_RELAYED_CALL_GAS_LIMIT + FORWARDER_HUB_OVERHEAD;
+    uint256 public constant CALLDATA_SIZE_LIMIT = 10500;
 
-    function getGasLimits() external view returns (IPaymaster.GasLimits) {
+    function getGasAndDataLimits() external view returns (IPaymaster.GasAndDataLimits limits) {
         return
-            IPaymaster.GasLimits(PAYMASTER_ACCEPTANCE_BUDGET, PRE_RELAYED_CALL_GAS_LIMIT, POST_RELAYED_CALL_GAS_LIMIT);
+            IPaymaster.GasAndDataLimits(
+                PAYMASTER_ACCEPTANCE_BUDGET,
+                PRE_RELAYED_CALL_GAS_LIMIT,
+                POST_RELAYED_CALL_GAS_LIMIT,
+                CALLDATA_SIZE_LIMIT
+            );
     }
 
     // this method must be called from preRelayedCall to validate that the forwarder
     // is approved by the paymaster as well as by the recipient contract.
     function _verifyForwarder(GsnTypes.RelayRequest relayRequest) public view {
-        require(address(trustedForwarder) == relayRequest.relayData.forwarder, "Forwarder is not trusted");
+        require(address(_trustedForwarder) == relayRequest.relayData.forwarder, "Forwarder is not trusted");
         GsnEip712Library.verifyForwarderTrusted(relayRequest);
     }
 
     /*
      * modifier to be used by recipients as access control protection for preRelayedCall & postRelayedCall
      */
     modifier relayHubOnly() {
-        require(msg.sender == getHubAddr(), "Function can only be called by RelayHub");
+        require(msg.sender == getHubAddr(), "can only be called by RelayHub");
         _;
     }
 
     function setRelayHub(IRelayHub hub) public onlyOwner {
         relayHub = hub;
     }
 
-    function setTrustedForwarder(IForwarder forwarder) public onlyOwner {
-        trustedForwarder = forwarder;
+    function setTrustedForwarder(address forwarder) public onlyOwner {
+        _trustedForwarder = forwarder;
     }
 
-    // check current deposit on relay hub.
-    function getRelayHubDeposit() public view returns (uint256) {
+    function trustedForwarder() external view returns (address) {
+        return _trustedForwarder;
+    }
+
+    /// check current deposit on relay hub.
+    function getRelayHubDeposit() external view returns (uint256) {
         return relayHub.balanceOf(address(this));
     }
 
-    // withdraw deposit from relayHub
+    // any eth moved into the paymaster is transferred as a deposit.
+    // This way, we don't need to understand the RelayHub API in order to replenish
+    // the paymaster.
+    function() external payable {
+        require(address(relayHub) != address(0), "relay hub address not set");
+        relayHub.depositFor.value(msg.value)(address(this));
+    }
+
+    /// withdraw deposit from relayHub
     function withdrawRelayHubDepositTo(uint256 amount, address target) public onlyOwner {
         relayHub.withdraw(amount, target);
     }

contracts/gsn/forwarder/IForwarder.sol

@@ -10,14 +10,20 @@ contract IForwarder {
         uint256 gas;
         uint256 nonce;
         bytes data;
+        uint256 validUntil;
     }
 
+    event DomainRegistered(bytes32 indexed domainSeparator, bytes domainValue);
+
+    event RequestTypeRegistered(bytes32 indexed typeHash, string typeStr);
+
     function getNonce(address from) external view returns (uint256);
 
     /**
      * verify the transaction would execute.
      * validate the signature and the nonce of the request.
      * revert if either signature or nonce are incorrect.
+     * also revert if domainSeparator or requestTypeHash are not registered.
      */
     function verify(
         ForwardRequest forwardRequest,
@@ -51,8 +57,8 @@ contract IForwarder {
     /**
      * Register a new Request typehash.
      * @param typeName - the name of the request type.
-     * @param typeSuffix - anything after the generic params can be empty string (if no extra fields are needed)
-     *        if it does contain a value, then a comma is added first.
+     * @param typeSuffix - any extra data after the generic params.
+     *  (must add at least one param. The generic ForwardRequest type is always registered by the constructor)
      */
     function registerRequestType(string typeName, string typeSuffix) external;
 

contracts/gsn/interfaces/GsnTypes.sol

@@ -4,15 +4,16 @@ pragma solidity 0.4.24;
 import "../forwarder/IForwarder.sol";
 
 contract GsnTypes {
+    /// @notice gasPrice, pctRelayFee and baseRelayFee must be validated inside of the paymaster's preRelayedCall in order not to overpay
     struct RelayData {
         uint256 gasPrice;
         uint256 pctRelayFee;
         uint256 baseRelayFee;
         address relayWorker;
         address paymaster;
+        address forwarder;
         bytes paymasterData;
         uint256 clientId;
-        address forwarder;
     }
 
     //note: must start with the ForwardRequest to be an extension of the generic forwarder

contracts/gsn/interfaces/IPaymaster.sol

@@ -29,16 +29,19 @@ contract IPaymaster {
      *      note that an OOG will revert the transaction, but the paymaster already committed to pay,
      *      so the relay will get compensated, at the expense of the paymaster
      */
-    struct GasLimits {
+    struct GasAndDataLimits {
         uint256 acceptanceBudget;
         uint256 preRelayedCallGasLimit;
         uint256 postRelayedCallGasLimit;
+        uint256 calldataSizeLimit;
     }
 
     /**
-     * Return the GasLimits constants used by the Paymaster.
+     * Return the Gas Limits and msg.data max size constants used by the Paymaster.
      */
-    function getGasLimits() external view returns (GasLimits memory limits);
+    function getGasAndDataLimits() external view returns (GasAndDataLimits memory limits);
+
+    function trustedForwarder() external view returns (address);
 
     /**
      * return the relayHub of this contract.
@@ -49,7 +52,7 @@ contract IPaymaster {
      * Can be used to determine if the contract can pay for incoming calls before making any.
      * @return the paymaster's deposit in the RelayHub.
      */
-    function getRelayHubDeposit() public view returns (uint256);
+    function getRelayHubDeposit() external view returns (uint256);
 
     /**
      * Called by Relay (and RelayHub), to validate if the paymaster agrees to pay for this call.
@@ -74,7 +77,7 @@ contract IPaymaster {
      *              Note that in most cases the paymaster shouldn't try use it at all. It is always checked
      *              by the forwarder immediately after preRelayedCall returns.
      *  @param approvalData - extra dapp-specific data (e.g. signature from trusted party)
-     *  @param maxPossibleGas - based on values returned from {@link getGasLimits},
+     *  @param maxPossibleGas - based on values returned from {@link getGasAndDataLimits},
      *         the RelayHub will calculate the maximum possible amount of gas the user may be charged for.
      *         In order to convert this value to wei, the Paymaster has to call "relayHub.calculateCharge()"
      *  return:

contracts/gsn/token_paymaster/TokenPaymaster.sol

@@ -6,21 +6,25 @@ import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol";
 import "../BasePaymaster.sol";
 import "./IUniswapV2Router02.sol";
 import "../../upgradeable_contracts/GSNForeignERC20Bridge.sol";
+import "../../upgradeable_contracts/Claimable.sol";
 
-contract TokenPaymaster is BasePaymaster {
-    address private token;
+contract TokenPaymaster is BasePaymaster, Claimable {
+    ERC20 private token;
     IUniswapV2Router02 private router;
     address private bridge;
 
     address[] private tokenWethPair = new address[](2);
     uint256 public postGasUsage = 300000;
+    // Default value from BasePaymaster, may be changed later
+    // if we need to increase number of signatures for bridge contract
+    uint256 public calldataSizeLimit = 10500;
 
     constructor(address _relayHub, address _forwarder, address _token, address _router, address _bridge) public {
         _setOwner(msg.sender);
-        relayHub = IRelayHub(_relayHub);
-        trustedForwarder = IForwarder(_forwarder);
+        setRelayHub(IRelayHub(_relayHub));
+        setTrustedForwarder(_forwarder);
 
-        token = _token;
+        token = ERC20(_token);
         router = IUniswapV2Router02(_router);
         bridge = _bridge;
 
@@ -29,7 +33,7 @@ contract TokenPaymaster is BasePaymaster {
     }
 
     function setToken(address t) external onlyOwner {
-        token = t;
+        token = ERC20(t);
     }
 
     function setRouter(IUniswapV2Router02 r) external onlyOwner {
@@ -40,33 +44,33 @@ contract TokenPaymaster is BasePaymaster {
         bridge = b;
     }
 
-    function versionPaymaster() external view returns (string memory) {
-        return "2.0.0+opengsn.tokengsn.ipaymaster";
+    function setPostGasUsage(uint256 gasUsage) external onlyOwner {
+        postGasUsage = gasUsage;
     }
 
-    function() external payable {
-        require(address(relayHub) != address(0), "relay hub address not set");
-        relayHub.depositFor.value(msg.value)(address(this));
+    function setCalldataSizeLimit(uint256 sizeLimit) external onlyOwner {
+        calldataSizeLimit = sizeLimit;
+    }
+
+    function versionPaymaster() external view returns (string memory) {
+        return "2.2.0+opengsn.bridgetokengsn.ipaymaster";
     }
 
     function deposit() external payable {
         require(address(relayHub) != address(0), "relay hub address not set");
         relayHub.depositFor.value(msg.value)(address(this));
     }
 
-    function getGasLimits() external view returns (IPaymaster.GasLimits memory limits) {
+    function getGasAndDataLimits() external view returns (IPaymaster.GasAndDataLimits memory limits) {
         return
-            IPaymaster.GasLimits(
+            IPaymaster.GasAndDataLimits(
                 PAYMASTER_ACCEPTANCE_BUDGET,
                 PRE_RELAYED_CALL_GAS_LIMIT,
-                postGasUsage // maximum postRelayedCall gasLimit
+                postGasUsage, // maximum postRelayedCall gasLimit
+                calldataSizeLimit
             );
     }
 
-    function erc20() internal view returns (ERC20) {
-        return ERC20(token);
-    }
-
     function readBytes32(bytes memory b, uint256 index) internal pure returns (bytes32 res) {
         require(b.length >= index + 32, "data too short");
         assembly {
@@ -79,7 +83,7 @@ contract TokenPaymaster is BasePaymaster {
         bytes signature,
         bytes approvalData,
         uint256 maxPossibleGas
-    ) public returns (bytes memory context, bool revertOnRecipientRevert) {
+    ) public relayHubOnly returns (bytes memory context, bool revertOnRecipientRevert) {
         (signature, approvalData);
         _verifyForwarder(relayRequest);
         bytes memory reqData = relayRequest.request.data;
@@ -108,12 +112,9 @@ contract TokenPaymaster is BasePaymaster {
         return a < b ? a : b;
     }
 
-    function setPostGasUsage(uint256 gasUsage) external onlyOwner {
-        postGasUsage = gasUsage;
-    }
-
     function postRelayedCall(bytes context, bool success, uint256 gasUseWithoutPost, GsnTypes.RelayData relayData)
         public
+        relayHubOnly
     {
         (success);
         // Extract data from context
@@ -137,7 +138,7 @@ contract TokenPaymaster is BasePaymaster {
         uint256 chargeWei = relayHub.calculateCharge(min(gasUseWithoutPost + postGasUsage, maxPossibleGas), relayData);
 
         // Uniswap
-        require(erc20().approve(address(router), maxTokensFee), "approve failed");
+        require(token.approve(address(router), maxTokensFee), "approve failed");
         // NOTE: Received eth automatically converts to relayhub deposit
         uint256 spentTokens = router.swapTokensForExactETH(
             chargeWei,
@@ -149,7 +150,11 @@ contract TokenPaymaster is BasePaymaster {
 
         // Send rest of tokens to user
         if (spentTokens < maxTokensFee) {
-            require(erc20().transfer(to, maxTokensFee - spentTokens));
+            require(token.transfer(to, maxTokensFee - spentTokens));
         }
     }
+
+    function claimTokens(address _token, address _to) external onlyOwner {
+        claimValues(_token, _to);
+    }
 }

contracts/libraries/Message.sol

@@ -15,7 +15,7 @@ library Message {
     // offset 32: 20 bytes :: address - recipient address
     // offset 52: 32 bytes :: uint256 - value
     // offset 84: 32 bytes :: bytes32 - transaction hash
-    // offset 104: 20 bytes :: address - contract address to prevent double spending
+    // offset 116: 20 bytes :: address - contract address to prevent double spending
 
     // mload always reads 32 bytes.
     // so we can and have to start reading recipient at offset 20 instead of 32.

contracts/mocks/ForeignBridgeErcToNativeMock.sol → contracts/mocks/XDaiForeignBridgeMock.sol

@@ -1,8 +1,8 @@
 pragma solidity 0.4.24;
 
-import "../upgradeable_contracts/erc20_to_native/ForeignBridgeErcToNative.sol";
+import "../upgradeable_contracts/erc20_to_native/XDaiForeignBridge.sol";
 
-contract ForeignBridgeErcToNativeMock is ForeignBridgeErcToNative {
+contract XDaiForeignBridgeMock is XDaiForeignBridge {
     /**
      * @dev Tells the address of the DAI token in the Ganache Testchain.
      */

contracts/upgradeable_contracts/GSNForeignERC20Bridge.sol

@@ -36,7 +36,8 @@ contract GSNForeignERC20Bridge is BasicForeignBridge, ERC20Bridge, BaseRelayReci
     * as a commission
     */
     function executeSignaturesGSN(bytes message, bytes signatures, uint256 maxTokensFee) external {
-        require(msg.sender == addressStorage[TRUSTED_FORWARDER], "invalid forwarder");
+        // Allow only forwarder calls
+        require(isTrustedForwarder(msg.sender), "invalid forwarder");
         Message.hasEnoughValidSignatures(message, signatures, validatorContract(), false);
 
         address recipient;
@@ -59,12 +60,9 @@ contract GSNForeignERC20Bridge is BasicForeignBridge, ERC20Bridge, BaseRelayReci
     function onExecuteMessageGSN(address recipient, uint256 amount, uint256 fee) internal returns (bool) {
         addTotalExecutedPerDay(getCurrentDay(), amount);
         // Send maxTokensFee to paymaster
-        uint256 unshiftMaxFee = _unshiftValue(fee);
-        bool first = erc20token().transfer(addressStorage[PAYMASTER], unshiftMaxFee);
-
-        // Send rest of tokens to user
-        uint256 unshiftLeft = _unshiftValue(amount - fee);
-        bool second = erc20token().transfer(recipient, unshiftLeft);
+        ERC20 token = erc20token();
+        bool first = token.transfer(addressStorage[PAYMASTER], fee);
+        bool second = token.transfer(recipient, amount - fee);
 
         return first && second;
     }

contracts/upgradeable_contracts/InterestReceiverStakeBuyback.sol

@@ -26,14 +26,6 @@ contract InterestReceiverStakeBuyback is InterestReceiverBase {
         // (min received %) * (amount / 1 DAI) * (STAKE per 1 DAI)
         uint256 minAmount = (minReceivedFraction * amount * uniswapRouterV2.getAmountsOut(1 ether, path)[2]) / 10**36;
 
-        bytes memory data = abi.encodeWithSelector(
-            uniswapRouterV2.swapExactTokensForTokens.selector,
-            amount,
-            minAmount,
-            path,
-            burnAddress,
-            now
-        );
-        address(uniswapRouterV2).call(data);
+        uniswapRouterV2.swapExactTokensForTokens(amount, minAmount, path, burnAddress, now);
     }
 }

contracts/upgradeable_contracts/InterestReceiverSwapToETH.sol

@@ -22,15 +22,7 @@ contract InterestReceiverSwapToETH is InterestReceiverBase {
         // (min received %) * (amount / 1 DAI) * (ETH per 1 DAI)
         uint256 minAmount = (minReceivedFraction * amount * uniswapRouterV2.getAmountsOut(1 ether, path)[1]) / 10**36;
 
-        bytes memory data = abi.encodeWithSelector(
-            uniswapRouterV2.swapExactTokensForETH.selector,
-            amount,
-            minAmount,
-            path,
-            address(this),
-            now
-        );
-        address(uniswapRouterV2).call(data);
+        uniswapRouterV2.swapExactTokensForETH(amount, minAmount, path, address(this), now);
     }
 
     /**