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

Error in `ChristmasDinner::nonReentrant()` modifier

k3ysk1lls

Summary

There is an error in ChristmasDinner::nonReentrant() modifier. The modifier does not lock contract from reentrancy.

Vulnerability Details & Impact

The ChristmasDinner::nonReentrant() checks if the contract is locked and than execute the code instead first locking the contract for reentrancy. Since the only function that uses this modifier i.e. ChristmasDinner::_refundETH() function uses transfer(), which limits the gas spending to 2300, to send ETH, the reentrancy attack is mitigated now. Hence I marked it as medium risk. But the _refundETH function is not inherently safe, even though the transfer method restricts the gas sent to 2300. The Ethereum network's gas requirements for operations have changed in the past (eg. December 2019 Istanbul Hard Fork), and they could change again in the future. For instance, if storage operations (or fallback execution) become cheaper, a previously "safe" gas limit may no longer protect against reentrancy.

Tools Used

Manual review (see the code below):

modifier nonReentrant() {
require(!locked, "No re-entrancy");
// here the modifier should lock the contract
_;
locked = false;
}

Recommendations

Simply add the following line of code as shown below:

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

Lead Judging Commences

0xtimefliez Lead Judge 9 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.