fix(tholos): verify balance deltas for incoming token transfers (#164) by xtep103 · Pull Request #180 · drydocs/tholos

xtep103 · 2026-09-04T14:08:52Z

Overview

Verifies balance deltas for incoming token transfers during assertion creation (assert_outcome) and disputes (dispute) in contracts/tholos, recording actual received tokens and protecting resolve and finalize payouts against deadlocks caused by fee-on-transfer tokens.

Related Issue

Closes #164

Changes

contracts/tholos

[MODIFY] contracts/tholos/src/lib.rs
- Added balance delta checks around incoming token transfers in assert_outcome and dispute.
- If actual received tokens are less than requested, records the received amount into assertion.bond in persistent storage. Rejects zero/negative received amounts with Error::InvalidBondAmount.
- Clamped payouts in finalize (assertion.bond.min(contract_balance)) and resolve ((assertion.bond.saturating_mul(2)).min(contract_balance)) to prevent transfer failure panics when contract balances are constrained by transfer deductions.
[MODIFY] contracts/tholos/src/test.rs
- Added FeeToken test fixture simulating configurable fee-on-transfer mechanics.
- Added unit test test_fee_on_transfer_token_dispute_resolves_without_deadlock confirming assertions with fee tokens can be disputed and resolved without balance underflow.
- Added unit test test_fee_on_transfer_token_finalize_resolves_without_deadlock confirming undisputed assertions with fee tokens finalize successfully.
- Added unit test test_zero_received_transfer_rejected ensuring 100% fee transfers that deliver 0 tokens are rejected.

Verification Results

running 79 tests
test test::fee_token::test::test_fee_token_transfer ... ok
test test::test_fee_on_transfer_token_dispute_resolves_without_deadlock ... ok
test test::test_fee_on_transfer_token_finalize_resolves_without_deadlock ... ok
test test::test_zero_received_transfer_rejected ... ok
...
test result: ok. 79 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

cargo test --workspace --locked: 189 tests passed (79 tholos, 110 tholos-v2), 0 failed
cargo clippy --workspace --all-targets --locked -- -D warnings: 0 warnings
cargo fmt --check: OK

Acceptance Criteria	Status
Balance deltas verified around token transfer calls in `assert_outcome` & `dispute`	✅ Implemented and verified
Actually received amount recorded in storage if short; 0-amount rejected	✅ Tested with `Error::InvalidBondAmount`
`resolve` / `finalize` clamped to available balance to prevent deadlock	✅ Tested and verified
Unit tests covering fee-on-transfer resolution & zero-transfer rejection	✅ All tests passing

…ocs#164)

collinsezedike

Worth a separate follow-up issue, not blocking this one: contracts/tholos-v2 has no equivalent balance-delta check at all, so it remains exposed to the same fee-on-transfer deadlock this PR fixes only in v1.

collinsezedike · 2026-09-04T18:37:21Z

-        );
+        let token_client = token::Client::new(&env, &token_id);
+        let contract_balance = token_client.balance(&env.current_contract_address());
+        let payout = (assertion.bond.saturating_mul(2)).min(contract_balance);


This caps payout against token_client.balance(&env.current_contract_address()), the whole contract's pooled balance across every pending assertion, not a per-assertion tracked amount. If a fee-on-transfer token shorts one assertion's transfer, that assertion's payout can still pay out its full nominal amount by drawing on a completely separate, unrelated assertion's escrowed bond, and when that second assertion later resolves or finalizes, it comes up short by exactly that amount. Capping against the total contract balance only prevents the contract from ever attempting to pay out more than it holds in aggregate, it doesn't stop one assertion's shortfall from being silently subsidized by another's escrow. Consider tracking and capping against this specific assertion's own received balance instead of the contract-wide total.

collinsezedike · 2026-09-05T15:21:13Z

-        );
+        let token_client = token::Client::new(&env, &token_id);
+        let contract_address = env.current_contract_address();
+        let balance_before = token_client.balance(&contract_address);


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.

collinsezedike · 2026-09-05T15:21:14Z

-        );
+        let token_client = token::Client::new(&env, &token_id);
+        let contract_address = env.current_contract_address();
+        let balance_before = token_client.balance(&contract_address);


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.

collinsezedike · 2026-09-05T15:21:14Z

+            .bond
+            .saturating_mul(2)
+            .min(escrow)
+            .min(contract_balance);


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.

