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

Transferring record does not check the current owner of the token

Author Revealed upon completion

Root + Impact

Description

one_shot::transfer_record_only is used to transfer a token from a user to another. Because it overwrites stats_res.stats.owner to to without checking that it was equal to from before, it is possible to call this function with any from that have a stats_res.owner_counts positive.

If this happens, the protocol will reach a corrupted state.

// In one_shot.move
public(friend) fun transfer_record_only(token_id: address, from: address, to: address)
acquires RapperStats {
let stats_res = borrow_global_mut<RapperStats>(@battle_addr);
let s = table::borrow_mut(&mut stats_res.stats, token_id);
// @audit Does not verify the current owner is `from`
s.owner = to;
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); };
if (table::contains(&stats_res.owner_counts, to)) {
let c_to = table::borrow_mut(&mut stats_res.owner_counts, to);
*c_to = *c_to + 1;
} else {
table::add(&mut stats_res.owner_counts, to, 1);
};
}

Risk

Likelihood:

This root cause is currently exploitable because rap_battle::go_on_stage_or_battle contains calls to one_shot::transfer_record_only without updating the object owner, but I believe that is another issue.

It means that a record can be transfered to a wrong owner in this case only

Impact:

The real owner of the NFT will not have their NFT count decrease, but another address will. This corrupts the protocol state regarding this token and may cause deeper issues with staking and battling.

Recommended Mitigation

Update the transfer record function to check current token owner is from:

public(friend) fun transfer_record_only(token_id: address, from: address, to: address)
acquires RapperStats {
let stats_res = borrow_global_mut<RapperStats>(@battle_addr);
let s = table::borrow_mut(&mut stats_res.stats, token_id);
+ assert!(s.owner == from, 0);
s.owner = to;
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); };
if (table::contains(&stats_res.owner_counts, to)) {
let c_to = table::borrow_mut(&mut stats_res.owner_counts, to);
*c_to = *c_to + 1;
} else {
table::add(&mut stats_res.owner_counts, to, 1);
};
}

Support

FAQs

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