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

User can drain contract by reentrancy

ahmedovv123

Summary

There is a reentrancy vulnerability in refund() function, it uses the nonReentrant modifier but it has an issue. This leaves an open door to all the functions that use the modifier.

Vulnerability Details

Malicious user can drain the ETH from contract by calling refund and reentering it. The nonReentrantdoes not set the lockedvariable to true and it stays false all the time which leads to pass the require statement.

function _refundETH(address payable _to) internal {
uint256 refundValue = etherBalance[_to];
_to.transfer(refundValue);
etherBalance[_to] = 0;
}

Function first sends the value to the msg.sender if the receiver is smart contract it can easily reentrant the same function and receive the same amount again and again untril the balance is 0.

Impact

Draining all the ETH amount of contract.

Tools Used

Manual

Recommendations

Even if the there is a nonReentrant mutex lock, always follow CEI standard, Check, Effect, Interact:

function _refundETH(address payable _to) internal {
uint256 refundValue = etherBalance[_to];
etherBalance[_to] = 0; // <- EFFECT
_to.transfer(refundValue); // <- INTERACT
}

Also fix the modifier:

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

Lead Judging Commences

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