feat(duels): Soroban commit-reveal arbiter for async script duels (#13) - #45
Conversation
…tcoindefi#13) contracts/duel-arena adds the phase machine the issue specifies: create deposits a stake and commits sha256 of the script before anything is visible, accept locks the second stake and fixes the shared seed from the ledger sequence neither player controls, reveals must hash-match their commitments, a published result waits out a dispute window that a counterclaim turns into consensus-required mode, settle pays the pot exactly once per nonce, and refund_no_show returns both stakes when a side stonewalls past the reveal deadline. Engine version and content.js hash ride along with every duel so old replays keep their own rules. Five host tests walk the happy path and every ugly one listed in the issue. Battle replay inside the contract is deliberately out of scope per its cost note: the contract stores claims under dispute instead.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| pub fn publish_result(env: &Env, nonce: u64, winner: Address) { | ||
| let mut d = get(env, &nonce); | ||
| if d.settled { | ||
| panic_with_error!(env, Error::AlreadySettled); | ||
| } | ||
| if d.settle_deadline.is_none() { | ||
| panic_with_error!(env, Error::WrongPhase); | ||
| } | ||
| if winner != d.challenger && winner != d.opponent { | ||
| panic_with_error!(env, Error::BadWinner); | ||
| } | ||
| if let Some(prev) = &d.claim { | ||
| if *prev != winner { | ||
| d.contested = true; | ||
| } |
There was a problem hiding this comment.
🚨 Security: publish_result has no auth and isn't bound to duel outcome
publish_result (lib.rs:203-222) never calls require_auth and never checks the claimed winner against the revealed scripts/seed. The first claim is stored, and after the 10-minute DISPUTE_WINDOW settle pays the whole pot to d.claim. A losing participant (or anyone) can call publish_result(nonce, loser_as_winner); if the true winner is offline and fails to dispute within DISPUTE_WINDOW, the loser steals the entire pot. Require authorization from the claimed winner (and/or the caller) and, ideally, derive the winner on-chain or require both participants' claims to match before settling.
Require the claimed winner to authorize the result assertion.:
pub fn publish_result(env: &Env, nonce: u64, winner: Address) {
let mut d = get(env, &nonce);
if d.settled { panic_with_error!(env, Error::AlreadySettled); }
if d.settle_deadline.is_none() { panic_with_error!(env, Error::WrongPhase); }
if winner != d.challenger && winner != d.opponent {
panic_with_error!(env, Error::BadWinner);
}
// Only a participant may assert a result, and they must authorize it.
winner.require_auth();
// ... existing claim/contested logic ...
}
Was this helpful? React with 👍 / 👎
| if let Some(prev) = &d.claim { | ||
| if *prev != winner { | ||
| d.contested = true; | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: Contested pot with no consensus locks funds permanently
Once contested is set (lib.rs:214-217), settle refuses (line 256) and only resolve_consensus — which requires BOTH participants' auth — can release funds. A losing participant can dispute the result and then simply refuse to sign consensus, permanently locking the winner's stake; refund_no_show is also unavailable because both sides revealed (lines 277-279). Add a fallback resolution (e.g. a longer arbitration timeout that refunds both stakes, or an authoritative tie-break) so contested pots cannot be griefed into a permanent lock.
Was this helpful? React with 👍 / 👎
| pub fn create_duel( | ||
| env: &Env, | ||
| nonce: u64, | ||
| challenger: Address, | ||
| opponent: Address, | ||
| stake: i128, | ||
| script_hash: BytesN<32>, | ||
| engine_version: String, | ||
| content_hash: BytesN<32>, | ||
| ) { | ||
| if duels(env).contains_key(nonce) { | ||
| panic_with_error!(env, Error::NonceTaken); | ||
| } | ||
| challenger.require_auth(); | ||
| let now = env.ledger().timestamp(); |
There was a problem hiding this comment.
💡 Edge Case: create_duel lacks stake>0 and challenger!=opponent validation
create_duel (lib.rs:94-130) does not validate stake > 0 nor that challenger != opponent. A zero/negative stake or a self-duel creates a nonsensical duel entry (self-duel guarantees the caller both wins and loses, defeating the escrow). Add explicit checks and reject with a dedicated error before transferring funds.
Was this helpful? React with 👍 / 👎
| if env.ledger().timestamp() > d.reveal_deadline { | ||
| panic_with_error!(env, Error::DeadlineNotReached); | ||
| } |
There was a problem hiding this comment.
💡 Quality: Misleading DeadlineNotReached error when reveal deadline passed
In do_reveal (lib.rs:158-160), a reveal submitted after the deadline panics with Error::DeadlineNotReached, but the actual condition is that the deadline was exceeded. This mislabels the failure for front ends/verifiers reading the error code. Introduce a distinct variant (e.g. DeadlinePassed) for the too-late case.
Was this helpful? React with 👍 / 👎
Code Review 🚫 Blocked 0 resolved / 4 findingsImplements the Soroban commit-reveal arbiter contract for async script duels, but publish_result lacks authentication and contested pots can lock funds permanently. 🚨 Security: publish_result has no auth and isn't bound to duel outcome📄 contracts/duel-arena/src/lib.rs:203-217 📄 contracts/duel-arena/src/lib.rs:243-257
Require the claimed winner to authorize the result assertion.
|
| Auto-apply | Compact |
|
|
Important
Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.
Was this helpful? React with 👍 / 👎 | Gitar
…rkspace `contracts/duel-arena` entro a main con el PR #45, despues de que esta rama se abriera. Sin tocar nada, cargo lo excluia del workspace en silencio: `cargo build` en `contracts/` compilaba jefe y medicion, y duel-arena no aparecia por ningun lado ni daba error. Quien arme el CI manana habria construido dos de tres contratos sin enterarse. Ahora la exclusion es explicita y dice por que existe. Las dos razones son reales: - duel-arena ancla `soroban-sdk = "=26.1.1"` con igual, mientras jefe y medicion usan 27.0.6, que es la version del protocolo vivo en mainnet y testnet. En un mismo workspace conviven pero se compilan dos copias del SDK. - duel-arena trae su propio `[profile.release]`, y un workspace solo honra el de la raiz, asi que meterlo adentro le cambiaria las opciones de compilacion por la espalda. Unificar las versiones es trabajo aparte y merece su propia issue. Lo que arregla este commit es que la exclusion sea una decision visible en vez de un olvido. cargo metadata sin error workspace ve medicion, jefe cargo test -p jefe 11/11
3 participants
PR #9 → issue #13⚠️ независимая ветка от main
Título
feat(duels): arbitro Soroban commit-reveal para duelos asincronicos de scripts (#13)
Cuerpo del PR (ES)
Que agrega
contracts/duel-arena- la maquina de fases que pide el issue, anclada a Stellar:sha256(script)ANTES de ver nadaHashMismatchsi no)resolve_consensusengine_version+ hash decontent.jsviajan dentro del duelo: los viejos se recalculan con sus propias reglas. El replay completo dentro del contrato queda fuera a proposito (nota de costo del issue): el contrato guarda claims bajo disputa.Tests (los feos incluidos) - 5/5
happy_path_commit_reveal_settle · rival_never_reveals_refunds_both · reveal_with_wrong_plaintext_is_rejected · double_settle_is_impossible · contested_window_requires_consensus
Testnet (evidence real)
CONTRACT_ID
CCYWIFOWCGJ5XXXP3OE2TAN2MUAEYCD4UINQBHVVXMHKU2O5XAB3LQWU· create TX 9aa0313a...7643 · ambos reveals verificados on-chain · estado final con claim+seed+deadline enevidence-issue13.md. Comando del juego para desafiar/revelar/cobrar se enchufa sobre este contrato una vez mergeado.Fixes #13
PR body (EN fallback)
contracts/duel-arena implements the issue's phase machine: deposit+commit before visibility, ledger-sequence shared seed, hash-checked reveals, dispute-window results with consensus fallback for contested pots, one-settle-per-nonce, and no-show refunds. engine_version + content hash ride along per duel. Five host tests cover the happy path and every ugly case listed. Deployed to testnet; live run evidence (CID, tx links, final state JSON) attached. Game-side challenge/reveal/claim commands plug into this contract post-merge. Fixes #13