Skip to content

fix(nssa): audit 91 issue fix#489

Merged
jonesmarvin8 merged 3 commits into
mainfrom
marvin/audit-issue-91
May 21, 2026
Merged

fix(nssa): audit 91 issue fix#489
jonesmarvin8 merged 3 commits into
mainfrom
marvin/audit-issue-91

Conversation

@jonesmarvin8
Copy link
Copy Markdown
Collaborator

@jonesmarvin8 jonesmarvin8 commented May 19, 2026

🎯 Purpose

The purpose of this PR is to address the security vulnerability highlighted in audit-issue 91. Specifically, a pair of malicious programs can be used to drain a victim's account; P1 injects a victim's account (and authorization) in a chained-call, P2 uses victim's account (with authorization) to call auth-transfer with victim's account as the sender. The fix ensures that a chained call can only propagate authorization that the caller's own program_output.pre_states already carry, never elevate beyond what the caller was authorized for.

⚙️ Approach

[X] - Updates authorized_accounts (in validate_state_diff.rs, line 268) to to get pre_states from program_output (validated by circuit logic) rather than unvalidated chained_call.
[X] - Add pair of test-program-methods: malicious_injector and malicious_launderer.
[X] - Add test to validate_state_diff using the malicious programs demonstrating the vulnerability has been patched.

🧪 How to Test

Run tests malicious_programs_cannot_drain_victim_without_signature and privacy_malicious_programs_cannot_drain_public_victim

  • The test passes provided the victim's balance is unaffected by the execution.

🔗 Dependencies

N/A

🔜 Future Work

N/A

📋 PR Completion Checklist

  • Complete PR description
  • Implement the core functionality
  • Add/update tests
  • Add/update documentation and inline comments

Copy link
Copy Markdown
Collaborator

@schouhy schouhy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks very cool. Are private txs safe? Can we add a test that checks there's no vulnerability in a private transaction too?

@jonesmarvin8
Copy link
Copy Markdown
Collaborator Author

thanks very cool. Are private txs safe? Can we add a test that checks there's no vulnerability in a private transaction too?

The privacy txs are safe from this attack against public accounts. The individual inner proofs for each chained call proofs are generated, but the outer proof is rejected by the validator. I've added a similar test for privacy transaction using the malicious programms.

@jonesmarvin8 jonesmarvin8 requested a review from schouhy May 19, 2026 21:37
Copy link
Copy Markdown
Collaborator

@schouhy schouhy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

Comment thread nssa/src/validated_state_diff.rs Outdated
Copy link
Copy Markdown
Collaborator

@Pravdyvy Pravdyvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Copy Markdown
Collaborator

@moudyellaz moudyellaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, left some comments. Thanks for the fix again

Comment thread nssa/src/validated_state_diff.rs
Comment thread nssa/src/validated_state_diff.rs Outdated
Comment thread nssa/src/validated_state_diff.rs
@moudyellaz
Copy link
Copy Markdown
Collaborator

One tiny writeup ask, totally optional. Could you add a sentence to the PR description or commit body spelling out the invariant the fix locks in? Something like "a chained call can only propagate authorization that the caller's own program_output.pre_states already carry, never elevate beyond what the caller was authorized for." It would save the next person who looks at this from having to re-derive the property from the diff. Really nice fix overall, thanks for taking this on.

Comment thread nssa/src/validated_state_diff.rs
@jonesmarvin8 jonesmarvin8 merged commit 694e484 into main May 21, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants