Christmas Dinner

Summary

The nonReentrant modifier is intended to prevent reentrancy attacks by using a locked flag. However, the implementation has a logic flaw where the locked flag is only set to false after the function body executes. This flaw could allow reentrancy if locked is not properly set to true before executing the function body. Additionally, the refund() function does not fully leverage the intended reentrancy protection, as locked = true is not set before executing its logic.

Vulnerability Details

Current nonReentrant Modifier Implementation:

Issues:

1. Initial locked Value Logic:

   • The modifier checks require(!locked, "No re-entrancy");. If locked = false, the check passes, which is expected. However, locked should immediately be set to true before entering the function body to ensure no other calls can pass the require() condition.

   • This implementation creates a small window where reentrancy might still be possible before locked = trueis manually set in the function.

2. Missing locked = true in Function Logic:

   • In the refund() function:

     function refund() external nonReentrant beforeDeadline {

     address payable _to = payable(msg.sender);

     _refundERC20(_to);

     _refundETH(_to);

     emit Refunded(msg.sender);

     }

     • The function relies on the nonReentrant modifier, but since locked is not set to true in the modifier before entering the function body, it does not fully protect against reentrancy.

3. Potential Misuse:

   • If developers rely on the current modifier implementation, they might assume reentrancy protection is active throughout the function body, which is not true without explicitly setting locked = true within the modifier.

Impact

• The current nonReentrant implementation is flawed and could potentially allow reentrancy if misused or combined with external calls.

• Functions relying on nonReentrant might be susceptible to reentrancy attacks unless additional protection logic is manually implemented.

Tools Used

• Manual code review.

• Analysis of nonReentrant pattern and Solidity best practices.

Recommendations

1. Fix the nonReentrant Modifier Logic: Update the nonReentrant modifier to set locked = true before the function body is executed. This ensures reentrancy protection is active during the entire execution of the function.

   modifier nonReentrant() {

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

   + locked = true; // Set locked to true immediately

   _;

   locked = false; // Reset locked after function execution

   }

2. Update the refund() Function: With the corrected modifier, the refund() function will be properly protected. The corrected nonReentrant modifier eliminates the need to manually set locked = true inside the function body:

   function refund() external nonReentrant beforeDeadline {

   address payable _to = payable(msg.sender);

   _refundERC20(_to);

   _refundETH(_to);

   emit Refunded(msg.sender);

   }

3. Review All Functions Using nonReentrant: Ensure that all functions relying on the nonReentrant modifier are updated to follow the corrected implementation.4

4. Since solidty 0.8.24 its allowed design contracts with Transient Storage and be used for Reentrancy Locks modifier pattern. Its more gas efficient and saving slots in storage (priced at 100 gas.). Still the syntax only on low-level lang. and might be more difficult to implement into the basecode.

Conclusion

The current nonReentrant modifier contains a logic flaw that reduces its effectiveness. Correcting the modifier to set locked = true before entering the function body will ensure proper reentrancy protection. The refund() function and others relying on this modifier will then be safeguarded against reentrancy attacks.