Skip to content

fix(order-handler): correct reversed position-manager delegation - #704

Merged
IbrahimIjai merged 1 commit into
SO4-Markets:mainfrom
dev-debbie-umoh:fix/535-position-manager-lookup-reversed
Aug 31, 2026
Merged

fix(order-handler): correct reversed position-manager delegation#704
IbrahimIjai merged 1 commit into
SO4-Markets:mainfrom
dev-debbie-umoh:fix/535-position-manager-lookup-reversed

Conversation

@dev-debbie-umoh

Copy link
Copy Markdown
Contributor

Summary

Issue #385 ("order_handler: create_order position-manager lookup is reversed, hijacks owner's own orders") was closed, but the exact vulnerable logic it described was still present, unchanged, in contracts/order_handler/src/lib.rs, with a TODO(#385) comment in the code explicitly admitting it had never actually been fixed:

// TODO(#385): This logic is reversed. See comment above for required fix.
match ds.get_position_manager(&caller, &params.market) {
    Some(owner) => (owner.clone(), owner),
    None => (caller.clone(), params.receiver),
}

get_position_manager(&caller, market) looks up "who is the manager for caller" — the code then used that to redirect the order to the manager. But this is backwards: any trader who calls exchange_router.set_position_manager(self, market, bot) to delegate trade execution to a bot loses the ability to trade for themselves — every direct create_order call they make afterward is silently rewritten so account = bot and receiver = bot, redirecting their own already-deposited collateral to the delegate. create_orders mirrored the same bug (issue #620 had it copy create_order's behavior "as-is, not a fix for #385").

Fix

  • Added on_behalf_of: Option<Address> to CreateOrderParams.
  • When Some(owner) is given, require get_position_manager(owner, market) == Some(caller) before opening the position for the named owner (receiver is forced to the owner — no redirect).
  • When None, the caller always acts for themselves, regardless of whether they have their own registered manager elsewhere.
  • Applied identically to create_order and create_orders so both entrypoints agree.
  • Replaced the prior test that asserted the reversed (buggy) redirection with regression tests:
    • a trader with a registered manager can still create orders for themselves (both entrypoints)
    • a registered manager can create an order on_behalf_of the owner who registered them
    • naming an owner who never registered the caller as their manager reverts

Closes #535 (and lands the fix #385 was closed without actually shipping)

Test plan

  • grep -n "TODO(#385)" contracts/order_handler/src/lib.rs — no matches
  • cargo test -p order-handler --lib — 65 passed, 0 failed (3 unrelated pre-existing failures on unmodified main too: custom_heartbeat_timeout_is_respected, execute_order_records_keeper_heartbeat, keeper_goes_stale_after_timeout_and_role_is_revocable — oracle price-lookup issue unrelated to this change, verified independently)

 SO4-Markets#535)

Issue SO4-Markets#385 ("order_handler: create_order position-manager lookup is
reversed, hijacks owner's own orders") was closed, but the vulnerable
logic it described was still present verbatim, marked with a
TODO(SO4-Markets#385) comment admitting it had never actually been fixed.

The lookup direction was backwards: create_order and create_orders
called get_position_manager(&caller, market) — "who is caller's own
manager" — instead of checking whether caller is a manager acting FOR
someone else. Any trader who delegated to a bot via
set_position_manager lost the ability to trade for themselves: every
direct create_order call they made afterward was silently rewritten so
account = bot and receiver = bot, redirecting their own deposited
collateral to the delegate.

Add on_behalf_of: Option<Address> to CreateOrderParams. When present,
require get_position_manager(on_behalf_of, market) == Some(caller)
before opening the position for the named owner; when absent, the
caller always acts for themselves regardless of whether they've
registered their own manager. Apply the same fix to both create_order
and create_orders so the two entrypoints agree.

Replaces the prior test that asserted the reversed (buggy) redirection
with regression tests covering: a trader with a registered manager can
still create orders for themselves (both entrypoints); a registered
manager can create an order on_behalf_of the owner who registered
them; and naming an owner who never registered the caller reverts.
@drips-wave

drips-wave Bot commented Aug 31, 2026

Copy link
Copy Markdown

@dev-debbie-umoh Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@IbrahimIjai
IbrahimIjai merged commit af10e47 into SO4-Markets:main Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants