31Third

31Third is highly dependant on external exchanges. As external exchanges can add or remove functionalities, the 31Third team should be very careful when adding new exchanges and stay updated about possible vulnerabilities in these systems. <br><br> When users call `BatchTrade.batchTrade()`, they can pass call data for a swap that differs from what is specified in the `Trade` struct. For example, if a user is trading using the 0x adapter, the actual call data sent to the 0x contract could target any available function. This could result in the user being able to call unintended functions in external contracts that could in turn drain `BatchTrade` of any funds held within.

The batch trade functionality builds upon the 0x exchange in order to perform each trade in the batch. The 0x protocol supports the submitter of the order to be different from the actual taker in the case of both [limit orders](https://protocol.0x.org/en/latest/basics/orders.html#limit-orders) and [RFQ orders](https://protocol.0x.org/en/latest/basics/orders.html#rfq-orders). Since the `batchTrade` function allows the user to pass arbitrary call data for the 0x swap, the user can easily specify within the order that the `taker` address which receives the result from the trade is different from the `spender`. Since the attacker can specify their own address as the `taker` they can receive the result of the trade directly and avoid paying the fees. In order to subvert the token balance checks within the `BatchTrade` contract, the attacker can use their own custom attack tokens as described in the attack scenario.

The `BatchTrade.batchTrade()` function might not revert on a failed trade if `_batchTradeConfig.revertOnError ` is set to `false`. This lack of reversion may result in some undesirable state changes persisting. Of particular concern is that token approvals to the exchange spender contract are not reset. During a trade, the function `_claimAndApproveFromToken()` is called, which transfers the input tokens for the swap, as well as approves the exchange spender to spend these tokens as shown below: ``` IERC20(_trade.from).safeIncreaseAllowance( _exchangeAdapter.getSpender(), _trade.fromAmount ); ``` The approval above is not reversed if the trade fails and could potentially be exploited in subsequent transactions. Furthermore, failed trades could be repeatedly executed in order to repeatedly increase the allowance above without resetting it.

A trader might reasonably expect that they would receive at least `Trade.minToReceive` from a trade. However, the actual minimum amount received by the user is actually `Trade.minToReceive - fees`. This is because the check involving `Trade.minToReceive` is performed immediately after the trade, as shown below: ``` if (toBalanceAfterTrade < toBalanceBeforeTrade + _trade.minToReceive) { revert NotEnoughReceived(_trade, _trade.minToReceive, toBalanceAfterTrade - toBalanceBeforeTrade); } ``` The fees are then subtracted from the amount received from the swap, leaving the resulting amount to be potentially less than `Trade.minToReceive`.

When transferring ERC20 tokens, approvals are only needed when the caller of the transfer is **not** the source address of the transfer. The following instances incorrectly use `IERC20.safeIncreaseAllowance()` to approve `BatchTrade` for transferring tokens out of `BatchTrade`: 1. `receiveFees()` 2. `_approveAndReturnToken()` 3. `_returnClaimedOnError()`

The `BatchToken` contract collects fees as a portion of the tokens received for each trade. The contract imposes no restrictions as to which tokens can be traded. Thus it is quite possible that inflationary or yield-bearing tokens could be collected as fees. One example of an inflationary token is `stETH`. If `stETH` is collected as fees for the contract, the contract's balance of `stETH` will grow over time, even if no more fees in `stETH` are collected. However, the increased balance will not be extractible since the fee amount is recorded at the time the fee was collected and stored in the contract, as shown below: ``` feeAmount = (_receivedAmount * feeBasisPoints) / 10000; if (feeAmount > 0) { accruedFees[_token] += feeAmount; } ``` Thus the `stETH` yield will remain locked in the contract forever.

If the owner renounces their ownership, all contracts inheriting from `Ownable` will be left without an owner. Consequently, any function guarded by the `onlyOwner` modifier will no longer be able to be executed. <br><br> Additionally, if ownership were to be revoked, the contracts would lose crucial functionality such as activating or deactivating the batch trading, modifying fees, feeless wallets, and fee recipients, as well as modifying the adapters used in the batch trading.

31Third relies on the 0x protocol for zero slippage. However, in the instance where an erroneous `Trade` struct is passed to `batchTrade()`, with a `_minToReceive` specified as zero, the trade will still execute normally. This could potentially lead to a loss of user funds, as `from` tokens will be received, but not `to` tokens.

`BatchTrade.receiveFees()` does not check if the accrued fees for that asset are not zero. This can lead to zero-amount transfers.