Christmas Dinner

First Flight #31
Beginner FriendlyFoundrySolidity
100 EXP
View results
Submission Details
Severity: high
Valid

Ineffective nonReentrant Modifier Implementation Could Lead to Future Vulnerabilities

Summary

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

Vulnerability Details

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.

modifier nonReentrant() {
require(!locked, "No re-entrancy");
_;
locked = false;
}

The issue is that:

  1. The locked flag is never set to true

  2. The flag is always set to false after execution

  3. 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.

Impact

Currently LOW impact since:

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

Tools Used

Manual review

Recommendations

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.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

mutex lock incomplete

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.