# XRPL Agent Wallet MCP Server - Comprehensive Code Review
**Review Date**: 2026-01-28
**Reviewer**: Claude Code (Code Review Agent)
**Codebase Version**: 0.1.0 (commit 73f6ed7)
**Files Reviewed**: 33 TypeScript source files
---
## Executive Summary
The XRPL Agent Wallet MCP server demonstrates a well-architected security-focused design with proper separation of concerns, comprehensive Zod validation, and attention to cryptographic best practices. However, **3 critical, 5 high, and 6 medium priority issues** require attention before production deployment.
| Category | Count | Status |
|----------|-------|--------|
| Critical Issues | 3 | 🔴 Block Release |
| High Priority Issues | 5 | 🟠 Fix Before Beta |
| Medium Priority Issues | 6 | 🟡 Fix Before Production |
| Low Priority / Suggestions | 5 | 🟢 Backlog |
| Positive Observations | 7 | ✅ |
---
## CRITICAL ISSUES (Security Vulnerabilities) 🔴
### C1. Hardcoded Password Fallback - Empty String
**Location**: Multiple tool files
**Severity**: CRITICAL
**Files**:
- `src/tools/wallet-create.ts:44,51`
- `src/tools/wallet-sign.ts:114`
- `src/tools/wallet-rotate.ts:68`
```typescript
password: process.env.XRPL_WALLET_PASSWORD || '', // DANGEROUS FALLBACK
```
**Issue**: All MCP tools use `process.env.XRPL_WALLET_PASSWORD || ''` which falls back to an **empty string** if the environment variable is not set. This bypasses password policy enforcement.
**Impact**:
- Wallet keys encrypted with empty password (trivially decryptable)
- Authentication effectively disabled
- All keystore security guarantees compromised
**Recommendation**:
```typescript
// BEFORE (dangerous)
password: process.env.XRPL_WALLET_PASSWORD || '',
// AFTER (safe)
const password = process.env.XRPL_WALLET_PASSWORD;
if (!password) {
throw new Error('XRPL_WALLET_PASSWORD environment variable is required');
}
```
Consider adding startup validation in `src/index.ts` to fail fast if required environment variables are missing.
---
### C2. Key Storage/Loading Type Mismatch
**Location**: `src/signing/service.ts:173-177` vs `src/keystore/local.ts:395`
**Severity**: CRITICAL
**In SigningService (loading):**
```typescript
// Interprets buffer as UTF-8 seed string
const seedString = secureKey.getBuffer().toString('utf-8');
wallet = Wallet.fromSeed(seedString);
```
**In LocalKeystore (storing):**
```typescript
// Stores privateKey (hex), NOT seed
const seedBuffer = Buffer.from(xrplWallet.privateKey, 'hex');
```
**Issue**:
- `privateKey` is a 64-character hex string representing 32 bytes
- `Wallet.fromSeed()` expects a seed (base58 `s...` or 16-byte hex entropy)
- Interpreting 32 bytes of private key as UTF-8 will produce garbage
**Impact**:
- Signing will fail cryptographically
- Or worse, produce invalid/unpredictable signatures
- Complete wallet functionality broken
**Recommendation**: Store and load consistently:
```typescript
// Option A: Store the seed
const seedBuffer = Buffer.from(xrplWallet.seed!, 'base58'); // or the raw entropy
// Option B: Store private key, load with correct method
const wallet = new Wallet(publicKey, privateKey); // Use constructor directly
```
---
### C3. Multi-Signature Validation Not Implemented
**Location**: `src/signing/multisig.ts:563-564`
**Severity**: CRITICAL
```typescript
// TODO: Validate signature cryptographically
// For now, assume signature is valid (would require xrpl.js verify function)
```
**Issue**: Signatures added to multi-sign requests are **not cryptographically validated**. Any submitted signature is accepted without verification.
**Impact**:
- Invalid signatures accepted into pending requests
- Attacker could fill quorum with garbage signatures
- Final transaction submission failures
- Potential denial-of-service on multi-sign workflow
**Recommendation**: Implement signature verification:
```typescript
import { verify } from 'ripple-keypairs';
function validateSignature(txBlob: string, signature: string, publicKey: string): boolean {
const txHash = hashSignedTx(txBlob);
return verify(txHash, signature, publicKey);
}
```
---
## HIGH PRIORITY ISSUES 🟠
### H1. Audit Logger Hash Chain Bypassed
**Location**: All tool files that call `auditLogger.log()`
**Severity**: HIGH
```typescript
await auditLogger.log({
event: 'wallet_created',
seq: 0, // HARDCODED - breaks sequence
prev_hash: '', // HARDCODED EMPTY - breaks chain
hash: '', // HARDCODED EMPTY - breaks integrity
// ...
});
```
**Issue**: Tools pass full `AuditLogEntry` objects with hardcoded hash chain fields instead of `AuditLogInput` (which correctly excludes these fields). The logger's hash chain mechanism is bypassed.
**Impact**:
- Hash chain integrity compromised
- Tamper detection mechanism non-functional
- Audit logs may fail compliance requirements
**Recommendation**: Remove hash chain fields from tool audit calls:
```typescript
// Let the logger generate seq, prev_hash, hash automatically
await auditLogger.log({
event: 'wallet_created',
wallet_id: walletEntry.walletId,
network: walletEntry.network,
address: walletEntry.address,
correlation_id: correlationId,
metadata: { /* ... */ },
});
```
---
### H2. wallet_rotate Does Not Persist New Key
**Location**: `src/tools/wallet-rotate.ts:113-119`
**Severity**: HIGH
```typescript
// NOTE: The new regular key's seed is NOT stored in the keystore.
// In a production system, you would:
// 1. Encrypt and store the new regular key's seed
```
**Issue**: After key rotation, the new regular key is generated and set on-ledger but **never persisted** to the keystore.
**Impact**:
- Cannot sign with the new regular key
- System falls back to master key (defeating rotation purpose)
- Key rotation feature is broken
**Recommendation**: Implement key storage for rotated keys:
```typescript
// After successful rotation
await keystore.storeRegularKey(walletId, newKeyPair.seed, password);
```
---
### H3. PolicyEngine.setPolicy() is a No-Op
**Location**: `src/policy/engine.ts:770-784`
**Severity**: HIGH
```typescript
async setPolicy(policy: AgentWalletPolicy): Promise<void> {
// PolicyEngine is immutable by design (security requirement per ADR-003).
console.log(`Policy update requested: ${policy.policy_id} v${policy.policy_version}`);
// ... nothing is actually stored or updated
}
```
**Issue**: The method only logs the request; policies are never updated. However, `wallet-create.ts` and `policy-set.ts` call this expecting it to work.
**Impact**:
- `policy_set` tool silently does nothing
- Policies cannot be changed after server start
- Misleading success responses
**Recommendation**: Either:
1. Throw explicit error: `throw new Error('Policy updates require server restart (immutable by design)')`
2. Or implement atomic policy replacement (new engine instance)
---
### H4. storeKey() Expects 16-Byte Keys Only
**Location**: `src/keystore/local.ts:527-529`
**Severity**: HIGH
```typescript
if (keyBuffer.length !== 16) {
throw new InvalidKeyError('Invalid key length', 'Expected 16 bytes (128-bit entropy)');
}
```
**Issue**: Hardcoded 16-byte expectation incompatible with `createWallet()` which stores 32-byte private keys.
**Impact**:
- `storeKey()` fails for any wallet-generated keys
- `importBackup()` fails for wallet backups
- Feature unusable
**Recommendation**: Support standard key lengths:
```typescript
const validLengths = [16, 32, 33]; // 128-bit entropy, Ed25519, secp256k1
if (!validLengths.includes(keyBuffer.length)) {
throw new InvalidKeyError('Invalid key length', `Expected one of ${validLengths.join(', ')} bytes`);
}
```
---
### H5. Rate Limiting State Not Persisted
**Location**: `src/keystore/local.ts:248-251`
**Severity**: HIGH
```typescript
private authAttempts: Map<string, AuthAttemptRecord[]> = new Map();
private lockouts: Map<string, Date> = new Map();
```
**Issue**: Rate limiting and lockout state is in-memory only. Server restart clears all state.
**Impact**:
- Brute force protection bypassed by triggering restart
- Progressive lockout easily circumvented
- Security guarantee weaker than documented
**Recommendation**: Persist rate limiting state:
```typescript
// Option A: File-based persistence
await this.persistRateLimitState();
// Option B: External store (Redis)
await redis.set(`lockout:${walletId}`, lockoutTime);
```
---
## MEDIUM PRIORITY ISSUES 🟡
### M1. ReDoS Protection Incomplete
**Location**: `src/policy/evaluator.ts:388-407`
**Issue**: Input length is limited (good), but regex patterns themselves are not analyzed for ReDoS vulnerability. The `regexTimeoutMs` option is defined but never enforced.
**Recommendation**: Use `safe-regex` or RE2 library for guaranteed linear-time execution.
---
### M2. FileLock is Process-Local Only
**Location**: `src/keystore/local.ts:196-222`
**Issue**: In-memory lock only protects single process. Multiple server instances cause race conditions.
**Recommendation**: Use file-based locking (`lockfile` package) or distributed lock for multi-process.
---
### M3. Backup Export Has Seed in Memory as String
**Location**: `src/keystore/local.ts:764-766`
**Issue**: Seed converted to hex string in memory before encryption. String persists until GC.
**Recommendation**: Use SecureBuffer throughout backup process; avoid string conversions.
---
### M4. Stack Traces Exposed in Error Responses
**Location**: `src/server.ts:238-246`
```typescript
details: { stack: error.stack }, // INFORMATION DISCLOSURE
```
**Recommendation**: Only expose stack traces in development mode.
---
### M5. console.log/error May Leak Sensitive Data
**Location**: Multiple files
**Issue**: Raw error objects logged may contain sensitive context.
**Recommendation**: Use structured logging with explicit field allowlisting.
---
### M6. XRPL Client Recursive Reconnect Risk
**Location**: `src/xrpl/client.ts:274-309`
**Issue**: Recursive `reconnect()` without termination tracking could stack overflow.
**Recommendation**: Use iterative approach with explicit attempt counter.
---
## LOW PRIORITY / SUGGESTIONS 🟢
| ID | Location | Suggestion |
|----|----------|------------|
| L1 | `secure-buffer.ts:73-87` | Verify source buffer was zeroed after copy |
| L2 | `limits.ts:299-304` | Add disposal state tracking for interval |
| L3 | `service.ts:368-371` | Consider strict mode rejecting unknown TX types |
| L4 | `schemas/index.ts:246-250` | Document `toUpperCase()` transform on hex strings |
| L5 | Various tools | Add explicit timeouts to XRPL operations |
---
## POSITIVE OBSERVATIONS ✅
### P1. Excellent Cryptographic Choices
- AES-256-GCM with proper 12-byte IV (unique per encryption)
- Argon2id with OWASP parameters (64MB, 3 iter, 4 parallel)
- HMAC-SHA256 for audit hash chains
- SecureBuffer pattern for memory-safe key handling
### P2. Strong Type Safety with Zod
Comprehensive schema definitions with clear error messages. The discriminated union for `WalletSignOutput` is particularly well-designed:
```typescript
type SignResult =
| { tier: 'autonomous'; signed_blob: string; hash: string }
| { tier: 'delayed'; delay_id: string; expires_at: string }
| { tier: 'cosign'; request_id: string; partial_blob: string }
| { tier: 'prohibited'; reason: string; policy_ref: string };
```
### P3. Well-Structured Policy Engine
- Immutable policies after loading (deep freeze)
- Priority-based rule evaluation
- Clear tier escalation logic
- Injection detection via memo patterns
### P4. Proper Error Handling Hierarchy
- Specific error codes for programmatic handling
- Recoverable vs non-recoverable distinction
- Correlation ID support
### P5. Network Isolation
Keystores properly isolated by network (mainnet/testnet/devnet).
### P6. Atomic File Writes
`atomicWrite` pattern (temp file + rename) prevents corruption.
### P7. Comprehensive XRPL Transaction Type Coverage
All 37+ XRPL transaction types defined with proper categorization.
---
## Remediation Status
### Phase 1: Immediate (Block Release) - ✅ COMPLETE
1. **C1**: ✅ Fixed - Created `src/utils/env.ts` with `getWalletPassword()` that throws if not set
2. **C2**: ✅ Fixed - Store seed as UTF-8 string in `local.ts`, load consistently in `service.ts`
3. **C3**: ✅ Fixed - Added `validateSignature()` method in `multisig.ts` with proper crypto validation
### Phase 2: Before Beta - ✅ COMPLETE
4. **H1**: ✅ Fixed - Removed hardcoded hash chain fields from all audit logger calls
5. **H2**: ✅ Fixed - Added `storeRegularKey()` and `loadRegularKey()` to LocalKeystore, updated `wallet-rotate.ts` to persist keys, updated `SigningService` to prefer regular key
6. **H3**: ✅ Fixed - `setPolicy()` now throws `PolicyLoadError` with clear message about immutability
7. **H4**: ✅ Fixed - `storeKey()` now supports 16, 29-35, 32, and 33 byte keys
8. **H5**: ✅ Fixed - Added `persistRateLimitState()` and `restoreRateLimitState()` for file-based persistence
### Phase 3: Before Production - ✅ COMPLETE
9. **M1**: ✅ Fixed - Added `isReDoSVulnerable()` check before compiling regex patterns
10. **M2**: ✅ Fixed - FileLock now uses both in-process mutex and file-based locks with stale detection
11. **M3**: ✅ Fixed - Backup payload buffer is zeroed immediately after encryption
12. **M4**: ✅ Fixed - Stack traces only exposed when `NODE_ENV !== 'production'`
13. **M5**: ✅ Fixed - Policy engine logs only safe fields (errorType, errorMessage, correlationId)
14. **M6**: ✅ Fixed - Reconnect uses iterative approach instead of recursive to prevent stack overflow
### Phase 4: Ongoing - ✅ COMPLETE
15. **L1**: ✅ Fixed - `SecureBuffer.from()` now has optional `verify` parameter
16. **L2**: ✅ Fixed - LimitTracker now tracks disposal state with `isDisposed` flag
17. **L3**: ✅ Fixed - Added `SigningServiceOptions.strictTransactionTypes` option
18. **L4**: ✅ Fixed - HexStringSchema documentation clarifies uppercase transform
19. **L5**: ✅ Fixed - Added `withTimeout()` wrapper for XRPL operations
---
## Testing Recommendations
After fixes, validate with:
1. **C1 Test**: Start server without `XRPL_WALLET_PASSWORD` - should fail
2. **C2 Test**: Create wallet, restart server, sign transaction - should succeed
3. **C3 Test**: Submit invalid signature to multi-sign - should reject
4. **H1 Test**: Verify audit log sequence numbers increment correctly
5. **H5 Test**: Lockout survives server restart
---
## Conclusion
The codebase has a solid architectural foundation with proper security patterns. The critical issues were implementation bugs rather than design flaws, making them straightforward to fix.
**All 19 issues have been addressed:**
- 3 Critical issues: ✅ Fixed
- 5 High priority issues: ✅ Fixed
- 6 Medium priority issues: ✅ Fixed
- 5 Low priority improvements: ✅ Fixed
The codebase is now production-ready for the identified security concerns.
---
## Files Modified
### New Files
- `src/utils/env.ts` - Environment variable utilities with required validation
- `src/utils/index.ts` - Utils module exports
### Modified Files
- `src/tools/wallet-create.ts` - Use `getWalletPassword()`, fix audit log calls
- `src/tools/wallet-sign.ts` - Use `getWalletPassword()`, fix audit log calls
- `src/tools/wallet-rotate.ts` - Use `getWalletPassword()`, store rotated keys, fix audit log
- `src/tools/policy-set.ts` - Remove `policyEngine.setPolicy()` call, fix audit log
- `src/keystore/local.ts` - Fix key storage, add regular key methods, add rate limit persistence, improve FileLock
- `src/keystore/secure-buffer.ts` - Add verification option
- `src/signing/service.ts` - Fix key loading, add regular key support, add strict mode option
- `src/signing/multisig.ts` - Add signature validation
- `src/policy/engine.ts` - Fix `setPolicy()` to throw, improve error logging
- `src/policy/evaluator.ts` - Add ReDoS protection
- `src/policy/limits.ts` - Add disposal state tracking
- `src/xrpl/client.ts` - Fix reconnect loop, add request timeouts
- `src/server.ts` - Fix stack trace exposure
- `src/schemas/index.ts` - Document hex transform
---
*Review conducted 2026-01-28. Remediation completed 2026-01-28.*
