docs: track superpowers working documents in git
The uups-bank-demo wave's SDD records (ledger, six task briefs and reports, review diffs) and the brainstorm design mockups were git-ignored, so they existed only on one sandbox VM and reached no remote — this repo had no remote at all until now. Removes `.superpowers/` from .gitignore and the `*` .gitignore the superpowers plugin writes inside .superpowers/sdd/; the second blocks the directory even with the first removed. Excluded as ephemeral local-server state, and now ignored by name: .last-port, .last-token (a 64-char session token for a brainstorm server on a port that is long gone), and the per-session state/ directories. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fnwzj6McD6kSkXwjUKFKxe
This commit is contained in:
@@ -0,0 +1,133 @@
|
||||
# Task 4 Report: V1 Custody Accounting and Adversarial Unit Tests
|
||||
|
||||
## Status
|
||||
|
||||
Complete. `BankV1` now supports exact-accounting deposits and CEI withdrawals behind the existing UUPS proxy. The focused custody suite, aggregate V1 suite, and full offline suite pass.
|
||||
|
||||
## Implementation
|
||||
|
||||
- Added `Deposited` and `Withdrawn` events and the exact `ZeroAmount`, `InsufficientBalance`, and `UnexpectedAssetDelta` errors.
|
||||
- Added `deposit(uint256)` guarded by `whenNotPaused` and `nonReentrant`. It measures proxy reserves before and after `SafeERC20.safeTransferFrom`, rejects any non-exact received amount, and credits customer balance/liabilities only after the transfer and delta validation succeed.
|
||||
- Added `withdraw(uint256)` guarded by `whenNotPaused` and `nonReentrant`. It validates zero/available balance, debits customer balance and liabilities before the external `SafeERC20.safeTransfer`, and emits the withdrawal event.
|
||||
- Preserved application storage exactly: `_asset` slot 0, `_balances` slot 1, `_totalLiabilities` slot 2, and `uint256[47] __gap` slot 3.
|
||||
- Kept all application behavior proxy-bound and added no sweep/rescue interface.
|
||||
|
||||
## Changed Files
|
||||
|
||||
- `src/BankV1.sol` — custody API, errors/events, SafeERC20 accounting.
|
||||
- `test/BankV1.t.sol` — 18 deterministic and 3 fuzz custody tests.
|
||||
- `test/mocks/FeeOnTransferToken.sol` — six-decimal ERC-20 whose `transferFrom` delivers 99% and burns the remainder.
|
||||
- `test/mocks/ReentrantToken.sol` — six-decimal ERC-20 with configurable deposit/withdraw callbacks, revert propagation, nested-call result/selector recording, and callback-time accounting observations.
|
||||
|
||||
## TDD RED
|
||||
|
||||
Tool paths were set separately in a dedicated shell:
|
||||
|
||||
```bash
|
||||
export PATH=/tmp/codius-node-v24.18.0/bin:/home/golem/.foundry/bin:$PATH
|
||||
```
|
||||
|
||||
The required RED command was run verbatim:
|
||||
|
||||
```bash
|
||||
npm_config_offline=true forge test --match-path test/BankV1.t.sol -vvv --force
|
||||
```
|
||||
|
||||
The first invocation found a Solidity tuple-declaration syntax error in the new `ReentrantToken` test double. Only that test-double syntax was corrected; production remained unchanged. The command was rerun verbatim and produced the valid expected RED:
|
||||
|
||||
```text
|
||||
Compiler run failed:
|
||||
Error (9582): Member "deposit" not found or not visible after argument-dependent lookup in contract BankV1.
|
||||
--> test/BankV1.t.sol:28:9:
|
||||
28 | bank.deposit(100e6);
|
||||
| ^^^^^^^^^^^^
|
||||
Error: Compilation failed
|
||||
```
|
||||
|
||||
This RED was expected because the custody tests called the required proxy-bound `deposit`/`withdraw` API before those functions existed. It therefore failed for the missing production behavior rather than a malformed assertion or environment problem.
|
||||
|
||||
## GREEN Verification
|
||||
|
||||
All upgrade-validation-bearing commands were kept offline.
|
||||
|
||||
```bash
|
||||
forge fmt
|
||||
```
|
||||
|
||||
Output: completed successfully and formatted `test/BankV1.t.sol` on the initial run; the final run completed with no further formatting changes.
|
||||
|
||||
```bash
|
||||
npm_config_offline=true forge test --match-path test/BankV1.t.sol -vvv --force
|
||||
```
|
||||
|
||||
Final output summary:
|
||||
|
||||
```text
|
||||
Ran 21 tests for test/BankV1.t.sol:BankV1CustodyTest
|
||||
Suite result: ok. 21 passed; 0 failed; 0 skipped
|
||||
Fuzz tests: 512 runs each
|
||||
```
|
||||
|
||||
```bash
|
||||
npm_config_offline=true forge test --match-path 'test/BankV1*.t.sol' --force
|
||||
```
|
||||
|
||||
Final output summary:
|
||||
|
||||
```text
|
||||
BankV1AdminTest: 12 passed; 0 failed; 0 skipped
|
||||
BankV1CustodyTest: 21 passed; 0 failed; 0 skipped
|
||||
Ran 2 test suites: 33 passed, 0 failed, 0 skipped
|
||||
```
|
||||
|
||||
Full-suite command:
|
||||
|
||||
```bash
|
||||
npm_config_offline=true forge test --force
|
||||
```
|
||||
|
||||
Final output summary:
|
||||
|
||||
```text
|
||||
BankV1AdminTest: 12 passed
|
||||
MockUSDCTest: 7 passed
|
||||
BankV1CustodyTest: 21 passed
|
||||
Ran 3 test suites: 40 passed, 0 failed, 0 skipped
|
||||
```
|
||||
|
||||
Solc emitted only two pre-existing dependency warnings in `openzeppelin-foundry-upgrades` about functions whose mutability could be `pure`; there were no project warnings or test failures.
|
||||
|
||||
Storage inspection:
|
||||
|
||||
```bash
|
||||
forge inspect src/BankV1.sol:BankV1 storage-layout
|
||||
```
|
||||
|
||||
Output confirmed `_asset` at slot 0, `_balances` at slot 1, `_totalLiabilities` at slot 2, and `__gap` at slot 3.
|
||||
|
||||
## Accounting and Adversarial Coverage
|
||||
|
||||
- Exact wallet, reserve, customer balance, and total-liability deltas for deposit and withdrawal.
|
||||
- Indexed event emission for each custody operation.
|
||||
- Zero amount and paused-state rejection for both operations.
|
||||
- Exact ERC-6093 inadequate allowance and wallet-balance failures, with empty bank accounting after revert.
|
||||
- Independent deposits for Alice and Bob and a withdrawal that leaves the other customer's balance unchanged.
|
||||
- Fee-on-transfer deposit rejection with `UnexpectedAssetDelta(100e6, 99e6)` and atomic rollback of sender balance, bank reserves, customer credit, and liabilities.
|
||||
- Deposit callback observes zero customer credit/liabilities before transfer completion, attempts nested deposit, and records `ReentrancyGuardReentrantCall`; swallowing the nested revert credits exactly once, while configured propagation atomically reverts the outer deposit.
|
||||
- Withdrawal callback observes already-debited customer balance/liabilities, attempts nested withdrawal, records `ReentrancyGuardReentrantCall`, and cannot double debit.
|
||||
- Direct-transfer surplus: 100 mUSDC liability plus 25 mUSDC direct transfer yields 125 mUSDC reserves; withdrawing the full 100 mUSDC customer balance leaves the 25 mUSDC surplus.
|
||||
- Deterministic fuzzing uses the existing fixed seed, bounds deposits to `[1, 1_000_000e6]`, bounds withdrawals to `[1, deposited]`, proves exact deltas, and proves every bounded over-withdraw reverts with exact available/requested values.
|
||||
|
||||
## Self-Review
|
||||
|
||||
- `git diff --check` passed.
|
||||
- Diff inspection confirmed no persistent storage changes and no owner sweep/rescue behavior.
|
||||
- Mutation review: tests fail for missing credit/debit/liability changes, incorrect transfer amount or event, removed zero/pause/available checks, fee-token over-credit, credit-before-deposit-transfer, interaction-before-withdraw-effects, absent reentrancy guard, customer cross-account corruption, or surplus consumption.
|
||||
- Independent read-only review reported no Critical, Important, or Minor findings and assessed the change ready to merge. The reviewer independently saw 21/21 focused and 33/33 aggregate tests pass and confirmed formatting/diff checks.
|
||||
- Only the four task-listed source/test files are staged for the commit; this report is intentionally outside that scoped commit.
|
||||
|
||||
## Concerns
|
||||
|
||||
- The bank intentionally supports only standard non-rebasing ERC-20 assets. Outbound transfer-fee/rebasing behavior is outside V1's stated contract.
|
||||
- `ReentrancyGuardTransient` requires Cancun/EIP-1153, matching the project's configured target.
|
||||
- The two compiler warnings originate in the pinned `openzeppelin-foundry-upgrades` dependency and are unrelated to this change.
|
||||
Reference in New Issue
Block a user