Dug - `MarginDex::execute_limit_order` will always revert

[sherlock-admin]

Dug

medium

MarginDex::execute_limit_order will always revert

Summary

All calls to the MarginDex's execute_limit_order will always revert due the limit order being cleared in storage ahead of a call to remove_limit_order.

Vulnerability Detail

The execute_limit_order function includes the following logic...

    self.limit_orders[_uid] = empty(LimitOrder)

    trade: Trade = self._open_trade(
        limit_order.account,
        limit_order.position_token,
        limit_order.min_position_amount_out,
        limit_order.debt_token,
        limit_order.debt_amount,
        limit_order.margin_amount,
        limit_order.tp_orders,
        limit_order.sl_orders
    )

    self._remove_limit_order(_uid)

The limit order is being set to empty(LimitOrder) in storage. Then a call to self._remove_limit_order is called for the same _uid.

_remove_limit_order has the following logic...

    order: LimitOrder = self.limit_orders[_uid]
    self.limit_orders[_uid] = empty(LimitOrder)

    uids: DynArray[bytes32, 1024] = self.limit_order_uids[order.account]
    for i in range(1024):
        if uids[i] == _uid:
            uids[i] = uids[len(uids) - 1]
            uids.pop()
            break
        if i == len(uids) - 1: // @audit - Reverts when uids is empty
            raise
    self.limit_order_uids[order.account] = uids

The newly-empty order is read from storage, and it's associated account is used fetch the limit_order_uids.

The issue is that this account is the zero address, which is the default value after the order was cleared in the preceding function. This will cause the uids array to be empty, and the loop will revert when i == len(uids) - 1.

Impact

This means that all calls to execute_limit_order will revert, and no limit orders can be executed, which is a major piece of functionality for the protocol.

Code Snippet

https://github.com/sherlock-audit/2023-06-unstoppable/blob/main/unstoppable-dex-audit/contracts/margin-dex/MarginDex.vy#L568-L597

https://github.com/sherlock-audit/2023-06-unstoppable/blob/main/unstoppable-dex-audit/contracts/margin-dex/MarginDex.vy#L619-L632

Tool used

Manual Review

Recommendation

execute_limit_order should not clear the limit order in storage before calling remove_limit_order. This will allow the limit_order_uids to be fetched correctly.

[Unstoppable-DeFi]

https://github.com/Unstoppable-DeFi/unstoppable-dex-audit/pull/15

[maarcweiss]

Fixed by removing the code where the limit order was deleted: self.limit_orders[_uid] = empty(LimitOrder)