One Shot: Reloaded
First Flight #47
Beginner FriendlyNFT
100 EXP
View all submissions
Previous Next
Submission Details
Impact: medium
Likelihood: low

Reentrancy-like State Corruption in transfer_record_only

Author Revealed upon completion

Root + Impact

Description

  • The function should atomically update both token ownership and owner counts without any intermediate state where they are inconsistent.

  • The function updates the token owner first, then updates the count tables, creating a window where ownership and counts are inconsistent.

public(friend) fun transfer_record_only(token_id: address, from: address, to: address) {
let s = table::borrow_mut(&mut stats_res.stats, token_id);
s.owner = to; // @> State changed BEFORE owner count update
let c_from = table::borrow_mut(&mut stats_res.owner_counts, from);
*c_from = *c_from - 1;
if (*c_from == 0) { table::remove(&mut stats_res.owner_counts, from); };
// @> WINDOW: Inconsistent state here - owner changed but counts not updated yet
}

Risk

Likelihood:

  • This occurs during any token transfer when the function is called, leaving inconsistent state if interrupted between the owner update and count adjustment.


Impact:

  • This can lead to permanent state corruption where token ownership doesn't match owner counts, breaking balance tracking and statistics.


Proof of Concept

Please add the following code in 'one_shot_tests.move' and then test it.

#[test]
#[expected_failure] // Demonstrates state corruption potential
public fun test_transfer_state_inconsistency() {
let module_owner = account::create_account_for_test(@battle_addr);
let owner1 = account::create_account_for_test(@owner1_addr);
let owner2 = account::create_account_for_test(@owner2_addr);
// Mint token to owner1
one_shot::mint_rapper(&module_owner, @owner1_addr);
let token_id = // get token ID from event or other means
// Simulate transfer that gets interrupted after owner change but before count update
// This would demonstrate the inconsistent state window
one_shot::transfer_record_only(token_id, @owner1_addr, @owner2_addr);
// Verify state consistency - this test should fail showing the vulnerability
let balance1 = one_shot::balance_of(@owner1_addr);
let balance2 = one_shot::balance_of(@owner2_addr);
assert!(balance1 == 0 && balance2 == 1, 1); // This will fail due to inconsistency
}

Recommended Mitigation

Updated the function by first updating count and then ownership. This ensures atomic state consistency by updating owner counts before changing token ownership, eliminating the window for inconsistent state.

​
public(friend) fun transfer_record_only(token_id: address, from: address, to: address) {
// Update counts FIRST, then ownership
let c_from = table::borrow_mut(&mut stats_res.owner_counts, from);
*c_from = *c_from - 1;
if (*c_from == 0) { table::remove(&mut stats_res.owner_counts, from); };
​
// Then update token owner
let s = table::borrow_mut(&mut stats_res.stats, token_id);
s.owner = to;
}

Support

FAQs

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