0x v3 Exchange

Description

When a transaction reaches the required number of confirmations in confirmTransaction(), its confirmation time is recorded:

code/contracts/multisig/contracts/src/MultiSigWalletWithTimeLock.sol:L86-L100

/// @dev Allows an owner to confirm a transaction.
/// @param transactionId Transaction ID.
function confirmTransaction(uint256 transactionId)
    public
    ownerExists(msg.sender)
    transactionExists(transactionId)
    notConfirmed(transactionId, msg.sender)
    notFullyConfirmed(transactionId)
{
    confirmations[transactionId][msg.sender] = true;
    emit Confirmation(msg.sender, transactionId);
    if (isConfirmed(transactionId)) {
        _setConfirmationTime(transactionId, block.timestamp);
    }
}

Before the time lock has elapsed and the transaction is executed, any of the owners that originally confirmed the transaction can revoke their confirmation via revokeConfirmation():

code/contracts/multisig/contracts/src/MultiSigWallet.sol:L249-L259

/// @dev Allows an owner to revoke a confirmation for a transaction.
/// @param transactionId Transaction ID.
function revokeConfirmation(uint256 transactionId)
    public
    ownerExists(msg.sender)
    confirmed(transactionId, msg.sender)
    notExecuted(transactionId)
{
    confirmations[transactionId][msg.sender] = false;
    emit Revocation(msg.sender, transactionId);
}

Immediately after, that owner can call confirmTransaction() again, which will reset the confirmation time and thus the time lock.

This is especially troubling in the case of a single compromised key, but it’s also an issue for disagreement among owners, where any m of the n owners should be able to execute transactions but could be blocked.

Mitigations

Only an owner can do this, and that owner has to be part of the group that originally confirmed the transaction. This means the malicious owner may have to front run the others to make sure they’re in that initial confirmation set.

Even once a malicious owner is in position to execute this perpetual delay, they need to call revokeConfirmation() and confirmTransaction() again each time. Another owner can attempt to front the attacker and execute their own confirmTransaction() immediately after the revokeConfirmation() to regain control.

Recommendation

There are several ways to address this, but to best preserve the original MultiSigWallet semantics, once a transaction has reached the required number of confirmations, it should be impossible to revoke confirmations. In the original implementation, this is enforced by immediately executing the transaction when the final confirmation is received.

Description

The signature types Wallet, Validator, and EIP1271Wallet require explicit validation to authorize each action performed on a given order. This means that if an order was signed using one of these methods, the Exchange must perform a validation step on the signature each time the order is submitted for a partial fill. In contrast, the other canonical signature types (EIP712, EthSign, and PreSigned) are only required to be validated by the Exchange on the order’s first fill; subsequent fills take the order’s existing fill amount as implicit validation that the order has a valid, published signature.

This re-validation step for Wallet, Validator, and EIP1271Wallet signatures is intended to facilitate their use with contracts whose validation depends on some state that may change over time. For example, a validating contract may call into a price feed and determine that some order is invalid if its price deviates from some expected range. In this case, the repeated validation allows 0x users to make orders with custom fill conditions which are evaluated at run-time.

We found that if the sender provides the contract with an invalid signature after the order in question has already been partially filled, the regular validation check required for Wallet, Validator, and EIP1271Wallet signatures can be bypassed entirely.

Examples

Signature validation takes place in MixinExchangeCore._assertFillableOrder. A signature is only validated if it passes the following criteria:

code/contracts/exchange/contracts/src/MixinExchangeCore.sol:L372-L381

