Fix race conditions and negative balance vulnerabilities

[reubenyap]

Summary

• Replace non-atomic read-then-write balance operations with atomic find_one_and_update with $gte guards
• Prevents double-spending and negative balance exploits in tips, withdrawals, and envelope claims
• Adds proper locking checks to withdrawal flow

Test plan

• Verify tips fail gracefully when sender has insufficient balance
• Verify concurrent tip attempts cannot overdraw an account
• Verify withdrawals check balance atomically
• Verify envelope claims are atomic and cannot be double-claimed

---

Note

High Risk
Modifies core balance accounting for tips, withdrawals, deposits, and envelopes using new atomic MongoDB update patterns; mistakes here could incorrectly credit/debit funds or lock balances.

Overview
Prevents double-spend/negative balances by replacing read-then-write balance updates with atomic MongoDB operations (primarily find_one_and_update with $gte guards and $inc) across tips, withdrawals, deposit processing, and red envelope creation/claims.

Improves value precision and safety by introducing Decimal helpers (to_decimal, decimal_to_store) for 8-decimal FIRO amounts, tightening amount validation (non-negative/minimums/fee checks), and adjusting withdrawal flow to move funds to Locked atomically with rollback on wallet send failure.

Operational changes include deduping already-processed transactions in update_balance via txId checks and replacing print/traceback usage with structured logging (including exc_info).

^{Written by Cursor Bugbot for commit 3c24ddd. This will update automatically on new commits. Configure here.}

[reubenyap]

bugbot run

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

[cursor]

Missing rollback when wallet API throws exception

High Severity

In withdraw_coins, the atomic find_one_and_update deducts the user's Balance and increments Locked before calling spendspark. If spendspark raises an exception (network timeout, connection error, JSON decode failure) rather than returning an error response, execution jumps to the outer except block, which only logs the error — no rollback is performed. The user's funds are permanently deducted from Balance and stuck in Locked with no corresponding on-chain transaction. The old code was safe here because the database update happened after the successful API call.

 

[navidR]

This should fix this PR:

diff --git a/tipbot.py b/tipbot.py
index c51c0aa..b81da5d 100644
--- a/tipbot.py
+++ b/tipbot.py
@@ -17,10 +17,11 @@ import time
 from decimal import Decimal, ROUND_DOWN
 from pymongo import MongoClient, ReturnDocument
 from telegram import Bot, InlineKeyboardMarkup, InlineKeyboardButton
+from telegram.utils.request import Request
 import uuid
 from api.firo_wallet_api import FiroWalletAPI
 
-plt.style.use('seaborn-whitegrid')
+plt.style.use('default')
 
 logger = logging.getLogger(__name__)
 logger.setLevel(logging.INFO)
@@ -66,7 +67,9 @@ WELCOME_MESSAGE = """
 class TipBot:
     def __init__(self, wallet_api):
         # INIT
-        self.bot = Bot(bot_token)
+        # Create request object with longer timeouts
+        request = Request(connect_timeout=10, read_timeout=30)
+        self.bot = Bot(bot_token, request=request)
         self.wallet_api = wallet_api
         # Firo Butler Initialization
         self.client = MongoClient(connectionString)
@@ -102,6 +105,8 @@ class TipBot:
                 self.processing_messages(new_messages)
             except Exception as exc:
                 logger.error(exc, exc_info=True)
+                # Wait before retrying to avoid rapid reconnection attempts
+                time.sleep(5)
 
     def pending_tasks(self):
         while True:
@@ -126,11 +131,16 @@ class TipBot:
                 self.balance_in_groth = to_decimal(self.balance_in_firo) * SATS_IN_BTC if self.balance_in_firo is not None else Decimal('0')
 
                 try:
-                    self._is_verified = self.col_users.find_one({"_id": self.user_id})['IsVerified']
-                    self._is_user_in_db = self._is_verified
+                    user_record = self.col_users.find_one({"_id": self.user_id})
+                    if user_record:
+                        self._is_verified = user_record['IsVerified']
+                        self._is_user_in_db = self._is_verified
+                    else:
+                        self._is_verified = False
+                        self._is_user_in_db = False
                 except Exception as exc:
                     logger.error(exc, exc_info=True)
-                    self._is_verified = True
+                    self._is_verified = False
                     self._is_user_in_db = False
 
                 logger.info("User: %s (%s) - %s", self.username, self.user_id, self.message_text)
@@ -180,11 +190,27 @@ class TipBot:
 
     def wait_new_message(self):
         while True:
-            updates = self.bot.get_updates(allowed_updates=["message", "callback_query"])
-            if len(updates) > 0:
-                break
+            try:
+                updates = self.bot.get_updates(
+                    allowed_updates=["message", "callback_query"],
+                    timeout=10  # 10 second timeout for long polling
+                )
+                if len(updates) > 0:
+                    break
+            except Exception as exc:
+                logger.warning(f"Failed to get updates: {exc}")
+                time.sleep(2)  # Wait before retrying
+                continue
+        
         update = updates[-1]
-        self.bot.get_updates(offset=update["update_id"] + 1, allowed_updates=["message", "callback_query"])
+        try:
+            self.bot.get_updates(
+                offset=update["update_id"] + 1, 
+                allowed_updates=["message", "callback_query"]
+            )
+        except Exception as exc:
+            logger.warning(f"Failed to acknowledge update: {exc}")
+        
         return updates
 
     @staticmethod
@@ -513,6 +539,8 @@ class TipBot:
         """
         try:
             _user = self.col_users.find_one({"_id": self.user_id})
