Skip to content

fix(auditor): validate input tokens against actions and check all recipients#1550

Open
Storm1289 wants to merge 7 commits into
hyperledger-labs:mainfrom
Storm1289:fix/auditor-check-inputs-and-recipients
Open

fix(auditor): validate input tokens against actions and check all recipients#1550
Storm1289 wants to merge 7 commits into
hyperledger-labs:mainfrom
Storm1289:fix/auditor-check-inputs-and-recipients

Conversation

@Storm1289
Copy link
Copy Markdown
Contributor

@Storm1289 Storm1289 commented Apr 13, 2026

Summary

  • dlog v1 finalization: auditor, input tokens validation (dlog v1 finalization: auditor, input tokens should be checked against the actions #998)
    GetAuditInfoForTransfers now verifies every input token's Pedersen commitment and owner bytes match the transfer action's embedded inputs, preventing forged input references from passing audit.

  • dlog v1 finalization: auditor, check recipients (dlog v1 finalization: auditor, check recipients #1000)
    For each non-redeemed output, every entry in Receivers is now validated via InspectIdentity, ensuring all declared recipients are checked individually, not just the first one.

  • Moved structural validation (Match, MatchInputs, ValidateReceivers) from token/metadata.go into the token/driver package so driver structs own their consistency checks.

  • Added Match, MatchInputs, and ValidateReceivers to driver.IssueMetadata, driver.TransferMetadata, and driver.TransferOutputMetadata in token/driver/match.go.

  • Updated token/metadata.go wrappers to perform nil/Validate() guards and delegate to driver-level methods.

  • Simplified GetAuditInfoForTransfers in auditor.go:

    • Replaced zkatdlog-specific G1 comparison with MatchInputs
    • Replaced output count check with Match
    • Replaced receiver nil/empty guard with ValidateReceivers

Changes

  • token/core/zkatdlog/nogh/v1/audit/auditor.go

    • Added ctx parameter
    • Added input-token commitment and owner validation
    • Added per-receiver InspectIdentity loop for non-redeemed outputs
    • Replaced inline logic with MatchInputs, Match, and ValidateReceivers
    • Removed TODO comments
  • token/core/zkatdlog/nogh/v1/audit/auditor_test.go

    • Updated calls for new signature
    • Fixed 4 tests using dummy tokens
    • Added 4 targeted tests
    • Updated error message assertions
  • token/driver/match.go

    • New file introducing Match, MatchInputs, and ValidateReceivers
  • token/driver/match_test.go

    • New file with table-driven unit tests for all four methods
  • token/metadata.go

    • Simplified wrappers delegating to driver-level validation methods

Test Plan

  • go test ./token/core/zkatdlog/nogh/v1/audit/... -v → all 28 tests pass
  • go test ./token/driver/... passes
  • go test ./token/... passes
  • make lint-auto-fix no lint issues
  • make checks all pre-CI checks pass

Added Test Coverage

  • GetAuditInfoForTransfers_input_token_commitment_mismatch → detects tampered commitment
  • GetAuditInfoForTransfers_input_token_owner_mismatch → detects tampered owner
  • GetAuditInfoForTransfers_no_receivers_for_output → detects missing receivers
  • GetAuditInfoForTransfers_receiver_audit_info_mismatch → detects invalid receiver audit info

Closes

#998, #1000

@adecaro
Copy link
Copy Markdown
Contributor

adecaro commented Apr 17, 2026

Hi @Storm1289 , thank you for submitting this but the approach needs to be different.

Metadata structs in token/metadata.go already support matching them against an action.
I think we need to move these implementation to the driver level. This way, also the auditor will be able to leverage them. Would you like to move the PR in this direction?

Please, for the future, before addressing an Issue, ask to be assigned.
We will put soon rules in place to better coordinate the activities.

Thanks much for your effort 🙏

@Storm1289
Copy link
Copy Markdown
Contributor Author

Hi @adecaro ,thank you for the feedback I understand the concern. Currently, our implementation in auditor.go deserializes directly to the zkatdlog-specific *transfer.Action type and accesses internal fields like actionInput.Token.Data (a G1 Pedersen commitment) to validate inputs. This is driver-specific logic that should not live in the auditor.

After looking at token/metadata.go, I can see that TransferMetadata.Match() already validates inputs/outputs count, extra signers, and issuer at the driver-interface level. I think the right direction is:

  1. Extend TransferMetadata.Match() to also validate input tokens using driver.TransferAction.GetSerializedInputs() to get the action's serialized inputs and comparing them against the serialized form of the provided input tokens. This keeps the check driver-agnostic.
  2. Move the receivers check (dlog v1 finalization: auditor, check recipients #1000) similarly to the metadata/driver level so the auditor can call a single Match() instead of doing field-level comparisons.

Could you confirm this is the direction you have in mind? Also, should the receiver validation be part of Match() itself, or a separate method on TransferMetadata?

Sorry, I should have asked to be assigned before opening this PR. I'll make sure to follow that process going forward.

@adecaro
Copy link
Copy Markdown
Contributor

adecaro commented Apr 17, 2026

Hi @adecaro ,thank you for the feedback I understand the concern. Currently, our implementation in auditor.go deserializes directly to the zkatdlog-specific *transfer.Action type and accesses internal fields like actionInput.Token.Data (a G1 Pedersen commitment) to validate inputs. This is driver-specific logic that should not live in the auditor.

Indeed, then we need to call the Validate function of the action.

After looking at token/metadata.go, I can see that TransferMetadata.Match() already validates inputs/outputs count, extra signers, and issuer at the driver-interface level. I think the right direction is:

  1. Extend TransferMetadata.Match() to also validate input tokens using driver.TransferAction.GetSerializedInputs() to get the action's serialized inputs and comparing them against the serialized form of the provided input tokens. This keeps the check driver-agnostic.
  2. Move the receivers check (dlog v1 finalization: auditor, check recipients #1000) similarly to the metadata/driver level so the auditor can call a single Match() instead of doing field-level comparisons.

Could you confirm this is the direction you have in mind? Also, should the receiver validation be part of Match() itself, or a separate method on TransferMetadata?

So, I would move the Match functions in token/metadata.go to the token/driver package. Given that we are talking about driver structs/interfaces, it should be the driver providing the basic checks.
Once this is done, the auditor can leverage these functions as well.
Does it make sense? 😅

Sorry, I should have asked to be assigned before opening this PR. I'll make sure to follow that process going forward.

No worries, we will soon set the rules more clearly from everyone 😄

@Storm1289 Storm1289 force-pushed the fix/auditor-check-inputs-and-recipients branch from 98c5f6f to 4d5a7fe Compare April 17, 2026 16:24
@Storm1289
Copy link
Copy Markdown
Contributor Author

Hi @adecaro, I’ve made the changes and updated the description. Kindly review.

@adecaro
Copy link
Copy Markdown
Contributor

adecaro commented Apr 22, 2026

Hi @Storm1289 , fantastic. I think the code is more robust this way. Please, add the unit-tests the match functions you moved under driver. I'll review the rest ASAP 🙏

@adecaro adecaro self-requested a review April 22, 2026 14:39
@adecaro adecaro self-assigned this Apr 22, 2026
@adecaro adecaro added this to the Q2/26 milestone Apr 22, 2026
@Storm1289
Copy link
Copy Markdown
Contributor Author

Storm1289 commented Apr 22, 2026

Hi @adecaro , thanks I’ve added table-driven unit tests in token/driver/match_test.go covering all four match functions, including success cases and edge cases (count mismatches, nil entries, byte mismatches, and receiver validation). Using existing counterfeiter mocks and I have updated the pr

@adecaro adecaro force-pushed the fix/auditor-check-inputs-and-recipients branch 2 times, most recently from b09c863 to 5676139 Compare April 23, 2026 12:43
@Storm1289
Copy link
Copy Markdown
Contributor Author

Hi @adecaro , the failing CI jobs (dlog-fabric-t8, fabtoken-fabric-t2, interop-dlog-t4) seem unrelated to this PR. They panic during integration test setup (BeforeEach) in fabric-smart-client (artifact generation / Postgres startup), before any test logic runs. None of the modified code paths are reached, and 0 specs execute. This looks like CI infrastructure flakiness. Could you please re-run the jobs? Happy to investigate further if the issue persists.

Storm1289 added 7 commits May 11, 2026 19:04
…ipients

Closes hyperledger-labs#998: GetAuditInfoForTransfers now verifies that every input token's
Pedersen commitment and owner bytes match the corresponding entry in the
deserialized transfer action, preventing forged or substituted input token
references from passing the audit.

Closes hyperledger-labs#1000: for each non-redeemed transfer output, every entry in the
Receivers slice is now inspected via InspectIdentity, ensuring all declared
recipients are validated in isolation and not just the first one.

Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Use receiver.Identity when set, fall back to the output owner for
single-recipient outputs. Simplify verbose block comments to single-line.

Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
… conversion

Remove hyperledger-labs#998 and hyperledger-labs#1000 inline references from auditor_test.go comments.
Remove unnecessary driver.Identity() conversion flagged by golangci-lint unconvert.

Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
…s/ValidateReceivers

Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
Add table-driven unit tests for IssueMetadata.Match,
TransferMetadata.Match, TransferMetadata.MatchInputs, and
TransferOutputMetadata.ValidateReceivers, covering success paths
and all error branches.

Signed-off-by: Storm1289 <divakarsharm2934@gmail.com>
@adecaro adecaro force-pushed the fix/auditor-check-inputs-and-recipients branch from a135aa7 to 05b7439 Compare May 11, 2026 17:04
@adecaro
Copy link
Copy Markdown
Contributor

adecaro commented May 11, 2026

Hi @Storm1289 , sorry for this very late reply. Apologies. I have restarted the CI and I'll review this again ASAP.
Thanks for the understanding 🙏

@Storm1289
Copy link
Copy Markdown
Contributor Author

Hi @adecaro, thanks for the review.😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants