Reentrancy modifier is suboptimal

[code423n4]

Handle

bitbopper

Vulnerability details

Impact

TL;DR: Guzzling more gas than needed in https://github.com/code-423n4/2022-01-yield/blob/e946f40239b33812e54fafc700eb2298df1a2579/contracts/ConvexStakingWrapper.sol#L81:L90

Longer version: The refund rules changed for clearing a storage slot. It is cheaper to keep the slot occupied. Since you used constants for ENTERED / NOT ENTERED no readability is lost. You might also want to adjust the eip-2200 comment.

Proof of Concept

Existing code

cat src/OldLockTest.sol 
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity ^0.8.6;

contract OldLockTest {
    bool private _status;
    bool private constant _NOT_ENTERED = false;
    bool private constant _ENTERED = true;

    modifier nonReentrant() {
        // On the first call to nonReentrant, _notEntered will be true
        require(_status != _ENTERED, "ReentrancyGuard: reentrant call");
        // Any calls to nonReentrant after this point will fail
        _status = _ENTERED;
        _;
        // By storing the original value once again, a refund is triggered (see
        // https://eips.ethereum.org/EIPS/eip-2200)
        _status = _NOT_ENTERED;
    }

    function doSomething() public nonReentrant returns (uint256) {
        return 1;
    }
}

Proposed code

cat src/NewLockTest.sol 
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity ^0.8.6;

contract NewLockTest {
    uint private _status = 1;
    uint private constant _NOT_ENTERED = 1;
    uint private constant _ENTERED = 2;

    modifier nonReentrant() {
        // On the first call to nonReentrant, _notEntered will be true
        require(_status != _ENTERED, "ReentrancyGuard: reentrant call");
        // Any calls to nonReentrant after this point will fail
        _status = _ENTERED;
        _;
        // By storing the original value once again, a refund is triggered (see
        // https://eips.ethereum.org/EIPS/eip-2200)
        _status = _NOT_ENTERED;
    }

    function doSomething() public nonReentrant returns (uint256) {
        return 1;
    }
}

Diff between existing and proposed code

< contract OldLockTest {
<     bool private _status;
<     bool private constant _NOT_ENTERED = false;
<     bool private constant _ENTERED = true;
---
> contract NewLockTest {
>     uint private _status = 1;
>     uint private constant _NOT_ENTERED = 1;
>     uint private constant _ENTERED = 2;

Outcome

dapp test
Running 2 tests for src/LockTest.t.sol:LockTestTest
[PASS] test_old() (gas: 23883)
[PASS] test_new() (gas: 1626)

Tools Used

Dapptools

Recommended Mitigation Steps

Use provided sample code

[iamsahu]

102

[GalloDaSballo]

Great submission, I believe this is the standard we should try to achieve for gas savings findings