fix(v2): enter reentrancy guard at start of entrypoints (#132)

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<u64, Error> {
         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<i128, Error> {
         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,

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();