# Security Review Fixes Status

This document tracks the status of each finding from the security review report.

---

## Smart Contracts (`maldo-contracts/`)

### Critical Issues

| ID | Finding | Status | Resolution |
|----|---------|--------|------------|
| C-01 | No validation on rating values (0-255 allowed) | **FIXED** | Added `MAX_RATING = 5` constant and validation `if (_rating > MAX_RATING \|\| _rating == 0) revert InvalidRating()` (ec29345) |
| C-02 | Multiple ratings per deal - no double-rating prevention | **FIXED** | Reworked rating system with `DealReview` struct that stores separate `taskerRating` and `customerRating`, with `AlreadyReviewed` check (5174df9) |

### High Priority Issues

| ID | Finding | Status | Resolution |
|----|---------|--------|------------|
| H-01 | Custom owner implementation instead of Ownable2Step | **FIXED** | Registry now extends `Ownable2Step` from OpenZeppelin (ec29345, ffbb2e8) |
| H-02 | setDisputeResolver missing zero-address validation | **FIXED** | Added `if (address(_disputeResolver) == address(0)) revert InvalidDisputeResolver()` (906c549) |
| H-03 | createDeal missing price and duration validation | **WONTFIX** | Intentional - services can be free and have undetermined duration |
| H-04 | Dispute function has no access control | **TBD** | Acknowledged - dispute flow needs revision, implementation approach unclear |
| H-05 | Dual-party rating system confusion | **PARTIALLY FIXED** | Reworked with `DealReview` struct containing separate `taskerRating`/`taskerReview` and `customerRating`/`customerReview` fields; event now includes reviewer address (5174df9) |
| H-06 | Badges.mintBatch inefficient implementation | **DEFERRED** | Badges contract not in use; state variable removed from Registry (5174df9) |

### Medium Priority Issues

| ID | Finding | Status | Resolution |
|----|---------|--------|------------|
| M-01 | Repeated validation should use modifier | **FIXED** | Added `onlyServiceOwner` modifier (2acc0e5) |
| M-02 | Storage organization - group immutables | **FIXED** | Reorganized storage with CONSTANTS, IMMUTABLES, and STORAGE sections (f5a072b) |
| M-03 | No pagination for rating arrays | **WONTFIX** | On-chain pagination considered an anti-pattern |
| M-04 | String parameters unbounded | **ACKNOWLEDGED** | Non-issue until IPFS is implemented |
| M-05 | No rate limiting on service/deal creation | **WONTFIX** | No need. |
| M-06 | Badges contract - unused storage variable | **DEFERRED** | Badges contract not in active use |
| M-07 | Ownable + AccessControl redundancy in Badges | **DEFERRED** | Badges contract not in active use |
| M-08 | ReentrancyGuard imported but never used in Badges | **DEFERRED** | Badges contract not in active use |
| M-13 | CEI pattern violation in createDeal | **FIXED** | Reordered to push deal first, then make external escrow call (c0c9ebd) |

### Low Priority / Code Quality

| ID | Finding | Status | Resolution |
|----|---------|--------|------------|
| LQ-01 | Missing NatSpec documentation | **FIXED** | Added NatSpec to `_createEscrowAgreement` (90c30df) |
| LQ-02 | Missing parameter validation in Badges.mint/mintBatch | **DEFERRED** | Badges contract not in active use |
| LQ-03 | Missing reviewer in Rated event | **FIXED** | Event now includes `address indexed _reviewer` parameter (5174df9) |
| LQ-04 | Missing interface for DisputeResolver | **FIXED** | Created `IDisputeResolver` interface; `disputeResolver` now typed as `IDisputeResolver` (54a4fcf) |
| LQ-05 | Integer overflow risk with uint40 IDs | **WONTFIX** | Acknowledged but by the time we reach 2^40 deals, we'll have already conquered the world
|

---

## Smart Contract Deployment Strategy

| Finding | Status | Resolution |
|---------|--------|------------|
| Migrate to Hardhat Ignition | **WONTFIX** | Setup considered too simple to warrant migration |

---

## Wallets API (`wallets-api/`)

### High Priority Issues

| ID | Finding | Status | Resolution |
|----|---------|--------|------------|
| H-01 | Vercel KV used as primary storage | **FIXED** | Migrated to MongoDB as persistent storage (a5515cf, a9b5f2e) |
| H-02 | Insufficient rate limiting | **ACKNOWLEDGED** | Quick fix planned: whitelist trusted service IPs |
| H-03 | Inconsistent error handling | **FIXED** | Added try-catch blocks and standardized error responses with `CommonErrors` helper across all endpoints (e247797) |

### Medium Priority Issues

| ID | Finding | Status | Resolution |
|----|---------|--------|------------|
| M-01 | Missing linter configuration for unused code | **FIXED** | Added ESLint config with `eslint-plugin-unused-imports` and `@typescript-eslint/no-unused-vars` rules (8c543c6) |
| M-02 | Code formatting consistency | **FIXED** | Added `.prettierrc` configuration (5622aec) |
| M-03 | Bun runtime concerns | **N/A** | Observation noted; no action required |

---

## Summary

| Component | Total | Fixed | Deferred | Won't Fix | TBD |
|-----------|-------|-------|----------|-----------|-----|
| Smart Contracts - Critical | 2 | 2 | 0 | 0 | 0 |
| Smart Contracts - High | 6 | 2 | 1 | 1 | 2 |
| Smart Contracts - Medium | 9 | 3 | 3 | 1 | 2 |
| Smart Contracts - Low | 5 | 3 | 1 | 0 | 1 |
| Wallets API - High | 3 | 2 | 0 | 0 | 1 |
| Wallets API - Medium | 3 | 2 | 0 | 0 | 1 |
| **Total** | **28** | **14** | **5** | **2** | **7** |
2