# Accounting System Audit

> Living record of the accounting/GL audit and its remediation status.
> **Keep this file updated** whenever an accounting finding is opened or closed — the
> original audit was lost once because it was never written down. Don't let that happen again.

Last reconciled against code: **2026-06-19** (branch `dev`, commits through `9532812`).
Verification at that point: `dotnet build` clean (0 errors); `dotnet test tests/PowderCoating.UnitTests` → **290 passed, 0 failed**.

---

## How the GL is modeled (context for the findings)

Balances are computed two different ways, and they must agree:

1. **At posting time** — controllers call `AccountBalanceService.DebitAsync/CreditAsync`, which
   mutate the denormalized `Account.CurrentBalance`.
2. **At report time** — `FinancialReportService` (Trial Balance, Balance Sheet, P&L, AR/AP aging,
   statements) and `LedgerService` (per-account ledger + `RecalculateBalances`) recompute balances
   from source documents.

`LedgerService` also backs the **Balance Reconciliation report** (the detective control added in
`c2cd19e`), which compares stored `CurrentBalance` vs the ledger-recomputed balance. Any account
whose recompute path is incomplete will (a) be silently corrupted by a "Recalculate Balances" run
and (b) produce misleading output in the reconciliation report.

---

## Resolved findings (this audit batch)

| # | Finding | Fix | Commit | Verified |
|---|---------|-----|--------|----------|
| **#4** | `Sales Tax Payable` (2200) was credited on every invoice but never relieved — the liability grew forever (no remittance flow). | `JournalEntries/SalesTaxPayment` GET+POST: DR 2200 / CR bank, balanced, in a transaction, honors the period lock and validates amount + bank-account tenancy. | `0c921ba` | Posting reviewed `JournalEntriesController:235-296`. Reporting needs no change (posted JE lines already flow through TB/BS/ledger). |
| **#7** | Most report queries relied on the global tenant filter, which is **bypassed for SuperAdmin** → cross-company leakage on P&L / Balance Sheet / Trial Balance / aging / statements. | Explicit `CompanyId == companyId` predicate added to every query in `FinancialReportService`. | `9532812` | Confirmed: every query in `FinancialReportService.cs` carries an explicit `CompanyId` predicate. |
| **#9a** | A cash refund posted **DR AR** (control up) while the customer subledger went down — opposite directions → guaranteed AR drift. | Refund now **reverses the sale**: DR Sales Returns (4960) + DR Sales Tax Payable (tax portion) / CR bank; AR untouched. Split centralized in `Core/Accounting/RefundAllocation.Split`. | `2a82a1d` | Posting (`InvoicesController.IssueRefund`/`CancelRefund`) and both recompute paths (`LedgerService` §5b, `FinancialReportService` TB/BS/P&L) all use `RefundAllocation.Split` — they agree by construction. |
| **#9b** | Store-credit refunds & credit memos posted nothing to the GL on issue (only a `CreditMemo` + `Customer.CreditBalance`) → outstanding store credit invisible on the balance sheet. | New **2350 Customer Credits** liability. Issue: DR 4950 / CR 2350. Apply: DR 2350 / CR AR. Void remainder: DR 2350 / CR 4950. Updated in all 8 posting sites + TB/BS/P&L + ledger §12b. | `9ce3612` | Posting sites confirmed (`CreditMemosController` 180-185/262-270/314-320; `InvoicesController` store-credit path). Reporting `cmContraRevenue`/`cmIssuedNonVoided` math reviewed. |
| — | No detective control to catch denormalized-balance drift. | **Balance Reconciliation report** (`Reports/Reconciliation`): per-account stored vs recomputed, plus AR/AP control-vs-subledger. | `c2cd19e` | Reviewed `FinancialReportService.GetBalanceReconciliationAsync`. NOTE: its usefulness is limited by O2 below. |

### Original numbering gap
The commits reference findings **#4, #7, #9a, #9b** — implying a list of at least #1–#9. The original
audit document was never persisted and is unrecoverable. Findings **#1, #2, #3, #5, #6, #8** cannot be
matched to commits and their status is **unknown**. If the original list resurfaces, reconcile it here.

---

## Resolved findings (2026-06-19 remediation)

