Metaswap

Description

@nomiclabs/buidler/console.sol is imported in a few contracts that don’t use its functionality.

Examples

code/contracts/adapters/WethAdapter.sol:L6

import "@nomiclabs/buidler/console.sol";

code/contracts/MetaSwap.sol:L3

import "@nomiclabs/buidler/console.sol";

Recommendation

Reducing code when possible is always a win. Consider removing these imports.

Description

Spender uses a fallback() function to receive ether from trades:

code/contracts/Spender.sol:L12-L13

/// @dev Receives ether from swaps
fallback() external payable {}

If only simple transfers are expected (with no payload), receive() is probably the more appropriate choice.

Recommendation

Consider using receive() instead.

Description

MetaSwap.swap() should have a reentrancy guard.

The adapters use this general process:

1. Collect the from token (or ether) from the user.
2. Execute the trade.
3. Transfer the contract’s balance of tokens (from and to) and ether to the user.

If an attacker is able to reenter swap() before step 3, they can execute their own trade using the same tokens and get all the tokens for themselves.

This is partially mitigated by the check against amountTo in CommonAdapter, but note that the amountTo typically allows for slippage, so it may still leave room for an attacker to siphon off some amount while still returning the required minimum to the user.

code/contracts/adapters/CommonAdapter.sol:L57-L62

