Root + Impact
Description
-
Normal behavior:
The contract should allow a registered user to claim if the contract account holds enough APT for the assigned amount.
-
Issue:
The contract tracks pool funds with an internal counter state.balance, updated only via fund_pizza_drop. If anyone transfers APT directly to the resource account, the actual on-chain balance increases but state.balance does not. The claim path checks state.balance instead of the real balance, so claims fail with E_INSUFFICIENT_FUND despite sufficient funds being present.
public entry fun fund_pizza_drop(owner: &signer, amount: u64) acquires ModuleData, State {
let state = borrow_global_mut<State>(get_resource_address());
assert!(signer::address_of(owner) == state.owner, E_NOT_OWNER);
let resource_addr = get_resource_address();
coin::transfer<AptosCoin>(owner, resource_addr, amount);
@> state.balance = state.balance + amount;
}
public entry fun claim_pizza_slice(user: &signer) acquires ModuleData, State {
let user_addr = signer::address_of(user);
let state = borrow_global_mut<State>(get_resource_address());
...
let amount = *table::borrow(&state.users_claimed_amount, user_addr);
@> assert!(state.balance >= amount, E_INSUFFICIENT_FUND);
...
transfer_from_contract(user_addr, amount);
@> state.balance = state.balance - amount;
...
}
Risk
Likelihood:
-
The resource account is coin-registered in init_module and can receive APT from any address.
-
Any direct transfer (donation/attack) creates a permanent divergence between coin::balance and state.balance.
-
No reconciliation logic exists; all claims continue to rely on the stale internal counter.
Impact:
-
Denial-of-service for claims: Users fail with E_INSUFFICIENT_FUND even though the account actually holds enough APT.
-
Stuck funds: Externally transferred APT becomes effectively unusable until the owner “tops up” via fund_pizza_drop or state is manually adjusted.
-
Incorrect observability: get_pizza_pool_balance() reports the stale counter, not the real balance; monitoring and accounting are wrong.
Proof of Concept
#[test(deployer = @pizza_drop, user = @0x123, framework = @0x1)]
#[expected_failure(abort_code = E_INSUFFICIENT_FUND)]
fun poc_balance_desync_external_deposit_causes_claim_fail(
deployer: &signer,
user: &signer,
framework: &signer
) acquires State, ModuleData {
use aptos_framework::timestamp;
use aptos_framework::aptos_coin;
use aptos_framework::coin;
use aptos_framework::account;
timestamp::set_time_has_started_for_testing(framework);
let (burn_cap, mint_cap) = aptos_coin::initialize_for_test(framework);
account::create_account_for_test(@pizza_drop);
account::create_account_for_test(signer::address_of(user));
init_module(deployer);
let ra = get_resource_address();
let ext_coins = coin::mint<aptos_coin::AptosCoin>(1_000, &mint_cap);
coin::deposit<aptos_coin::AptosCoin>(ra, ext_coins);
let actual = get_actual_apt_balance();
let tracked = get_pizza_pool_balance();
assert!(actual == 1_000, 1);
assert!(tracked == 0, 2);
let u = signer::address_of(user);
register_pizza_lover(deployer, u);
claim_pizza_slice(user);
coin::destroy_burn_cap(burn_cap);
coin::destroy_mint_cap(mint_cap);
}
Recommended Mitigation
- assert!(state.balance >= amount, E_INSUFFICIENT_FUND);
+ // Use the actual on-chain balance of the resource account
+ let ra = get_resource_address();
+ let actual = coin::balance<AptosCoin>(ra);
+ assert!(actual >= amount, E_INSUFFICIENT_FUND);
- // Maintain state.balance and mutate it on fund/claim
- state.balance = state.balance + amount;
- state.balance = state.balance - amount;
+ // Remove state.balance entirely, or treat it as advisory only.
+ // Prefer reading coin::balance<AptosCoin>(ra) at claim-time.
+ // If a tracker is kept, provide a 'sync' path that reconciles it with on-chain balance
+ // and make view getters (like get_pizza_pool_balance) read the real balance.
+ // (Optional) Emit a Funding event and a Reconciled event for observability.
