Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Manual Reentrancy Guard Instead of OpenZeppelin's ReentrancyGuard

r4y4n3

Root + Impact

Description

  • Normal behavior:
    Smart contracts should use well-established, gas-optimized, and battle-tested libraries like OpenZeppelin's ReentrancyGuard for reentrancy protection.

    Specific issue:
    The contract implements a manual reentrancy guard using a boolean flag and custom modifier instead of using OpenZeppelin's ReentrancyGuard. While functional, this approach is less gas-efficient and more prone to implementation errors.

// src/Game.sol
bool private _locked; // Manual flag
​
modifier nonReentrant() {
require(!_locked, "ReentrancyGuard: reentrant call");
_locked = true; // @> Less efficient SSTORE operations
_;
_locked = false; // @> Additional SSTORE operation
}

Risk

Likelihood:

  • Affects every transaction using the nonReentrant modifier.

Impact:

  • Higher gas costs for users due to less optimized storage operations.

  • Increased maintenance burden and potential for implementation bugs.

  • Deviation from industry standards and best practices.

Proof of Concept

The following test demonstrates that the manual implementation works but consumes gas inefficiently:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {Test, console2} from "forge-std/Test.sol";
import {Game} from "../src/Game.sol";
​
contract GameReentrancyTest is Test {
Game public game;
address public deployer;
address public player1;
​
uint256 public constant INITIAL_CLAIM_FEE = 0.1 ether;
uint256 public constant GRACE_PERIOD = 1 days;
uint256 public constant FEE_INCREASE_PERCENTAGE = 10;
uint256 public constant PLATFORM_FEE_PERCENTAGE = 5;
​
function setUp() public {
deployer = makeAddr("deployer");
player1 = makeAddr("player1");
​
vm.deal(deployer, 10 ether);
vm.deal(player1, 10 ether);
​
vm.startPrank(deployer);
game = new Game(
INITIAL_CLAIM_FEE,
GRACE_PERIOD,
FEE_INCREASE_PERCENTAGE,
PLATFORM_FEE_PERCENTAGE
);
vm.stopPrank();
}
​
function testManualReentrancyGuardGasCost() public {
// Test gas cost of claimThrone with manual reentrancy guard
vm.startPrank(player1);
uint256 gasBefore = gasleft();
game.claimThrone{value: INITIAL_CLAIM_FEE}();
uint256 gasAfter = gasleft();
uint256 gasUsed = gasBefore - gasAfter;
console2.log("Gas used with manual reentrancy guard:", gasUsed);
vm.stopPrank();
​
// The manual implementation uses more gas due to:
// 1. SSTORE operations to set _locked = true and _locked = false
// 2. Less optimized storage patterns
// 3. Additional boolean state changes
​
assertTrue(gasUsed > 0, "Should consume gas");
}
​
function testReentrancyProtectionWorks() public {
// This test shows that the manual reentrancy guard works,
// but it's less efficient than OpenZeppelin's version
vm.startPrank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
vm.stopPrank();
​
// The protection works, but at higher gas cost
// OpenZeppelin's ReentrancyGuard would be more efficient
assertEq(game.currentKing(), player1, "Player1 should be king");
}
}

Recommended Mitigation

Replace the manual reentrancy guard with OpenZeppelin's ReentrancyGuard:

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
- contract Game is Ownable {
+ contract Game is Ownable, ReentrancyGuard {
​
- bool private _locked;
-
- modifier nonReentrant() {
- require(!_locked, "ReentrancyGuard: reentrant call");
- _locked = true;
- _;
- _locked = false;
- }
Updates

Appeal created

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!