diff --git a/contracts/tholos-v2/src/lib.rs b/contracts/tholos-v2/src/lib.rs index 9f2fccc..6afe249 100644 --- a/contracts/tholos-v2/src/lib.rs +++ b/contracts/tholos-v2/src/lib.rs @@ -852,22 +852,15 @@ impl TholosV2 { } /// Acquires the contract-wide reentrancy mutex, failing with - /// `ReentrancyGuardActive` if it's already held. Every function that - /// initiates an external token transfer calls this immediately before - /// that transfer (after writing whatever state the transfer follows, - /// matching the existing state-before-external-call ordering) and - /// `exit_reentrancy_guard` immediately after. A non-standard token - /// whose `transfer` implementation calls back into this contract mid- - /// transfer, instead of a well-behaved SEP-41 token that just updates - /// balances, would otherwise be able to act on state that looks - /// complete (because it was written before the transfer) while the - /// tokens backing it haven't actually moved yet. + /// `ReentrancyGuardActive` if it's already held. Guarded entrypoints + /// should call this immediately after authentication, before validation + /// or any state changes that a reentrant call could observe or act on. + /// The guard remains held through any external token transfer and must + /// be released with `exit_reentrancy_guard` afterward. /// /// `reveal`, `resolve_outcome`, `settle`, and `cancel_round` also check - /// this at their own entry, even though none of them move tokens - /// themselves: all four can act on a position's weight, credit, or - /// terminal state, which the guard above exists specifically to keep - /// provisional until its funding transfer actually completes. + /// this guard on entry so they cannot act on partially completed state + /// while a guarded entrypoint is in progress. fn enter_reentrancy_guard(env: &Env) -> Result<(), Error> { Self::check_reentrancy_guard(env)?; env.storage() @@ -961,6 +954,7 @@ impl TholosV2 { /// the new assertion id. Emits `Asserted`. pub fn assert_outcome(env: Env, asserter: Address, outcome: bool) -> Result { asserter.require_auth(); + Self::enter_reentrancy_guard(&env)?; Self::require_not_paused(&env)?; let policy: PolicySnapshotV2 = env @@ -975,7 +969,6 @@ impl TholosV2 { // not-yet-incremented id. let id = Self::create_pending_assertion(&env, asserter.clone(), outcome)?; - Self::enter_reentrancy_guard(&env)?; token::Client::new(&env, &policy.token).transfer( &asserter, env.current_contract_address(), @@ -1072,6 +1065,7 @@ impl TholosV2 { /// Emits `Disputed`. pub fn dispute(env: Env, disputer: Address, id: u64) -> Result<(), Error> { disputer.require_auth(); + Self::enter_reentrancy_guard(&env)?; let mut assertion: AssertionV2 = env .storage() @@ -1138,7 +1132,6 @@ impl TholosV2 { }; Self::set_resolution(&env, id, &resolution, &policy); - Self::enter_reentrancy_guard(&env)?; token::Client::new(&env, &policy.token).transfer( &disputer, env.current_contract_address(), @@ -1185,6 +1178,7 @@ impl TholosV2 { commitment: BytesN<32>, ) -> Result<(), Error> { voter.require_auth(); + Self::enter_reentrancy_guard(&env)?; let assertion: AssertionV2 = env .storage() @@ -1280,7 +1274,6 @@ impl TholosV2 { resolution.eligible_total = new_total; Self::set_resolution(&env, id, &resolution, &assertion.policy); - Self::enter_reentrancy_guard(&env)?; token::Client::new(&env, &assertion.policy.token).transfer( &voter, env.current_contract_address(), @@ -1972,7 +1965,7 @@ impl TholosV2 { destination: Address, ) -> Result { owner.require_auth(); - Self::check_reentrancy_guard(&env)?; + Self::enter_reentrancy_guard(&env)?; let assertion: AssertionV2 = env .storage() @@ -2015,7 +2008,6 @@ impl TholosV2 { .ok_or(Error::SettlementArithmeticOverflow)?; Self::set_resolution(&env, id, &resolution, &assertion.policy); - Self::enter_reentrancy_guard(&env)?; token::Client::new(&env, &assertion.policy.token).transfer( &env.current_contract_address(), &destination, diff --git a/contracts/tholos-v2/src/test.rs b/contracts/tholos-v2/src/test.rs index 776ecd5..9eb85bc 100644 --- a/contracts/tholos-v2/src/test.rs +++ b/contracts/tholos-v2/src/test.rs @@ -2586,10 +2586,9 @@ fn test_reentrancy_guard_blocks_calls_while_held() { // reentered mid-transfer and never released it, without needing a // custom malicious-token contract to actually trigger reentrancy. // - // dispute() and register() only check the guard right before their own - // transfer (after their other validation), so each needs its own state - // that would otherwise succeed, to prove the guard is what's actually - // blocking them rather than an unrelated validation error. + // All guarded entrypoints check or acquire the guard at entry (immediately + // after auth), keeping validation and state changes protected while + // a transfer is in flight. let f = Fixture::new(); let asserter = f.funded_address(); let disputer = f.funded_address(); @@ -2682,6 +2681,61 @@ fn test_reentrancy_guard_blocks_calls_while_held() { assert_eq!(cause, TerminalCause::OptimisticTimeout); } +#[test] +fn test_reentrancy_guard_blocks_calls_before_validation() { + // This regression test does not simulate an external callback. Instead, it + // holds the reentrancy guard before each entrypoint call and deliberately + // supplies inputs that would fail at the old pre-guard validation points: + // assert_outcome is paused, while dispute/register/withdraw use invalid or + // nonexistent assertion state. If any of those validations runs before the + // guard, a different error would be returned. ReentrancyGuardActive winning + // in every case therefore pins down the required ordering: after + // require_auth(), the guard must be acquired before validation or state + // lookups. + let f = Fixture::new(); + let asserter = f.funded_address(); + let disputer = f.funded_address(); + let voter = f.funded_address(); + + // Pause contract so assert_outcome would otherwise fail with Paused. + f.client.set_paused_v2(&true); + + // Hold the guard. + f.env.as_contract(&f.client.address, || { + f.env + .storage() + .instance() + .set(&DataKey::ReentrancyGuard, &true); + }); + + // Without the early guard this would return Paused first. + assert_eq!( + f.client.try_assert_outcome(&asserter, &true), + Err(Ok(Error::ReentrancyGuardActive)) + ); + + // Without the early guard this would return AssertionNotFound first. + assert_eq!( + f.client.try_dispute(&disputer, &999_999), + Err(Ok(Error::ReentrancyGuardActive)) + ); + + // Without the early guard this would reach the invalid amount/assertion + // validation first. + assert_eq!( + f.client + .try_register(&voter, &999_999, &0i128, &commitment(&f.env, 1)), + Err(Ok(Error::ReentrancyGuardActive)) + ); + + // Without the early guard this would reach the assertion lookup/credit + // validation first. + assert_eq!( + f.client.try_withdraw(&asserter, &999_999, &asserter), + Err(Ok(Error::ReentrancyGuardActive)) + ); +} + #[test] fn test_set_paused_v2_blocks_new_assertions() { let f = Fixture::new(); diff --git a/contracts/tholos-v2/test_snapshots/test/test_reentrancy_guard_blocks_calls_before_validation.1.json b/contracts/tholos-v2/test_snapshots/test/test_reentrancy_guard_blocks_calls_before_validation.1.json new file mode 100644 index 0000000..bdbdceb --- /dev/null +++ b/contracts/tholos-v2/test_snapshots/test/test_reentrancy_guard_blocks_calls_before_validation.1.json @@ -0,0 +1,805 @@ +{ + "generators": { + "address": 7, + "nonce": 0, + "mux_id": 0 + }, + "auth": [ + [], + [ + [ + "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEGWF", + { + "function": { + "contract_fn": { + "contract_address": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "function_name": "set_admin", + "args": [ + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM" + } + ] + } + }, + "sub_invocations": [] + } + ] + ], + [], + [ + [ + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4", + { + "function": { + "contract_fn": { + "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M", + "function_name": "initialize", + "args": [ + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4" + }, + { + "address": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL" + }, + { + "i128": "100" + }, + { + "u64": "3600" + }, + { + "u32": 0 + }, + { + "u64": "3600" + }, + { + "u64": "300" + }, + { + "u64": "3900" + }, + { + "u64": "3600" + }, + { + "i128": "1000000" + }, + { + "i128": "10000000" + } + ] + } + }, + "sub_invocations": [] + } + ] + ], + [ + [ + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", + { + "function": { + "contract_fn": { + "contract_address": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "function_name": "mint", + "args": [ + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAK3IM" + }, + { + "i128": "1000" + } + ] + } + }, + "sub_invocations": [] + } + ] + ], + [ + [ + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", + { + "function": { + "contract_fn": { + "contract_address": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "function_name": "mint", + "args": [ + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMDR4" + }, + { + "i128": "1000" + } + ] + } + }, + "sub_invocations": [] + } + ] + ], + [ + [ + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", + { + "function": { + "contract_fn": { + "contract_address": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "function_name": "mint", + "args": [ + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM" + }, + { + "i128": "1000" + } + ] + } + }, + "sub_invocations": [] + } + ] + ], + [ + [ + "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4", + { + "function": { + "contract_fn": { + "contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M", + "function_name": "set_paused_v2", + "args": [ + { + "bool": true + } + ] + } + }, + "sub_invocations": [] + } + ] + ], + [], + [], + [], + [], + [] + ], + "ledger": { + "protocol_version": 26, + "sequence_number": 0, + "timestamp": 0, + "network_id": "0000000000000000000000000000000000000000000000000000000000000000", + "base_reserve": 0, + "min_persistent_entry_ttl": 4096, + "min_temp_entry_ttl": 16, + "max_entry_ttl": 6312000, + "ledger_entries": [ + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "account": { + "account_id": "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEGWF", + "balance": "0", + "seq_num": "0", + "num_sub_entries": 0, + "inflation_dest": null, + "flags": 0, + "home_domain": "", + "thresholds": "01010101", + "signers": [], + "ext": "v0" + } + }, + "ext": "v0" + }, + "live_until": null + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEGWF", + "key": { + "ledger_key_nonce": { + "nonce": "801925984706572462" + } + }, + "durability": "temporary", + "val": "void" + } + }, + "ext": "v0" + }, + "live_until": 6311999 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", + "key": { + "ledger_key_nonce": { + "nonce": "1033654523790656264" + } + }, + "durability": "temporary", + "val": "void" + } + }, + "ext": "v0" + }, + "live_until": 6311999 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", + "key": { + "ledger_key_nonce": { + "nonce": "2032731177588607455" + } + }, + "durability": "temporary", + "val": "void" + } + }, + "ext": "v0" + }, + "live_until": 6311999 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", + "key": { + "ledger_key_nonce": { + "nonce": "4837995959683129791" + } + }, + "durability": "temporary", + "val": "void" + } + }, + "ext": "v0" + }, + "live_until": 6311999 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M", + "key": "ledger_key_contract_instance", + "durability": "persistent", + "val": { + "contract_instance": { + "executable": { + "wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + }, + "storage": [ + { + "key": { + "vec": [ + { + "symbol": "Admin" + } + ] + }, + "val": { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4" + } + }, + { + "key": { + "vec": [ + { + "symbol": "NextId" + } + ] + }, + "val": { + "u64": "0" + } + }, + { + "key": { + "vec": [ + { + "symbol": "Paused" + } + ] + }, + "val": { + "bool": true + } + }, + { + "key": { + "vec": [ + { + "symbol": "Policy" + } + ] + }, + "val": { + "map": [ + { + "key": { + "symbol": "anti_snipe_extension_secs" + }, + "val": { + "u64": "300" + } + }, + { + "key": { + "symbol": "anti_snipe_hard_max_secs" + }, + "val": { + "u64": "3900" + } + }, + { + "key": { + "symbol": "base_bond" + }, + "val": { + "i128": "100" + } + }, + { + "key": { + "symbol": "challenge_window_secs" + }, + "val": { + "u64": "3600" + } + }, + { + "key": { + "symbol": "finalize_reward_bps" + }, + "val": { + "u32": 0 + } + }, + { + "key": { + "symbol": "max_position" + }, + "val": { + "i128": "1000000" + } + }, + { + "key": { + "symbol": "max_total_weight" + }, + "val": { + "i128": "10000000" + } + }, + { + "key": { + "symbol": "min_resolution_bond" + }, + "val": { + "i128": "100" + } + }, + { + "key": { + "symbol": "payout_rule" + }, + "val": { + "vec": [ + { + "symbol": "ProRataV1" + } + ] + } + }, + { + "key": { + "symbol": "registration_duration_secs" + }, + "val": { + "u64": "3600" + } + }, + { + "key": { + "symbol": "reveal_duration_secs" + }, + "val": { + "u64": "3600" + } + }, + { + "key": { + "symbol": "timeout_default" + }, + "val": { + "vec": [ + { + "symbol": "AssertedOutcomeStands" + } + ] + } + }, + { + "key": { + "symbol": "token" + }, + "val": { + "address": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL" + } + }, + { + "key": { + "symbol": "weight_rule" + }, + "val": { + "vec": [ + { + "symbol": "LinearStakeV1" + } + ] + } + } + ] + } + }, + { + "key": { + "vec": [ + { + "symbol": "ReentrancyGuard" + } + ] + }, + "val": { + "bool": true + } + } + ] + } + } + } + }, + "ext": "v0" + }, + "live_until": 518400 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4", + "key": { + "ledger_key_nonce": { + "nonce": "4270020994084947596" + } + }, + "durability": "temporary", + "val": "void" + } + }, + "ext": "v0" + }, + "live_until": 6311999 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4", + "key": { + "ledger_key_nonce": { + "nonce": "5541220902715666415" + } + }, + "durability": "temporary", + "val": "void" + } + }, + "ext": "v0" + }, + "live_until": 6311999 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "key": { + "vec": [ + { + "symbol": "Balance" + }, + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAK3IM" + } + ] + }, + "durability": "persistent", + "val": { + "map": [ + { + "key": { + "symbol": "amount" + }, + "val": { + "i128": "1000" + } + }, + { + "key": { + "symbol": "authorized" + }, + "val": { + "bool": true + } + }, + { + "key": { + "symbol": "clawback" + }, + "val": { + "bool": false + } + } + ] + } + } + }, + "ext": "v0" + }, + "live_until": 518400 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "key": { + "vec": [ + { + "symbol": "Balance" + }, + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMDR4" + } + ] + }, + "durability": "persistent", + "val": { + "map": [ + { + "key": { + "symbol": "amount" + }, + "val": { + "i128": "1000" + } + }, + { + "key": { + "symbol": "authorized" + }, + "val": { + "bool": true + } + }, + { + "key": { + "symbol": "clawback" + }, + "val": { + "bool": false + } + } + ] + } + } + }, + "ext": "v0" + }, + "live_until": 518400 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "key": { + "vec": [ + { + "symbol": "Balance" + }, + { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAOLZM" + } + ] + }, + "durability": "persistent", + "val": { + "map": [ + { + "key": { + "symbol": "amount" + }, + "val": { + "i128": "1000" + } + }, + { + "key": { + "symbol": "authorized" + }, + "val": { + "bool": true + } + }, + { + "key": { + "symbol": "clawback" + }, + "val": { + "bool": false + } + } + ] + } + } + }, + "ext": "v0" + }, + "live_until": 518400 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CBEPDNVYXQGWB5YUBXKJWYJA7OXTZW5LFLNO5JRRGE6Z6C5OSUZPCCEL", + "key": "ledger_key_contract_instance", + "durability": "persistent", + "val": { + "contract_instance": { + "executable": "stellar_asset", + "storage": [ + { + "key": { + "symbol": "METADATA" + }, + "val": { + "map": [ + { + "key": { + "symbol": "decimal" + }, + "val": { + "u32": 7 + } + }, + { + "key": { + "symbol": "name" + }, + "val": { + "string": "aaa:GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEGWF" + } + }, + { + "key": { + "symbol": "symbol" + }, + "val": { + "string": "aaa" + } + } + ] + } + }, + { + "key": { + "vec": [ + { + "symbol": "Admin" + } + ] + }, + "val": { + "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM" + } + }, + { + "key": { + "vec": [ + { + "symbol": "AssetInfo" + } + ] + }, + "val": { + "vec": [ + { + "symbol": "AlphaNum4" + }, + { + "map": [ + { + "key": { + "symbol": "asset_code" + }, + "val": { + "string": "aaa\\0" + } + }, + { + "key": { + "symbol": "issuer" + }, + "val": { + "bytes": "0000000000000000000000000000000000000000000000000000000000000002" + } + } + ] + } + ] + } + } + ] + } + } + } + }, + "ext": "v0" + }, + "live_until": 120960 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_code": { + "ext": "v0", + "hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "code": "" + } + }, + "ext": "v0" + }, + "live_until": 518400 + } + ] + }, + "events": [] +} \ No newline at end of file