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

Mutex lock never set to true in `nonReentrant` modifier

marcoroccon

Description:
The nonReentrant modifier is intended to protect the refund function from reentrancy attacks. To achieve this, a boolean variable, locked, is used as the mutex lock. However, the modifier does not set the locked variable to true after the require statement, causing the modifier to lose its purpose and allowing multiple calls to refund.

Impact:
At first glance, this might seem like a high-severity issue, since nonReentrant allows multiple calls to refund. However, refund ultimately handles the refund process via the _refundERC20 and _refundETH internal functions, which in turn call the transfer function from Solidity. The latter has a predetermined gas limit of 2300, which ensures that there are no multiple calls, reverting in that case with an EvmError: OutOfGas. Therefore, the finding is classified as medium-severity, since there is still the possibility of a gas price increase that could make the reentrancy attack feasible.

For more details, refer to the Slither detector documentation:
https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-4

Tools Used:
Slither, manual review

Proof of Concept:
Create the following contract in ChristmasDinnerTest.t.sol, which will simulate a malicious reentrancy attacker smart contract.

contract ReentrancyAttacker {
ChristmasDinner cd;
​
constructor(ChristmasDinner _cd) {
cd = _cd;
}
​
receive() external payable {
_drainAllFunds();
}
​
fallback() external payable {
_drainAllFunds();
}
​
function attack() external payable {
address payable victimContract = payable(address(cd));
(bool success,) = victimContract.call{value: 1e16}("");
require(success, "transfer failed");
cd.refund();
}
​
function _drainAllFunds() internal {
if(address(cd).balance > 0) {
cd.refund();
}
}
}

Add the following test to ChristmasDinnerTest.t.sol. The test will fail with an EvmError: OutOfGas, due to the predetermined 2300 gas limit of Solidity's transfer function.

function test_reentrancyRefund() public {
address payable _cd = payable(address(cd));
​
vm.deal(user1, 1e16);
vm.prank(user1);
(bool sent,) = _cd.call{value: 1e16}("");
require(sent, "transfer failed");
​
ReentrancyAttacker attackerContract = new ReentrancyAttacker(cd);
address attackerUser = makeAddr("attackerUser");
vm.deal(address(attackerContract), 1e16);
​
vm.prank(attackerUser);
vm.expectRevert();
attackerContract.attack();
}

Recommended Mitigation:

  • The easiest solution to the issue is to set the locked variable to true when passing the first check in the modifier. This would prevent a malicious actor from re-entering refund before its execution is completed.

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

  • It is recommended to use well-known and tamper-proof contract libraries to enhance the security level of smart contracts. To avoid reentrancy attacks, ChristmasDinner could inherit from ReentrancyGuard contract from OpenZeppelin, which already has a battle-tested nonReentrant modifier approved by the community.

For reference, see ReentrancyGuard.sol
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.5/contracts/security/ReentrancyGuard.sol

  • Even though the documentation states that a mutex variable is sufficient for avoiding reentrancy attacks, it is still recommended to write each function following the Checks-Effects-Interactions (CEI) pattern.

For example, _refundETH could be rephrased as follows:

function _refundETH(address payable _to) internal {
uint256 refundValue = etherBalance[_to];
+ etherBalance[_to] = 0;
_to.transfer(refundValue);
- etherBalance[_to] = 0;
}
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!