Christmas Dinner

Summary

The nonReentrant modifier is intended to prevent reentrancy attacks by implementing a mutex lock. However, the current implementation does not properly set the locked variable to true before executing the function body. As a result, the reentrancy protection is ineffective, leaving functions like ChristmasDinner::refund() and its associated internal calls (ChristmasDinner::_refundERC20 and ChristmasDinner::_refundETH) vulnerable to reentrancy attacks.

Vulnerability Details

Code in question :

The modifier checks that locked is false before executing the function.
However, it does not set locked = true before the function body executes (_;), leaving the contract unprotected during function execution.
The modifier does set locked = false after execution, but this is redundant since locked is already false by default and remains unchanged during execution.

The modifier is used only on ChristmasDinner::refund() which subsequently calls both ChristmasDinner::_refundERC20 and ChristmasDinner::_refundETH functions that directly interact with user balances and can transfer funds. Without proper reentrancy protection, an attacker could call refund() recursively, exploiting the state changes and draining funds.

Impact

The ineffective nonReentrant modifier fails to protect critical functions like refund(), which could allow an attacker to recursively drain funds.
Participants’ balances could be depleted by malicious actors, causing financial harm to users and loss of trust in the contract.
The presence of the nonReentrant modifier in ChristmasDinner::refund(), mentioned also several times in the natspec as 'not needed here, because already implemented @...', creates a false sense of security.

Tools Used

Manual code review

Recommendations

Update the modifier to set locked = true before executing the function body: