The nonReentrant modifier in the ChristmasDinner contract is incorrectly implemented, which could potentially allow reentrancy attacks if unsafe token transfers were used.
The severity is HIGH because:
While currently mitigated by safe transfer methods, any future updates to use different token transfer mechanisms would be immediately vulnerable
The modifier is completely ineffective at preventing reentrancy
The contract explicitly states it may handle future token implementations
The impact would be severe if exploited, potentially leading to fund drainage
The nonReentrant modifier in the contract at https://github.com/Cyfrin/2024-12-christmas-dinner/blob/main/src/ChristmasDinner.sol#L77-L81 is implemented incorrectly.
The issue is that:
The locked flag is never set to true
The flag is always set to false after execution
This makes the reentrancy check completely ineffective
While the current implementation using safeTransfer and transfer prevents reentrancy attacks, the broken modifier means there's no protection if the contract is modified to use less secure transfer methods in the future.
Currently LOW impact since:
safeTransfer and transfer are used which prevent reentrancy
The modifier is only used in the refund() function at https://github.com/Cyfrin/2024-12-christmas-dinner/blob/main/src/ChristmasDinner.sol#L140
However, this could become HIGH impact if:
The contract is upgraded to use less secure transfer methods
New functions are added that rely on this modifier for reentrancy protection
Manual review
Implement the nonReentrant modifier correctly by setting locked = true before execution and locked = false after execution. This ensures the lock is properly set during execution and prevents potential reentrancy if less secure transfer methods are used in the future.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.