One Shot: Reloaded
First Flight #48
Beginner FriendlyNFT
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

L04. Rapper NFT Ownership Not Validated Before Metadata Update

accessdenied

Root + Impact

Description

  • Normal behavior: Players stake a rapper NFT and CRED to go on stage. The contract updates the RapperStats metadata to mark the NFT as on stage and transfers the NFT to the arena.

  • Issue: The function calls one_shot::transfer_record_only(token_id, player_addr, @battle_addr) before transferring the NFT:

let token_id = object::object_address(&rapper_token);
arena.defender_token_id = token_id;
​
one_shot::transfer_record_only(token_id, player_addr, @battle_addr);
object::transfer(player, rapper_token, @battle_addr);
​

  • If the player does not actually own rapper_token, transfer_record_only still executes.

  • Mitigating factor: object::transfer reverts if the signer does not own the NFT, rolling back the transaction and preventing persistent corruption.

Risk

Likelihood:

  • Low, because normal users can only pass NFTs they control.

  • Could occur if a malicious module attempts to call the function with an NFT the signer does not own.

Impact:

  • Minimal in production, as the transaction reverts and no permanent corruption occurs.

  • Metadata could be temporarily inconsistent during transaction execution, but the abort restores state.

Proof of Concept

// Pseudocode
rap_battle::go_on_stage_or_battle(&player_signer, unowned_rapper_token, 10);
​
// transfer_record_only executes and updates stats
// object::transfer reverts because player_signer is not the owner
// Transaction aborts
// Stats metadata is rolled back
​

Recommended Mitigation

- one_shot::transfer_record_only(token_id, player_addr, @battle_addr);
+ assert!(object::owner(&rapper_token) == signer::address_of(player), E_NOT_OWNER);
+ object::transfer(player, rapper_token, @battle_addr);
+ one_shot::transfer_record_only(token_id, player_addr, @battle_addr);
​

  • Explanation:

    • Check ownership explicitly before modifying metadata.

    • Transfer NFT first, then update RapperStats only if the transfer succeeds.

    • Reduces reliance on transaction rollback for safety and improves clarity.

Summary:

  • Low-risk issue in practice, but adding an ownership check makes the contract safer and prevents temporary metadata inconsistencies during transaction execution.

Updates

Lead Judging Commences

bube Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.