Skip to content

Commit

Permalink
Merge the develop branch to the master branch, preparation to v6.0.0
Browse files Browse the repository at this point in the history
This update for the `master` branch contains the changes made to address findings discovered during a security audit:
 * [Fix] Stricter preconditions for payInterest (#623)
 * [Fix] Fix offset in comments (#624)
 * [Fix] Use fixed lower call gas limit (#627)
 * [Fix] Separate XDaiForeignBridge contract with compound and GSN support (#626)
 * [Fix] Update GSN interface (#628)
 * [Fix] Block ERC20 selectors in AMB requests (#630)
 * [Other] Bump package and contracts interfaces version prior to 6.0.0 (#629)
  • Loading branch information
akolotov authored Sep 6, 2021
2 parents b3511bf + e44f4d8 commit 908a481
Show file tree
Hide file tree
Showing 26 changed files with 1,476 additions and 624 deletions.
41 changes: 29 additions & 12 deletions contracts/gsn/BasePaymaster.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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);
Expand All @@ -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);
}
Expand Down
10 changes: 8 additions & 2 deletions contracts/gsn/forwarder/IForwarder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down
3 changes: 2 additions & 1 deletion contracts/gsn/interfaces/GsnTypes.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 8 additions & 5 deletions contracts/gsn/interfaces/IPaymaster.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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:
Expand Down
55 changes: 30 additions & 25 deletions contracts/gsn/token_paymaster/TokenPaymaster.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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);
}
}
2 changes: 1 addition & 1 deletion contracts/libraries/Message.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
*/
Expand Down
12 changes: 5 additions & 7 deletions contracts/upgradeable_contracts/GSNForeignERC20Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
10 changes: 1 addition & 9 deletions contracts/upgradeable_contracts/InterestReceiverSwapToETH.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
Loading

0 comments on commit 908a481

Please sign in to comment.