From 60a2d76ce0a28eefd2489b77368dcc2aa5bcc6f9 Mon Sep 17 00:00:00 2001
From: Omowumi Balogun <omowumibalogun81@gmail.com>
Date: Mon, 22 Jun 2026 07:07:27 +0100
Subject: [PATCH] fix: apply CEI pattern and reentrancy guard to
 release_payment

---
 contracts/payment/src/lib.rs | 97 ++++++++++++++++++++++++++++++++----
 1 file changed, 86 insertions(+), 11 deletions(-)

diff --git a/contracts/payment/src/lib.rs b/contracts/payment/src/lib.rs
index 54c9faf..157d5e8 100644
--- a/contracts/payment/src/lib.rs
+++ b/contracts/payment/src/lib.rs
@@ -75,6 +75,10 @@ pub enum DataKey {
     /// Fee in basis points (100 bps = 1 %). Default: 100 (1 %).
     FeeBps,
     Payment(u64),
+    /// Reentrancy guard: set to true while release_payment is executing for
+    /// this order ID.  Prevents a malicious token from re-entering and
+    /// draining escrow twice.
+    Releasing(u64),
 }
 
 // ---------------------------------------------------------------------------
@@ -182,12 +186,14 @@ impl PaymentContract {
     // Release / Refund (admin or restaurant wallet)
     // -----------------------------------------------------------------------
 
-    /// Release escrowed funds to the restaurant.
+    /// Release escrowed funds to the restaurant following Checks-Effects-
+    /// Interactions (CEI) order with an explicit reentrancy guard.
     ///
     /// Callable by the admin or the restaurant wallet recorded in the payment.
     /// The platform fee is sent to the treasury; the remainder goes to the
     /// restaurant wallet.
     pub fn release_payment(env: Env, caller: Address, order_id: u64) {
+        // ── CHECKS ────────────────────────────────────────────────────────
         caller.require_auth();
 
         let mut payment: Payment = env
@@ -205,6 +211,36 @@ impl PaymentContract {
             panic!("unauthorized");
         }
 
+        // Reentrancy guard: reject any re-entry for this order during
+        // execution (e.g. a malicious token calling back into release_payment
+        // inside its transfer() implementation).
+        if env
+            .storage()
+            .instance()
+            .has(&DataKey::Releasing(order_id))
+        {
+            panic!("reentrant call");
+        }
+        env.storage()
+            .instance()
+            .set(&DataKey::Releasing(order_id), &true);
+
+        // ── EFFECTS ───────────────────────────────────────────────────────
+        // Persist the status change BEFORE any external calls so that any
+        // reentrant invocation sees Released (not Escrowed) and is rejected
+        // by the status check above.
+        payment.status = PaymentStatus::Released;
+        payment.settled_at = env.ledger().timestamp();
+
+        let ttl: u32 = 2_073_600;
+        env.storage()
+            .persistent()
+            .set(&DataKey::Payment(order_id), &payment);
+        env.storage()
+            .persistent()
+            .extend_ttl(&DataKey::Payment(order_id), ttl, ttl);
+
+        // ── INTERACTIONS ──────────────────────────────────────────────────
         let token_client = token::Client::new(&env, &payment.token);
         let net_amount = payment.amount - payment.fee_amount;
 
@@ -225,16 +261,10 @@ impl PaymentContract {
             );
         }
 
-        payment.status = PaymentStatus::Released;
-        payment.settled_at = env.ledger().timestamp();
-
-        let ttl: u32 = 2_073_600;
-        env.storage()
-            .persistent()
-            .set(&DataKey::Payment(order_id), &payment);
+        // Clear reentrancy guard.
         env.storage()
-            .persistent()
-            .extend_ttl(&DataKey::Payment(order_id), ttl, ttl);
+            .instance()
+            .remove(&DataKey::Releasing(order_id));
 
         env.events().publish(
             (symbol_short!("released"), symbol_short!("pay")),
@@ -436,4 +466,49 @@ mod test {
         client.escrow_payment(&payer, &3, &restaurant, &token_addr, &20_000_000);
         client.escrow_payment(&payer, &3, &restaurant, &token_addr, &20_000_000);
     }
-}
+
+    /// Verifies CEI ordering: after release_payment the status must be
+    /// Released, confirming state was persisted before the transfers ran.
+    #[test]
+    fn test_release_status_is_released_after_successful_call() {
+        let (env, client, admin, _treasury, _cid) = setup();
+        let token_admin = Address::generate(&env);
+        let payer = Address::generate(&env);
+        let restaurant = Address::generate(&env);
+
+        let (token_addr, sac) = create_token(&env, &token_admin);
+        sac.mint(&payer, &100_000_000);
+
+        client.escrow_payment(&payer, &4, &restaurant, &token_addr, &20_000_000);
+        client.release_payment(&admin, &4);
+
+        let payment = client.get_payment(&4);
+        assert_eq!(
+            payment.status,
+            PaymentStatus::Released,
+            "status must be Released after successful release_payment"
+        );
+    }
+
+    /// A second release call on the same order must be rejected.
+    ///
+    /// This captures the reentrancy scenario: because status is set to
+    /// Released BEFORE the token transfers (CEI fix), any reentrant
+    /// release_payment() call during transfer() sees status=Released and
+    /// panics — preventing a double-spend.
+    #[test]
+    #[should_panic(expected = "payment is not in escrow")]
+    fn test_double_release_panics_protecting_against_reentrancy() {
+        let (env, client, admin, _treasury, _cid) = setup();
+        let token_admin = Address::generate(&env);
+        let payer = Address::generate(&env);
+        let restaurant = Address::generate(&env);
+
+        let (token_addr, sac) = create_token(&env, &token_admin);
+        sac.mint(&payer, &100_000_000);
+
+        client.escrow_payment(&payer, &5, &restaurant, &token_addr, &30_000_000);
+        client.release_payment(&admin, &5); // first call succeeds
+        client.release_payment(&admin, &5); // reentrant/duplicate call must panic
+    }
+}
\ No newline at end of file