// Transfer remaining balance of tokenTo to sender
if (address(tokenTo) != Constants.ETH) {
    uint256 balance = tokenTo.balanceOf(address(this));
    require(balance >= amountTo, "INSUFFICIENT_AMOUNT");
    _transfer(tokenTo, balance, recipient);
} else {

Examples

As an example of how this could be exploited, 0x supports an “EIP1271Wallet” signature type, which invokes an external contract to check whether a trade is allowed. A malicious maker might front run the swap to reduce their inventory. This way, the taker is sending more of the taker asset than necessary to MetaSwap. The excess can be stolen by the maker during the EIP1271 call.

Recommendation

Use a simple reentrancy guard, such as OpenZeppelin’s ReentrancyGuard to prevent reentrancy in MetaSwap.swap(). It might seem more obvious to put this check in Spender.swap(), but the Spender contract intentionally does not use any storage to avoid interference between different adapters.

Description

The purpose of the MetaSwap contract is to save users gas costs when dealing with a number of different aggregators. They can just approve() their tokens to be spent by MetaSwap (or in a later architecture, the Spender contract). They can then perform trades with all supported aggregators without having to reapprove anything.

A downside to this design is that a malicious (or buggy) adapter has access to a large collection of valuable assets. Even a user who has diligently checked all existing adapter code before interacting with MetaSwap runs the risk of having their funds intercepted by a new malicious adapter that’s added later.

Recommendation

There are a number of designs that could be used to mitigate this type of attack. After discussion and iteration with the client team, we settled on a pattern where the MetaSwap contract is the only contract that receives token approval. It then moves tokens to the Spender contract before that contract DELEGATECALLs to the appropriate adapter. In this model, newly added adapters shouldn’t be able to access users’ funds.

Description

MetaSwap owners can front-run users to swap an adapter implementation. This could be used by a malicious or compromised owner to steal from users.

Because adapters are DELEGATECALLed, they can modify storage. This means any adapter can overwrite the logic of another adapter, regardless of what policies are put in place at the contract level. Users must fully trust every adapter because just one malicious adapter could change the logic of all other adapters.

Recommendation

At a minimum, disallow modification of existing adapters. Instead, simply add new adapters and disable the old ones. (They should be deleted, but the aggregator IDs of deleted adapters should never be reused.)

This is, however, insufficient. A new malicious adapter could still overwrite the adapter mapping to modify existing adapters. To fully address this issue, the adapter registry should be in a separate contract. Through discussion and iteration with the client team, we settled on the following pattern:

1. MetaSwap contains the adapter registry. It calls into a new Spender contract.
2. The Spender contract has no storage at all and is just used to DELEGATECALL to the adapter contracts.

Description

WethAdapter does some arithmetic to keep track of how much ether is being provided as a fee versus as funds that should be transferred into WETH:

code/contracts/adapters/WethAdapter.sol:L41-L59

// Some aggregators require ETH fees
uint256 fee = msg.value;

if (address(tokenFrom) == Constants.ETH) {
    // If tokenFrom is ETH, msg.value = fee + amountFrom (total fee could be 0)
    require(amountFrom <= fee, "MSG_VAL_INSUFFICIENT");
    fee -= amountFrom;
    // Can't deal with ETH, convert to WETH
    IWETH weth = getWETH();
    weth.deposit{value: amountFrom}();
    _approveSpender(weth, spender, amountFrom);
} else {
    // Otherwise capture tokens from sender
    // tokenFrom.safeTransferFrom(recipient, address(this), amountFrom);
    _approveSpender(tokenFrom, spender, amountFrom);
}

// Perform the swap
aggregator.functionCallWithValue(abi.encodePacked(method, data), fee);

This code can be simplified by using address(this).balance instead.

Recommendation

Consider something like the following code instead:

if (address(tokenFrom) == Constants.ETH) {
    getWETH().deposit{value: amountFrom}(); // will revert if the contract has an insufficient balance
    _approveSpender(weth, spender, amountFrom);
} else {
    tokenFrom.safeTransferFrom(recipient, address(this), amountFrom);
    _approveSpender(tokenFrom, spender, amountFrom);
}

// Send the remaining balance as the fee.
aggregator.functionCallWithValue(abi.encodePacked(method, data), address(this).balance);

Aside from being a little simpler, this way of writing the code makes it obvious that the full balance is being properly consumed. Part is traded, and the rest is sent as a fee.

Description

MetaSwap doesn’t check that an adapter exists before calling into Spender:

code/contracts/MetaSwap.sol:L87-L100

function swap(
    string calldata aggregatorId,
    IERC20 tokenFrom,
    uint256 amount,
    bytes calldata data
) external payable whenNotPaused nonReentrant {
    Adapter storage adapter = adapters[aggregatorId];

    if (address(tokenFrom) != Constants.ETH) {
        tokenFrom.safeTransferFrom(msg.sender, address(spender), amount);
    }

    spender.swap{value: msg.value}(
        adapter.addr,

Then Spender performs the check and reverts if it receives address(0).

code/contracts/Spender.sol:L15-L16

function swap(address adapter, bytes calldata data) external payable {
    require(adapter != address(0), "ADAPTER_NOT_SUPPORTED");

It can be difficult to decide where to put a check like this, especially when the operation spans multiple contracts. Arguments can be made for either choice (or even duplicating the check), but as a general rule it’s a good idea to avoid passing invalid parameters internally. Checking for adapter existence in MetaSwap.swap() is a natural place to do input validation, and it means Spender can have a simpler model where it trusts its inputs (which always come from MetaSwap).

Recommendation

Drop the check from Spender.swap() and perform the check instead in MetaSwap.swap().

Description

MetaSwap.swapUsingGasToken() allows users to make use of gas tokens held by the MetaSwap contract itself to reduce the gas cost of trades.

This mechanism is unsafe because any tokens held by the contract can be used by an attacker for other purposes.

Examples

If gas tokens are held by MetaSwap, an attacker can use them all up by performing a gas-heavy operation via a call to swapUsingGasToken(). For example, an attacker could create a token called EVIL and establish an ETH/EVIL pair on Uniswap. The implementation for EVIL’s transfer() or transferFrom() method could do arbitrary gas-heavy operations. Finally, the attacker can invoke swapUsingGasToken(), using the Uniswap adapter and ETH/EVIL as the trading pair. When EVIL’s transfer functions are called, they can consume a large amount of gas. When the operation is complete, swapUsingGasTokens() will burn as much CHI gas tokens as possible to help offset the gas use.

An attack could also be made by using an existing token that makes external calls (e.g. an ERC777 token) or a mechanism in an aggregated exchange that makes external calls (e.g. wallet signatures in 0x).

Recommendation

The simplest way to avoid this vulnerability is to never transfer CHI gas tokens to MetaSwap at all. An alternative would be to only allow gas tokens to be used by approved transactions from the MetaSwap API. A possible mechanism for that would be to require a signature from the MetaSwap API. If such a signature were only provided in known-good situations (which are admittedly hard to define), it wouldn’t be possible for an attacker to misuse the tokens.

Most ERC20-compatible tokens can be used with the FeeDistributor contract, but it’s wise to document some assumptions made by the contract:

• Token balances will not be too big (relative to the number of shares). Specifically, the total number of token units received by the contract must be able to be multiplied by the largest share amount held by a recipient.
• Token balances will not be too small (relative to share amounts). It’s impossible to divide a balance of 1 among more than 1 recipient. To be safe, it would be good to make sure that no one cares about losing less than totalShares token units. For example, if there are 1,000,000 total shares, an asset like ether would not be a problem because 1,000,000 wei is a trivial amount.
• Token balances will not decrease without an explicit transfer. The contract makes the assumption that it can always compute the total received tokens by adding tokenBalance(token) and _totalWithdrawn[token]. This is not the case if the token balance can be manipulated externally.

Description

The current code does some fairly complex and redundant calculations during withdrawal to keep track of various pieces of state. In particular, the pair of _available[recipient][token] and _totalOnLastUpdate[recipient][token] is difficult to describe and reason about.

Recommendation

For a given token and recipient, we recommend instead just tracking how much has already been withdrawn. The rest can be easily calculated:

function earned(IERC20 token, address recipient) public view returns (uint256) {
    uint256 totalReceived = tokenBalance(token).add(_totalWithdrawn[token]);
    return totalReceived.mul(shares[recipient]).div(totalShares);
}

function available(IERC20 token, address recipient) public view returns (uint256) {
    return earned(token, recipient).sub(_withdrawn[token][recipient]);
}

function withdraw(IERC20[] calldata tokens) external {
    for (uint256 i = 0; i < tokens.length; i++) {
        IERC20 token = tokens[i];
        uint256 amount = available(token, msg.sender);

        _withdrawn[token][msg.sender] += amount;
        _totalWithdrawn[token] += amount;
        _transfer(token, msg.sender, amount);
    }
    emit Withdrawal(tokens, msg.sender);
}

This code is easier to reason about:

• It’s easy to see that withdrawn[token][msg.sender] is correct because it’s only increased when there’s a corresponding transfer.
• It’s easy to see that _totalWithdrawn[token] is correct for the same reason.
• It’s easy to see that earned() is correct under standard assumptions about ERC20 balances.
• It’s easy to see that available() is correct, as it’s just the earned amount less the already-withdrawn amount.
• Remainders are better handled. If 1 token unit is available and you own half the shares, nothing happens on withdrawal, and if there are later 2 token units available, you can withdraw 1. (Under the previous code, if you tried to withdraw when 1 token unit was available, you would be unable to withdraw when 2 were available.)