Skip to content

Commit cf2b1a7

Browse files
onspeedhponspeedhp
committed
Refactor: Consolidate audit tests, fix all 17 issues, and update documentation
1 parent 255db38 commit cf2b1a7

18 files changed

Lines changed: 2404 additions & 980 deletions

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
[workspace]
22
resolver = "2"
33
members = ["program", "no-padding", "assertions"]
4+
exclude = ["tests-e2e"]
45

56
[workspace.dependencies]
67
pinocchio = { version = "0.9", features = ["std"] }

‎README.md‎

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
Granular permission management for every key:
1515
- **Owner (Role 0)**: Full control. Can add/remove authorities and transfer ownership.
1616
- **Admin (Role 1)**: Can create Sessions and add Spenders. Cannot remove Owners.
17-
- **Spender (Role 2)**: Limited to executing transactions. ideal for hot wallets or automated bots.
17+
- **Spender (Role 2)**: Limited to executing transactions. Ideal for hot wallets or automated bots.
1818

1919
### ⏱️ Ephemeral Session Keys
2020
- Create temporary, time-bound keys with specific expiry (Slot Height).
@@ -33,36 +33,59 @@ The contract uses a PDA (Program Derived Address) architecture to manage state:
3333

3434
| Account Type | Description |
3535
| :--- | :--- |
36-
| **Wallet PDA** | The main identity anchor. |
36+
| **Wallet PDA** | The main identity anchor. Derived from `["wallet", user_seed]`. |
3737
| **Vault PDA** | Holds assets (SOL/SPL Tokens). Only the Wallet PDA can sign for it. |
38-
| **Authority PDA** | Separate PDA for each authorized key (Device/User). Stores role & counter. |
38+
| **Authority PDA** | Separate PDA for each authorized key (Device/User). Stores role & counter. Derived from `["authority", wallet_pda, key_or_hash]`. |
3939
| **Session PDA** | Temporary authority derived from a session key and wallet. |
4040

4141
---
4242

43+
## 📂 Project Structure
44+
45+
- `program/src/`: Main contract source code.
46+
- `processor/`: Instruction handlers (`create_wallet`, `execute`, `manage_authority`, etc.).
47+
- `auth/`: Authentication logic for Ed25519 and Secp256r1.
48+
- `state/`: Account data structures (`Wallet`, `Authority`, `Session`).
49+
- `tests-e2e/`: Comprehensive End-to-End Test Suite.
50+
- `scenarios/`: Test scenarios covering Happy Path, Failures, and Audit Retro.
51+
- `scenarios/audit/`: Dedicated regression tests for security vulnerabilities.
52+
53+
---
54+
4355
## 🛠️ Usage
4456

45-
### Build & Test
57+
### Build
4658
```bash
4759
# Build SBF program
4860
cargo build-sbf
61+
```
4962

50-
# Run E2E Test Suite (Devnet)
63+
### Test
64+
Run the comprehensive E2E test suite (LiteSVM-based):
65+
```bash
5166
cd tests-e2e
5267
cargo run --bin lazorkit-tests-e2e
5368
```
5469

55-
### Deployment (Devnet)
56-
Currently deployed at:
57-
> **Program ID**: `2r5xXopRxWYcKHVrrzGrwfRJb3N2DSBkMgG93k6Z8ZFC`
58-
5970
---
6071

