fix(evm): a second bug-hunt pass, five subsystems plus nwo and statedelta - #2232
Open
atharrva01 wants to merge 35 commits into
Open
fix(evm): a second bug-hunt pass, five subsystems plus nwo and statedelta#2232atharrva01 wants to merge 35 commits into
atharrva01 wants to merge 35 commits into
Conversation
Needed to build this branch locally; already open separately as LFDT-Panurus#2228 and will drop out here once that merges and this branch rebases. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The finality watcher polled StatusByAnchor until either the anchor appeared or the timeout expired, and on timeout it always reported Invalid. If the chain was unreachable for the whole window, every poll errored and got skipped, so the loop reached the timeout having never actually observed the ledger, and reported Invalid anyway. The shared ttx listener maps Invalid straight to a deleted transaction, so a connectivity outage on the reading side could make a transaction that actually committed look failed, and its tokens would be dropped from local bookkeeping. The watcher now tracks whether any poll in the window actually reached the chain, valid or not. Only then does an absent anchor at the timeout mean Invalid. If every attempt errored, it reports OnError instead, the same signal the interface already defines for "the finality event could not be delivered." The transaction stays Pending rather than being marked deleted, and the driver's existing recovery sweep picks it up again later with a fresh read. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The default block tag is finalized, and reading at that tag has a real time-to-finality of roughly 13 minutes (design §7.2). The default finality timeout was 5 minutes, so a deployment running on defaults alone would time out on every single transaction, valid or not, and report it Invalid before the chain could ever finalize it. The design already documents this exact constraint (§7.5: "any deployment must configure finality.timeout above ... the chain's finality"), but nothing enforced it. DefaultFinalityTimeout is now 20 minutes, with real margin over the ~13 minute floor rather than sitting at its edge. Validate also now rejects a finalized-tag configuration whose timeout is shorter than that floor, so a deployer who explicitly sets an unsafe combination gets a startup error instead of every transaction silently failing later. The floor applies only to the finalized tag; safe and latest resolve on their own, faster schedules and are not validated here. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The watcher recorded a new version as seen before calling the handler that actually applies it, and the handler had no way to report failure at all (UpdateHandler returned nothing). If applying a version failed for any reason, the watcher had already moved past it: the next poll only looks at what changed since the last seen version, so a failed reload was silently never retried, and the node kept serving stale public parameters with nothing left to notice the gap. UpdateHandler now returns an error, and the watcher only advances past a version once its handler actually succeeds; a failure is logged and the same version is retried on the next poll. applyPublicParams collects and returns the combined error of every TMS that failed to update, so a partial failure is visible to the watcher rather than swallowed. Retrying the whole batch is safe: updating a TMS with parameters it already holds is a no-op, so a TMS that already succeeded is not disturbed by covering it again. Covered at the watcher, where the actual defect lived: a new test drives a handler that fails twice then succeeds and asserts the same version is retried rather than skipped, and that seen only advances on the eventual success. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
ChainProvider.PublicParams read the parameter bytes and the version as two separate, unsynchronised calls. An endorsed setup delta landing between them could tear the pair: bytes from before the update, version from after, or the reverse. The contract checks both fields together against what it currently holds and reverts StalePublicParams on a mismatch, so a torn read here did not corrupt state, but it turned a purely local race in this function into a doomed, gas-spending transaction the contract was always going to reject. The version is now read before and after the bytes, and the whole read is retried if it moved: version and bytes only ever change together, in the same transaction, so two matching reads bracketing the bytes read is proof nothing landed in between. The retry is bounded (three attempts) so a pathological chain that never settles fails with an error instead of spinning. TestWatcherSurvivesAFailedRead needed a related fix: it modelled the version as the raw RPC call count, which does not hold once PublicParams reads the version twice per attempt, two call counts a few lines apart would themselves look torn. Rewritten to use the same stable chain-state double the other watcher tests already use. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
SetupPublicParams checked only Network.endorsement, the field tests inject a stub into directly. In production nothing ever sets that field: the driver wires the per-TMS endorsement factory through endorsementFor instead, keyed by an already-built management service. SetupPublicParams never has one of those to hand it, that is the entire reason it takes a bare TMSID rather than a management service, so it could never reach the factory at all. Every call failed with "no endorsement service configured" on any real deployment, and first-time setup of a namespace, the one thing this method exists to make possible, could not work. Nothing caught this: the shared ppsetup view exercises the real production path, but the EVM integration suite bootstraps and updates parameters through its own harness-side submitter instead, bypassing this method entirely, so the gap was invisible to every existing test. Network now also carries endorsementForID, a TMSID-keyed counterpart to endorsementFor, and SetupPublicParams resolves through that instead. The driver installs it in installEndorsement next to the existing factory: both ultimately call the same per-TMS ServiceFactory.ForTMS, one entered from a management service, the other from the id alone. New tests cover what nothing did before: SetupPublicParams resolving through the id-based factory and reaching it with the requested TMS, and a failed endorsement collection not broadcasting. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…nonce allocation NonceManager split allocation and recovery into two separate critical sections: Next handed out a nonce and released the lock, and a failed Submit later called Reset, which walked the whole sequence back to whatever eth_getTransactionCount(pending) currently showed. That call only reflects transactions the node has actually seen. A different, concurrent Submit that had already allocated a higher nonce but not yet reached SendRawTransaction was invisible to it, and Reset would hand that same nonce to a third caller, producing two transactions racing for one nonce. Every path that called Reset already treated its own failure as certain proof the transaction never reached the chain: gas estimation and fee suggestion are read-only, signing is local, and a rejected broadcast is documented as never judged by the chain. So walking back to the chain's view was never actually necessary, it just happened to be how the recovery was implemented, and that implementation was what raced. NonceManager.Next and Reset are replaced by WithNonce, which holds the lock for the whole allocate-and-use step. The sequence advances only if the callback succeeds; on failure the nonce is simply left where it was, with no round trip to the chain, and nothing else could have been mid allocation while the callback ran. Submitter.Submit now runs its entire body inside that callback. New tests cover the failure path directly (a failed attempt does not advance the sequence and needs no re-sync) and drive many goroutines with a mix of successes and failures to check every successful attempt still gets a distinct nonce. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…discarding it silently Driver.New runs once per (network, channel) the node is configured for, and installEndorsement, called from it, builds a fresh per-network ServiceFactory and tries to register this node's endorsement responder every time. That registration used a sync.Once scoped to the whole Driver, not to a network, so on a node configured to endorse for more than one EVM network, only the first network's registration ever ran. Every later network's factory, key and allowlist were silently discarded, with nothing logged to say so. The reason a straight per-network fix is not possible: FSC routes an incoming session to a responder by the initiating view's Go type alone, with no notion of "this responder, but only for network X". Only one Responder can ever be registered for endorsement.Initiator across a process's lifetime, so whichever network's factory happens to win the race is baked into it permanently, EIP-712 domain, chain client and all. Routing a second network's requests through it would not fail cleanly, it would validate against the right TMS but sign and read against the wrong chain. registerEndorser now tracks which network it registered for and refuses, loudly, when a different network also wants to endorse: an operator gets a clear error naming both networks instead of a request that silently never gets answered. The same network registering twice (a network rebuilt over a node's life) and a network that never wanted to endorse in the first place both remain unaffected. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…t registration Two comments, on the Allowlist field and on Authorizer itself, promised that an empty allowlist would default to "the TMS network's nodes, resolved at config load in Week 5". That resolution was never built. Authorizer.NewAuthorizer is deliberately fail-closed and rejects an empty allowlist outright, which is the right call for authorization, but nothing upstream of it ever supplied the promised default, so an operator who left Allowlist unset trusting the documented behavior got a node that came up looking healthy and silently never registered as an endorser, the failure logged as an error easy to miss during wiring rather than surfaced as the startup failure it should have been. Validate now rejects an endorser.enabled configuration with no allowlist, matching this file's own stated philosophy that a bad configuration should be a startup error, not a surprise later. Both comments are corrected to describe the actual, intentional fail-closed behavior instead of a fallback that does not exist. The integration harness is unaffected: it already builds the allowlist itself from every node in the TMS rather than relying on the driver to do it, which is what the documented default was supposed to be doing. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…gree Broadcast checked only that an envelope carried a delta at all, never that the envelope's own anchor and the anchor baked into that delta actually named the same transaction. Under the normal flow they always agree, since RequestApproval and SetupPublicParams both derive the envelope's anchor and the delta's anchor from the same value, but Broadcast has no way to know how the envelope it was actually handed was built. A mismatch here is not just a theoretical validation gap. The chain only ever looks at the delta's anchor: that is what applyStateDelta checks for replay, what the digest covers, what StateCommitted is emitted for. The local side tracks the transaction by the envelope's anchor instead, finality listeners and the ttx store are keyed on it. If the two ever diverged, the transaction would apply and commit on chain under one anchor while everything local kept waiting on a different one, and after the finality timeout wrongly report a transaction that actually succeeded as failed, the same failure shape HIGH #1 fixed, just reachable through a construction bug instead of a chain-read one. Broadcast now parses the envelope's anchor and compares it against the delta's before spending any gas, and refuses if they disagree. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…ted tags FinalityConfig.BlockTag's own comment said state is read at finalized or safe, but Validate has accepted latest as a third legal value since it was introduced for the local, instant-mining test harness. The comment now names all three and repeats, next to the field itself, what BlockTagLatest's own doc comment already says: it carries no reorg protection and is only appropriate for a local chain. No behavior changes; latest was already accepted before this commit. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
AnchorFromTxID hex-decodes a token-request anchor, and every caller hands it attacker-influenced input before anything about it has been checked: an endorser decodes a request's own anchor before it has validated that request (endorsement/delta.go), a node decodes an input's TxId while translating a transfer action (statedelta/ translator.go), and a node decodes an envelope's anchor straight off the wire (network.go). AGENTS.md asks for a FuzzXxx on exactly this shape of function, and this one had none while its sibling parsers (eip712.RecoverAddress, eip712.NewSignerFromBytes, the envelope and ABI decoders) already do. The function itself already handles malformed input cleanly, hex decode errors and length checks both return proper errors rather than panicking, confirmed by a million-plus fuzz executions with no failures. This closes the coverage gap the rule asks for rather than fixing a live bug. Wired into nightly-fuzz.yml so it runs under extended -fuzztime, not just its seed corpus. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…ero base fee
baseFee unmarshaled the node's response into a plain struct value, not
a pointer. Unmarshaling a JSON null into a non-pointer target is a
documented no-op in encoding/json, so a node returning result: null
for eth_getBlockByNumber("latest") left the struct at its zero value,
identical to the legitimate case the empty-string check exists for: a
pre-London or zero-fee chain that simply has no baseFeePerGas field.
Both were read as a real base fee of zero.
That zero fed straight into SuggestGasFees's maxFee = baseFee*2 + tip,
which every Submit call uses to price its transaction, so a transient
or malformed response from the node silently produced an underpriced
transaction instead of surfacing as an error.
head is now a pointer, the same pattern GetTransactionReceipt and
IsPending already use to tell a null result apart from a present one
with an empty field.
Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…ady does Call went through the generic call() wrapper and returned every JSON-RPC error the same way, while EstimateGas explicitly classifies a revert as ErrExecutionReverted so a caller can tell a permanent rejection from a node that simply failed to answer. Both eth_call and eth_estimateGas can revert against a real node; nothing in the EVMClient interface said only one of them would. No caller is affected today: every method Call is currently used for (getToken, getPublicParameters, getTransferMetadata, getTokenRequestHash, getPublicParamsVersion) is a plain storage read with no revert condition in the Solidity source. This closes the gap in the interface itself before the first Call against a method that can revert has to rediscover it. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
decodeHex (types.go, used by HexToAddress/HexToHash) tolerated both 0x and 0X, but decodeHexBytes, parseHexUint and parseHexBig in jsonrpc.go, which decode every JSON-RPC response quantity and data field, only stripped a lowercase 0x. The two paths disagreed on what counts as a hex prefix for the same syntax in the same package. Every real node emits lowercase 0x, so this never produced a wrong value, only an avoidable inconsistency; a 0X-prefixed response failed with a parse error rather than being misread. Extracted the prefix stripping decodeHex already had into a shared trimHexPrefix and pointed all four parsers at it, so there is one rule instead of two copies that can drift. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
jsonLog.toLog and jsonReceipt.toReceipt turn a response body from whatever node the driver is pointed at into driver types, and are called from GetLogs (log scanning for anchor resolution) and GetTransactionReceipt (finality). Every sub-parser they call (HexToAddress, HexToHash, decodeHexBytes, parseHexUint) already fails safely and most are already fuzzed individually, but the assembly functions themselves had no direct coverage, unlike the ABI decoders and the envelope wire decoder, which do. No panic found in either fuzz target. Wired both into nightly-fuzz.yml so they run under extended -fuzztime rather than only their seed corpus. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Authorizer's doc comment said the allowlist is configured per TMS. Traced end to end, it isn't: configNetworkResolver.ConfigFor loads the EVM Config of whichever TMS declares the network first and that single Config, allowlist included, is what every TMS on the network gets checked against (Responder.factoryFor resolves a factory per TMS, but Authorize runs before that, off the one Authorizer the node was built with). The connection fields in Config really are shared across a network's TMS; the endorsement policy fields ride along with them by accident of being in the same struct, not by design. Comment now says what the code actually does and names the reason, so the gap between two TMS wanting different requester sets is a known, documented limitation instead of a silent surprise for the next person who reads only this file. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…ad of lying Both functions resolve spent status through tokenMarker[tokenID], the content-bound marker recorded at output creation. A graph-hiding driver never populates that marker: translator.go leaves OutputToken .SNMarker at its zero value for graph-hiding outputs, since that mode spends by serial number instead, and graph-hiding spends only ever write serialUsed, never snSpent. So on a graph-hiding clone tokenMarker[tokenID] is always 0x0, snSpent[0x0] is never set, and isSpent/areTokensSpent always answer false, including for a token that was in fact spent via serialUsed. On-chain enforcement in applyStateDelta was never affected: it branches on graphHiding directly and checks serialUsed, not these query functions. But any caller of the public ABI (a wallet, an explorer, a monitoring tool) that asks isSpent against a graph-hiding TMS got a plausible-looking wrong answer instead of an error. Both functions now revert with UnsupportedForGraphHiding on that clone; isSerialUsed is the correct query for it and already existed. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…ies actually check something FuzzJSONLogToLog and FuzzJSONReceiptToReceipt, added earlier this session in bcb57ef right after two real bugs in this same decoding file, claimed in their own doc comment that malformed input is rejected rather than panicking the caller, but the fuzz bodies discarded the result entirely (_, _ = j.toLog()). That only proves no panic; it gives no signal at all if a future change drops one of the inner error checks and starts returning a zero-valued field instead of an error. Confirmed the gap was real before fixing it: temporarily dropped the error check on HexToAddress inside toLog and reran the fuzzer, which took under 15 seconds to find the exact class of bug the finding described, undetected by the old property. Reverted the injection immediately after confirming it. Both properties now re-derive every field of a successful decode directly from the raw JSON strings via the same already-fuzzed sub-parsers (HexToAddress, HexToHash, decodeHexBytes, parseHexUint) and require an exact match, through a shared assertLogMatchesRaw helper. This checks that toLog/toReceipt actually wire each field through its parser and propagate that parser's error, without reimplementing hex parsing itself. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The doc comment said the interesting case is a value that does not fit in 64 bits, which must be rejected rather than silently truncated, but the fuzz body discarded the result (_, _ = DecodeUint64(ret)), unlike its siblings FuzzDecodeBytes and FuzzDecodeBoolArray in the same file, which do check a real post-condition. A regression narrowing DecodeUint64's high-byte check (an off-by-one on the loop bound, for instance) would pass this fuzz target undetected. On a successful decode, the property now independently re-checks that every byte outside the low 8 is zero (via bytes.Equal against a zero-filled slice, not the same byte-loop DecodeUint64 uses) and that the decoded value matches reading the low 8 bytes directly. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The fuzz body re-called checkSignatureFormat on a signature RecoverAddress had already accepted. RecoverAddress calls that exact function itself first and returns before recovery on any format failure, so by the time the fuzz body reached this line the answer was already guaranteed nil - a pure function asked the same question of the same input a second time, which can never fail and verified nothing. The doc comment overclaimed too: it read as if this target proves Go's format rules match the EndorsementVerifier contract's, but that cross -check happens elsewhere (TestRecoverRejectsMalformed, EndorsementVerifier.t.sol's format-rejection cases, and the Go<->Solidity golden fixture in TestGoldenFixtureEndorsement / GoEndorsement.t.sol). Removed the dead check and rewrote the comment to describe what this target actually verifies: no panic, and any accepted signature recovers a real, non-zero address. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Validate's IsSetup branch checked SpentRefs and Outputs are empty and SetupParameters is present, but never checked MetadataKeys/ MetadataVals. TokenState.sol's _applySetup reverts MalformedSetupDelta on non-empty metadataKeys too, so this was an asymmetry between what Go refuses to sign and what the contract refuses to apply. Translator itself never reaches this path (writeSetup already blocks mixing setup with prior metadata), but Validate is also the safety net for a StateDelta assembled directly rather than through Translator - nwo/setup.go builds one by hand, and only happens not to trigger this today because it leaves metadata nil. Validate's own doc comment says it exists so endorsers fail fast rather than sign a malformed delta; this closes the one field it was missing. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
writeIssue and writeTransfer both called action.IsGraphHiding() fresh inside their per-output loop. It is an interface method, not a guaranteed-pure field, so nothing enforced that it answers the same way on every call within one action; if it ever didn't, one action's outputs could split across both marker styles, with whichever outputs saw the wrong answer silently losing their real SNMarker. The Fabric reference translator this package mirrors reads it once, before its own output loop, specifically to rule this out. Neither shipped driver can trigger this today (fabtoken and zkatdlog/nogh both hardcode IsGraphHiding to a constant false), so this is a latent divergence from the audited-safe reference pattern rather than a live bug, confirmed via a counterfeiter mock returning different values on successive calls in the new regression tests. Cached the value once at the top of each function instead. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Nothing in this package's unit tests drove Responder.endorse with a SetupAction before this, and the integration harness's own way of bumping public parameters mid-test (nwo.SetupUpdater) hand-assembles and signs a StateDelta directly rather than calling into Responder, DeltaFactory, or Translator.writeSetup at all. So the production code path a real endorser actually runs for an administrative PP update had zero coverage anywhere in the repo - a bug in Authorize, Build, or writeSetup specific to the setup shape could ship undetected while every other test stayed green. TestResponderEndorsesASetupAction drives one through Handle end to end and confirms the signature recovers to the endorser over the independently-recomputed digest, the same no-blind-sign check TestResponderSignsWhatItBuilds already does for issue actions. It passed on the first run: this closes a real coverage gap rather than a live bug in the pipeline itself. This does not touch nwo.SetupUpdater or the integration harness's bypass, which is a separate, larger question about whether integration tests should route PP updates through real endorser sessions instead. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
NewSetupUpdater validated Client, Submitter, ChainID and EndorserKeys but not TokenState, so a caller that wires one up before Deploy has populated the real contract address (the real integration caller does exactly this, building config from a Deployment struct that starts zero-valued) got a constructor that succeeded silently. The failure only surfaced later, less clearly, inside buildDelta's first getPublicParameters call against the zero address. Also fixed a test bug this introduced: TestNewSetupUpdaterValidatesItsInput's base() config left TokenState zero by default, so without giving it a real address first, every other case in that table (no client, no submitter, ...) would have started failing on the new check before ever reaching the field it was actually named for. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
SetupUpdaterConfig.Threshold, in this same package, documents zero as 'use every endorser' and NewSetupUpdater implements exactly that. DeploySpec.Threshold carried no such note, and the one concrete Backend, ForgeBackend.Deploy (integration/nwo/token/evm/deploy.go), treats zero as a hard validation error instead. Both config types describe the same authority per this package's own doc.go framing, so a reader forming an expectation from one about the other would be wrong. No behavior change; documents what Deploy already enforces and names why deploy-time and update-time thresholds differ on purpose. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Mirrors the plan.md precedent: a working file for the round-3 hunt that should never land in a PR diff. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…shed An endorser's reply carries the StateDelta it built, and the initiator JSON-decodes and EIP-712-hashes it before it has any reason to trust the endorser's signature over it: bind() only rejects a mismatched delta after eip712.Digest has already hashed every field. StateDelta.Validate had no upper bound on the number of outputs, spent refs or metadata entries, or on any variable-length field, so a single dishonest registered endorser could force real CPU and memory cost per request with an oversized, self-consistent but wrong delta. Validate now rejects a delta whose entry counts or variable-length fields exceed generous fixed bounds before doing any per-element work, closing the gap for every caller of Validate, not just the endorsement path. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
NewSignerFromBytes and LoadKey both went through secp256k1.PrivKeyFromBytes, which silently reduces an out-of-range scalar modulo the curve order instead of erroring - the overflow flag ModNScalar.SetByteSlice returns was discarded. A key file whose bytes are at or above the order would load as a different, unrelated key with no error, catchable only via LoadKeyForAddress's separate address check, and not at all on the documented empty-expected skip path. Both now go through eip712.DecodePrivateKeyScalar, which checks the overflow flag itself before accepting the scalar. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
The wire and domain Log types had no field for eth_getLogs's removed flag, so a log the node reports as belonging to a block a reorg has undone was indistinguishable from a canonical one. TxHashByAnchor could report a transaction hash, or trip its own duplicate-commit guard, off a log that no longer reflects the chain. jsonLog/Log now carry Removed, decoded from the wire, and TxHashByAnchor drops removed logs before deciding whether the anchor was applied. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…earch TxHashByAnchor always searched up to "latest" regardless of the operator's configured reorg-safety tag, unlike every other reader in the module (contractReader, ChainProvider, VersionKeeper all take and use one). An operator who set finalized/safe everywhere else still got an unpinned, head-relative log search here, with no way to configure otherwise. LogFilter gains ToBlockTag, sent as the upper bound instead of the numeric ToBlock when set; finality.Manager now takes and threads through the same blockTag its network.go caller already reads off Config.Finality for the reader and the version keeper. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…gging it registerEndorser swallowed every failure past the second-network and empty-allowlist checks (an unusable signing key, allowlist resolution, authorizer or responder construction, view-registry registration) as a log line. installEndorsement and Driver.New always returned success regardless, so a node explicitly configured with endorser.enabled came up looking healthy while never answering an endorsement request - discoverable only once a quorum it was needed for timed out, with nothing connecting the timeout back to the startup log. registerEndorser now returns an error that installEndorsement and New propagate, the same contract a broken submitter key already gets in newSubmitter. registeredFor is still only set once every step succeeds, so this does not change the existing retry-on-later-call or refuse-a-second-network behavior. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Deferred finding LFDT-Panurus#3 (configNetworkResolver.ConfigFor picks one Config for the first TMS to declare an EVM network/channel; Provider memoizes one *Network per (network, channel) with the namespace left out of the key) was confirmed by reading the code, not by running it. This drives the actual Driver.New/Network.Connect/QueryTokens/Broadcast entry points with two TMS on one network, each with its own TokenState address, and observes live that TMS B's reads, its submitter, and its EIP-712 signing domain all silently target TMS A's TokenState. Kept as the regression test for whenever Config gets split into network-shared and per-TMS parts: it currently asserts the contamination and should flip to asserting isolation once that lands. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
LFDT-Panurus#2180 enabled containedctx and fixed the ttx occurrence. The evm module is a separate Go module and was not linted in that pass, so make lint has been failing on it since. Neither field can be dropped. Ledger.ctx exists because driver.GetStateFnc passes no context to GetState, and fakeContext.ctx exists because it implements view.Context, whose Context() method has to return one. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Making registerEndorser return an error (the earlier commit in this PR, "propagate a broken endorser registration instead of only logging it") turned every unchecked call to it in driver_test.go into an errcheck violation. golangci-lint's max-same-issues default (3) only surfaced the first three in CI, hiding the rest until those were fixed - checked every call site in the file, not just the reported ones, to avoid a second round trip. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
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.
Follow-up to #2231. That one came from an unstructured first pass. This one is five separate, targeted adversarial reviews, one per trust-critical area, plus two more once the first five turned up clean enough to ask what was still unreviewed. Same process: verify every finding against the real code before touching anything, one commit per fix.
Builds on evm-bugfix-hunt-findings (#2231), so this stacks on that branch until it merges.
Crypto and wire format (crypto/, eip712/, keys/, statedelta/):
JSON-RPC client and ABI:
Endorsement trust boundary (endorsement/):
Solidity contracts (contracts/src/):
Test suite quality:
nwo/ (never independently reviewed before):
statedelta/ (previously treated as an already-correct precondition, first real review here):
Round 3: every directory had already had a dedicated pass by round 2, so this time five subagents split by bug class instead (nonce handling, JSON-RPC/fee/finality robustness, crypto and signing correctness, Solidity contract risk, driver orchestration), plus a fresh look at the endorsement trust boundary since endorsers now build their own delta (#2229) instead of the initiator.
One more finding needs the same fix Ledger.GetState from round 2 still needs (resolve a block tag to a concrete block number, not currently possible with EVMClient): a multi-token query can straddle a finalization boundary the same way, reading a mix of pre- and post-finalization state across a batch. Not fixed here, deferred alongside Ledger.GetState. (A third candidate in this class, ChainProvider.PublicParams reading bytes and version as two unsynchronized calls, was already independently fixed earlier in this same PR by the torn-read retry commit, confirmed before writing this up.)
Also, the per-TMS allowlist/threshold sharing bug from round 2 turns out to be bigger than scoped there: two TMS on one network/channel share the whole Config, including Contracts.TokenState, not just the endorsement policy, because Network is memoized per (network, channel) with the namespace left out of the key. Still not fixed here, needs splitting Config into network-shared and per-TMS parts.
Round 4: rounds 1-3 were static review. This one is adversarial and PoC-driven instead: for each trust boundary already reviewed, actually try to break it with a constructed exploit or a live reproduction, not just re-read the code. Four targets, one subagent each.
No new bugs. Every boundary held under an actual attempt to break it.