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

An incorrect custom implementation of the nonReentrant guard.

eth0x

Summary

The goal of the nonReentrant guard is to prevent reentrancy attacks in smart contracts.

Vulnerability Details

In the nonReentrant modifier, there is no setter for setting the locked flag to true. As a result, the locked flag always remains false.

Please compare the nonReentrant modifier with OpenZeppelin's implementation of this modifier. In OpenZeppelin's implementation, there is a function _nonReentrantBefore where the proper status for the locked flag is set.

modifier nonReentrant() {
_nonReentrantBefore();
_;
_nonReentrantAfter();
}
​
function _nonReentrantBefore() private {
// On the first call to nonReentrant, _status will be NOT_ENTERED
if (_status == ENTERED) {
revert ReentrancyGuardReentrantCall();
}
​
// Any calls to nonReentrant after this point will fail
_status = ENTERED;
}
​
function _nonReentrantAfter() private {
// By storing the original value once again, a refund is triggered (see
// https://eips.ethereum.org/EIPS/eip-2200)
_status = NOT_ENTERED;
}

In the protocol is only check, but there is not setter to locked=true

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

Impact

The custom implementation of nonReentrant is not secure and does not provide a proper guard against reentrancy attacks. As a result, all funds within the protocol could be vulnerable to hacking.

Tools Used

manual review

Recommendations

I recommend using OpenZeppelin's implementation of the nonReentrant modifier instead of a custom solution. OpenZeppelin's implementation is widely tested and trusted, ensuring proper protection against reentrancy attacks. Alternatively, you could modify the custom modifier, but this approach is not recommended due to the complexities and potential security risks involved. Sticking with OpenZeppelin's solution ensures robustness and reliability.

modifier nonReentrant() {
​
require(!locked, "No re-entrancy");
// @audit add this
locked = true;
_;
locked = false;
}
Updates

Lead Judging Commences

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

mutex lock incomplete

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