Review a PHP Money Transfer

1. No transaction

Critical, line 10-11

If the process dies or the second statement fails after the first, money leaves one account and never arrives in the other.

Fix: Wrap both updates in $db->beginTransaction() / commit(), with rollBack() in a catch, and set PDO to throw exceptions (PDO::ERRMODE_EXCEPTION).

2. Race condition allows overdrafts

Critical, line 5-10

The balance check and the debit are separate steps. Two concurrent transfers can both read the same balance, both pass the check, and together spend more than the account holds.

Fix: Lock the row inside the transaction (SELECT ... FOR UPDATE) or make the update conditional: UPDATE accounts SET balance = balance - ? WHERE id = ? AND balance >= ? and check that one row was affected.

3. Negative and zero amounts are accepted

Major, line 3

A negative $amount reverses the transfer and moves money into the caller's account from someone else's.

Fix: Validate that the amount is positive and that the two accounts differ.

4. Money stored as float

Major, line 3

Floating point cannot represent most decimal amounts exactly (0.1 + 0.2 != 0.3), so balances drift and comparisons fail at the edges.

Fix: Use integer minor units (cents) or DECIMAL columns with string or decimal arithmetic in PHP.

5. Values interpolated into SQL

Minor, line 5, 10-11

The integers are type-hinted today, but interpolating values into SQL is a habit that becomes injection the moment a string slips through, and it prevents statement reuse.

Fix: Use prepared statements with bound parameters for every query.

6. Deadlock risk under load

Minor, line 10-11

Two opposite transfers (A to B and B to A) lock the rows in opposite order and can deadlock.

Fix: Lock both rows in a consistent order (lower ID first) and retry the transaction on deadlock errors.

Money movement is the canonical transactions question. Strong answers name atomicity (transaction), isolation (lock or conditional update to prevent the overdraft race), input validation, and money representation, in that order of severity.

<?php

function transfer(PDO $db, int $fromId, int $toId, int $amountCents): void
{
    if ($amountCents <= 0 || $fromId === $toId) {
        throw new InvalidArgumentException('Invalid transfer');
    }
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $db->beginTransaction();
    try {
        // Lock both rows in a consistent order to avoid deadlocks.
        $lock = $db->prepare('SELECT id, balance_cents FROM accounts WHERE id IN (?, ?) ORDER BY id FOR UPDATE');
        $lock->execute([$fromId, $toId]);
        $balances = $lock->fetchAll(PDO::FETCH_KEY_PAIR);
        if (count($balances) !== 2) {
            throw new RuntimeException('Account not found');
        }
        if ($balances[$fromId] < $amountCents) {
            throw new RuntimeException('Insufficient funds');
        }
        $db->prepare('UPDATE accounts SET balance_cents = balance_cents - ? WHERE id = ?')->execute([$amountCents, $fromId]);
        $db->prepare('UPDATE accounts SET balance_cents = balance_cents + ? WHERE id = ?')->execute([$amountCents, $toId]);
        $db->commit();
    } catch (Throwable $e) {
        $db->rollBack();
        throw $e;
    }
}