Skip to content

fix: require signed OTC wallet actions#5563

Merged
Scottcjn merged 3 commits into
Scottcjn:mainfrom
kekehanshujun:codex/otc-wallet-auth-bound-intent-4957
May 18, 2026
Merged

fix: require signed OTC wallet actions#5563
Scottcjn merged 3 commits into
Scottcjn:mainfrom
kekehanshujun:codex/otc-wallet-auth-bound-intent-4957

Conversation

@kekehanshujun
Copy link
Copy Markdown

Summary

  • require Ed25519 wallet_auth for OTC match and cancel actions
  • derive the native RTC wallet from the submitted public key before allowing mutation
  • bind match_order signatures to eth_address so a valid signature cannot be replayed with a different settlement address
  • reject non-object JSON bodies on match/cancel and document the signed payload shape

Fixes #4957.

Validation

  • python -B -m pytest -q tests/test_otc_bridge_wallet_auth.py
  • python -B -m pytest -q tests/test_otc_bridge_query_validation.py
  • python -B -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.py
  • git diff --check -- otc-bridge/otc_bridge.py otc-bridge/README.md otc-bridge/requirements.txt tests/test_otc_bridge_wallet_auth.py

Note

The signed match intent includes eth_address; this closes the address-substitution gap where a short-lived wallet signature could otherwise be reused with a different quote-chain destination.

Bounty

RTC wallet: RTC02811ff5e2bb4bb4b95eee44c5429cd9525496e7

@github-actions github-actions Bot added documentation Improvements or additions to documentation BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes size/L PR: 201-500 lines labels May 17, 2026
Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@iamdinhthuan iamdinhthuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclosure: I am reviewing this under Scottcjn/rustchain-bounties#73 as @iamdinhthuan.

Requesting changes on head 5431c3dfa2fe16db3dab24236a018d8d0eebdb5b. The new wallet_auth checks close the unsigned match and cancel paths, but the same OTC wallet-action boundary is still open at order creation.

POST /api/orders still accepts an arbitrary wallet string without wallet_auth. That lets an unauthenticated caller create visible buy orders under someone else's RTC wallet, and the sell path will proceed to the escrow call using the claimed maker_wallet before any proof of wallet ownership is checked. This leaves part of #4957's order-manipulation/weak-wallet-validation problem in place: the order book can still be seeded with orders attributed to public wallets that the caller does not control.

Local probe on the PR head with only the RustChain network calls stubbed:

unauthenticated POST /api/orders {"side":"buy","wallet":"RTC1111111111111111111111111111111111111111",...}
-> 201, open buy order created

unauthenticated POST /api/orders {"side":"sell","wallet":"RTC1111111111111111111111111111111111111111",...}
-> 201, open sell order created and escrow_job_id recorded when rtc_create_escrow_job returns ok

Suggested fix: require the same action-scoped wallet_auth proof for create_order, bind it to {action:"create_order", side, pair, amount_rtc, price_per_rtc, ttl_seconds, wallet, ...} or another canonical order-intent payload, and reject non-native RTC maker wallets before any escrow call or database insert. Add regressions showing an unsigned buy order and an unsigned sell order are rejected and preserve no open order.

Validation I ran:

git diff --check review-pr-5563^ review-pr-5563 -- otc-bridge/otc_bridge.py otc-bridge/README.md otc-bridge/requirements.txt tests/test_otc_bridge_wallet_auth.py
python3 -B -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.py
python3 -B -m pytest -q tests/test_otc_bridge_wallet_auth.py --noconftest
# 6 passed
python3 -B -m pytest -q tests/test_otc_bridge_query_validation.py --noconftest
# 9 passed

Copy link
Copy Markdown

@strongkeep-debug strongkeep-debug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wallet-auth direction is stronger than the older #4960 shape because the match signature now also binds eth_address, so a signed match intent cannot be replayed with a different settlement address. The focused behavior checks passed for me on head 5431c3d:

python -m pytest -q tests/test_otc_bridge_wallet_auth.py
# 6 passed

python -m pytest -q tests/test_otc_bridge_query_validation.py
# 9 passed

python -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.py
# passed

git diff --check origin/main...HEAD
# passed

The remaining merge gate is the new test file header:

python tools/bcos_spdx_check.py --base-ref origin/main
# fails: tests/test_otc_bridge_wallet_auth.py is a new file without an SPDX header

Please add the SPDX header to the new regression file. I would be comfortable re-checking after that; I did not see a functional blocker in the signed match/cancel behavior covered by the focused tests above.

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I reviewed and re-tested the signed OTC wallet-action path at head 5431c3dfa2fe16db3dab24236a018d8d0eebdb5b.

Validation run:

git diff --check origin/main...HEAD -- otc-bridge/README.md otc-bridge/otc_bridge.py otc-bridge/requirements.txt tests/test_otc_bridge_wallet_auth.py
uv run --no-project --with cryptography --with flask --with flask-cors --with requests python -B -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.py
uv run --no-project --with pytest --with cryptography --with flask --with flask-cors --with requests python -B -m pytest -q -o addopts='' --noconftest tests/test_otc_bridge_wallet_auth.py
uv run --no-project --with cryptography --with flask --with flask-cors --with requests python <focused Flask/Ed25519 runtime probe>

