fix(auth): replace non-atomic exists+del with getdel to prevent TOCTOU race

[AbelOsaretin]

Closes #72

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Summary

The verifyChallenge method in auth.service.ts had a TOCTOU race condition where redis.exists() was called at line 81 and redis.del() at line 150, with a long processing window in between (XDR decoding, signature verification, database queries). Two concurrent verify requests for the same address could both pass the exists check before either deleted the key, enabling double JWT issuance from a single SEP-10 challenge.

This PR replaces the non-atomic exists/del pair with a single redis.getdel() call that atomically retrieves and deletes the challenge, ensuring each challenge is consumed exactly once.

Motivation / Context

Fixes #72 — an attacker can obtain multiple JWTs from a single SEP-10 challenge, enabling token multiplication. The issue was identified as a security concern with clear acceptance criteria: two concurrent verify requests for the same challenge should only succeed once.

Detailed Changes

src/modules/auth/auth.service.ts:

• Replaced redis.exists() check + redis.del() cleanup with redis.getdel() at the start of verifyChallenge
• getdel atomically returns the value if the key exists and deletes it — only the first concurrent request will receive the challenge data
• Removed the later redis.del() call since the challenge is now consumed atomically at entry
• If getdel() returns null, the existing "Challenge expired or not found" error is thrown

tests/unit/services/sep10-auth.test.ts:

• Updated Redis mock to use getdel instead of exists/del
• Updated all test assertions to use mockRedis.getdel appropriately

Testing

• Unit tests: 10/10 passing in tests/unit/services/sep10-auth.test.ts
• Lint: 0 errors (only pre-existing warnings)
• Typecheck: pre-existing error about missing async-lock module (unrelated to this change)

[DeFiVC]

Review

Approve

All CI checks green. The fix correctly replaces the non-atomic + pair with , which atomically retrieves and deletes the challenge in a single operation. This eliminates the TOCTOU race condition where concurrent requests could both pass the existence check.

What's good:

• Minimal, focused change — exactly what the issue asked for
• Correct use of for atomic check-and-delete
• Tests properly updated to mock the new behavior
• Single clean commit with descriptive message

No issues found. Ready to merge.

[DeFiVC]

Review

Approve

All CI checks green. The fix correctly replaces the non-atomic redis.exists() + redis.del() pair with redis.getdel(), which atomically retrieves and deletes the challenge in a single operation. This eliminates the TOCTOU race condition where concurrent requests could both pass the existence check.

What is good:

• Minimal, focused change - exactly what the issue asked for
• Correct use of getdel for atomic check-and-delete
• Tests properly updated to mock the new behavior
• Single clean commit with descriptive message

No issues found. Ready to merge.