docs(security): add AUDIT_SCOPE.md attack surface catalogue, link from SECURITY.md#1221
Merged
Merged
Conversation
|
@V1ctor-o 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
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
Adds
docs/reference/AUDIT_SCOPE.md— an attack surface catalogue and audit-prep document for a third-party security review of the VaultDAO contract. Updatesdocs/reference/SECURITY.mdto link to it.What's in the document (6,344 words)
Attack surface catalogue (§1)
Six attack surfaces, each with affected functions, existing mitigations, what's not mitigated, and a residual risk rating:
proposal.statusstaysApproved(notExecuted) during the external token-transfer callrole_satisfies, givingDisputeArbitratorinconsistent privileges depending on which function you calltimelock_thresholdat all, andset_signer_tiernever validates a tier limit against itrefund_spending_limitscredits the current day/week bucket, not the bucket the original spend was debited from, causing accounting drift across bucket boundariesi128multiplications on user-influenced values (stream.rate,payment.amount,lock.amount) use raw arithmetic, andCargo.tomlhas no[profile.release] overflow-checks = truepropose_upgradediscards the actual proposed WASM hash and stores a placeholder string;execute_upgradedeploys a hardcoded[0u8; 32]hash instead. The entire unanimous-approval, timelocked upgrade governance flow currently has no causal connection to what code actually gets deployed§1.6 was not in the issue's original list — it was found during this review and included because a security document shouldn't omit a finding of that severity once discovered.
Invariants (§2)
9 formal properties (INV-1 through INV-9), each stated as a condition rather than prose, with the enforcing function(s) and cross-referenced test name(s) — verified individually against current source (file, function name, and line number checked for each citation). Three are explicitly marked as violated or only partially holding, with the violation explained:
Out of scope (§3)
Frontend/backend, the Soroban host/protocol itself, third-party token contracts, off-chain indexing, network-level DoS, key-compromise/social engineering, and economic/incentive-design review — each with a one-line reason for exclusion.
Prior issues and resolutions (§4)
A table of every structural defect found across this and prior PRs in this repo (the 3
lib.rsbrace/corruption bugs, the duplicatestaking_configfield, the duplicatePermissionNotFoundenum variant, the ~23-siteInitConfigduplication intest.rs), their current status (open/fixed, and on which branch), and how each relates to the findings in §1.Test coverage cross-reference (§5)
Maps each §1 finding to the test file(s) that exercise it, and is explicit about coverage gaps — most notably, no test exists that asserts
execute_upgradedeploys the WASM hash that was actually proposed (§1.6), and no test specifically probes the timelock/unilateral-tier interaction (§1.3).Also included
docs/reference/SECURITY.md: added a link toAUDIT_SCOPE.mdin the "Audits" section.Important caveat — please read before reviewing
This document states up front (and repeats in §4/§5) that
contracts/vaultdoes not currently compile cleanly onmain— due to the pre-existing duplicate-declaration bugs listed in §4, none of which are fixed by this PR. That means:cargo testto a green state.update_current_contract_wasm([0u8; 32])does in practice.This is disclosed explicitly in the document itself so reviewers and future auditors don't mistake "documented" for "independently executed and confirmed."
Verification
cargo check/cargo test(expected to still fail due to pre-existing, unrelatedtest.rsissues — see §4)Notes for reviewers
§1.6 is the most severe finding in the document and is flagged as the top priority in the Summary of Findings table at the top of the file. Recommend treating it as its own follow-up issue regardless of how this PR is reviewed.
Closes #1185