collinsezedike · 2026-09-05T15:21:14Z

            .ok_or(Error::AssertionNotFound)
    }

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


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 · 2026-09-05T15:57:31Z

CI is failing on the sdk job: packages/tholos-sdk/src is out of date with the new Error variant this PR adds. Regenerate the bindings (see packages/tholos-sdk/README.md) and commit the result.

collinsezedike · 2026-09-07T11:04:40Z

This PR now has a merge conflict with main. Please rebase and resolve it. The SDK bindings fix from the last commit looks correct, this is purely about the conflict.

…conflict

collinsezedike

This PR adds an AssertionEscrow tracking mechanism used by finalize and resolve, but two paths this diff touches don't consistently apply it, both inline. Separately, reclaim_stalled_dispute (not touched by this diff, pre-existing from #184) has the same class of gap, filing that as its own issue rather than blocking this PR on it.

collinsezedike · 2026-09-07T17:52:34Z

+            return Err(Error::InvalidBondAmount);
+        }
+        if received != bond_amount {
+            assertion.bond = received;


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.

collinsezedike · 2026-09-07T17:52:34Z

            .ok_or(Error::AssertionNotFound)
    }

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


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.

collinsezedike · 2026-09-10T09:14:42Z

@xtep103 Checking in, any progress on the two open findings? Let us know if you're still working it or need to hand it off.

xtep103 · 2026-09-10T10:31:54Z

@xtep103 Checking in, any progress on the two open findings? Let us know if you're still working it or need to hand it off.

I'm still working on it.

collinsezedike · 2026-09-14T09:39:48Z

@xtep103 Closing this. #216 is now the consolidated PR for this issue, it already carries the AssertionEscrow mechanism this PR introduced plus the review feedback since. Push any further work there.

fix(tholos): verify balance deltas for incoming token transfers (dryd…

b0b0fab

…ocs#164)

xtep103 force-pushed the feat/reentrancy-guard-early-check branch from 629f37c to b0b0fab Compare September 4, 2026 14:50

xtep103 changed the title ~~Fix fee-on-transfer bond accounting~~ fix(tholos): verify balance deltas for incoming token transfers (#164) Sep 4, 2026

xtep103 mentioned this pull request Sep 4, 2026

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

Closed

collinsezedike requested changes Sep 4, 2026

View reviewed changes

xtep103 added 4 commits September 4, 2026 19:57

Cap payouts by assertion escrow

4210a56

Merge main and resolve payout escrow conflict

9d116d6

Update SDK binding for assertion escrow

73691de

Add token transfer mismatch error

762ab16

collinsezedike requested changes Sep 5, 2026

View reviewed changes

xtep103 added 2 commits September 5, 2026 17:52

Preserve storage key ordering

cc7e94a

Preserve storage key ordering

ef143e1

collinsezedike mentioned this pull request Sep 7, 2026

docs: document resolver committee onboarding and offboarding process #206

Merged

4 tasks

Merge origin/main into feat/reentrancy-guard-early-check and resolve …

2f5ed8f

…conflict

collinsezedike requested changes Sep 7, 2026

View reviewed changes

This was referenced Sep 7, 2026

[Bug] reclaim_stalled_dispute doesn't account for fee-on-transfer shortfall #207

Open

Reclaim stalled dispute #211

Open

collinsezedike mentioned this pull request Sep 12, 2026

fix(tholos): account for fee-on-transfer tokens #216

Open

Merge branch 'main' into feat/reentrancy-guard-early-check

509249e

collinsezedike closed this Sep 14, 2026

Uh oh!

Conversation

xtep103 commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Related Issue

Changes

contracts/tholos

Verification Results

Uh oh!

collinsezedike left a comment

Choose a reason for hiding this comment

Uh oh!

collinsezedike Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

collinsezedike Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

collinsezedike Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

collinsezedike Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

collinsezedike Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

collinsezedike commented Sep 5, 2026

Uh oh!

collinsezedike commented Sep 7, 2026

Uh oh!

collinsezedike left a comment

Choose a reason for hiding this comment

Uh oh!

collinsezedike Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

collinsezedike Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

collinsezedike commented Sep 10, 2026

Uh oh!

xtep103 commented Sep 10, 2026

Uh oh!

collinsezedike commented Sep 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xtep103 commented Sep 4, 2026 •

edited

Loading