Skip to content

Commit 66dfaf4

Browse files
committed
fix(escrow): allow multi-sig escrows to enter the Disputed state
`dispute()` read only `EscrowEntry.arbiter`, returning `NoArbiter` when it was `None`. Escrows created by `deposit_with_arbiters` deliberately leave `arbiter` as `None` and carry their arbiters in `arbiters` + `arbiter_threshold`, so every multi-sig escrow was rejected. Because `dispute()` is the only transition into `Disputed`, and both `vote_for_dispute` and `resolve_dispute_multi_sig` require that status, the entire multi-arbiter dispute path was unreachable. A multi-sig escrow could only ever be withdrawn or refunded after expiry; its arbiters could never act. `dispute()` now accepts either shape. The emitted `EscrowDisputed` event still carries a single representative arbiter: the assigned one for single-arbiter escrows, otherwise the first listed arbiter — the same deterministic fallback `dispute::resolve_expiry_recipient` already uses. Why this was not caught: - `test_deposit_with_arbiters_creates_escrow_and_is_disputable` never calls `dispute()`; it only asserts the escrow is `Pending`. - `test_multi_sig_invalid_signer_cannot_vote`, `test_multi_sig_insufficient_votes_cannot_resolve` and the single-vote test all build their escrow with `deposit(..., Some(arbiter))` — a single-arbiter escrow, not a multi-sig one — then assert `is_err()`, which passed for the wrong reason. Adds `lifecycle_test.rs` covering the real transition: a multi-sig escrow reaching `Disputed`, and an assigned arbiter voting once it is. Also adds `reentrancy_test.rs`, which drives the money paths with a hostile token that calls back into the contract. `deposit_with_commitment` and `partial_payment` transfer before writing state, unlike the other eight money paths; the tests pin down that the Soroban host refuses the re-entrant frame, so the duplicate-commitment and overpayment guards cannot be bypassed. If that platform guarantee ever changes, these fail rather than silently allowing duplicate settlement. Tests: 407 -> 411, all passing. clippy clean on the changed files. Repo-wide `cargo fmt` is not clean (326 pre-existing diffs, which is why it is disabled in CI), so only the two new files were formatted.
1 parent 5978569 commit 66dfaf4

4 files changed

Lines changed: 389 additions & 3 deletions

File tree

app/contract/contracts/Folder/src/escrow.rs

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1098,8 +1098,30 @@ pub fn dispute(env: &Env, commitment: BytesN<32>) -> Result<(), RustAcademyErro
10981098
let entry: EscrowEntry =
10991099
get_escrow(env, &commitment_bytes).ok_or( RustAcademyError::CommitmentNotFound)?;
11001100

1101-
// Guard: must have an arbiter assigned
1102-
let arbiter = entry.arbiter.as_ref().ok_or( RustAcademyError::NoArbiter)?;
1101+
// Guard: must have an arbiter assigned.
1102+
//
1103+
// Two escrow shapes carry arbiters: single-arbiter escrows populate
1104+
// `arbiter`, while multi-sig escrows created by `deposit_with_arbiters`
1105+
// leave `arbiter` as `None` and populate `arbiters` + `arbiter_threshold`.
1106+
// Both must be able to reach `Disputed`, otherwise `vote_for_dispute` and
1107+
// `resolve_dispute_multi_sig` are unreachable for multi-sig escrows.
1108+
//
1109+
// The event carries a single representative arbiter. For multi-sig escrows
1110+
// the first listed arbiter is used, matching the deterministic fallback
1111+
// already applied in `dispute::resolve_expiry_recipient`.
1112+
let event_arbiter = match entry.arbiter.as_ref() {
1113+
Some(arbiter) => arbiter.clone(),
1114+
None => {
1115+
if entry.arbiter_threshold == 0 {
1116+
return Err( RustAcademyError::NoArbiter);
1117+
}
1118+
entry
1119+
.arbiters
1120+
.first()
1121+
.ok_or( RustAcademyError::NoArbiter)?
1122+
.clone()
1123+
}
1124+
};
11031125

11041126
// Guard: escrow must be in Pending state
11051127
if entry.status != EscrowStatus::Pending {
@@ -1113,7 +1135,7 @@ pub fn dispute(env: &Env, commitment: BytesN<32>) -> Result<(), RustAcademyErro
11131135
// Issue #49: snapshot timeout and default expiry action at dispute creation.
11141136
dispute::record_dispute_expiry(env, commitment.clone());
11151137

1116-
events::publish_escrow_disputed(env, commitment.clone(), arbiter.clone());
1138+
events::publish_escrow_disputed(env, commitment.clone(), event_arbiter);
11171139

11181140
Ok(())
11191141
}

