audit: security fixes and fuzzing infrastructure for vc-vault v1 acta#4
Open
audit: security fixes and fuzzing infrastructure for vc-vault v1 acta#4
Conversation
- VCStatus key changed from VCStatus(String) to VCStatus(Address, String) so status entries are scoped per vault owner, preventing cross-vault collision where any issuer could overwrite another vault's revocation status - VCOwner key removed; revoke now takes explicit owner parameter - FeeCustom(Address) moved from instance to persistent storage to avoid exhausting the instance storage budget with per-issuer entries - SponsoredVaultSponsors Vec replaced with individual persistent entries keyed by SponsoredVaultSponsor(Address); authorization is now O(1) - PendingAdmin key added for two-step admin transfer - NoPendingAdmin and VCAlreadyExists error codes added - Debug derive added to VCStatus to support assert_eq! in tests
Emits on-chain events using the #[contractevent] macro for: VaultCreated, SponsoredVaultCreated, VaultRevoked, IssuerAuthorized, IssuerRevoked, VCIssued, VCRevoked Previously no events were emitted, making it impossible for indexers and off-chain tools to observe state changes without polling storage. pub mod visibility added to contract and model for external access.
- authorize_issuer now removes the issuer from VaultDeniedIssuers when re-authorizing, keeping the authorized and denied lists consistent - authorize_issuers deduplicates the input list before writing to prevent a revoke_issuer call from leaving ghost authorized entries - revoke_issuer rewritten to filter all occurrences in a single pass instead of removing only the first match by index
- create_vault: remove bootstrap side-effect that allowed the first caller to claim contract admin by calling create_vault before initialize - verify_vc: return typed VCStatus instead of untyped Map<String, String>; issuance_status_to_map helper removed - issue: add VCAlreadyExists check to block re-issuance of an existing vc_id; also blocks re-issuance of a vc_id that was pushed to another vault by checking VCStatus in addition to vault payload existence - revoke: check vault payload exists (not just status) to block revoking a vc that was pushed away; also blocks double-revocation - push: require VCStatus == Valid before moving a credential, blocking transfer of revoked credentials - nominate_admin + accept_contract_admin replace set_contract_admin with a two-step transfer requiring the nominee to sign acceptance - remove set_vault_did (DID immutability is intentional design) - remove redundant validate_vault_initialized calls from push - initialize: remove unused default_issuer_did parameter and storage key - migrate: fix read_legacy_issuance_revocations to return empty map instead of panicking when no revocations exist in legacy storage
- setup_no_mock helper added for tests that require explicit auth mocking - Five targeted auth tests added using mock_auths() to verify require_auth() guards on initialize, nominate_admin, create_vault, authorize_issuer and issue - test_nominate_and_accept_admin replaces test_set_contract_admin - All verify_vc assertions updated to use typed VCStatus comparisons - Three regression tests added for post-fuzz findings: test_issue_after_push_same_vc_id_panics (A-22) test_revoke_after_push_panics (A-23) test_push_revoked_vc_panics (A-24)
build.sh: - set -eu added so any failure exits immediately - switched to cargo rustc -- --crate-type cdylib to produce the WASM artifact without declaring cdylib in Cargo.toml (required for fuzzing compatibility on macOS) release.sh: - set -eu added - stellar config network add and stellar keys generate wrapped in idempotency checks instead of silently swallowing errors with || true storage/mod.rs: - TTL constants reduced from 31_536_000 to values within all known network limits (THRESHOLD: 518_400, EXTEND_TO: 3_110_400 ledgers) to prevent deterministic trap on networks with lower max_entry_ttl
Removes cdylib from crate-type. Having both cdylib and rlib caused cargo to build a native dylib during non-WASM builds (tests, fuzzing) which failed to link sancov symbols on macOS aarch64. The WASM artifact is now produced explicitly by build.sh via cargo rustc -- --crate-type cdylib.
fuzz/corpus/ grows unboundedly and is rebuilt automatically by the fuzzer. fuzz/artifacts/ contains crash inputs for already-fixed bugs and has no value in git history once the corresponding fix is applied.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks vc-vault: replaces default-issuer with admin nomination/accept flow, scopes VC status to owner, adds sponsored-vault and fee APIs, introduces events and new errors, exposes modules, updates tests, adds fuzz targets, bumps workspace deps, changes crate type and build/release scripts, and updates .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Contract
participant Storage
participant Events
rect rgba(100,150,200,0.5)
note over Caller,Contract: Admin nomination lifecycle
Caller->>Contract: initialize(e, contract_admin)
Contract->>Storage: write_contract_admin(contract_admin)
Caller->>Contract: nominate_admin(e, new_admin)
Contract->>Storage: write_pending_admin(new_admin)
Caller->>Contract: accept_contract_admin(e) (signed by new_admin)
Contract->>Storage: read_pending_admin()
Contract->>Storage: write_contract_admin(new_admin)
Contract->>Storage: remove_pending_admin()
end
rect rgba(150,200,100,0.5)
note over Caller,Contract: Issue / Revoke (owner-scoped)
Caller->>Contract: issue(e, owner, vc_id, issuer_ref)
Contract->>Storage: read_vc_status(owner, vc_id)
alt VC missing
Contract->>Storage: write_vc_status(owner, vc_id, Valid)
Contract->>Events: vc_issued(owner, vc_id, issuer_ref)
else VC exists
Contract-->>Caller: VCAlreadyExists error
end
Caller->>Contract: revoke(e, owner, vc_id, date)
Contract->>Storage: read_vc_status(owner, vc_id)
Contract->>Storage: write_vc_status(owner, vc_id, Revoked(date))
Contract->>Events: vc_revoked(owner, vc_id, date)
end
sequenceDiagram
participant Sponsor
participant ContractAdmin
participant Contract
participant Storage
rect rgba(200,150,100,0.5)
note over Sponsor,Contract: Sponsored vault creation
ContractAdmin->>Contract: set_sponsored_vault_open_to_all(e, true)
Contract->>Storage: write_sponsored_vault_open_to_all(true)
Sponsor->>Contract: create_sponsored_vault(e, sponsor, owner, did_uri)
Contract->>Storage: is_authorized_sponsor(sponsor)
alt authorized or open_to_all
Contract->>Storage: write_vault_did(owner, did_uri)
Contract->>Storage: write_vault_admin(owner, sponsor)
Contract->>Events: sponsored_vault_created(sponsor, owner, did_uri)
else not authorized
Contract-->>Sponsor: NotAuthorizedSponsor error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![almanax-ai[bot]](https://avatars.githubusercontent.com/in/934833?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/vc-vault/src/contract.rs (1)
258-273:⚠️ Potential issue | 🔴 CriticalMove
VCStatuswhen pushing a VC between vaults.At Line [260], status is validated using owner-scoped keys, but Line [265]-Line [269] only move payload and ID. The destination status remains
Invalid, soverify_vc/revokebreak after push, and stale source status can block re-issuance for the source owner.🔧 Proposed fix
- if storage::read_vc_status(&e, &from_owner, &vc_id) != VCStatus::Valid { + let status = storage::read_vc_status(&e, &from_owner, &vc_id); + if status != VCStatus::Valid { panic_with_error!(e, ContractError::VCNotFound); } let vc = vc_opt.unwrap(); @@ storage::remove_vault_vc(&e, &from_owner, &vc_id); storage::remove_vault_vc_id(&e, &from_owner, &vc_id); storage::write_vault_vc(&e, &to_owner, &vc_id, &vc); storage::append_vault_vc_id(&e, &to_owner, &vc_id); + storage::write_vc_status(&e, &to_owner, &vc_id, &status); + storage::write_vc_status(&e, &from_owner, &vc_id, &VCStatus::Invalid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/contract.rs` around lines 258 - 273, The push logic only moves the VC payload and id but leaves the owner-scoped VCStatus behind; read the status with storage::read_vc_status(&e, &from_owner, &vc_id) into a variable, then remove the source status (e.g., storage::remove_vault_vc_status(&e, &from_owner, &vc_id)) and write the same status for the destination (e.g., storage::write_vault_vc_status(&e, &to_owner, &vc_id, &status)) before extending TTLs; if the remove/write status helpers don't exist add them to storage to mirror remove_vault_vc / write_vault_vc behavior so verify_vc and revoke work correctly after a push.
🧹 Nitpick comments (1)
contracts/vc-vault/src/issuance/mod.rs (1)
11-14: Error message may be misleading for non-existent VCs.When
vc_statusisVCStatus::Invalid(VC doesn't exist), the function panics withVCAlreadyRevoked, which is misleading. Consider distinguishing between "VC not found" and "VC already revoked" scenarios.♻️ Proposed fix to use appropriate error codes
pub fn revoke_vc(e: &Env, owner: &Address, vc_id: String, date: String) { let vc_status = storage::read_vc_status(e, owner, &vc_id); - if vc_status != VCStatus::Valid { - panic_with_error!(e, ContractError::VCAlreadyRevoked) + match vc_status { + VCStatus::Valid => {}, + VCStatus::Invalid => panic_with_error!(e, ContractError::VCNotFound), + VCStatus::Revoked(_) => panic_with_error!(e, ContractError::VCAlreadyRevoked), } storage::write_vc_status(e, owner, &vc_id, &VCStatus::Revoked(date)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/issuance/mod.rs` around lines 11 - 14, The current check treats any non-Valid status as VCAlreadyRevoked; update the logic after calling storage::read_vc_status(e, owner, &vc_id) to distinguish VCStatus::Invalid (non-existent) vs VCStatus::Revoked (actually revoked): if vc_status == VCStatus::Invalid return/panic_with_error!(e, ContractError::VCNotFound) (or add that error variant), else if vc_status == VCStatus::Revoked panic_with_error!(e, ContractError::VCAlreadyRevoked); leave the Valid branch unchanged; ensure you reference the VCStatus enum, read_vc_status function and panic_with_error! macro when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/vc-vault/src/contract.rs`:
- Around line 345-360: The create_sponsored_vault function currently only
authenticates the sponsor, allowing callers to create a vault for any owner when
open_to_all is true; add an owner.require_auth() call immediately after
sponsor.require_auth() in create_sponsored_vault (same placement used by
create_vault) so the owner must consent to vault/DID assignment and cannot be
targeted by a sponsor without their authorization.
In `@contracts/vc-vault/src/storage/mod.rs`:
- Around line 423-427: is_authorized_sponsor currently only reads existence of
DataKey::SponsoredVaultSponsor(sponsor.clone()) and does not refresh its TTL,
allowing sponsors to expire unexpectedly; change it so that when the key exists
you refresh its TTL (as done in other auth checks) by re-writing or explicitly
updating the persistent storage entry for
DataKey::SponsoredVaultSponsor(sponsor.clone()) with the same value to extend
the TTL (use the same TTL duration used elsewhere for sponsor keys) and then
return true; otherwise return false.
In `@contracts/vc-vault/src/vault/issuer.rs`:
- Around line 18-27: authorize_issuers replaces the issuer list but doesn't
clear any denied flags like authorize_issuer does; update authorize_issuers (the
function that builds deduped and calls storage::write_vault_issuers) to also
remove denied status for each issuer in the deduped list by calling the same
storage helper used by authorize_issuer (e.g., storage::remove_vault_denied(e,
owner, issuer) or the equivalent function) for each issuer before writing the
new issuers.
In `@scripts/release.sh`:
- Around line 5-8: The network existence check uses a substring grep ("grep -q
testnet") which can false-positive; change it to test for an exact alias match
before skipping creation—e.g., replace the grep check on the output of "stellar
config network ls" with an exact-match test (match the whole line or the alias
column) so that only an alias literally equal to "testnet" will cause the script
to skip running "stellar config network add testnet".
---
Outside diff comments:
In `@contracts/vc-vault/src/contract.rs`:
- Around line 258-273: The push logic only moves the VC payload and id but
leaves the owner-scoped VCStatus behind; read the status with
storage::read_vc_status(&e, &from_owner, &vc_id) into a variable, then remove
the source status (e.g., storage::remove_vault_vc_status(&e, &from_owner,
&vc_id)) and write the same status for the destination (e.g.,
storage::write_vault_vc_status(&e, &to_owner, &vc_id, &status)) before extending
TTLs; if the remove/write status helpers don't exist add them to storage to
mirror remove_vault_vc / write_vault_vc behavior so verify_vc and revoke work
correctly after a push.
---
Nitpick comments:
In `@contracts/vc-vault/src/issuance/mod.rs`:
- Around line 11-14: The current check treats any non-Valid status as
VCAlreadyRevoked; update the logic after calling storage::read_vc_status(e,
owner, &vc_id) to distinguish VCStatus::Invalid (non-existent) vs
VCStatus::Revoked (actually revoked): if vc_status == VCStatus::Invalid
return/panic_with_error!(e, ContractError::VCNotFound) (or add that error
variant), else if vc_status == VCStatus::Revoked panic_with_error!(e,
ContractError::VCAlreadyRevoked); leave the Valid branch unchanged; ensure you
reference the VCStatus enum, read_vc_status function and panic_with_error! macro
when making the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.gitignoreCargo.tomlcontracts/vc-vault/Cargo.tomlcontracts/vc-vault/src/api/mod.rscontracts/vc-vault/src/contract.rscontracts/vc-vault/src/error.rscontracts/vc-vault/src/events/mod.rscontracts/vc-vault/src/issuance/mod.rscontracts/vc-vault/src/lib.rscontracts/vc-vault/src/model/vc_status.rscontracts/vc-vault/src/storage/mod.rscontracts/vc-vault/src/test.rscontracts/vc-vault/src/vault/issuer.rsscripts/build.shscripts/release.sh
…ing and understanding each function of the audit
There was a problem hiding this comment.
♻️ Duplicate comments (2)
contracts/vc-vault/src/contract.rs (2)
250-264:⚠️ Potential issue | 🔴 CriticalVC status is not migrated to the destination vault during push.
After transferring the VC payload from
from_ownertoto_owner, the status entry remains keyed under(from_owner, vc_id). This means:
verify_vc(to_owner, vc_id)returnsInvalid(no status exists forto_owner)revoke(to_owner, vc_id, ...)fails withVCNotFoundThe recipient cannot verify or manage the pushed credential.
Proposed fix: migrate status when pushing
storage::remove_vault_vc(&e, &from_owner, &vc_id); storage::remove_vault_vc_id(&e, &from_owner, &vc_id); + // Migrate VC status to destination owner + let status = storage::read_vc_status(&e, &from_owner, &vc_id); + storage::write_vc_status(&e, &to_owner, &vc_id, &status); + storage::remove_vc_status(&e, &from_owner, &vc_id); storage::write_vault_vc(&e, &to_owner, &vc_id, &vc); storage::append_vault_vc_id(&e, &to_owner, &vc_id);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/contract.rs` around lines 250 - 264, The VC status isn't migrated when pushing a credential: after verifying status with storage::read_vc_status and moving the payload (vc via vc_opt) using storage::remove_vault_vc / storage::write_vault_vc and the id helpers, read the existing status for (from_owner, vc_id), write that status for (to_owner, vc_id) and remove the old status for (from_owner, vc_id) so verify_vc and revoke work for the recipient; update the same block (around read_vc_status, vc_opt, storage::write_vault_vc, storage::append_vault_vc_id, storage::extend_vc_ttl) to perform status migration and handle any potential missing-status cases consistently (e.g., preserve error behavior or map to ContractError::VCNotFound).
333-354:⚠️ Potential issue | 🔴 CriticalMissing
owner.require_auth()allows non-consensual vault creation.A sponsor can create a vault for any arbitrary
owneraddress without that owner's consent. This permanently assigns a DID to the owner's identity, and the owner cannot later callcreate_vaultdue toAlreadyInitialized. This enables identity squatting.Proposed fix
fn create_sponsored_vault(e: Env, sponsor: Address, owner: Address, did_uri: String) { sponsor.require_auth(); + owner.require_auth(); if !storage::has_contract_admin(&e) {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/contract.rs` around lines 333 - 354, In create_sponsored_vault add an explicit consent check by calling owner.require_auth() so the target owner must authorize creation; keep this call (owner.require_auth()) right after sponsor.require_auth() and before the existing storage::has_vault_admin check and any writes, so a sponsor cannot unilaterally assign a DID to an owner (this will prevent the AlreadyInitialized lockout described by storage::has_vault_admin and ContractError::AlreadyInitialized).
🧹 Nitpick comments (3)
contracts/vc-vault/src/contract.rs (2)
450-460: Consider emitting an event when auto-authorizing issuers.When
ensure_issuer_authorizedauto-authorizes an issuer (line 458), no event is emitted. In contrast, explicitauthorize_issuercalls emitissuer_authorized. This makes it harder to audit when issuers were implicitly added.Suggested improvement
if storage::is_issuer_denied(e, owner, issuer_addr) { panic_with_error!(e, ContractError::IssuerNotAuthorized) } vault::authorize_issuer(e, owner, issuer_addr); + events::issuer_authorized(e, owner, issuer_addr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/contract.rs` around lines 450 - 460, The ensure_issuer_authorized function currently auto-authorizes issuers via vault::authorize_issuer without emitting the issuer_authorized event; update ensure_issuer_authorized to emit the same issuer_authorized event immediately after calling vault::authorize_issuer so implicit authorizations are auditable, using the same event payload/fields as the explicit authorize_issuer flow (i.e., match the event name and payload structure used elsewhere) and keep the denial check (storage::is_issuer_denied) and panic behavior unchanged.
473-483: Clarify fee behavior whenfee_overrideis zero.When
fee_enabledis true butfee_override <= 0, no fee is transferred. If zero means "waive fee," this is fine. If the intent is to fall back to a standard fee amount, the logic needs adjustment.Consider adding a comment clarifying the expected behavior, or implement fallback to
read_fee_standardwhenfee_overrideis zero.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/contract.rs` around lines 473 - 483, The current branch checks storage::read_fee_enabled(e) but only transfers when fee_override > 0, leaving behavior ambiguous when fee_override == 0; update the logic in the block using storage::read_fee_token_contract, storage::read_fee_dest and fee_override so that when fee_override <= 0 you read a fallback via storage::read_fee_standard(e) and use that amount instead (only invoke e.invoke_contract::<()>(..., symbol_short!("transfer"), (issuer_addr.clone(), fee_dest, amount).into_val(e)) when the chosen amount > 0), or alternatively add a clear comment above the branch explaining that fee_override == 0 intentionally waives fees—pick one and implement consistently.contracts/vc-vault/src/issuance/mod.rs (1)
11-12: Error variantVCAlreadyRevokedis misleading for theInvalidcase.The condition
vc_status != VCStatus::Validcatches bothRevokedandInvalidstatuses, but the errorVCAlreadyRevokedonly describes one scenario. Consider either:
- Adding a separate check for
Invalidwith a distinct error (e.g.,VCNotFound), or- Renaming/documenting the error to clarify it covers "VC not in Valid state."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/issuance/mod.rs` around lines 11 - 12, The check using VCStatus (`if vc_status != VCStatus::Valid`) conflates Revoked and Invalid states while the error variant ContractError::VCAlreadyRevoked only describes one case; split the logic to discriminate statuses (check `vc_status == VCStatus::Revoked` and `vc_status == VCStatus::Invalid`) and panic with distinct errors (e.g., keep ContractError::VCAlreadyRevoked for Revoked and add ContractError::VCNotFound or ContractError::VCNotValid for Invalid), or alternatively rename/update the existing ContractError variant to something like VCNotValid and update the panic_with_error!(...) call accordingly so the error accurately reflects the VCStatus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@contracts/vc-vault/src/contract.rs`:
- Around line 250-264: The VC status isn't migrated when pushing a credential:
after verifying status with storage::read_vc_status and moving the payload (vc
via vc_opt) using storage::remove_vault_vc / storage::write_vault_vc and the id
helpers, read the existing status for (from_owner, vc_id), write that status for
(to_owner, vc_id) and remove the old status for (from_owner, vc_id) so verify_vc
and revoke work for the recipient; update the same block (around read_vc_status,
vc_opt, storage::write_vault_vc, storage::append_vault_vc_id,
storage::extend_vc_ttl) to perform status migration and handle any potential
missing-status cases consistently (e.g., preserve error behavior or map to
ContractError::VCNotFound).
- Around line 333-354: In create_sponsored_vault add an explicit consent check
by calling owner.require_auth() so the target owner must authorize creation;
keep this call (owner.require_auth()) right after sponsor.require_auth() and
before the existing storage::has_vault_admin check and any writes, so a sponsor
cannot unilaterally assign a DID to an owner (this will prevent the
AlreadyInitialized lockout described by storage::has_vault_admin and
ContractError::AlreadyInitialized).
---
Nitpick comments:
In `@contracts/vc-vault/src/contract.rs`:
- Around line 450-460: The ensure_issuer_authorized function currently
auto-authorizes issuers via vault::authorize_issuer without emitting the
issuer_authorized event; update ensure_issuer_authorized to emit the same
issuer_authorized event immediately after calling vault::authorize_issuer so
implicit authorizations are auditable, using the same event payload/fields as
the explicit authorize_issuer flow (i.e., match the event name and payload
structure used elsewhere) and keep the denial check (storage::is_issuer_denied)
and panic behavior unchanged.
- Around line 473-483: The current branch checks storage::read_fee_enabled(e)
but only transfers when fee_override > 0, leaving behavior ambiguous when
fee_override == 0; update the logic in the block using
storage::read_fee_token_contract, storage::read_fee_dest and fee_override so
that when fee_override <= 0 you read a fallback via
storage::read_fee_standard(e) and use that amount instead (only invoke
e.invoke_contract::<()>(..., symbol_short!("transfer"), (issuer_addr.clone(),
fee_dest, amount).into_val(e)) when the chosen amount > 0), or alternatively add
a clear comment above the branch explaining that fee_override == 0 intentionally
waives fees—pick one and implement consistently.
In `@contracts/vc-vault/src/issuance/mod.rs`:
- Around line 11-12: The check using VCStatus (`if vc_status !=
VCStatus::Valid`) conflates Revoked and Invalid states while the error variant
ContractError::VCAlreadyRevoked only describes one case; split the logic to
discriminate statuses (check `vc_status == VCStatus::Revoked` and `vc_status ==
VCStatus::Invalid`) and panic with distinct errors (e.g., keep
ContractError::VCAlreadyRevoked for Revoked and add ContractError::VCNotFound or
ContractError::VCNotValid for Invalid), or alternatively rename/update the
existing ContractError variant to something like VCNotValid and update the
panic_with_error!(...) call accordingly so the error accurately reflects the
VCStatus.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/vc-vault/src/contract.rscontracts/vc-vault/src/issuance/mod.rscontracts/vc-vault/src/vault/issuer.rs
push moved the VC payload but never wrote the VCStatus entry in the destination namespace. verify_vc(to_owner, vc_id) returned Invalid and revoke(to_owner, vc_id) panicked with VCNotFound for any pushed credential. Two regression tests added: test_verify_vc_valid_after_push_on_destination and test_revoke_after_push_on_destination_succeeds.
push unconditionally wrote VCStatus::Valid for the destination, which could overwrite an existing Revoked status — allowing a credential revocation to be silently undone by a third party with a colliding vc_id. Added the same precondition check already present in issue: destination must have no existing payload and no existing non-Invalid status before the push is allowed to proceed. One regression test added: test_push_to_destination_with_existing_vc_id_panics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/vc-vault/src/contract.rs (1)
456-466: Auto-authorization during issue does not emitissuer_authorizedevent.When
ensure_issuer_authorizedauto-authorizes an issuer (line 464), no event is emitted. This differs from the explicitauthorize_issuerentrypoint (line 178) which emitsissuer_authorized. This inconsistency may cause issues for off-chain indexers tracking issuer authorizations.Suggested fix
if !vault::is_authorized(&issuers, issuer_addr) { if storage::is_issuer_denied(e, owner, issuer_addr) { panic_with_error!(e, ContractError::IssuerNotAuthorized) } vault::authorize_issuer(e, owner, issuer_addr); + events::issuer_authorized(e, owner, issuer_addr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/contract.rs` around lines 456 - 466, ensure_issuer_authorized currently auto-authorizes an issuer via vault::authorize_issuer but does not emit the same issuer_authorized event that the public authorize_issuer entrypoint emits; after calling vault::authorize_issuer within ensure_issuer_authorized, invoke the same event emission used by the authorize_issuer entrypoint (the issuer_authorized event emitter) with the same parameters (owner and issuer_addr) so off-chain indexers see auto-authorizations consistently.contracts/vc-vault/src/test.rs (2)
561-570: Consider adding test for owner vault operations after sponsored creation.The sponsored vault tests verify creation authorization but don't test that the owner can subsequently perform vault operations (e.g.,
authorize_issuer,set_vault_admin). Adding such a test would confirm the vault is fully functional for the owner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/test.rs` around lines 561 - 570, Add a new test (e.g., test_owner_operations_after_sponsored_creation) that uses the existing setup(), calls client.initialize(&admin) and client.create_sponsored_vault(&admin, &owner, &did_uri) as in test_admin_creates_sponsored_vault, then acts as the owner to perform typical vault operations: call client.authorize_issuer(&owner, &some_issuer), verify the issuer appears in the authorized list (or other appropriate getter), call client.set_vault_admin(&owner, &new_admin) and assert admin change took effect, and finally ensure client.list_vc_ids(&owner) still behaves (e.g., returns empty Vec) to prove the owner can perform vault operations after sponsored creation; reference the existing functions create_sponsored_vault, authorize_issuer, set_vault_admin, and list_vc_ids on the client to locate the code.
542-550: Missing positive test case formigratefunction.Only the error path (no legacy vault) is tested. A positive test that performs migration and then calls
verify_vcon a migrated credential would help catch the issue where migrated VCs lack status entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/test.rs` around lines 542 - 550, Add a positive test that verifies successful migration: use setup(), client.initialize(&admin) and Address::generate to create an owner, create a legacy vault entry (using the same legacy creation path your migration expects), issue a credential under that legacy vault that includes a status entry, call client.migrate(&owner), then call client.verify_vc(&credential) and assert it returns valid/ok and that the migrated VC's status is present in the new vault storage; name the test something like test_migrate_with_legacy_vault_succeeds and use the same functions referenced in the diff (setup, client.create_vault/legacy creation, client.migrate, client.verify_vc) so the behavior and presence of status entries post-migration are asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/vc-vault/src/contract.rs`:
- Around line 398-409: The migrated VCs are stored via vault::store_vc but no
status entry is written, so read_vc_status falls back to VCStatus::Invalid;
after storing each vc in the migration loop (the block calling vault::store_vc
for each vc.id), write a corresponding status record (use the storage module's
status-writing API, e.g., storage::write_vc_status or the existing storage
function that persists VC status) to set the correct initial status (e.g.,
Valid/Issued) for vc.id and owner before calling
storage::remove_legacy_vault_vcs and storage::extend_vault_ttl so migrated VCs
are not treated as Invalid by verify_vc.
---
Nitpick comments:
In `@contracts/vc-vault/src/contract.rs`:
- Around line 456-466: ensure_issuer_authorized currently auto-authorizes an
issuer via vault::authorize_issuer but does not emit the same issuer_authorized
event that the public authorize_issuer entrypoint emits; after calling
vault::authorize_issuer within ensure_issuer_authorized, invoke the same event
emission used by the authorize_issuer entrypoint (the issuer_authorized event
emitter) with the same parameters (owner and issuer_addr) so off-chain indexers
see auto-authorizations consistently.
In `@contracts/vc-vault/src/test.rs`:
- Around line 561-570: Add a new test (e.g.,
test_owner_operations_after_sponsored_creation) that uses the existing setup(),
calls client.initialize(&admin) and client.create_sponsored_vault(&admin,
&owner, &did_uri) as in test_admin_creates_sponsored_vault, then acts as the
owner to perform typical vault operations: call client.authorize_issuer(&owner,
&some_issuer), verify the issuer appears in the authorized list (or other
appropriate getter), call client.set_vault_admin(&owner, &new_admin) and assert
admin change took effect, and finally ensure client.list_vc_ids(&owner) still
behaves (e.g., returns empty Vec) to prove the owner can perform vault
operations after sponsored creation; reference the existing functions
create_sponsored_vault, authorize_issuer, set_vault_admin, and list_vc_ids on
the client to locate the code.
- Around line 542-550: Add a positive test that verifies successful migration:
use setup(), client.initialize(&admin) and Address::generate to create an owner,
create a legacy vault entry (using the same legacy creation path your migration
expects), issue a credential under that legacy vault that includes a status
entry, call client.migrate(&owner), then call client.verify_vc(&credential) and
assert it returns valid/ok and that the migrated VC's status is present in the
new vault storage; name the test something like
test_migrate_with_legacy_vault_succeeds and use the same functions referenced in
the diff (setup, client.create_vault/legacy creation, client.migrate,
client.verify_vc) so the behavior and presence of status entries post-migration
are asserted.
Six fuzz targets covering the full contract surface: fuzz_issue, fuzz_revoke, fuzz_verify_vc, fuzz_push, fuzz_issuer_ops, and fuzz_lifecycle. fuzz_lifecycle discovered A-22, A-23, and A-24 during the audit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/vc-vault/fuzz/fuzz_targets/common.rs`:
- Around line 8-11: The current s function slices input by bytes which can panic
on a multi-byte UTF-8 boundary; update s to safely truncate to a valid UTF-8
boundary before calling SStr::from_str: compute the largest byte index <= 256
that is at a char boundary (e.g., use input.char_indices() to find the last
index < = 256, or fall back to whole input), create safe from that valid slice,
and pass it to SStr::from_str(env, safe); keep references to the same symbols
(s, safe, SStr::from_str, Env) so reviewers can find the change quickly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/vc-vault/fuzz/Cargo.tomlcontracts/vc-vault/fuzz/fuzz_targets/common.rscontracts/vc-vault/fuzz/fuzz_targets/fuzz_issue.rscontracts/vc-vault/fuzz/fuzz_targets/fuzz_issuer_ops.rscontracts/vc-vault/fuzz/fuzz_targets/fuzz_lifecycle.rscontracts/vc-vault/fuzz/fuzz_targets/fuzz_push.rscontracts/vc-vault/fuzz/fuzz_targets/fuzz_revoke.rscontracts/vc-vault/fuzz/fuzz_targets/fuzz_verify_vc.rs
Comment on lines
+8
to
+11
| pub fn s(env: &Env, input: &str) -> SStr { | ||
| let safe = &input[..input.len().min(256)]; | ||
| SStr::from_str(env, safe) | ||
| } |
There was a problem hiding this comment.
Potential panic on multi-byte UTF-8 boundary slice.
Slicing &input[..input.len().min(256)] at a byte index can panic if input contains multi-byte UTF-8 characters and the slice boundary falls mid-character. Fuzz inputs from arbitrary can produce arbitrary UTF-8 sequences, making this likely to trigger.
🐛 Proposed fix using char_indices for safe truncation
pub fn s(env: &Env, input: &str) -> SStr {
- let safe = &input[..input.len().min(256)];
+ let safe = if input.len() <= 256 {
+ input
+ } else {
+ // Find the last valid char boundary at or before byte 256
+ match input.char_indices().take_while(|(i, _)| *i < 256).last() {
+ Some((i, c)) => &input[..i + c.len_utf8()],
+ None => "",
+ }
+ };
SStr::from_str(env, safe)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn s(env: &Env, input: &str) -> SStr { | |
| let safe = &input[..input.len().min(256)]; | |
| SStr::from_str(env, safe) | |
| } | |
| pub fn s(env: &Env, input: &str) -> SStr { | |
| let safe = if input.len() <= 256 { | |
| input | |
| } else { | |
| // Find the last valid char boundary at or before byte 256 | |
| match input.char_indices().take_while(|(i, _)| *i < 256).last() { | |
| Some((i, c)) => &input[..i + c.len_utf8()], | |
| None => "", | |
| } | |
| }; | |
| SStr::from_str(env, safe) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/vc-vault/fuzz/fuzz_targets/common.rs` around lines 8 - 11, The
current s function slices input by bytes which can panic on a multi-byte UTF-8
boundary; update s to safely truncate to a valid UTF-8 boundary before calling
SStr::from_str: compute the largest byte index <= 256 that is at a char boundary
(e.g., use input.char_indices() to find the last index < = 256, or fall back to
whole input), create safe from that valid slice, and pass it to
SStr::from_str(env, safe); keep references to the same symbols (s, safe,
SStr::from_str, Env) so reviewers can find the change quickly.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/setup-fuzzing.md (2)
58-58: Clarify the relationship betweenset -euand stale artifacts.The current phrasing suggests that
set -euprevents deploying stale artifacts, butset -euonly ensures the script fails fast on errors. It won't prevent deployment of a stale optimized artifact if a previous build succeeded but subsequent source changes were made without rebuilding. Consider rephrasing to clarify thatset -euensures build failures are caught, and separately emphasize always runningbuild.shbefore deployment.✍️ Suggested rewording
-The build script uses `cargo rustc -- --crate-type cdylib` to force cdylib output for the WASM build without declaring it in `Cargo.toml`. This is required because declaring `cdylib` in `Cargo.toml` would cause cargo to build a native `.dylib` during fuzzing, which fails to link sancov symbols on macOS. The script runs `set -eu` so it will fail fast on any error. Never deploy a stale artifact. +The build script uses `cargo rustc -- --crate-type cdylib` to force cdylib output for the WASM build without declaring it in `Cargo.toml`. This is required because declaring `cdylib` in `Cargo.toml` would cause cargo to build a native `.dylib` during fuzzing, which fails to link sancov symbols on macOS. The script runs `set -eu` so it will fail fast on any error. Always run `build.sh` before deployment to ensure you deploy the latest code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/setup-fuzzing.md` at line 58, Update the paragraph so it no longer implies that `set -eu` prevents deploying stale artifacts: explicitly state that `set -eu` only makes the script fail fast on errors, and add a separate sentence advising to always run the build (e.g., run `build.sh`) before deployment to avoid shipping stale optimized artifacts; keep references to the existing phrasing about `cargo rustc -- --crate-type cdylib` intact and replace the original combined sentence with two clear sentences — one about `set -eu` behavior and one about the requirement to rebuild to avoid stale artifacts.
25-36: Consider adding a language specifier to the code block.Adding a language specifier (e.g.,
text) improves markdown rendering consistency across different viewers.📝 Proposed improvement
-``` +```text contracts/ vc-vault/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/setup-fuzzing.md` around lines 25 - 36, The fenced code block in docs/setup-fuzzing.md (the block showing the project tree starting with "contracts/" and "vc-vault/") lacks a language specifier; update the opening fence from ``` to ```text (or another appropriate language) so markdown renderers treat it as plain text and preserve formatting, e.g., change the triple-backtick that precedes the "contracts/" tree to ```text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/setup-fuzzing.md`:
- Line 58: Update the paragraph so it no longer implies that `set -eu` prevents
deploying stale artifacts: explicitly state that `set -eu` only makes the script
fail fast on errors, and add a separate sentence advising to always run the
build (e.g., run `build.sh`) before deployment to avoid shipping stale optimized
artifacts; keep references to the existing phrasing about `cargo rustc --
--crate-type cdylib` intact and replace the original combined sentence with two
clear sentences — one about `set -eu` behavior and one about the requirement to
rebuild to avoid stale artifacts.
- Around line 25-36: The fenced code block in docs/setup-fuzzing.md (the block
showing the project tree starting with "contracts/" and "vc-vault/") lacks a
language specifier; update the opening fence from ``` to ```text (or another
appropriate language) so markdown renderers treat it as plain text and preserve
formatting, e.g., change the triple-backtick that precedes the "contracts/" tree
to ```text.
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
1 participant
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 by CodeRabbit
New Features
Updates
Error Handling
Chores / Tests