Allow taker to be different from sender with optional taker signature

[abandeali1]

Summary

In the current implementation of the Exchange contract, the taker of an order is always msg.sender. By optionally allowing the signature of the taker to be passed into fillOrder, we can decouple these two addresses and allow for much greater flexibility.

Motivation

This change would allow for much easier implementations of Exchange wrapper contracts. Wrapper contracts are currently impractical because they are required to hold the taker's tokens that will be exchanged. Some basic use cases for wrapper contracts include:

• creating orders that can only be filled by addresses in a whitelist
• creating orders that can only be filled with a signature from another party
• creating custom functions for conditionally filling multiple orders (similar to batchFillOrders or fillOrdersUpTo)

In addition, this change greatly simplifies logic for relayers using the matching strategy. There is now a clear distinction between the maker and taker of an order in this model, simplifying logic for calculating fees. Tokens will also be swapped directly from the maker to taker, which results in fewer intermediate state changes and lower gas costs.

Implementation

For the purposes of this proposal it makes sense to rename the taker parameter in the message format to sender, since the two are no longer necessarily the same (note: a case can be made for including both taker and sender parameters in the order message format).

Psuedocode for fillOrder:

struct Order {
    address maker;
    address sender;
    address makerToken;
    address takerToken;
    address feeRecipient;
    uint256 makerTokenAmount;
    uint256 takerTokenAmount;
    uint256 makerFee;
    uint256 takerFee;
    uint256 expirationTimestampInSec;
    uint8 v;
    bytes32 r;
    bytes32 s;     
}

struct Fill {
    uint256 takerTokenFillAmount;
    uint256 salt;
    uint8 v;
    bytes32 r;
    bytes32 s;
}

mapping (bytes32 => uint256) filled;

function fillOrder(Order memory order, Fill memory fill)
    public
    returns (uint256 takerTokenFilledAmount)
{
    require(order.sender == msg.sender || order.sender == address(0));

    address taker;
    bytes32 fillHash;
    bytes32 orderHash = getOrderHash(order);
    
    if (fill.r == bytes32(0)) {
        taker = msg.sender;
    } else {
        fillHash = keccak256(orderHash, fill.takerTokenFillAmount, fill.salt);
        bytes32 msgHash = keccak256("\x19Ethereum Signed Message:\n32", fillHash);
        taker = ecrecover(msgHash, fill.v, fill.r, fill.s);
    }
    
    ... // remaining fill logic

    if (fillHash != bytes32(0)) {
        filled[fillHash] = takerTokenFilledAmount;  // the filled mapping must be updated an extra time to prevent replay attacks
    }
}

function getOrderHash(Order memory order)
    internal
    returns (bytes32 orderHash)
{
    orderHash = keccak256(...) // calculate order's hash
}

[abandeali1]

The above approach is a naive solution to this issue. Realistically, the taker needs to be able to choose which method is being called in order to have the full range of functionality that a taker calling the Exchange contract directly would have. This is slightly tricky because all of the Exchange functions take different arguments (this could be solved with #16).

A slightly different approach (and replacement for #16) involves creating a new message type: a 0x transaction message. This would be represented as two (probably tightly packed) byte arrays. The first would contain the methodId and function arguments of the desired function to call, and the second would contain a signature of the hash of the data (from the taker in this case). It could look something like the following:

struct Order {
    address senderAddress;
    address makerAddress;
    address takerAddress;
    address makerTokenAddress;
    address takerTokenAddress;
    address feeRecipientAddress;
    uint256 makerTokenAmount;
    uint256 takerTokenAmount;
    uint256 makerFeeAmount;
    uint256 takerFeeAmount;
    uint256 expirationTimestamp;
    uint256 salt;
    bytes32 orderHash;
    bytes signature;
}

bytes4 public constant FILL_ORDER_METHOD_ID = bytes4(keccak256("fillOrder(Order order,uint256 takerTokenFillAmount,address taker)"));
bytes4 public constant BATCH_FILL_ORDERS_METHOD_ID = bytes4(keccak256("batchFillOrders(Order[] orders,uint256[] takerTokenFillAmounts,address taker)"));

// this is the entry point for every single function on the Exchange contract
function sendTransaction(bytes data, bytes signature)
    external
{
    address signer;
    if (signature == 0x0) {
        signer = msg.sender;
    } else {
        bytes32 txHash = keccak256(data);
        signer = ecrecovery(txHash, signature);
    }
    bytes4 methodId = getMethodId(data);
    if (methodId == FILL_ORDER_METHOD_ID) {
        (order, takerTokenFillAmount) = getFillOrderArgs(data);
        return fillOrder(order, takerTokenAmount, signer);
    } else if (methodId == BATCH_FILL_ORDERS_METHOD_ID) {
        (orders, takerTokenFillAmounts) = getBatchFillOrdersArgs(data);
        return batchFillOrders(orders, takerTokenFillAmounts, signer);
    } else if (...) {
        // add conditional for all fill/cancel derivatives
    }
}

function fillOrder(Order order, uint256 takerTokenFillAmount, address taker)
    internal
    returns (uint takerTokenFilledAmount)
{
    // validate and fill order
}

function batchFillOrders(Order[] orders, uint256[] takerTokenFillAmounts, taker)
    internal
{
    for (uint256 i = 0; i < orders.length; i++) {
        fillOrder(orders[i], takerTokenFillAmounts[i], taker);
    }
}

function getMethodId(bytes data)
    internal
    returns (bytes4 methodId)
{
    // returns first 4 bytes of data
}

function getFillOrderArgs(bytes data)
    internal
    returns (Order order, uint256 takerTokenFillAmount)
{
    // parses data and returns formatted args
}

function getBatchFillOrderArgs(bytes data) 
    internal
    returns (Order[] orders, uint256[] takerTokenFillAmounts)
{
    // parses data and returns formatted args
}

function ecrecovery(bytes32 hash, bytes signature)
    internal
    returns (address signer)
{
    // returns signer of hash
}

[dekz]

We discussed the potential for a Relayer (or anyone if the sender is not set) to mount a replay attack with this proposed improvement.

In this scenario, a Taker would sign methodId, data and submit it to the Exchange Wrapper contract.

Any listener (or malicious Relayer if sender is set) could re-submit this transaction N times up until the order is filled.

A simple replay attack prevention scheme would be to store txHash => bool in the Exchange contract and prevent the txHash from being used again. This does prevent an intentional partial fill of the same amount on the same order. We could change the signed data to method, data, nonce to allow for this.

[abandeali1]

Here is a simpler to implement version of my last comment using call instead of manually checking the methodId. In this approach, sendTransaction does not have to be the single entry point, and the passed in data does not have to be manually parsed. Need to do a gas analysis of each version.

function sendTransaction(bytes data, bytes signature)
    external
{
    bytes32 dataHash = keccak256(data);
    address signer = ecrecovery(dataHash, signature);
    require(address(this).call(data, signer, msg.sender));
}

// All public functions would have the `taker/signer` and `sender` args, which are ignored
// unless called by the Exchange address
function fillOrder(Order order, address taker, address sender)
    public
    returns (uint256  takerTokenFilledAmount)
{
    if (msg.sender != address(this) {
        taker = msg.sender;
        sender = msg.sender;
    }
    // remaining checks and fill logic
}

[ydennisy]

Hey @abandeali1 this is a great ZEIP! Have you made any decisions if/when this could be implemented?

[abandeali1]

This has already been implemented and will be included in the next upgrade (scheduled for September).

https://github.com/0xProject/0x-monorepo/blob/development/packages/contracts/src/2.0.0/protocol/Exchange/MixinTransactions.sol