app/contract/contracts/Folder/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,12 @@ pub mod nonce;
3333
mod nonce_test;
3434
mod oracle;
3535
mod privacy;
36+
#[cfg(test)]
37+
mod reentrancy_test;
3638
mod legacy_privacy;
3739
#[cfg(test)]
40+
mod lifecycle_test;
41+
#[cfg(test)]
3842
mod role_test;
3943
mod stealth;
4044
#[cfg(test)]
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
//! Lifecycle transition tests for multi-arbiter (multi-sig) escrows.
2+
//!
3+
//! `deposit_with_arbiters` stores its arbiters in `EscrowEntry.arbiters` with
4+
//! `arbiter: None`, but `dispute()` reads only `EscrowEntry.arbiter`. These
5+
//! tests pin down that a multi-sig escrow can actually reach `Disputed` and be
6+
//! resolved, which is the precondition for `vote_for_dispute` and
7+
//! `resolve_dispute_multi_sig` being reachable at all.
8+
9+
10+
use soroban_sdk::{testutils::Address as _, token, Address, Bytes, Env, Vec};
11+
12+
use crate::{types::EscrowStatus, RustAcademyContract, RustAcademyContractClient};
13+
14+
fn setup<'a>() -> (Env, RustAcademyContractClient<'a>, Address) {
15+
let env = Env::default();
16+
env.mock_all_auths();
17+
let id = env.register(RustAcademyContract, ());
18+
let client = RustAcademyContractClient::new(&env, &id);
19+
let admin = Address::generate(&env);
20+
client.initialize(&admin);
21+
let token = env
22+
.register_stellar_asset_contract_v2(Address::generate(&env))
23+
.address();
24+
(env, client, token)
25+
}
26+
27+
/// A multi-sig escrow must be able to enter `Disputed`.
28+
///
29+
/// Without this, `vote_for_dispute` and `resolve_dispute_multi_sig` are
30+
/// unreachable: both require `status == Disputed`, and `dispute()` is the only
31+
/// transition into that state.
32+
#[test]
33+
fn multi_sig_escrow_can_be_disputed() {
34+
let (env, client, token) = setup();
35+
let owner = Address::generate(&env);
36+
let amount: i128 = 3_000;
37+
let salt = Bytes::from_slice(&env, b"multisig_dispute_lifecycle");
38+
39+
token::StellarAssetClient::new(&env, &token).mint(&owner, &amount);
40+
41+
let mut arbiters = Vec::new(&env);
42+
arbiters.push_back(Address::generate(&env));
43+
arbiters.push_back(Address::generate(&env));
44+
arbiters.push_back(Address::generate(&env));
45+
46+
let commitment =
47+
client.deposit_with_arbiters(&token, &amount, &owner, &salt, &0u64, &arbiters, &2u32);
48+
49+
assert_eq!(
50+
client.get_commitment_state(&commitment),
51+
Some(EscrowStatus::Pending),
52+
"escrow should start Pending"
53+
);
54+
55+
let result = client.try_dispute(&commitment);
56+
assert!(
57+
result.is_ok(),
58+
"a multi-sig escrow must be disputable; otherwise its arbiters can never act"
59+
);
60+
61+
assert_eq!(
62+
client.get_commitment_state(&commitment),
63+
Some(EscrowStatus::Disputed),
64+
"escrow should be Disputed after dispute()"
65+
);
66+
}
67+
68+
/// Once a multi-sig escrow is disputed, its assigned arbiters must be able to
69+
/// vote. This is the step that is dead code today.
70+
#[test]
71+
fn multi_sig_arbiters_can_vote_once_disputed() {
72+
let (env, client, token) = setup();
73+
let owner = Address::generate(&env);
74+
let amount: i128 = 3_000;
75+
let salt = Bytes::from_slice(&env, b"multisig_vote_lifecycle");
76+
77+
token::StellarAssetClient::new(&env, &token).mint(&owner, &amount);
78+
79+
let a1 = Address::generate(&env);
80+
let a2 = Address::generate(&env);
81+
let mut arbiters = Vec::new(&env);
82+
arbiters.push_back(a1.clone());
83+
arbiters.push_back(a2.clone());
84+
85+
let commitment =
86+
client.deposit_with_arbiters(&token, &amount, &owner, &salt, &0u64, &arbiters, &2u32);
87+
88+
client.dispute(&commitment);
89+
90+
let vote = client.try_vote_for_dispute(&a1, &commitment, &true);
91+
assert!(
92+
vote.is_ok(),
93+
"an assigned multi-sig arbiter must be able to vote on a disputed escrow"
94+
);
95+
}

0 commit comments

Comments
 (0)