fix: require signed OTC wallet actions#5563
Conversation
iamdinhthuan
left a comment
There was a problem hiding this comment.
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
strongkeep-debug
left a comment
There was a problem hiding this comment.
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.
TJCurnutte
left a comment
There was a problem hiding this comment.
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 --checkpassed.py_compilepassed 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_requiredand the order remainsopen; - a match request whose signature was bound to a different
eth_addressreturns401 wallet_auth_invalid_signature; - a maker-signed cancel returns
200and moves the order tocancelled.
- unauthenticated cancel returns
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.
iamdinhthuan
left a comment
There was a problem hiding this comment.
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 passedDirect 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.
strongkeep-debug
left a comment
There was a problem hiding this comment.
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.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
strongkeep-debug
left a comment
There was a problem hiding this comment.
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.
Summary
wallet_authfor OTC match and cancel actionsmatch_ordersignatures toeth_addressso a valid signature cannot be replayed with a different settlement addressFixes #4957.
Validation
python -B -m pytest -q tests/test_otc_bridge_wallet_auth.pypython -B -m pytest -q tests/test_otc_bridge_query_validation.pypython -B -m py_compile otc-bridge/otc_bridge.py tests/test_otc_bridge_wallet_auth.pygit diff --check -- otc-bridge/otc_bridge.py otc-bridge/README.md otc-bridge/requirements.txt tests/test_otc_bridge_wallet_auth.pyNote
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