### O1 — Account 2300 mislabeled "Payroll Liabilities" while used as Customer Deposits — **RESOLVED**
- **Root cause refined:** there is no payroll feature posting to 2300; the deposit GL code has always used
  2300 (resolved by number) as the Customer Deposits liability. Migration `AccountingDepositsGL` already
  renamed it to "Customer Deposits" for tenants that lacked a 2300, but its `NOT EXISTS` guard **skipped**
  pre-existing tenants, leaving them mislabeled. The two seed files still created new tenants with the
  wrong name.
- **Fix (chosen approach: rename 2300, move payroll to a new 2400):**
  - Seed files now create **2300 = "Customer Deposits" (IsSystem)** and a separate **2400 = "Payroll
    Liabilities"** — `SeedDataService.Accounts.cs`, `SeedData.cs`.
  - `EnsureSystemAccountsAsync` self-heals: renames any 2300 still named "Payroll Liabilities" → "Customer
    Deposits" (preserving user renames) and ensures 2400 exists.
  - Migration `20260619233108_RenameDepositsAccountAddPayroll` does the same for existing tenants (rename
    only where Name is still the default; insert 2400 where missing). Down is best-effort (soft-deletes
    only empty 2400s; does not re-introduce the mislabel).
- Account **number 2300 is unchanged**, so the deposit posting code (`DepositsController`,
  `InvoicesController`) needed no changes — its lookups are by `"2300"`.

### O2 — `LedgerService` never recomputed 4950 Sales Discounts → recalc corrupted it — **RESOLVED**
- **Fix:** added a 4950 section to both `LedgerService.GetAccountLedgerAsync` and `ComputePriorBalanceAsync`
  that reproduces the *actual postings* so a balance recalc matches the stored balance:
  invoice discounts (DR at invoice date) + credit-memo issuance (DR full amount at issue) − the unapplied
  remainder of voided memos (CR at void). This mirrors what `AccountBalanceService` posts at
  `InvoicesController:849/1108/1629` and `CreditMemosController`, so the Balance Reconciliation report no
  longer shows false drift on 4950.
- **Note / micro-discrepancy:** the ledger uses `memo.AmountApplied` for the voided remainder (matching the
  true posting), whereas `FinancialReportService` TB/BS derive the voided portion from applications against
  non-voided invoices. These differ only in the rare case of a voided memo applied to a since-voided
  invoice. Left as-is (report-side nuance, not O2's target); documented here so it isn't mistaken for a bug.
- Regression test: `LedgerServiceTests.GetAccountLedgerAsync_SalesDiscounts4950_IncludesInvoiceDiscountsAndCreditMemoContraRevenue`.

Verification of O1+O2: `dotnet build` clean; `dotnet test tests/PowderCoating.UnitTests` → **291 passed**;
migration applied to the dev database successfully.

---

## Open findings (deferred — to do after O1/O2 per user)

### O3 — Write-path account lookups omit explicit `CompanyId` (defense-in-depth gap) — **LOW-MEDIUM**
- Finding #7 added explicit `CompanyId` predicates to the **read** path, but several **posting** lookups
  still rely on the global filter alone, e.g. `CreditMemosController` (182-184, 268, 317-318) and
  `InvoicesController` refund/credit account lookups use `FirstOrDefaultAsync(a => a.AccountNumber == "2350" && a.IsActive)`
  with no `CompanyId`. `JournalEntriesController.SalesTaxPayment` (246) does it correctly.
- **Impact:** low in practice (these run under a non-SuperAdmin company context where the global filter
  applies), but it violates the repo's standing rule and would post to the wrong tenant's account if ever
  executed in a SuperAdmin/multi-company context.
- **Fix direction:** add `a.CompanyId == companyId` to every account lookup on the posting path.

### O4 — Sales-tax remittance can over-remit (drive 2200 negative) — **LOW**
- `SalesTaxPayment` validates amount > 0 but does **not** cap the payment at the outstanding 2200 balance.
- **Impact:** a typo can push Sales Tax Payable negative (a debit-balance liability), which is an
  abnormal balance that then surfaces oddly on the balance sheet.
- **Fix direction:** show the current 2200 balance (already shown on the GET form) and reject/​warn when
  `amount` exceeds it.

---

## Suggested priority order
1. **O1** (mislabeled/commingled liability — real balance-sheet correctness issue)
2. **O2** (recalc corruption + reconciliation report reliability)
3. **O3** (defense-in-depth on write path)
4. **O4** (input guard)