Winner in rap_battle registry takes ownership of both * *Rapper NFTs
Description
-
Normal behavior:
In a fair battle flow, the winner should retain their own Rapper and only update its stats (e.g., increment wins). The loser should keep their NFT, even if they lose the wager.
-
Issue:
if (winner == defender_addr) {
@> 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 {
@> 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);
}
This means the winner’s address is set as the owner of both NFTs in the internal RapperStats registry.
The loser permanently loses their Rapper NFT in the protocol’s state, even though physical transfer of the object is never executed.
This creates a spec mismatch: design intent likely was “winner keeps their NFT, loser just loses wager”, but in reality, the registry enforces “winner takes all NFTs”.
Risk
Likelihood:
Impact: Medium → High
-
Medium if this is “intended game design,” as it’s very user-hostile but not a classical exploit.
-
High if unintended: NFTs are non-fungible assets the loser should retain. Current implementation allows silent confiscation.
-
Can be abused for theft: repeatedly challenge new players, win a single battle, and “legally” grab their Rapper NFT.
Proof of Concept
Need to added two #[test_only] helper functions for PoC:
In one_shot.move:
#[test_only]
public fun test_get_owner(token_id: address): address acquires RapperStats {
use aptos_std::table;
let stats = &borrow_global<RapperStats>(@battle_addr).stats;
let s = table::borrow(stats, token_id);
s.owner
In rap_battle.move (test-only battle flow without coin/timestamp deps):
#[test_only]
public fun test_go_on_stage_or_battle_no_coin(
player: &signer,
rapper_token: Object<Token>,
bet_amount: u64
) acquires BattleArena {
let player_addr = signer::address_of(player);
let arena = borrow_global_mut<BattleArena>(@battle_addr);
if (arena.defender == @0x0) {
assert!(arena.defender_bet == 0, E_BATTLE_ARENA_OCCUPIED);
arena.defender = player_addr;
arena.defender_bet = bet_amount;
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);
} else {
assert!(arena.defender_bet == bet_amount, E_BETS_DO_NOT_MATCH);
let defender_addr = arena.defender;
let chall_addr = player_addr;
let chall_token_id = object::object_address(&rapper_token);
one_shot::transfer_record_only(chall_token_id, chall_addr, @battle_addr);
object::transfer(player, rapper_token, @battle_addr);
let defender_skill = one_shot::skill_of(arena.defender_token_id);
let challenger_skill = one_shot::skill_of(chall_token_id);
let total_skill = defender_skill + challenger_skill;
let rnd = Self::test_now_seconds() % (if (total_skill == 0) { 1 } else { total_skill });
let winner = if (rnd < defender_skill) { defender_addr } else { chall_addr };
if (winner == defender_addr) {
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 {
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);
};
arena.defender = @0x0;
arena.defender_bet = 0;
arena.defender_token_id = @0x0;
}
Test file tests/winner_takes_both_poc.move:
module battle_addr::winner_takes_both_poc {
use battle_addr::one_shot;
use battle_addr::rap_battle;
use aptos_framework::object::{Self as object, Object};
use aptos_token_v2::token::Token;
#[test(battle_addr = @0x42, alice = @0xa11ce, bob = @0xb0b)]
public fun test_registry_assigns_both_to_winner(
battle_addr: &signer,
alice: &signer,
bob: &signer,
) {
rap_battle::test_init(battle_addr);
let (alice_obj, alice_id) = one_shot::test_mint_to_player_and_return_object(battle_addr, alice);
let (bob_obj, bob_id) = one_shot::test_mint_to_player_and_return_object(battle_addr, bob);
rap_battle::test_go_on_stage_or_battle_no_coin(alice, alice_obj, 0);
rap_battle::test_go_on_stage_or_battle_no_coin(bob, bob_obj, 0);
let alice_owner = one_shot::test_get_owner(alice_id);
let bob_owner = one_shot::test_get_owner(bob_id);
assert!(alice_owner == bob_owner, 1001);
}
}
Recommended Mitigation
Update rap_battle::go_on_stage_or_battle to only update the winner’s NFT stats, not ownership of both NFTs.
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);
+ one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, defender_addr);
+ // keep challenger’s NFT ownership unchanged
} 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);
+ one_shot::transfer_record_only(chall_token_id, @battle_addr, chall_addr);
+ // keep defender’s NFT ownership unchanged
}