Results:

  • git diff --check passed.
  • py_compile passed with the added Ed25519 dependency available.
  • Focused pytest passed: 6 passed in 2.17s.
  • Runtime probe created a fresh OTC order (201), then verified:
    • unauthenticated cancel returns 401 wallet_auth_required and the order remains open;
    • a match request whose signature was bound to a different eth_address returns 401 wallet_auth_invalid_signature;
    • a maker-signed cancel returns 200 and moves the order to cancelled.

The signature payload binds action, order id, wallet, timestamp, and match-specific fields, and the tests cover missing auth, wrong key, stale/invalid binding, and successful signed match/cancel flows. This closes the open impersonation/cancel surface for OTC wallet actions without breaking the happy path.

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@iamdinhthuan iamdinhthuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclosure: I am reviewing this under Scottcjn/rustchain-bounties#73 as @iamdinhthuan.

Re-reviewed current head 823ca7e51f8c26095db99bc4eb80136d4e8b94dc. The create-order wallet-auth gap I previously reported on head 5431c3dfa2fe16db3dab24236a018d8d0eebdb5b is resolved here: POST /api/orders now requires wallet_auth before any escrow call or database insert, binds the create signature to side, pair, integer amount/price units, ttl_seconds, and eth_address, and rejects non-native RTC maker wallets through the same boundary.

Validation run:

git diff --check origin/main...HEAD -- otc-bridge/otc_bridge.py otc-bridge/README.md otc-bridge/requirements.txt tests/test_otc_bridge_wallet_auth.py
python3 tools/bcos_spdx_check.py --base-ref origin/main
uv run --no-project --with pytest --with cryptography --with flask --with flask-cors --with requests python -B -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.py
uv run --no-project --with pytest --with cryptography --with flask --with flask-cors --with requests python -B -m pytest -q -o addopts='' --noconftest tests/test_otc_bridge_wallet_auth.py
# 9 passed

Direct Flask probe with network/escrow calls stubbed:

unsigned buy create -> 401 wallet_auth_required, open orders remain 0
unsigned sell create -> 401 wallet_auth_required, escrow_create call count remains 0, open orders remain 0
signed buy create -> 201 ok
tampered amount after signing -> 401 wallet_auth_invalid_signature

Non-blocking docs nit: the README Create Order example still shows an unsigned my-wallet payload and the auth paragraph says only match and cancel require wallet_auth. Please update that example/text so integrators do not copy a request that the fixed endpoint now rejects. I do not consider that a functional blocker for this security fix.

Copy link
Copy Markdown

@strongkeep-debug strongkeep-debug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head 823ca7e after the SPDX and create-order wallet-auth follow-ups. The original SPDX blocker is fixed, and the focused wallet-auth coverage passes now:

git diff --check origin/main...HEAD -- otc-bridge/README.md otc-bridge/otc_bridge.py otc-bridge/requirements.txt tests/test_otc_bridge_wallet_auth.py
# passed
python -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.py
# passed
python tools/bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK
pytest tests/test_otc_bridge_wallet_auth.py -q
# 9 passed

One current-head blocker remains in the adjacent OTC validation suite:

pytest tests/test_otc_bridge_wallet_auth.py tests/test_otc_bridge_query_validation.py -q
# 1 failed, 17 passed

The failing case is test_unexpected_order_errors_are_generic: it still posts /api/orders without the new wallet_auth, so the request now stops at 401 before the monkeypatched unexpected-error path is exercised. That means the regression no longer proves the generic 500 redaction behavior for order creation under the new signed-create-order flow.

Please update that test to provide valid create-order wallet auth before triggering the injected failure, or otherwise add equivalent coverage that still reaches the generic-error path after the new auth gate.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work on this PR. 🚀

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@strongkeep-debug strongkeep-debug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head bc41fdf2f602e238a7ad927d17979e87a2b85529 after the signed create-order error-path follow-up.

This resolves the blocker I raised on the previous head. The generic-error regression now builds a valid signed create-order payload, so it reaches the mocked internal failure path instead of being stopped by the new wallet-auth gate with 401. That keeps the signed create-order requirement intact while still proving unexpected order creation errors remain generic.

Validation I ran locally:

git diff --check origin/main...HEAD
# passed

python -B -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.py tests/test_otc_bridge_query_validation.py
# passed

python -B tools/bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK

uv run --with pytest --with flask --with cryptography --with requests python -B -m pytest -q tests/test_otc_bridge_wallet_auth.py --tb=short -p no:cacheprovider
# 9 passed

uv run --with pytest --with flask --with cryptography --with requests python -B -m pytest -q tests/test_otc_bridge_query_validation.py --tb=short -p no:cacheprovider
# 9 passed

uv run --with pytest --with flask --with cryptography --with requests python -B -m pytest -q tests/test_otc_bridge_wallet_auth.py tests/test_otc_bridge_query_validation.py --tb=short -p no:cacheprovider
# 18 passed

A plain scoped pytest run in this checkout was blocked before module import by the missing optional requests dependency, so the pytest validation above supplies the same missing test dependencies explicitly with uv.

Final pre-review state check: #5563 was still open at this head with no PR comments and no reviews on commit bc41fdf2f602e238a7ad927d17979e87a2b85529; my two earlier changes-requested reviews were on older commits.

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Scottcjn Scottcjn merged commit 9242696 into Scottcjn:main May 18, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) documentation Improvements or additions to documentation size/L PR: 201-500 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: OTC bridge allows order cancellation by anyone who knows wallet address

7 participants