61-
## 🔒 Security
72+
## 🔒 Security & Audit
73+
74+
LazorKit V2 has undergone a rigorous internal audit and security review.
75+
76+
**Status**: ✅ **17/17 Security Issues Resolved**
77+
78+
We have fixed and verified vulnerabilities including:
79+
- **Critical**: Cross-Wallet Authority Deletion (Issue #3).
80+
- **High**: Signature Replay (Issues #16, #13, #11), DoS prevention (Issue #4), OOB Reads (Issue #17).
81+
- **Medium**: Rent Theft protections (Issue #14) and Signature Binding (Issues #8, #9).
82+
83+
👉 **[View Full Audit Report](Report.md)**
6284

63-
- **Audited Logic**: Comprehensive checks for Replay Attacks, Privilege Escalation, and Memory Alignment.
64-
- **Version Control**: Built-in Schema Versioning (V1) for future-proof upgrades.
65-
- **Safe Math**: Strict arithmetic checks for all balance operations.
85+
### Security Features
86+
- **Discriminator Checks**: All PDAs are strictly validated by type constant.
87+
- **Signature Binding**: Payloads are strictly bound to target accounts and instructions to prevent replay/swapping attacks.
88+
- **Reentrancy Guards**: Initialized to prevent CPI reentrancy.
6689

6790
---
6891

‎Report.md‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
# Final Audit Report - LazorKit Wallet Contract
2+
3+
## Executive Summary
4+
This report documents the resolution of 17 reported issues in the LazorKit wallet management contract. All identified vulnerabilities, ranging from Critical to Low severity, have been addressed, remediated, and verified through a comprehensive refactored end-to-end (E2E) test suite.
5+
6+
**Status**: ✅ All Issues Fixed & Verified
7+
8+
## Verified Issues
9+
10+
### [Issue #17] OOB Read On Get Slot Hash
11+
- **Severity**: High
12+
- **Status**: ✅ Fixed
13+
- **Description**: `get_slot_hash` lacked proper bounds checking, allowing out-of-bounds reads.
14+
- **Fix**: Added explicit logic to return `AuthError::InvalidSignatureAge` (mapped to `PermissionDenied`) if the requested index is out of bounds.
15+
- **Verification**: Verified by `audit/cryptography.rs` (Scenario: Slot Not Found / OOB Rejected).
16+
17+
### [Issue #16] Old Nonces Can be Submitted Due To Truncation
18+
- **Severity**: Medium
19+
- **Status**: ✅ Fixed
20+
- **Description**: Slot truncation in nonce validation allowed reuse of old nonces after wrap-around.
21+
- **Fix**: Removed slot truncation logic; validation now uses full slot numbers and strict SlotHashes lookups.
22+
- **Verification**: Verified by `audit/cryptography.rs` (Scenario: Nonce Replay).
23+
24+
### [Issue #15] System Program Account Not Checked
25+
- **Severity**: Low
26+
- **Status**: ✅ Fixed
27+
- **Description**: The System Program account passed to `create_wallet` was not validated, allowing spoofing.
28+
- **Fix**: Added an explicit check `if system_program.key() != &solana_system_program::id()`.
29+
- **Verification**: Verified by `audit/access_control.rs` (Scenario: Fake System Program).
30+
31+
### [Issue #14] Missing Payer in Signed Payload (Transfer Ownership)
32+
- **Severity**: Medium
33+
- **Status**: ✅ Fixed
34+
- **Description**: The payer was not bound to the signature in `transfer_ownership`, allowing potential rent theft by replacing the payer.
35+
- **Fix**: Added the payer's public key to the `signed_payload` in `transfer_ownership`.
36+
- **Verification**: Verified by `audit/cryptography.rs` (Scenario: Transfer Ownership Signature Binding).
37+
38+
### [Issue #13] Missing Accounts in Signed Payload (Remove Authority)
39+
- **Severity**: High
40+
- **Status**: ✅ Fixed
41+
- **Description**: `process_remove_authority` did not bind `target_auth_pda` and `refund_dest` to the signature, allowing an attacker to reuse a signature to delete arbitrary authorities or redirect rent.
42+
- **Fix**: Included `target_auth_pda` and `refund_dest` pubkeys in the `signed_payload`.
43+
- **Verification**: Verified by `audit/cryptography.rs` (Scenario: Remove Authority Signature Binding).
44+
45+
### [Issue #12] Secp256r1 Authority Layout Mismatch
46+
- **Severity**: Medium
47+
- **Status**: ✅ Fixed
48+
- **Description**: Inconsistent writing (padding) vs. reading of Secp256r1 authority data caused validation failures.
49+
- **Fix**: Standardized the layout to consistent byte offsets for both read and write operations.
50+
- **Verification**: Verified implicitly by the success of all Secp256r1 operations in the test suite.
51+
52+
### [Issue #11] Missing Accounts in Signed Payload (Execute)
53+
- **Severity**: High
54+
- **Status**: ✅ Fixed
55+
- **Description**: `execute` instruction bound signatures only to account indices, allowing account swapping/reordering attacks.
56+
- **Fix**: Included full account public keys in the `signed_payload` instead of just indices.
57+
- **Verification**: Verified by `audit/cryptography.rs` (Scenario: Execute Signature Binding - Swapped Accounts).
58+
59+
### [Issue #10] Unintended Self-Reentrancy Risk
60+
- **Severity**: Low
61+
- **Status**: ✅ Fixed
62+
- **Description**: Risk of reentrancy via CPI.
63+
- **Fix**: Added a specific check `if get_stack_height() > 1` (or equivalent reentrancy guard) to critical paths.
64+
- **Verification**: Verified by code inspection and `audit/access_control.rs` (Scenario: Reentrancy Protection).
65+
66+
### [Issue #9] Secp256r1 Authenticator Allows Anyone to Submit
67+
- **Severity**: High
68+
- **Status**: ✅ Fixed
69+
- **Description**: Valid signatures could be submitted by any relayer without binding to a specific executor/payer.
70+
- **Fix**: Bound the transaction signature to the Payer's public key in `Secp256r1Authenticator`.
71+
- **Verification**: Verified by `audit/cryptography.rs` (Scenario: Secp256r1 Payer Binding).
72+
73+
### [Issue #8] Missing Discriminator in Signed Payload
74+
- **Severity**: Medium
75+
- **Status**: ✅ Fixed
76+
- **Description**: Signatures could be replayed across different instructions due to lack of domain separation.
77+
- **Fix**: Added instruction-specific discriminators to all `signed_payload` constructions.
78+
- **Verification**: Verified by `audit/cryptography.rs` (Scenario: Cross-Instruction Replay).
79+
80+
### [Issue #7] Wallet Validation Skips Discriminator Check
81+
- **Severity**: Low
82+
- **Status**: ✅ Fixed
83+
- **Description**: Wallet PDAs were checked for ownership but not for the specific `Wallet` discriminator, allowing other PDAs to masquerade as wallets.
84+
- **Fix**: Added `AccountDiscriminator::Wallet` check in `create_session` and other entry points.
85+
- **Verification**: Verified by `audit/access_control.rs` (Scenario: Wallet Discriminator Validation).
86+
87+
### [Issue #6] General Notes (N1, N2, N3)
88+
- **Status**: ✅ Fixed
89+
- **Fixes**:
90+
- **N1**: `auth_bump` is now properly utilized/checked.
91+
- **N2**: System Program ID validation added across instructions.
92+
- **N3**: RP ID Hash validation added to Secp256r1 authenticator.
93+
- **Verification**: Verified by `audit/access_control.rs`.
94+
95+
### [Issue #5] Hardcoded Rent Calculations
96+
- **Severity**: Low
97+
- **Status**: ✅ Fixed
98+
- **Description**: Rent was calculated using hardcoded constants, risking desynchronization with network parameters.
99+
- **Fix**: Switched to using `Rent::get()?.minimum_balance(size)` or `Rent` sysvar.
100+
- **Verification**: Verified by `audit/dos_and_rent.rs` (Scenario: Rent Calculation).
101+
102+
### [Issue #4] DoS via Pre-funding (Create Account)
103+
- **Severity**: High
104+
- **Status**: ✅ Fixed
105+
- **Description**: Attackers could DoS account creation by pre-funding the address with 1 lamport, causing `system_program::create_account` to fail.
106+
- **Fix**: Implemented "Transfer-Allocate-Assign" pattern (`initialize_pda_account` util) which handles pre-funded accounts gracefully.
107+
- **Verification**: Verified by `audit/dos_and_rent.rs` (Scenario: DoS Protection / Pre-funded accounts).
108+
109+
### [Issue #3] Cross-Wallet Authority Deletion
110+
- **Severity**: Critical
111+
- **Status**: ✅ Fixed
112+
- **Description**: `remove_authority` failed to check if the target authority belonged to the same wallet as the admin.
113+
- **Fix**: Added strict check: `target_header.wallet == wallet_pda.key()`.
114+
- **Verification**: Verified by `audit/access_control.rs` (Scenario: Cross-Wallet Authority Removal).
115+
116+
### [Issue #1 & #2] Audit Progress
117+
- **Status**: ✅ Complete
118+
- **Description**: Tracking tickets for the audit process itself. All items verified and closed.
119+
120+
## Test Suite Refactoring
121+
To ensure long-term maintainability and prevent regression, the test suite has been refactored:
122+
- **Location**: `tests-e2e/src/scenarios/audit/`
123+
- **Modules**:
124+
- `access_control.rs`: Covers logical permissions and validations (Issues #3, #7, #10, #15, #6).
125+
- `dos_and_rent.rs`: Covers DoS and Rent fixes (Issues #4, #5).
126+
- `cryptography.rs`: Covers signature binding and replay protections (Issues #8, #9, #11, #13, #14, #16, #17).
127+
128+
All tests are passing.

0 commit comments

Comments
 (0)