Skip to content

fix: transfer rejects previously burned destinations (#1059) - #1684

Merged
blurbeast merged 2 commits into
LabsCrypt:mainfrom
Ademiitura:fix/1059-burned-destination-transfer-bypass
Aug 30, 2026
Merged

fix: transfer rejects previously burned destinations (#1059)#1684
blurbeast merged 2 commits into
LabsCrypt:mainfrom
Ademiitura:fix/1059-burned-destination-transfer-bypass

Conversation

@Ademiitura

Copy link
Copy Markdown
Contributor

What Changed

transfer() only rejected a destination via has_any_remittance_state(to), which checks for existing Metadata, Score, or RecipientCommitment. burn_internal() clears all three of those on burn, leaving only the Burned flag set — so a previously burned (defaulted) account always looked "empty" to transfer() and could regain a clean, credit-bearing identity simply by receiving a transfer. This bypassed the BurnedRequiresApproval gate that mint() and admin_remint() already enforce for exactly this scenario.

Fix: added a hard check for DataKey::Burned(to) in transfer(), right next to the existing has_any_remittance_state check, returning the same NftError::BurnedRequiresApproval error mint() uses. No storage layout change (reads the existing Burned key), no new panics — pure Result early-return.

Related Issues

Closes #1059

Testing Done

  1. Added test_transfer_rejects_burned_destination: mints and burns an account, then asserts transfer into it fails with BurnedRequiresApproval and that neither side's state was mutated.
  2. Added test_transfer_into_burned_destination_never_leaves_credit_bearing_state: explicitly asserts the burned destination has no metadata after a rejected transfer — i.e. it's never simultaneously Burned and credit-bearing.
  3. cargo test — 78 passed, 0 failed.
  4. cargo fmt --check — clean.
  5. cargo clippy — clean (1 pre-existing, unrelated warning: unused STROOP_SCALE constant, not touched by this change).

Pull Request Checklist

  • I have read the CONTRIBUTING.md document.
  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the documentation accordingly. — N/A, no public API/docs changes; error behavior already existed for mint()/admin_remint().
  • I have verified the changes locally.

- transfer() checked has_any_remittance_state(to), which only looks at
  Metadata, Score, and RecipientCommitment. burn_internal() clears all
  three, leaving only the Burned flag, so a burned account always looked
  empty to transfer and could regain a clean identity via the transfer
  path — bypassing the BurnedRequiresApproval gate mint()/admin_remint()
  already enforce
- Add a hard check for DataKey::Burned(to) in transfer(), matching mint()'s
  existing gate exactly. No storage layout change, no new panics
- Add 2 regression tests: transfer into a burned address fails with
  BurnedRequiresApproval and mutates no state on either side; the burned
  destination never ends up credit-bearing after a rejected transfer
- Verified: cargo test (78 passed), cargo fmt --check clean, cargo clippy
  clean (1 pre-existing unrelated warning: unused STROOP_SCALE constant)
…_nft

POINTS_DENOMINATOR now references Self::STROOP_SCALE instead of a
hardcoded duplicate literal, matching its own doc comment. Same value
(1_000_000_000), no behavior change. Unrelated to the LabsCrypt#1059 security
fix itself, but was causing cargo clippy -D warnings to fail CI on
this file.

Verified: cargo test (78 passed), cargo fmt --check clean,
cargo clippy --all-targets --all-features -- -D warnings clean
@blurbeast
blurbeast merged commit 898925e into LabsCrypt:main Aug 30, 2026
11 of 14 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.

[Security] RemittanceNFT transfer to a previously burned address bypasses the BurnedRequiresApproval gate

2 participants