Reject transaction signers without verification contracts - #121
Open
Jim8y wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is narrowly scoped, matches the stated audit goals (early explicit failures), and is backed by a focused harness validating the new behavior paths.
Pull request overview
This PR addresses audit item P2-14 by making unsigned transaction construction fail explicitly (with dAPI error codes) when the wallet is missing, no implicit payer can be selected, or any requested signer lacks a known local verification contract—preventing null dereferences and preventing unsupported signer hashes from reaching simulation/fee calculation.
Changes:
- Update
RpcClient.MakeTransactionAsyncto throwDapiExceptionfor missing wallets/implicit payers (10003) and for signers without known local verification scripts (10001), before any RPC is sent. - Add a standalone console-based test harness (
tests/p2-14) that exercises the unsupported-signer and implicit-payer paths against the realRpcClientsources with an in-memory HTTP handler. - Register the new harness project in
OneGateApp.slnx.
File summaries
| File | Description |
|---|---|
| tests/p2-14/README.md | Documents the P2-14 harness purpose, how to run it, and expected error-code behavior. |
| tests/p2-14/Program.cs | Implements 13-case harness validating early rejection and witness/script handling via a fake HTTP transport. |
| tests/p2-14/PlatformStubs.cs | Stubs MAUI filesystem + unused diagnostics type needed to compile linked production sources in the harness. |
| tests/p2-14/P2-14.Tests.csproj | Adds an executable harness project compiling/linking the production RpcClient and related models. |
| OneGateApp/Services/RPC/RpcClient.cs | Adds explicit error handling and pre-RPC signer verification-script enforcement during transaction construction. |
| OneGateApp.slnx | Includes the new P2-14 harness project in the solution. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+137
to
+142
| var client = new RpcClient(new WalletProvider(wallet), ProtocolSettings.Default); | ||
| // Test-only replacement of the private transport: no real node requests, signatures or wallet saves. | ||
| var transport = typeof(RpcClient).GetField("http", BindingFlags.Instance | BindingFlags.NonPublic)!; | ||
| ((HttpClient)transport.GetValue(client)!).Dispose(); | ||
| transport.SetValue(client, new HttpClient(rpc)); | ||
| return client; |
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.
Summary
Fix audit P2-14: fail explicitly when unsigned transaction construction cannot obtain a verification script for every signer, instead of dereferencing a missing wallet account or contract.
This changes the existing transaction-construction path only. It does not add a dAPI method, address-read permission prompt, external multisig coordination, or deployment-contract witness support.
Validation
dotnet run --project tests/p2-14/P2-14.Tests.csproj --no-restore. The test project is registered inOneGateApp.slnxand has a non-build/non-output reference to the app project.RpcClient, Neo wallet models and serializers. Only the upstream HTTP transport was controlled. Missing witnesses failed before any RPC; local known-contract construction, implicit payer selection, and multisig signer order remained valid.6a3336c0c633579eeddc42a70200697435c59955546df24f8f74ad4aa31fd0a9, independently based onmaster@623603d634f07eaead14745da87920a356159be0. Both platform evidence sets match this hash.Scope and limits
The native fixture called the real transaction builder with in-memory HTTP responses; it was not a live-node transaction, JavaScript-provider round trip, wallet authorization, signature or broadcast test. No real private key or funded wallet was used. Unsupported external/deployed witnesses remain unsupported, now with an explicit error.
Screenshots, fixture code and result JSON stay outside the repository; no GitHub asset upload or hosted CI pass is claimed. Before publication, recheck the current master, current feedback, duplicate PRs and the complete diff. Related independently scoped transaction/NFT changes share
RpcClient; preserve their source and linked-test dependencies if later integrated, without treating these isolated tests as combined validation.