xiaoming90 - Slippage Control `trade.limit` Is Ignored By 0x Adaptor

[sherlock-admin]

xiaoming90

high

Slippage Control trade.limit Is Ignored By 0x Adaptor

Summary

The slippage control (trade.limit) is not parsed and ignored by 0x adaptor when executing a trade.

Vulnerability Detail

Note: This issue only affects the 0x adaptor. The rest of the in-scope adaptors (Curve, Balancer V2, Uniswap V2, Uniswap V3) adhere to the trade.limit setting.

The trade.limit parameter exists to ensure that the vault receives a minimum amount of purchased/output tokens during the trade. If the vault received less than the trade.limit, the transaction would revert. Refer to the next section for more detail about trade.limit. However, the problem is that the trade.limit is ignored by the 0x adaptor and not explicitly enforced within the 0x adaptor. This also means that there is no slippage control at all if the trade is executed via 0x DEX since the slippage control is implemented via the use of the trade.limit that is ignored by 0x adaptor.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/adapters/ZeroExAdapter.sol#L7

File: ZeroExAdapter.sol
07: library ZeroExAdapter {
08:     /// @dev executeTrade validates pre and post trade balances and also
09:     /// sets and revokes all approvals. We are also only calling a trusted
10:     /// zero ex proxy in this case. Therefore no order validation is done
11:     /// to allow for flexibility.
12:     function getExecutionData(address from, Trade calldata trade)
13:         internal view returns (
14:             address spender,
15:             address target,
16:             uint256 /* msgValue */,
17:             bytes memory executionCallData
18:         )
19:     {
20:         spender = Deployments.ZERO_EX;
21:         target = Deployments.ZERO_EX;
22:         // msgValue is always zero
23:         executionCallData = trade.exchangeData;
24:     }
25: }

Note about the comments on Line 08-11 in ZeroExAdapter library

The comments mentioned that:

executeTrade validates pre and post trade balances... Therefore no order validation is done to allow for flexibility.

Based on the comments, it seems that the development team assumes that all processes use the executeTrade function, and validation of pre and post trade balances have already been performed. Thus, there is no need to enforce the trade.limit within the 0x trade order.

However, that is not true. For instance, the emergency vault settlement process uses executeTradeWithDynamicSlippage instead of executeTrade. Also, the emergency vault settlement process does not perform pre and post trade balances. As such, without enforcing the trade.limit within the 0x adaptor contract, there is no slippage control.

Why the slippage or trade.limit is ignored by the 0x adaptor?

Within the executeTradeWithDynamicSlippage, it will use the dynamicSlippageLimit to calculate the trade.limit, which is the minimum amount of purchased/output tokens the contract is expected to receive from the DEX. The trade.limit parameter is adhered to by the adaptor of Curve, Balancer V2, Uniswap V2, and Uniswap V3. If the trade does not receive the appropriate amount of purchased/output tokens, it will revert, and the trade will not happen.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L109

File: TradingModule.sol
109:     function executeTradeWithDynamicSlippage(
110:         uint16 dexId,
111:         Trade memory trade,
112:         uint32 dynamicSlippageLimit
113:     ) external override returns (uint256 amountSold, uint256 amountBought) {
114:         // This method calls back into the implementation via the proxy so that it has proper
115:         // access to storage.
116:         trade.limit = PROXY.getLimitAmount(
117:             trade.tradeType,
118:             trade.sellToken,
119:             trade.buyToken,
120:             trade.amount,
121:             dynamicSlippageLimit
122:         );

Following is an example taken from CurveAdapter showing that the trade.limit will be passed to the ICurveRouter.exchange functions during the trade so that if the slippage exceeds the defined amount, the trade will revert.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/adapters/CurveAdapter.sol#L22

File: CurveAdapter.sol
22:     function _exactInBatch(Trade memory trade) internal view returns (bytes memory executionCallData) {
23:         CurveBatchData memory data = abi.decode(trade.exchangeData, (CurveBatchData));
24: 
25:         return abi.encodeWithSelector(
26:             ICurveRouter.exchange.selector,
27:             trade.amount, // @audit-info Amount of the input token being swapped.
28:             data.route,
29:             data.indices,
30:             trade.limit // @audit-info min_received Minimum amount of the output token to be received. If the actual amount received is less the call will revert.
31:         );
32:     }

However, this is not the case for ZeroExAdapter. The trade.limit parameter is ignored entirely within the getExecutionData function. Only the trade.exechangeData is relevant here. A malicious user has the full flexibility here to craft the trade.exchangeData.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/adapters/ZeroExAdapter.sol#L7

File: ZeroExAdapter.sol
07: library ZeroExAdapter {
08:     /// @dev executeTrade validates pre and post trade balances and also
09:     /// sets and revokes all approvals. We are also only calling a trusted
10:     /// zero ex proxy in this case. Therefore no order validation is done
11:     /// to allow for flexibility.
12:     function getExecutionData(address from, Trade calldata trade)
13:         internal view returns (
14:             address spender,
15:             address target,
16:             uint256 /* msgValue */,
17:             bytes memory executionCallData
18:         )
19:     {
20:         spender = Deployments.ZERO_EX;
21:         target = Deployments.ZERO_EX;
22:         // msgValue is always zero
23:         executionCallData = trade.exchangeData;
24:     }
25: }

Impact

Malicious users can trigger the permissionless settleVaultEmergency function and cause the trade to suffer huge slippage. This results in loss of assets for the vaults and their users.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/adapters/ZeroExAdapter.sol#L7 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L109 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/adapters/CurveAdapter.sol#L22

Tool used

Manual Review

Recommendation

Update the ZeroExAdapter contract to explicitly set the minOutputTokenAmount of the trade order to trade.limit so that the trade will revert if the number of purchased/output tokens received does not meet the requirement.

library ZeroExAdapter {
    function getExecutionData(address from, Trade calldata trade)
        internal view returns (
            address spender,
            address target,
            uint256 /* msgValue */,
            bytes memory executionCallData
        )
    {
        spender = Deployments.ZERO_EX;
        target = Deployments.ZERO_EX;

        // msgValue is always zero
        executionCallData = _setTradeLimit(from, trade);
    }

    function _setTradeLimit(address from, Trade calldata trade) internal view returns (bytes memory executionCallData) {
        bytes calldata _data = trade.exchangeData;

        address inputToken;
        address outputToken;
        address recipient;
        uint256 inputTokenAmount;
        uint256 minOutputTokenAmount;

        require(_data.length >= 4, "Invalid calldata");
        bytes4 selector;
        assembly {
            selector := and(
                // Read the first 4 bytes of the _data array from calldata.
                calldataload(add(36, calldataload(164))), // 164 = 5 * 32 + 4
                0xffffffff00000000000000000000000000000000000000000000000000000000
            )
        }

        if (selector == 0xf7fcd384) {
            // sellToLiquidityProvider()
            (
                inputToken, 
                outputToken, 
                , 
                recipient, 
                inputTokenAmount, 
                minOutputTokenAmount
            ) = abi.decode(_data[4:], (address, address, address, address, uint256, uint256));

            minOutputTokenAmount = trade.limit

            return abi.encodeWithSelector(
                LiquidityProviderFeature.sellToLiquidityProvider.selector,
                inputToken,
                outputToken,
                ,
                recipient, 
                inputTokenAmount, 
                minOutputTokenAmount,
            );
        }
        // The same can be done for other 0x functions.
    }
}

Duplicate of #75

[jeffywu]

Duplicate of #75