fix: audit all require_auth() call sites vs. require_auth_for_args - #160
Open
blessedDev-xyz wants to merge 1 commit into
Open
fix: audit all require_auth() call sites vs. require_auth_for_args#160blessedDev-xyz wants to merge 1 commit into
blessedDev-xyz wants to merge 1 commit into
Conversation
…s require_auth_for_args Reviewed all 12 require_auth() call sites across initialize, set_fee_recipient, transfer_admin, register_solver, deregister_solver, withdraw_bond, submit_intent, accept_intent, fill_intent, cancel_intent, and require_admin helper. Conclusion: plain require_auth() is correct in all current cases. Every call is made on the address that IS the signing principal (no wrapper-contract delegation pattern exists today). Added inline audit comments at each call site documenting: - why require_auth() is sufficient for the current usage - where require_auth_for_args would reduce risk if delegated invocation is introduced in future (accept_intent and fill_intent are the strongest candidates) No call sites were changed — the audit found no security regression. Closes stellar-vortex-protocol#45
|
@blessedDev-xyz Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
2 participants
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
Audited all 12
require_auth()call sites inintent_settlement/src/lib.rsagainst Soroban SDK docs onrequire_authvsrequire_auth_for_args.Findings
Conclusion:
require_auth()is correct at every current call site. No call sites were changed.The contract never uses a delegated-invocation pattern — every
require_auth()is called on the address that is itself the signing principal (the tx signer).require_auth_for_argsis the right tool when an intermediary contract invokes on behalf of a user with scoped argument constraints; that pattern does not exist here.Call sites reviewed
initializeadminset_fee_recipientadmintransfer_admin(current)admintransfer_admin(new)new_adminregister_solversolverderegister_solversolverwithdraw_bondsolversubmit_intentuserrequire_auth_for_argscandidate if composable submission addedaccept_intentsolverfill_intentsolvercancel_intentuserrequire_adminhelperadminChanges
Inline audit comments added at each of the 12 call sites in
lib.rs. No logic changes.Validation
cargo fmt,cargo clippy -- -D warnings,cargo test— 55 tests pass, 0 fail.Closes #45