+            if not _user:
+                return None, None, None, None
             if self.update_address_and_balance(_user):
                 _user = self.col_users.find_one({"_id": self.user_id})
             return _user['Address'], to_decimal(_user['Balance']), to_decimal(_user['Locked']), _user['IsWithdraw']
@@ -522,6 +550,9 @@ class TipBot:
 
     def update_address_and_balance(self, _user):
         # Check if User has a Spark address
+        if not _user or not _user.get('Address'):
+            return False
+            
         valid = wallet_api.validate_address(_user['Address'][0])['result']
         is_valid_spark = 'isvalidSpark'
         if is_valid_spark in valid:
@@ -531,7 +562,7 @@ class TipBot:
             # Get a spark address and update user
             spark_address = wallet_api.create_user_wallet()
             self.col_users.update_one(
-                _user,
+                {"_id": _user['_id']},
                 {
                     "$set":
                         {
@@ -586,13 +617,29 @@ class TipBot:
                 )
                 return
 
-            # Atomic check-and-deduct: only succeeds if Balance >= amount
+            # Atomic check-and-deduct with withdrawal lock:
+            # - Balance must be sufficient
+            # - No active withdrawal lock (IsWithdraw != True)
+            # - No locked amount left from a previous flow
             user = self.col_users.find_one_and_update(
-                {"_id": self.user_id, "Balance": {"$gte": decimal_to_store(amount)}},
-                {"$inc": {
-                    "Balance": decimal_to_store(-amount),
-                    "Locked": decimal_to_store(amount),
-                }},
+                {
+                    "_id": self.user_id,
+                    "Balance": {"$gte": decimal_to_store(amount)},
+                    "$or": [
+                        {"IsWithdraw": {"$exists": False}},
+                        {"IsWithdraw": False}
+                    ],
+                    "Locked": {"$lte": 0}
+                },
+                {
+                    "$inc": {
+                        "Balance": decimal_to_store(-amount),
+                        "Locked": decimal_to_store(amount),
+                    },
+                    "$set": {
+                        "IsWithdraw": True
+                    }
+                },
                 return_document=ReturnDocument.AFTER
             )
             if not user:
@@ -610,10 +657,15 @@ class TipBot:
                 # Rollback: restore the exact amount we moved
                 self.col_users.update_one(
                     {"_id": self.user_id},
-                    {"$inc": {
-                        "Balance": decimal_to_store(amount),
-                        "Locked": decimal_to_store(-amount),
-                    }}
+                    {
+                        "$inc": {
+                            "Balance": decimal_to_store(amount),
+                            "Locked": decimal_to_store(-amount),
+                        },
+                        "$set": {
+                            "IsWithdraw": False
+                        }
+                    }
                 )
                 self.send_message(
                     self.user_id, "Not enough inputs. Try to repeat a bit later!"
@@ -1061,6 +1113,7 @@ class TipBot:
                     "creator_id": self.user_id,
                     "msg_id": msg_id,
                     "takers": [],
+                    "taker_ids": [],
                     "created_at": int(datetime.datetime.now().timestamp())
                 }
             )
@@ -1077,7 +1130,12 @@ class TipBot:
                 return
 
             _is_ended = to_decimal(envelope['remains']) == 0
-            _is_user_catched = str(self.user_id) in str(envelope['takers'])
+            taker_ids = envelope.get('taker_ids', [])
+            if not taker_ids and envelope.get('takers'):
+                # Backward compatibility for old envelopes without taker_ids.
+                # Old schema stores takers as [user_id, amount] tuples.
+                taker_ids = [x[0] for x in envelope.get('takers', []) if isinstance(x, list) and len(x) > 0]
+            _is_user_catched = self.user_id in taker_ids
 
             if _is_user_catched:
                 self.answer_call_back(text="��You have already caught Firo from this envelope��",
@@ -1104,18 +1162,23 @@ class TipBot:
             store_catch = decimal_to_store(catch_amount)
 
             # Atomic envelope update: only deducts if remains >= catch_amount
-            # and user hasn't already caught (takers check above is advisory;
-            # the real guard is the find_one check, but since this is single-threaded
-            # polling the race window is minimal)
+            # and user hasn't already caught from this envelope.
             updated_envelope = self.col_envelopes.find_one_and_update(
                 {
                     "_id": envelope_id,
-                    "remains": {"$gte": store_catch}
+                    "remains": {"$gte": store_catch},
+                    "$or": [
+                        {"taker_ids": {"$exists": False}},
+                        {"taker_ids": {"$ne": self.user_id}}
+                    ]
                 },
                 {
                     "$push": {
                         "takers": [self.user_id, store_catch]
                     },
+                    "$addToSet": {
+                        "taker_ids": self.user_id
+                    },
                     "$inc": {
                         "remains": -store_catch
                     }
@@ -1124,9 +1187,14 @@ class TipBot:
             )
 
             if not updated_envelope:
-                # Envelope ran out between our read and update
-                self.answer_call_back(text="�RED ENVELOPE ENDED��",
-                                      query_id=self.new_message.callback_query.id)
+                # Either envelope was emptied or user claimed concurrently.
+                latest = self.col_envelopes.find_one({"_id": envelope_id})
+                if latest and self.user_id in latest.get('taker_ids', []):
+                    self.answer_call_back(text="��You have already caught Firo from this envelope��",
+                                          query_id=self.new_message.callback_query.id)
+                else:
+                    self.answer_call_back(text="�RED ENVELOPE ENDED��",
+                                          query_id=self.new_message.callback_query.id)
                 return
 
             # Credit the user atomically