Skip to content

[token-objects] fix royalty lookup after rename - #416

Open
seanyoung wants to merge 1 commit into
m1from
royalty
Open

[token-objects] fix royalty lookup after rename#416
seanyoung wants to merge 1 commit into
m1from
royalty

Conversation

@seanyoung

@seanyoung seanyoung commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Description

token::royalty falls back to the collection's royalty when a token carries none of its own. It picked that collection by re-deriving collection::create_collection_address(creator, collection_name). Collection names are mutable via collection::set_name, which moves the name out from under that derivation, so after any rename the lookup no longer resolves to the token's own collection. Two consequences, both reachable:

  1. The view aborts. If no collection of that creator holds the new name, object::address_to_object aborts with EOBJECT_DOES_NOT_EXIST for every token in the collection. royalty is a #[view], and marketplaces call it to compute payouts — see compute_royalty in aptos-move/move-examples/marketplace/sources/listing.move, which calls tokenv2::royalty(listing.object) directly. Sales of any renamed collection break, unrecoverably.

  2. Royalty is misdirected. If the creator already holds a second collection under that name, the derivation lands on that collection and the token pays out its royalty. This defeats immutability: a royalty published without a royalty::MutatorRef cannot be edited, but renaming repoints which royalty is read at all.

The fix reads the Object<Collection> recorded on the token at mint time (Token.collection) rather than re-deriving it. That field is written once at creation and is unaffected by renames. Folding the read into the existing borrow also preserves the pre-existing ETOKEN_DOES_NOT_EXIST guard ordering, so a non-token object still aborts before any royalty is read.

Note on the threat model

A related patch upstream (aptos-labs#20385) describes this as a third-party "shadow collection" attack, where an outside attacker registers a same-named collection at the derived address. That is not possible. create_collection_address(creator, name) is object::create_object_address(creator, name), and Collection.creator is always signer::address_of of the account that published the named object — so the derived address always sits in the legitimate creator's own named-object space, which no other account can publish into. The reachable failure modes are the two above; the second requires only the creator's own two collections, no attacker account.

Scope

create_common (token.move) and aptos_token::collection_object also derive collections by name, but there the caller supplies the name — they are lookup-by-name APIs, and collection::set_name's doc comment already states that contract. Changing them would alter public API semantics rather than fix a bug, so they are left alone.

How Has This Been Tested?

Four regression tests added to aptos-token-objects/sources/token.move. Run with:

RUST_MIN_STACK=134217728 cargo test -p aptos-framework --test move_unit_test move_token_objects_unit_tests
Test Before the fix After
test_collection_royalty_fallback_survives_rename abort 393218 (EOBJECT_DOES_NOT_EXIST) PASS
test_collection_royalty_not_repointed_by_name_collision abort 2 — token reported 99% to @0xbad PASS
test_royalty_absent_survives_rename abort 393218 PASS
test_token_royalty_takes_precedence_over_collection PASS (token royalty short-circuits) PASS

The first three fail against the old lookup; the fourth is a guard against over-correcting. The collision test additionally asserts that the name-derived address really does resolve to the other collection and really does hold its royalty, so it pins the mechanism rather than just the symptom.

Full aptos-token-objects suite after the fix: 87 passed, 0 failed.

Key Areas to Review

  • Whether preserving the ETOKEN_DOES_NOT_EXIST-before-royalty::get ordering matters to any caller. let collection = borrow(&token).collection; keeps the old behaviour, where the bare borrow(&token); ran first.
  • Behaviour change for already-renamed collections on chain: royalty() currently aborts for them and will start returning the original collection's royalty. That is the intended fix, but it does change observed values for any collection that was renamed onto a sibling's name.

Type of Change

  • Bug fix
  • Tests

Which Components or Systems Does This Change Impact?

  • Aptos Framework

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

token::royalty picked the collection to fall back to by re-deriving
create_collection_address(creator, collection_name). collection::set_name
moves the name out from under that derivation, so after any rename the
lookup no longer resolves to the token's own collection:

- if no collection of that creator holds the new name, the #[view] aborts
  with EOBJECT_DOES_NOT_EXIST for every token in the collection, breaking
  marketplaces that call it to compute payouts

- if the creator already holds another collection under that name, the
  derivation lands on that one and the token pays out its royalty instead,
  defeating a royalty published without a MutatorRef and therefore meant
  to be immutable

Read the Object<Collection> recorded on the token at mint time instead. It
is written once at creation and is unaffected by renames. Folding it into
the existing borrow also preserves the ETOKEN_DOES_NOT_EXIST guard
ordering.

Regression tests cover the rename fallback, the name-collision
misdirection, token-level royalty precedence, and the absent-royalty case.
The first three fail against the old lookup.

Co-Authored-By: Claude
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.

1 participant