Root + Impact
Description
Users can place their rapper NFTs in the arena for battle. After the battle, both NFTs should be sent back to their owner, but the code only transfers the record and fails to transfer the actual object.
For every one_shot::transfer_record_only, there should be an object::transfer with the corresponding parameters, but this is not the case when transferring NFTs to owners after battle.
[...]
if (arena.defender == @0x0) {
[...]
one_shot::transfer_record_only(token_id, player_addr, @battle_addr);
@> object::transfer(player, rapper_token, @battle_addr);
[...]
} else {
[...]
one_shot::transfer_record_only(chall_token_id, chall_addr, @battle_addr);
@> object::transfer(player, rapper_token, @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 happens after every battles.
Impact:
Users won't actually own their NFTs anymore and won't be able to use them anymore.
Proof of Concept
You will first need to add test_only functions in the code:
#[test_only]
public fun mint_cred(to: address,amount: u64) acquires CredCapabilities {
let caps = borrow_global<CredCapabilities>(@battle_addr);
let coins = coin::mint(amount, &caps.mint_cap);
coin::deposit(to, coins);
}
#[test_only]
public entry fun init(account: &signer) {
init_module(account);
}
#[test_only]
public fun get_mint_rapper_event_minter(event: &MintRapperEvent): address {
event.minter
}
#[test_only]
public fun get_mint_rapper_event_token_id(event: &MintRapperEvent): address {
event.token_id
}
#[test_only]
public entry fun init(sender: &signer) {
init_module(sender);
}
Add this code in the test file:
#[test(
module_owner = @battle_addr, alice = @0x999, bob = @998, aptos_framework = @aptos_framework
)]
public fun test_rapper_not_transferred(
module_owner: signer, alice: signer, bob: signer, aptos_framework: signer
) {
timestamp::set_time_has_started_for_testing(&aptos_framework);
aptos_coin::ensure_initialized_with_apt_fa_metadata_for_test();
let alice_address = signer::address_of(&alice);
let bob_address = signer::address_of(&bob);
cred_token::init(&module_owner);
rap_battle::init(&module_owner);
one_shot::mint_rapper(&module_owner, alice_address);
let events = event::emitted_events<one_shot::MintRapperEvent>();
let event = events.borrow(0);
let alice_rapper = one_shot::get_mint_rapper_event_token_id(event);
one_shot::mint_rapper(&module_owner, bob_address);
let events = event::emitted_events<one_shot::MintRapperEvent>();
let event = events.borrow(1);
let bob_rapper = one_shot::get_mint_rapper_event_token_id(event);
rap_battle::go_on_stage_or_battle(&alice, object::address_to_object<token::Token>(alice_rapper), 0);
rap_battle::go_on_stage_or_battle(&bob, object::address_to_object<token::Token>(bob_rapper), 0);
rap_battle::go_on_stage_or_battle(&alice, object::address_to_object<token::Token>(alice_rapper), 0);
}
Recommended Mitigation
Add object::transfer to owners after a battle ends.
