Christmas Dinner

Summary

The nonReentrant modifier in the ChristmasDinner smart contract is intended to prevent reentrancy attacks. However, its implementation contains a critical flaw that renders it ineffective. Additionally, the locked state variable, which is used by this modifier, is redundant and can be removed to save gas.

Vulnerability Details

The nonReentrant modifier is defined as follows:

Flaw in the Implementation

1. Initial State of locked:

   • The locked boolean is initialized to false by default, which allows the require(!locked) check to pass on the first call.

2. Missing State Change Before _;:

   • The locked variable is not set to true before the execution of _, which means the critical section of the function is not protected against reentrancy.

3. State Reset After Execution:

   • The locked variable is reset to false after the critical section (_;). This does not prevent a malicious contract from re-entering the function, as locked is never set to true to block subsequent calls during execution.

Redundancy of locked

The locked state variable is not used effectively, and its presence increases gas costs unnecessarily.

Impact

1. Reentrancy Vulnerability:

   • Functions using the nonReentrant modifier are still susceptible to reentrancy attacks, potentially leading to unauthorized withdrawals, state manipulation, or other security risks.

2. Gas Inefficiency:

   • The locked variable occupies unnecessary storage, which results in higher deployment and execution costs.

Tools Used

Manual code review

Recommendations

1. Fix the nonReentrant Modifier:
   Update the modifier to properly implement reentrancy protection:

   modifier nonReentrant() {

   require(!locked, "No re-entrancy");

   locked = true; // Set to true before `_` to block reentrancy

   _;

   locked = false; // Reset to false after execution

   }

2. Consider Removing locked:
   Instead of implementing custom reentrancy protection, leverage Solidity's ReentrancyGuard from OpenZeppelin's library. This approach is well-tested and eliminates the need for a manually defined locked variable:

   • Import ReentrancyGuard:

     import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

   • Modify the contract to inherit ReentrancyGuard:

     contract ChristmasDinner is ReentrancyGuard {

   • Replace nonReentrant with OpenZeppelin's implementation:

     function refund() external nonReentrant beforeDeadline {

     ...

     }

3. Remove the Redundant locked Variable:
   Eliminate the bool private locked = false; declaration to save gas and avoid confusion.