Potential Reentrancy Risk
Description
The pizza_drop::transfer_from_contract function calls coin::transfer which could potentially be reentrant if the receiver is a malicious contract that implements a fallback function.
fun transfer_from_contract(to: address, amount: u64) acquires ModuleData {
let module_data = borrow_global<ModuleData>(@pizza_drop);
let resource_signer = account::create_signer_with_capability(&module_data.signer_cap);
// @? REENTRANCY RISK: coin::transfer can potentially trigger
coin::transfer<AptosCoin>(&resource_signer, to, amount);
}
Risk
Likelihood: Medium
Impact:
Proof of Concept
A malicious user can create a contract that implements a fallback function.
When coin::transfer is called in transfer_from_contract, the malicious contract's fallback function gets executed In this fallback function, the malicious contract could attempt to call other functions in the PizzaDrop contract.
If any state changes haven't been committed yet, the malicious contract might be able to manipulate the contract's state
module malicious::reentrancy_attacker {
use pizza_drop::airdrop;
use aptos_framework::coin;
use aptos_framework::aptos_coin::AptosCoin;
struct AttackState has key {
claimed: bool,
target_contract: address
}
public entry fun attack_pizza_drop(target: address, user: &signer) {
// Set up attack state
move_to(user, AttackState {
claimed: false,
target_contract: target
})
// airdrop::claim_pizza_slice(user)
}
// This function gets called when receiving APT
public fun receive_coins(sender: address, amount: u64) {
let attack_state = borrow_global_mut<AttackState>(sender);
// the first call is still executing
if (!attack_state.claimed) {
attack_state.claimed = true
// airdrop::claim_pizza_slice(...)
}
}
}
Recommended Mitigation
We need to follow the check-effects-interactions(CEI) pattern by updating all state variables before making external calls.
// In claim_pizza_slice function (lines 70-92)
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());
// Checks first
assert!(table::contains(&state.users_claimed_amount, user_addr), E_NOT_REGISTERED);
assert!(!table::contains(&state.claimed_users, user_addr), E_ALREADY_CLAIMED);
let amount = *table::borrow(&state.users_claimed_amount, user_addr);
// Check if contract has sufficient balance
assert!(state.balance >= amount, E_INSUFFICIENT_FUND);
// Effects (state changes) - BEFORE external calls
+ // Mark as claimed BEFORE external call to prevent reentrancy
+ table::add(&mut state.claimed_users, user_addr, true);
+
+ // Update balance BEFORE external call
+ state.balance = state.balance - amount;
+
+ // Register user to receive APT if not already registered
+ if (!coin::is_account_registered<AptosCoin>(user_addr)) {
+ coin::register<AptosCoin>(user);
+ };
+
+ // Emit event BEFORE external call
+ event::emit(PizzaClaimed {
+ user: user_addr,
+ amount: amount,
+ });
- // Register user to receive APT if not already registered
- if (!coin::is_account_registered<AptosCoin>(user_addr)) {
- coin::register<AptosCoin>(user);
- };
-
- transfer_from_contract(user_addr, amount);
-
- // Update balance
- state.balance = state.balance - amount;
-
- table::add(&mut state.claimed_users, user_addr, true);
-
- event::emit(PizzaClaimed {
- user: user_addr,
- amount: amount,
- });
+ // Interactions (external calls) - LAST
+ transfer_from_contract(user_addr, amount);
}
We can use reentrancy guard to the State struct.
struct State has key {
users_claimed_amount: Table<address, u64>,
claimed_users: Table<address, bool>,
owner: address,
balance: u64,
+ locked: bool, // Reentrancy guard
}
Also add this in fun pizza_drop::claim_pizza_slice function
public entry fun claim_pizza_slice(user: &signer) acquires ModuleData, State {
+ let state = borrow_global_mut<State>(get_resource_address());
+
+ // @? REENTRANCY PROTECTION: Check if function is already executing
+ assert!(!state.locked, 100); // E_REENTRANCY
+ state.locked = true;
let user_addr = signer::address_of(user);
let state = borrow_global_mut<State>(get_resource_address());
assert!(table::contains(&state.users_claimed_amount, user_addr), E_NOT_REGISTERED);
assert!(!table::contains(&state.claimed_users, user_addr), E_ALREADY_CLAIMED);
let amount = *table::borrow(&state.users_claimed_amount, user_addr);
// Check if contract has sufficient balance
assert!(state.balance >= amount, E_INSUFFICIENT_FUND);
// Register user to receive APT if not already registered
if (!coin::is_account_registered<AptosCoin>(user_addr)) {
coin::register<AptosCoin>(user);
};
transfer_from_contract(user_addr, amount);
// Update balance
state.balance = state.balance - amount;
table::add(&mut state.claimed_users, user_addr, true);
event::emit(PizzaClaimed {
user: user_addr,
amount: amount,
});
+ // Reset lock at the end
+ state.locked = false;
}
