Dug - `MarginDex::execute_limit_order` will always revert
Opened this issue · 2 comments
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
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.
Fixed by removing the code where the limit order was deleted: self.limit_orders[_uid] = empty(LimitOrder)