Contract audit trail fix#727
Merged
OlaGreat merged 3 commits intoJun 27, 2026
Merged
Conversation
# Add audit-trail events to `pause()` and `unpause()`
## Problem
`pause()` and `unpause()` flip the contract's `Paused` flag but never emit an event. Soroban events are the standard audit-trail mechanism for this contract — `support()` and `withdraw()` both publish one — so admin pause/unpause actions are currently invisible on-chain. There's no record of *when* the contract was paused/unpaused or *which* admin address triggered it.
This breaks any off-chain monitoring or indexer that relies on event feeds to track contract state, and makes post-incident analysis impossible if a pause needs to be investigated after the fact.
## Fix
Publish an event in both functions, following the existing pattern used by `support`/`withdraw`:
```rust
e.events().publish((symbol_short!("pause"), admin), e.ledger().timestamp());
e.events().publish((symbol_short!("unpause"), admin), e.ledger().timestamp());
```
Each event includes the admin address (topic) and the ledger timestamp (data), giving a complete audit record of who paused/unpaused the contract and when.
## Changes
- `pause()` — publish `("pause", admin)` event after setting `Paused = true`
- `unpause()` — publish `("unpause", admin)` event after setting `Paused = false`
## Testing
- Existing `support_fails_when_contract_is_paused` and `support_succeeds_after_unpause` tests still pass unchanged (they assert on state/panic behavior, not events).
- **TODO:** add an assertion on `e.events().all()` in at least one pause/unpause test to verify the event payload (admin + timestamp), since none of the current tests check emitted event data.
## Out of scope (follow-up)
`initialize()` has the same gap — it sets `Admin` and `Paused` for the first time without emitting anything, so there's no on-chain record of who the admin was set to or when the contract went live. Suggest a follow-up PR to add an `init` event there for consistency.
# Fix: `generateChallenge()` uses `Math.random()` — not cryptographically secure
## Problem
`auth.ts:31-34`:
```ts
const randomBytes = Buffer.from(
Array.from({ length: 16 }, () => Math.floor(Math.random() * 256))
).toString('hex');
```
`Math.random()` in Node.js is backed by V8's XorShift128+ PRNG — a fast, non-cryptographic generator seeded once at process start. It is not designed to resist prediction: given enough observed outputs, its internal state can be recovered, after which all future outputs become predictable.
`generateChallenge()` is called from the public `POST /auth/challenge` endpoint, so an attacker can observe a stream of challenges and use them to statistically predict — or in the worst case fully reconstruct — future nonces, potentially pre-generating valid challenges before they are issued.
## Fix
Use Node's `crypto` module, which draws from the OS-level CSPRNG and has no such weakness:
```ts
import { randomBytes } from "node:crypto";
export function generateChallenge(walletAddress: string): string {
const timestamp = Date.now();
const randomHex = randomBytes(16).toString("hex");
return `novasupport:${walletAddress}:${timestamp}:${randomHex}`;
}
```
Note: the local variable is named `randomHex` rather than `randomBytes` to avoid shadowing the imported `randomBytes` function — the original code's naming made it easy to accidentally call the wrong thing in future edits.
## Changes
- `auth.ts` — import `randomBytes` from `node:crypto`; replace the `Math.random()`-based byte array with `crypto.randomBytes(16)` in `generateChallenge()`
## Testing
- Manually verified `generateChallenge()` still returns a string in the same `novasupport:{wallet}:{timestamp}:{nonce}` format, with a 32-character hex nonce (16 bytes).
- No changes to `verifySignature`, `signJWT`, `verifyJWT`, or middleware — challenge format and downstream consumers are unaffected.
- **TODO:** add a unit test asserting two consecutive calls to `generateChallenge()` for the same wallet never produce the same nonce, and that the nonce is valid hex of the expected length.
## Out of scope (flagging for follow-up)
The challenge string has no visible expiry/TTL enforcement in this file. Worth confirming there's a TTL check elsewhere (route handler or challenge store) so a captured challenge+signature pair can't be replayed indefinitely. If this is already handled, disregard.
# Fix: `Profile.walletAddress` has no `@unique` constraint — duplicate wallet claims possible ## Problem `schema.prisma`: ```prisma walletAddress String ``` The wallet address is the root identity for authentication in NovaSupport. Nothing in the database prevents two `Profile` rows from sharing the same `walletAddress`. Since auth lookups resolve a profile by `walletAddress`, a duplicate produces non-deterministic authentication results — whichever row the query happens to return first wins. Concretely, this means an attacker who registers a second profile with a victim's wallet address could intercept that victim's webhook deliveries and milestone notifications, since lookups by wallet may resolve to the attacker's profile instead. ## Fix Add the uniqueness constraint at the schema level: ```prisma walletAddress String @unique ``` This is the correct invariant: every other identity-bearing field on `Profile` (`username`, `email`, `emailVerificationToken`) is already `@unique`, and `AuthChallenge.walletAddress` is `@unique` too — `walletAddress` was the one identity field missing it. ## Migration A straight `ALTER TABLE ... ADD CONSTRAINT UNIQUE` will fail outright if any duplicate `walletAddress` values already exist in production, blocking the deploy. The migration instead: 1. **Ranks** any duplicate `walletAddress` rows by `createdAt`, treating the oldest profile as canonical. 2. **Renames** the conflicting (non-canonical) rows by appending a `::dup-N` suffix to their `walletAddress` — this only unblocks the constraint, it does **not** decide which profile is legitimate. 3. **Adds** the unique index. 4. Leaves commented SQL for a manual post-deploy pass: find all `::dup-N`-suffixed rows, contact the affected owners, and re-verify wallet ownership via signature challenge before restoring or reassigning the address. Auto-resolving (e.g. deleting or nulling the duplicate) was deliberately avoided — `walletAddress` is required, and a wrong automated decision could lock out a legitimate user. This needs a human in the loop. ```sql -- inspect duplicates before running in production SELECT "walletAddress", COUNT(*), array_agg(id ORDER BY "createdAt") FROM "Profile" GROUP BY "walletAddress" HAVING COUNT(*) > 1; ``` ## Changes - `schema.prisma` — `Profile.walletAddress` is now `@unique` - `migration.sql` — dedupes existing rows (non-destructively) and adds the unique index ## Testing - Verified the schema change doesn't affect any other model — no other field references `Profile.walletAddress` as a join key, so this is purely an additive constraint. - **TODO:** add a test asserting that creating a second `Profile` with an already-claimed `walletAddress` throws/rejects at the database layer. - **TODO:** run the duplicate-detection query against a staging snapshot of production data before merging, to size the manual cleanup work ahead of deploy. ## Follow-up (out of scope here) Confirm whether the auth lookup in `app.ts` uses `findFirst` or `findUnique` for `walletAddress`. Once the constraint exists, `findFirst` will behave correctly, but switching to `findUnique` better expresses the new invariant and should be marginally faster.
|
@blockchainrafik Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security & data-integrity fixes: auth randomness, wallet uniqueness, pause/unpause audit events
This PR bundles four related fixes found during a review pass over the auth flow, the Soroban contract, and the Prisma schema. Each is small and independent, but grouped here since they touch adjacent code and were found in the same pass.
Closes #701
Closes #702
Closes #703
Closes #700
1.
generateChallenge()usedMath.random()— not cryptographically secureCloses #701
Problem
Math.random()in Node.js is backed by V8's XorShift128+ PRNG — fast, but not cryptographically secure. Given enough observed outputs its internal state can be recovered, after which future outputs become predictable.generateChallenge()is called from the publicPOST /auth/challengeendpoint, so an attacker can observe a stream of challenges and predict — or reconstruct — future nonces, potentially pre-generating valid challenges before they're issued.Fix
(Local variable renamed to
randomHexto avoid shadowing the importedrandomBytesfunction.)Testing
novasupport:{wallet}:{timestamp}:{32-char hex nonce}.Follow-up (out of scope)
No visible TTL/expiry enforcement on challenges in
auth.tsitself — confirm this is enforced elsewhere (e.g.AuthChallenge.expiresAtin the schema) so a captured challenge+signature can't be replayed indefinitely.2.
Profile.walletAddresshad no@uniqueconstraint — duplicate wallet claims possibleCloses #702
Problem
Wallet address is the root identity for authentication. Nothing prevented two
Profilerows from sharing the samewalletAddress, so auth lookups by wallet could resolve non-deterministically. An attacker registering a second profile with a victim's wallet address could intercept that victim's webhook deliveries and milestone notifications.Fix
walletAddress String @uniqueBrings
walletAddressin line with every other identity field onProfile(username,email,emailVerificationTokenare all already@unique), and matchesAuthChallenge.walletAddress, which was already@unique.Migration
A bare
ADD CONSTRAINT UNIQUEfails outright if duplicates already exist in production.migration.sqlinstead:walletAddressrows bycreatedAt, treating the oldest as canonical.::dup-Nsuffix to unblock the constraint, without deciding which profile is "legitimate."::dup-Nrows, contact affected owners, and re-verify wallet ownership via signature before resolving.No automatic deletion or nulling —
walletAddressis required, and a wrong automated call could lock out a legitimate user.Testing
Profile.walletAddress, so this is a purely additive constraint.Profilecreated with an already-claimedwalletAddressis rejected.Follow-up (out of scope)
Confirm whether the auth lookup in
app.tsusesfindFirstorfindUniqueforwalletAddress. Both work correctly once the constraint exists, butfindUniquebetter expresses the new invariant.3. Duplicate
AcceptedAssetrows from concurrent PATCH requests**Closes #700 **
Problem
AcceptedAssethad no constraint preventing the same(profileId, code, issuer)combination from being inserted more than once. ConcurrentPATCHrequests against a profile's accepted-assets endpoint could race and insert duplicate rows for the same asset.Fix
@@unique([profileId, code, issuer])Testing
(profileId, code, issuer)and asserting only one succeeds.4.
pause()/unpause()emit no on-chain audit events**Closes #704 **
Problem
pause()andunpause()flip the contract'sPausedflag but never emit an event, unlikesupport()andwithdraw()which both publish one. There's no on-chain record of when the contract was paused/unpaused or which admin triggered it, breaking off-chain monitoring and post-incident analysis.Fix
Testing
support_fails_when_contract_is_pausedandsupport_succeeds_after_unpausetests pass unchanged.e.events().all()in a pause/unpause test to verify the emitted admin address and timestamp.Follow-up (out of scope)
initialize()has the same gap — no event emitted whenAdmin/Pausedare first set. Suggest a follow-up PR for aninitevent.Summary of changed files
auth.ts— crypto-secure nonce generationschema.prisma—walletAddress @unique,AcceptedAssetcomposite unique constraintmigration.sql— non-destructive dedup + unique index forwalletAddresslib.rs(Soroban contract) — pause/unpause events