Christmas Dinner

Summary

The logic for the function modifier nonRentrant in ChrismasDinner.sol needs adjusting as the mutex is set up incorrectly. The lock should be set to true before code execution and then set to false after code execution. In the current implementation there no lock before the code execution. This vulnerbility can cause loss of funds through reentrancy attacks.

Vulnerability Details

Currently the code is implemented as above where the require statement will always have locked = false as it is never set to trueduring code execution. This leaves the functions implementing this function modifier vulnerable to rentrancy attacks.

Attack Vector:

1. Attacker calls vulnerable function (e.g., refund())

2. During execution, the contract makes external call

3. Attacker contract re-enters the function

4. Since locked is never set to true, the require check passes

5. This cycle can repeat until funds are drained

Impact

The impact of this could leave to total loss of funds in the protocol due to the faulty implementation of the reentracy gaurd. The refund()would be the one vulnerable in this function and downstream this refundERC20()and refundETH()would be effects as well too.

Recommendations

Adjust the function modifer to have proper mutex workflow by setting the lockedvaribable to true before the code execution.

This execution flow will allow functions that use this modifer would lock the contract as they execute it so no other executions that use this modifer would be allowed until this code gets executed and then return the locked varible to false.

Alternatively, you can use an opensource library such as OpenZeppelin that provide this functionality to protect from reentrancy attacks: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/ReentrancyGuard.sol