Root + Impact
Description
The view function get_claimed_amount is supposed to return the actual amount a user claimed, but it actually returns the assigned amount for a user, and will return a positive value even when the user has not claimed yet.
#[view]
public fun get_claimed_amount(user: address): u64 acquires ModuleData, State {
let state = borrow_global<State>(get_resource_address());
if (!table::contains(&state.users_claimed_amount, user)) {
return 0
};
let amount = table::borrow(&state.users_claimed_amount, user);
*amount
}
This happens because the function get_random_slice saves the amount of APT to receive in the state variable state.users_claimed_amount, which is also named incorrectly and supports this confusion.
Risk
Likelihood: Medium
The issue only occurs after a user is registered by the owner, and before he claims his pizza slice.
Impact: Low
This view function is not used directly in the code. Other programs relying on it and users outside the blockchain may be tricked by its purpose.
Proof of Concept
This PoC demonstrates that get_claimed_amount returns a value for a registered user before their claim.
#[test(deployer = @pizza_drop, user = @0x123, framework = @0x1)]
fun test_get_claimed_amount_does_not_return_claimed_amount(deployer: &signer, user: &signer, framework: &signer) acquires State, ModuleData {
use aptos_framework::timestamp;
use aptos_framework::aptos_coin;
timestamp::set_time_has_started_for_testing(framework);
let (burn_cap, mint_cap) = aptos_coin::initialize_for_test(framework);
init_module(deployer);
let funding_amount = 10000;
let deployer_coins = coin::mint<AptosCoin>(funding_amount, &mint_cap);
coin::deposit<AptosCoin>(@pizza_drop, deployer_coins);
let contract_funding = 10000;
fund_pizza_drop(deployer, contract_funding);
let user_addr = signer::address_of(user);
register_pizza_lover(deployer, user_addr);
assert!(is_registered(user_addr), 2);
debug::print(&utf8(b"User registered successfully"));
let claimed_amount = get_claimed_amount(user_addr);
assert!(claimed_amount == 100, 3);
assert!(!has_claimed_slice(user_addr), 4);
coin::destroy_burn_cap(burn_cap);
coin::destroy_mint_cap(mint_cap);
}
Recommended Mitigation
The function get_claimed_amount should either be named get_assigned_amount, or return 0 when state.claimed_users does not contain the user passed in parameter.
#[view]
public fun get_claimed_amount(user: address): u64 acquires ModuleData, State {
let state = borrow_global<State>(get_resource_address());
- if (!table::contains(&state.users_claimed_amount, user)) {
+ if (!table::contains(&state.claimed_users, user)) {
return 0
};
let amount = table::borrow(&state.users_claimed_amount, user);
*amount
}
