Root + Impact
Description
- The - rap_battlemodule's normal behavior is to handle custody of both Rapper NFTs during a battle and return them to their respective owners once the battle is complete. The specific issue is that while the code updates the internal ownership records using- one_shot::transfer_record_only, it does not physically transfer the losing Rapper NFT back to its owner using- object::transfer. This leaves the NFT permanently stuck in the protocol's custody at the- @battle_addr.
 
/if (winner == defender_addr) {
    coin::deposit(defender_addr, pool);
    one_shot::increment_wins(arena.defender_token_id);
    one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, defender_addr); @>
    one_shot::transfer_record_only(chall_token_id, @battle_addr, defender_addr); @>
} else {
    coin::deposit(chall_addr, pool);
    one_shot::increment_wins(chall_token_id);
    one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, chall_addr); @>
    one_shot::transfer_record_only(chall_token_id, @battle_addr, chall_addr); @>
}
Risk
Likelihood:
- 
This will occur whenever a battle ends with a winner and a loser. 
- 
The object::transferfunction is not called for the losing NFT, so it will remain at the@battle_addr.
 
Impact:
- 
A player who loses a battle suffers a permanent loss of their Rapper NFT. 
- 
This results in a severe resource leak and asset loss for the user, which can lead to a complete loss of trust in the protocol. 
Proof of Concept
if (winner == defender_addr) {
    coin::deposit(defender_addr, pool);
    one_shot::increment_wins(arena.defender_token_id);
    one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, defender_addr); @>
    one_shot::transfer_record_only(chall_token_id, @battle_addr, defender_addr); @>
} else {
    coin::deposit(chall_addr, pool);
    one_shot::increment_wins(chall_token_id);
    one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, chall_addr); @>
    one_shot::transfer_record_only(chall_token_id, @battle_addr, chall_addr); @>
}
Recommended Mitigation
-  one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, defender_addr);
- one_shot::transfer_record_only(chall_token_id, @battle_addr, defender_addr);
+ object::transfer(arena.defender_token, defender_addr);
+ object::transfer(challenger_token, chall_addr);