Root + Impact
Description
In a normal battle, the protocol should compute the outcome based on Rapper stats, award the CRED prize pool to the winner, and record a win on the winner's Rapper NFT while returning both NFTs to their respective owners.
The specific issue is that after determining the winner, the code updates the internal ownership records for both the winner's and loser's Rapper NFTs to the winner's address, effectively transferring ownership of the loser's NFT to the winner without any physical object transfer or intended mechanics for NFT forfeiture.
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:
Impact:
-
Loser permanently loses control over their NFT in protocol records.
-
Winner gains unintended ownership of an extra NFT, enabling economic exploits.
Proof of Concept
Simulate a battle where challenger loses:
Assume defender wins (rnd < defender_skill)
After execution, check one_shot::read_stats for chall_token_id: owner should be defender_addr instead of original chall_addr.
#[test(module_owner = @battle_addr, player1 = @0x123, player2 = @0x456)]
fun test_winner_steals_loser_nft(module_owner: &signer, player1: &signer, player2: &signer) acquires battle_addr::rap_battle::BattleArena, battle_addr::one_shot::Collection, battle_addr::one_shot::RapperStats {
battle_addr::cred_token::init_module(module_owner);
battle_addr::one_shot::init_module(module_owner);
battle_addr::rap_battle::init_module(module_owner);
battle_addr::one_shot::mint_rapper(module_owner, signer::address_of(player1));
battle_addr::one_shot::mint_rapper(module_owner, signer::address_of(player2));
let rapper1 = /* assume fetched Object<Token> for player1
battle_addr::rap_battle::go_on_stage_or_battle(player1, rapper1, 1);
let rapper2 = /* assume fetched Object<Token> for player2
battle_addr::rap_battle::go_on_stage_or_battle(player2, rapper2, 1);
// After battle, check ownership records
let token_id1 = /* assume token_id of rapper1 */
let token_id2 = /* assume token_id of rapper2 */
let (_, _, _, _, wins1) = battle_addr::one_shot::read_stats(token_id1);
let (_, _, _, _, wins2) = battle_addr::one_shot::read_stats(token_id2);
let winner = if (wins1 > 0) { signer::address_of(player1) } else { signer::address_of(player2) };
// Demonstrate vulnerability: Both tokens now owned by winner in records
let stats = borrow_global<battle_addr::one_shot::RapperStats>(@battle_addr);
let stats1 = table::borrow(&stats.stats, token_id1);
let stats2 = table::borrow(&stats.stats, token_id2);
assert!(stats1.owner == winner, 0);
assert!(stats2.owner == winner, 0);
}
Recommended Mitigation
Apply the following changes to ensure the bug is mitigated.
- one_shot::transfer_record_only(chall_token_id, @battle_addr, defender_addr);
+ one_shot::transfer_record_only(chall_token_id, @battle_addr, chall_addr); // Return loser NFT record to original owner
- one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, chall_addr);
+ one_shot::transfer_record_only(arena.defender_token_id, @battle_addr, defender_addr); // Return loser NFT record to original owner
