Skip to content

fix(tholos): verify balance deltas for incoming token transfers (#164) #180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xtep103 wants to merge 9 commits into from
Closed

fix(tholos): verify balance deltas for incoming token transfers (#164) #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b0b0fab
fix(tholos): verify balance deltas for incoming token transfers (#164)
xtep103 Sep 4, 2026
4210a56
Cap payouts by assertion escrow
xtep103 Sep 4, 2026
9d116d6
Merge main and resolve payout escrow conflict
xtep103 Sep 4, 2026
73691de
Update SDK binding for assertion escrow
xtep103 Sep 4, 2026
762ab16
Add token transfer mismatch error
xtep103 Sep 4, 2026
cc7e94a
Preserve storage key ordering
xtep103 Sep 5, 2026
ef143e1
Preserve storage key ordering
xtep103 Sep 7, 2026
2f5ed8f
Merge origin/main into feat/reentrancy-guard-early-check and resolve …
xtep103 Sep 7, 2026
509249e
Merge branch 'main' into feat/reentrancy-guard-early-check
xtep103 Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 78 additions & 30 deletions contracts/tholos/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ pub enum DataKey {
/// (#184). `None` means the assertion was disputed before this upgrade
/// or is still pending; `Some(ts)` is the ledger timestamp at dispute.
DisputedAt(u64),
/// The total token amount actually received for an assertion's escrow.
AssertionEscrow(u64),
}

#[contracterror]
Expand Down Expand Up @@ -259,6 +261,8 @@ pub enum Error {
/// assertion's asserter or disputer. A party voting on their own case
/// biases (and, on a size-1 committee, determines) the outcome.
SelfVote = 27,
/// Token transfer escrow overflow when adding a received amount.
TokenTransferMismatch = 28,
}

const DAY_IN_LEDGERS: u32 = 17280;
Expand Down Expand Up @@ -983,7 +987,7 @@ impl Tholos {
// transfer can't be allocated the same not-yet-incremented id.
let id: u64 = Self::get(&env, &DataKey::NextId)?;
env.storage().instance().set(&DataKey::NextId, &(id + 1));
let assertion = Assertion {
let mut assertion = Assertion {
asserter: asserter.clone(),
final_outcome: None,
outcome,
Expand All @@ -1001,11 +1005,20 @@ impl Tholos {
Self::set_assertion(&env, id, &assertion);

let token_id: Address = Self::get(&env, &DataKey::Token)?;
token::Client::new(&env, &token_id).transfer(
&asserter,
env.current_contract_address(),
&bond_amount,
);
let token_client = token::Client::new(&env, &token_id);
let contract_address = env.current_contract_address();
let balance_before = token_client.balance(&contract_address);

@collinsezedike collinsezedike Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This derives the received amount from a balance snapshot taken before the external transfer, but v1 has no contract-wide reentrancy mutex, unlike tholos-v2's ReentrancyGuard. A token whose transfer reenters a different entrypoint for a different assertion id before returning (both callers can be attacker-controlled via Soroban's sub-invocation auth tree, no third-party collusion needed) inflates the contract balance before this call reads balance_after, so received here counts tokens that actually belong to the nested assertion's escrow, corrupting both assertions' bookkeeping. Consider adding the same reentrancy guard v2 already has.

token_client.transfer(&asserter, &contract_address, &bond_amount);
let balance_after = token_client.balance(&contract_address);
let received = balance_after.saturating_sub(balance_before);
if received <= 0 {
return Err(Error::InvalidBondAmount);
}
if received != bond_amount {
assertion.bond = received;

@collinsezedike collinsezedike Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This correction write uses the original pre-transfer in-memory assertion struct, so it silently discards any state a reentrant call made during the transfer. With a token that's both fee-on-transfer and reentrant, a reentrant callback can call dispute(id) on this still-Pending assertion before the outer call resumes. dispute() succeeds and moves status to Disputed. When assert_outcome resumes and finds received != bond_amount, assertion.bond = received; Self::set_assertion(...) runs against the stale pre-transfer local (status: Pending, disputer: None), clobbering the reentrant dispute back to Pending while the disputer's tokens stay transferred in with no dispute record, stuck and unaccounted for. The existing evil_token reentrancy tests don't catch this because EvilToken's transfer always delivers the exact requested amount, so received != bond_amount is never true there.

Self::set_assertion(&env, id, &assertion);
}
Self::set_assertion_escrow(&env, id, received);

Asserted {
id,
Expand Down Expand Up @@ -1070,11 +1083,19 @@ impl Tholos {
Self::set_assertion(&env, id, &assertion);

let token_id: Address = Self::get(&env, &DataKey::Token)?;
token::Client::new(&env, &token_id).transfer(
&disputer,
env.current_contract_address(),
&assertion.bond,
);
let token_client = token::Client::new(&env, &token_id);
let contract_address = env.current_contract_address();
let balance_before = token_client.balance(&contract_address);

@collinsezedike collinsezedike Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This balance-before/transfer/balance-after/zero-check block is duplicated near-verbatim from assert_outcome above, differing only in the sender address and what happens with the result. A future fix to this pattern, handling a negative received differently, or adding a minimum-received threshold, has to be applied in two places. Consider extracting a shared helper.

token_client.transfer(&disputer, &contract_address, &assertion.bond);
let balance_after = token_client.balance(&contract_address);
let received = balance_after.saturating_sub(balance_before);
if received <= 0 {
return Err(Error::InvalidBondAmount);
}
let escrow = Self::get_assertion_escrow(&env, id, &assertion)
.checked_add(received)
.ok_or(Error::TokenTransferMismatch)?;
Self::set_assertion_escrow(&env, id, escrow);

Disputed { id, disputer }.publish(&env);

Expand Down Expand Up @@ -1115,13 +1136,6 @@ impl Tholos {
return Err(Error::ChallengeWindowOpen);
}

let reward_bps: u32 = Self::get(&env, &DataKey::FinalizeRewardBps)?;
let reward = if reward_bps > 0 {
assertion.bond * (reward_bps as i128) / 10_000
} else {
0
};

// State is written before the external token transfers below so that
// a reentrant call from a non-standard token sees this assertion as
// already resolved, rather than still `Pending`.
Expand All @@ -1130,8 +1144,18 @@ impl Tholos {
assertion.finalizer = Some(caller.clone());
Self::set_assertion(&env, id, &assertion);

let reward_bps: u32 = Self::get(&env, &DataKey::FinalizeRewardBps)?;
let token_id: Address = Self::get(&env, &DataKey::Token)?;
let token_client = token::Client::new(&env, &token_id);
let contract_balance = token_client.balance(&env.current_contract_address());
let escrow = Self::get_assertion_escrow(&env, id, &assertion);
let total_payout = assertion.bond.min(escrow).min(contract_balance);

let reward = if reward_bps > 0 {
total_payout * (reward_bps as i128) / 10_000
} else {
0
};

if reward > 0 {
// Pay the caller their reward first, then pay the asserter the
Expand All @@ -1140,12 +1164,14 @@ impl Tholos {
token_client.transfer(&env.current_contract_address(), &caller, &reward);
}

let asserter_payout = assertion.bond - reward;
token_client.transfer(
&env.current_contract_address(),
&assertion.asserter,
&asserter_payout,
);
let asserter_payout = total_payout - reward;
if asserter_payout > 0 {
token_client.transfer(
&env.current_contract_address(),
&assertion.asserter,
&asserter_payout,
);
}

Finalized {
id,
Expand Down Expand Up @@ -1220,7 +1246,6 @@ impl Tholos {
return Ok(None);
};

let payout = assertion.bond * 2;
let winner = if winner_is_asserter {
assertion.asserter.clone()
} else {
Expand All @@ -1244,11 +1269,17 @@ impl Tholos {
Self::set_assertion(&env, id, &assertion);

let token_id: Address = Self::get(&env, &DataKey::Token)?;
token::Client::new(&env, &token_id).transfer(
&env.current_contract_address(),
&winner,
&payout,
);
let token_client = token::Client::new(&env, &token_id);
let contract_balance = token_client.balance(&env.current_contract_address());
let escrow = Self::get_assertion_escrow(&env, id, &assertion);
let payout = assertion
.bond
.saturating_mul(2)
.min(escrow)
.min(contract_balance);

@collinsezedike collinsezedike Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This capped-payout computation is duplicated between finalize (above, around line 877) and here, with only the bond multiplier differing. A change to the capping rule has to be made twice, and the two copies have already drifted slightly. Consider a shared helper.

if payout > 0 {
token_client.transfer(&env.current_contract_address(), &winner, &payout);
}
Resolved {
id,
outcome: final_outcome,
Expand All @@ -1269,6 +1300,23 @@ impl Tholos {
.ok_or(Error::AssertionNotFound)
}

fn get_assertion_escrow(env: &Env, id: u64, assertion: &Assertion) -> i128 {

@collinsezedike collinsezedike Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This fallback assumes both bonds are already in the contract, but dispute calls it before the disputer's bond has been added. For any assertion missing an AssertionEscrow entry (pre-upgrade data with no such record yet), this returns bond2 as if the disputer already paid, then dispute's checked_add adds the disputer's actual payment on top, storing an escrow of bond2 + received instead of bond + received. Only the separate contract_balance cap elsewhere currently prevents this from being paid out, so the bug is masked rather than fixed.

@collinsezedike collinsezedike Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This fallback for an assertion with no stored AssertionEscrow assumes a full 2x bond was already escrowed. At the point dispute() calls this, only the asserter's single deposit has actually happened, so for a pre-migration assertion this overstates true escrow by a full bond. Concretely: a pre-upgrade Pending assertion (no AssertionEscrow(id) entry) gets disputed post-upgrade with a 10% fee-on-transfer token. dispute() computes new escrow as this fallback (bond2, e.g. 200) plus the disputer's actually-received amount (e.g. 90), storing escrow=290, when the contract's real funds tied to this id are only about 180. resolve()'s min(bond2, escrow, contract_balance) can then reach 200 if other assertions' pooled balance covers it, paying this assertion 20 more than it actually escrowed out of other assertions' funds, the exact cross-assertion drain AssertionEscrow was added to prevent.

env.storage()
.persistent()
.get(&DataKey::AssertionEscrow(id))
.unwrap_or(assertion.bond.saturating_mul(2))
}

fn set_assertion_escrow(env: &Env, id: u64, escrow: i128) {
let key = DataKey::AssertionEscrow(id);
env.storage().persistent().set(&key, &escrow);
env.storage().persistent().extend_ttl(
&key,
ASSERTION_LIFETIME_THRESHOLD,
ASSERTION_BUMP_AMOUNT,
);
}

/// Writes an assertion and extends its persistent storage TTL. Every
/// write site uses this rather than a bare `.set()` so an assertion's
/// ledger entry can't be archived out from under it while it's still
Expand Down
Loading
Loading