// Validate either on the first fill or if the signature type requires
// regular validation.
address makerAddress = order.makerAddress;
if (orderInfo.orderTakerAssetFilledAmount == 0 ||
    _doesSignatureRequireRegularValidation(
        orderInfo.orderHash,
        makerAddress,
        signature
    )
) {

In effect, signature validation only occurs if:

• orderInfo.orderTakerAssetFilledAmount == 0 OR
• _doesSignatureRequireRegularValidation(orderHash, makerAddress, signature)

If an order is partially filled, the first condition will evaluate to false. Then, that order’s signature will only be validated if _doesSignatureRequireRegularValidation evaluates to true:

code/contracts/exchange/contracts/src/MixinSignatureValidator.sol:L183-L206

function _doesSignatureRequireRegularValidation(
    bytes32 hash,
    address signerAddress,
    bytes memory signature
)
    internal
    pure
    returns (bool needsRegularValidation)
{
    // Read the signatureType from the signature
    SignatureType signatureType = _readSignatureType(
        hash,
        signerAddress,
        signature
    );

    // Any signature type that makes an external call needs to be revalidated
    // with every partial fill
    needsRegularValidation =
        signatureType == SignatureType.Wallet ||
        signatureType == SignatureType.Validator ||
        signatureType == SignatureType.EIP1271Wallet;
    return needsRegularValidation;
}

The SignatureType returned from _readSignatureType is directly cast from the final byte of the passed-in signature. Any value that does not cast to Wallet, Validator, and EIP1271Wallet will cause _doesSignatureRequireRegularValidation to return false, skipping validation.

The result is that an order whose signature requires regular validation can be forced to skip validation if it has been partially filled, by passing in an invalid signature.

Recommendation

There are a few options for remediation:

1. Have the Exchange validate the provided signature every time an order is filled.
2. Record the first seen signature type or signature hash for each order, and check that subsequent actions are submitted with a matching signature.

The first option requires the fewest changes, and does not require storing additional state. While this does mean some additional cost validating subsequent signatures, we feel the increase in flexibility is well worth it, as a maker could choose to create multiple valid signatures for use across different order books.

Description

Once a transaction has been confirmed in the AssetProxyOwner, it cannot be executed until a lock period has passed. During that time, any change to the number of required confirmations will cause this transaction to no longer be executable.

If the number of required confirmations was decreased, then one or more owners will have to revoke their confirmation before the transaction can be executed.

If the number of required confirmations was increased, then additional owners will have to confirm the transaction, and when the new required number of confirmations is reached, a new confirmation time will be recorded, and thus the time lock will restart.

Similarly, if an owner that had previously confirmed the transaction is replaced, the number of confirmations will drop for existing transactions, and they will need to be confirmed again.

This is not disastrous, but it’s almost certainly unintended behavior and may make it difficult to make changes to the multisig owners and parameters.

Examples

executeTransaction() requires that at the time of execution, the transaction is confirmed:

code/contracts/multisig/contracts/src/AssetProxyOwner.sol:L115-L118

function executeTransaction(uint256 transactionId)
    public
    notExecuted(transactionId)
    fullyConfirmed(transactionId)

isConfirmed() checks for exact equality with the number of required confirmations. Having too many confirmations is just as bad as too few:

code/contracts/multisig/contracts/src/MultiSigWallet.sol:L318-L335

/// @dev Returns the confirmation status of a transaction.
/// @param transactionId Transaction ID.
/// @return Confirmation status.
function isConfirmed(uint256 transactionId)
    public
    view
    returns (bool)
{
    uint256 count = 0;
    for (uint256 i = 0; i < owners.length; i++) {
        if (confirmations[transactionId][owners[i]]) {
            count += 1;
        }
        if (count == required) {
            return true;
        }
    }
}

If additional confirmations are required to reconfirm a transaction, that resets the time lock:

code/contracts/multisig/contracts/src/MultiSigWalletWithTimeLock.sol:L86-L100

/// @dev Allows an owner to confirm a transaction.
/// @param transactionId Transaction ID.
function confirmTransaction(uint256 transactionId)
    public
    ownerExists(msg.sender)
    transactionExists(transactionId)
    notConfirmed(transactionId, msg.sender)
    notFullyConfirmed(transactionId)
{
    confirmations[transactionId][msg.sender] = true;
    emit Confirmation(msg.sender, transactionId);
    if (isConfirmed(transactionId)) {
        _setConfirmationTime(transactionId, block.timestamp);
    }
}

Recommendation

As in issue 6.1, the semantics of the original MultiSigWallet were that once a transaction is fully confirmed, it’s immediately executed. The time lock means this is no longer possible, but it is possible to record that the transaction is confirmed and never allow this to change. In fact, the confirmation time already records this. Once the confirmation time is non-zero, a transaction should always be considered confirmed.

Description

In MixinTransactions, executeTransaction() and batchExecuteTransactions() do not have the nonReentrant modifier. Because of that, it is possible to execute nested transactions or call these functions during other reentrancy attacks on the exchange. The reason behind that decision is to be able to call functions with nonReentrant modifier as delegated transactions.

Nested transactions are partially prevented with a separate check that does not allow transaction execution if the exchange is currently in somebody else’s context:

code/contracts/exchange/contracts/src/MixinTransactions.sol:L155-L162

// Prevent `executeTransaction` from being called when context is already set
address currentContextAddress_ = currentContextAddress;
if (currentContextAddress_ != address(0)) {
    LibRichErrors.rrevert(LibExchangeRichErrors.TransactionInvalidContextError(
        transactionHash,
        currentContextAddress_
    ));
}

This check still leaves some possibility of reentrancy. Allowing that behavior is dangerous and may create possible attack vectors in the future.

Recommendation

Add a new modifier to executeTransaction() and batchExecuteTransactions() which is similar to nonReentrant but uses different storage slot.

Description

The market buy/sell functions gather a list of orders together for the same asset and try to fill them in order until a target amount has been traded.

These functions use MixinWrapperFunctions._fillOrderNoThrow() to attempt to fill each order but ignore failures. This way, if one order is unfillable for some reason, the overall market order can still succeed by filling other orders.

Orders can still force _fillOrderNoThrow() to revert by using an external contract for signature validation and having that contract consume all available gas.

This makes it possible to advertise a “poison” order for a low price that will block all market orders from succeeding. It’s reasonable to assume that off-chain order books will automatically include the best prices when constructing market orders, so this attack would likely be quite effective. Note that such an attack costs the attacker nothing because all they need is an on-chain contract that consumers all available gas (maybe via an assert). This makes it a very appealing attack vector for, e.g., an order book that wants to temporarily disable a competitor.

Details

_fillOrderNoThrow() forwards all available gas when filling the order:

code/contracts/exchange/contracts/src/MixinWrapperFunctions.sol:L340-L348

// ABI encode calldata for `fillOrder`
bytes memory fillOrderCalldata = abi.encodeWithSelector(
    IExchangeCore(address(0)).fillOrder.selector,
    order,
    takerAssetFillAmount,
    signature
);

(bool didSucceed, bytes memory returnData) = address(this).delegatecall(fillOrderCalldata);

Similarly, when the Exchange attempts to fill an order that requires external signature validation (Wallet, Validator, or EIP1271Wallet signature types), it forwards all available gas:

code/contracts/exchange/contracts/src/MixinSignatureValidator.sol:L642

(bool didSucceed, bytes memory returnData) = verifyingContractAddress.staticcall(callData);

If the verifying contract consumes all available gas, it can force the overall transaction to revert.

Pedantic Note

Technically, it’s impossible to consume all remaining gas when called by another contract because the EVM holds back a small amount, but even at the block gas limit, the amount held back would be insufficient to complete the transaction.

Recommendation

Constrain the gas that is forwarded during signature validation. This can be constrained either as a part of the signature or as a parameter provided by the taker.

Description

Calls to matchOrders() are made to extract profit from the price difference between two opposite orders: left and right.

code/contracts/exchange/contracts/src/MixinMatchOrders.sol:L106-L111

function matchOrders(
    LibOrder.Order memory leftOrder,
    LibOrder.Order memory rightOrder,
    bytes memory leftSignature,
    bytes memory rightSignature
)

The caller only pays protocol and transaction fees, so it’s almost always profitable to front run every call to matchOrders(). That would lead to gas auctions and would make matchOrders() difficult to use.

Recommendation

Consider adding a commit-reveal scheme to matchOrders() to stop front running altogether.

Description

If the owner calls either of these functions, the resulting delegatecall can pass onlyOwner modifiers even if the transaction signer is not the owner. This is because, regardless of the contextAddress set through _executeTransaction, the onlyOwner modifier checks msg.sender.

Examples

1. _executeTransaction sets the context address to the signer address, which is not msg.sender in this case:

   code/contracts/exchange/contracts/src/MixinTransactions.sol:L102-L104

   // Set the current transaction signer
   address signerAddress = transaction.signerAddress;
   _setCurrentContextAddressIfRequired(signerAddress, signerAddress);
   

2. The resulting delegatecall could target an admin function like this one:

   code/contracts/exchange/contracts/src/MixinAssetProxyDispatcher.sol:L38-L61

   /// @dev Registers an asset proxy to its asset proxy id.
   ///      Once an asset proxy is registered, it cannot be unregistered.
   /// @param assetProxy Address of new asset proxy to register.
   function registerAssetProxy(address assetProxy)
       external
       onlyOwner
   {
       // Ensure that no asset proxy exists with current id.
       bytes4 assetProxyId = IAssetProxy(assetProxy).getProxyId();
       address currentAssetProxy = _assetProxies[assetProxyId];
       if (currentAssetProxy != address(0)) {
           LibRichErrors.rrevert(LibExchangeRichErrors.AssetProxyExistsError(
               assetProxyId,
               currentAssetProxy
           ));
       }
   
       // Add asset proxy and log registration.
       _assetProxies[assetProxyId] = assetProxy;
       emit AssetProxyRegistered(
           assetProxyId,
           assetProxy
       );
   }
   

3. The onlyOwner modifier does not check the context address, but checks msg.sender:

   code/contracts/utils/contracts/src/Ownable.sol:L35-L45

   function _assertSenderIsOwner()
       internal
       view
   {
       if (msg.sender != owner) {
           LibRichErrors.rrevert(LibOwnableRichErrors.OnlyOwnerError(
               msg.sender,
               owner
           ));
       }
   }
   

Recommendation

Add a check to _executeTransaction that prevents the owner from calling this function.

Description

In order to cancel an order, an authorized address (maker or sender) calls cancelOrder(LibOrder.Order memory order). When calling that function, all data for the order becomes visible to everyone on the network, and anyone can fill that order before it’s canceled.

Usually, a maker is canceling an order because it’s no longer profitable for them, so an attacker is likely to profit from front running the cancelOrder() transaction.

Recommendation

Make it impossible to front run order cancelation by providing less data to the cancelOrder() function such that this data is insufficient to execute the order.

Description

ZeroExTransactions are meta transactions supported by the Exchange. They do not require that they are executed with a specific amount of gas, so the transaction relayer can choose how much gas to provide. By choosing a low gas limit, a relayer can affect the outcome of the transaction.

A ZeroExTransaction specifies a signer, an expiration, and call data for the transaction:

code/contracts/exchange-libs/contracts/src/LibZeroExTransaction.sol:L41-L47

struct ZeroExTransaction {
    uint256 salt;                   // Arbitrary number to ensure uniqueness of transaction hash.
    uint256 expirationTimeSeconds;  // Timestamp in seconds at which transaction expires.
    uint256 gasPrice;               // gasPrice that transaction is required to be executed with.
    address signerAddress;          // Address of transaction signer.
    bytes data;                     // AbiV2 encoded calldata.
}

In MixinTransactions._executeTransaction(), all available gas is forwarded in the delegate call, and the transaction is marked as executed:

code/contracts/exchange/contracts/src/MixinTransactions.sol:L107-L108

transactionsExecuted[transactionHash] = true;
(bool didSucceed, bytes memory returnData) = address(this).delegatecall(transaction.data);

Examples

A likely attack vector for this is front running a ZeroExTransaction that ultimately invokes _fillNoThrow(). In this scenario, an attacker sees the call to executeTransaction() and makes their own call with a lower gas limit, causing the order being filled to run out of gas but allowing the transaction as a whole to succeed.

If such an attack is successful, the ZeroExTransaction cannot be replayed, so the signer must produce a new signature and try again, ad infinitum.

Recommendation

Add a gasLimit field to ZeroExTransaction and forward exactly that much gas via delegatecall. (Note that you must explicitly check that sufficient gas is available because the EVM allows you to supply a gas parameter that exceeds the actual remaining gas.)

Description

MixinWrapperFunctions defines a number of functions for market buy/sell orders. These functions take a list of orders and a target asset amount to buy or sell. They fill each order in turn until the target has been reached.

These functions provide an appealing opportunity for front running because of the near-guaranteed profit to be had. This is most easily explained with an example:

1. Alice wishes to buy 10 FOO tokens. She creates a market buy order to purchase tokens first from Bob, who is selling 4 FOO tokens at $9 each, and then from Eve, who is selling 20 tokens at $10 each.
2. Eve front runs this market order with a transaction that buys all 4 FOO tokens from Bob for $9 each.
3. Alice’s transaction goes through, but because Bob’s inventory has been depleted, all 10 FOO tokens are purchased from Eve at a price of $10 each. By front running, Eve gained $4.

In a more traditional front running scheme, Alice would have just been trying to make a simple purchase of FOO tokens at $9 each, and Eve would be taking on non-trivial risk by buying them first and hoping Alice (or another buyer) would be willing to pay a higher price later.

With a market order, however, Eve’s front running is nearly risk free because she knows the market order already commits Alice to buying at the higher price.

Recommendation

For the most part, traders will simply have to understand the risks of market orders and take care to only authorize trades they will be happy with.

That said, each order in a market order could specify a maximum quantity, e.g. “I want 10 FOO tokens, and I’m willing to buy up to 10 from Bob but only up to 5 from Eve.” This would limit the trader’s exposure to increased prices due to front running, but it would retain the convenience and efficiency of market orders.

Description

The nonReentrant and refundFinalBalance modifiers always appear together across the 0x monorepo. When used, they invariably appear with nonReentrant listed first, followed by refundFinalBalance. This specific order appears inconsequential at first glance but is actually important. The order of execution is as follows:

1. The nonReentrant modifier runs (_lockMutexOrThrowIfAlreadyLocked).
2. If refundFinalBalance had a prefix, it would run now.
3. The function itself runs.
4. The refundFinalBalance modifier runs (_refundNonZeroBalanceIfEnabled).
5. The nonReentrant modifier runs (_unlockMutex).

The fact that the refundFinalBalance modifier runs before the mutex is unlocked is of particular importance because it potentially invokes an external call, which may reenter. If the order of the two modifiers were flipped, the mutex would unlock before the external call, defeating the purpose of the reentrancy guard.

Examples

code/contracts/exchange/contracts/src/MixinExchangeCore.sol:L64-L65

nonReentrant
refundFinalBalance

Recommendation

Although the order of the modifiers is correct as-is, this pattern introduces cognitive overhead when making or reviewing changes to the 0x codebase. Because the two modifiers always appear together, it may make sense to combine the two into a single modifier where the order of operations is explicit.

Description

Several functions in LibBytes have integer overflows.

Examples

LibBytes.readBytesWithLength returns a pointer to a bytes array within an existing bytes array at some given index. The length of the nested array is added to the given index and checked against the parent array to ensure the data in the nested array is within the bounds of the parent. However, because the addition can overflow, the bounds check can be bypassed to return an array that points to data out of bounds of the parent array.

code/contracts/utils/contracts/src/LibBytes.sol:L546-L553

if (b.length < index + nestedBytesLength) {
    LibRichErrors.rrevert(LibBytesRichErrors.InvalidByteOperationError(
        LibBytesRichErrors
            .InvalidByteOperationErrorCodes.LengthGreaterThanOrEqualsNestedBytesLengthRequired,
        b.length,
        index + nestedBytesLength
    ));
}

The following functions have similar issues:

• readAddress
• writeAddress
• readBytes32
• writeBytes32
• readBytes4

Recommendation

An overflow check should be added to the function. Alternatively, because readBytesWithLength does not appear to be used anywhere in the 0x project, the function should be removed from LibBytes. Additionally, the following functions in LibBytes are also not used and should be considered for removal:

• popLast20Bytes
• writeAddress
• writeBytes32
• writeUint256
• writeBytesWithLength
• deepCopyBytes

Description

The ISignatureValidator contract defines an enum SignatureType to represent the different types of signatures recognized within the exchange. The final enum value, NSignatureTypes, is not a valid signature type. Instead, it is used by MixinSignatureValidator to check that the value read from the signature is a valid enum value. However, Solidity now includes its own check for enum casting, and casting a value over the maximum enum size to an enum is no longer possible.

Because of the added NSignatureTypes value, Solidity’s check now recognizes 0x08 as a valid SignatureType value.

Examples

The check is made here:

code/contracts/exchange/contracts/src/MixinSignatureValidator.sol:L441-L449

// Ensure signature is supported
if (uint8(signatureType) >= uint8(SignatureType.NSignatureTypes)) {
    LibRichErrors.rrevert(LibExchangeRichErrors.SignatureError(
        LibExchangeRichErrors.SignatureErrorCodes.UNSUPPORTED,
        hash,
        signerAddress,
        signature
    ));
}

Recommendation

The check should be removed, as should the SignatureTypes.NSignatureTypes value.