Repository navigation
Conversation
c084a26 to
86748d0
Compare
86748d0 to
6c1f21a
Compare
There was a problem hiding this comment.
I remember we discussed to have this monitoring outside NG. Also, in https://github.com/Lunar-Rails/ep-backend/pull/2338/changes#r4142040308 you mentioned ep-recheck (I would name it differently btw like ep-audit or whatever)
My point here (a bit of nitpick) is, this document currently belongs to NodeGuard. Perhaps either we create such repo apart of NG. Either on Notion, in ep-backend or even creating the new component agreeing first to have it. As you can see, there is more than one feature to place on it so probably it will be a good thing if we all agree on that.
WDYT?
@daliclovr pinging you as well as we need consensus on: "Should we create an ep-audit/ep-recheck/ep-whatever to hold accounting tasks"?
There was a problem hiding this comment.
I believe that we discussed to have this monitor outside ep-backend, not outside nodeguard. Specifically because onchain outflows belong to nodeguard
| 2. **Unjustified** — a matching request exists, but it was not created by a logged-in user (UI) or by | ||
| the backend through the authenticated gRPC API. |
|
|
||
| Raise an alert when money leaves a NodeGuard wallet and either: | ||
|
|
||
| 1. **Unmatched** — the transaction does not correspond to any request in the NodeGuard database, or |
There was a problem hiding this comment.
What about unsolicited bumps, that is, fee bumps done outside NG? That's a risk as well because we would lose the track of the tx
| | Keys used outside NodeGuard (e.g. signers co-sign a tx directly) | §4 matching — no request | | ||
| | Request row inserted/edited directly in the DB | §5 audit cross-check, §6 signatures | | ||
| | Request `TxId` edited to point at a rogue tx | §4 output check, §6 TxId signature | | ||
| | Request row *and* audit row forged directly in the DB | §6 signatures only | | ||
| | Request backdated to fall under an exemption | No date-based exemptions exist (§5, §6.5) | | ||
| | Withdrawal template PSBT outputs edited before anyone signs | §6 template signature; blocked before signing by §11 | | ||
| | Change shrunk to inflate the fee | Withdrawals: §6 template signature, §11. Channel opens: §11 fee rate limit | | ||
| | Channel-open outputs edited after human approval (SIGHASH_NONE) | §11 checks before the internal wallet signs | | ||
| | Stolen API token used to request withdrawals | **Not covered** — the request is created through the legitimate gRPC path and counts as justified | | ||
| | Attacker with code execution on the app host | **Not covered** — they can create legitimate-looking requests and get them signed. KMS (§6.6) only stops them from keeping the key after they lose access | |
There was a problem hiding this comment.
What do you think about this scenario that comes to my mind?
No activity on NG's withdrawal request, but we catch a tx with different hash, recently mined with same inputs than a NG withdrawal request?
There was a problem hiding this comment.
The job monitors the mempool and the blockchain, not the withdrawal requests. So as soon an input from one of our tracked wallets is in the mempool, nbxplorer will launch an event and we'll do all the needed checks, so that's also contemplated in this feature.
| A long-running job, `WalletOutflowSubscriptorJob`, started at boot like `NodeSubscriptorJob`, using | ||
| `IDbContextFactory<ApplicationDbContext>`. | ||
|
|
||
| - **Startup scan.** For every NodeGuard wallet, `GetTransactionsAsync` from NBXplorer and evaluate |
There was a problem hiding this comment.
Related to https://github.com/Elenpay/NodeGuard/pull/665/changes#r4478512055 , I think it's also worthy to scan every tx on mined blocks.
Not a blocker for initial release OFC but something to be added to the checklist.
There was a problem hiding this comment.
At the moment this feature trusts nbxplorer to tell us with GetTransactionsAsync, which are the outgoing transactions in each wallet. But I believe you are right, we should't trust nbxplorer and we should scan all transactions. Will add it as a next steps thing
Summary
Today, NodeGuard does not alert when funds leave a NodeGuard wallet without a matching request, or with a request that no legitimate user or API token created. This PR adds a design document for an outflow monitor that detects these cases and alerts through Grafana. The design also adds HMAC signatures on request and template PSBT rows, so a direct database write cannot fake a justified outflow. This PR contains only the design; implementation follows in three PRs.
Scope
Validation
No build or tests ran, because this PR changes only Markdown.
Suggested review order
TxIdrace.