_lock in noRenter can be optimized to use less gas

[code423n4]

Handle

mtz

Vulnerability details

Impact

The noRenter modifier sets the value of the _locked variable to 1 to acquire the lock and prevent reentrancy and sets _locked to 0 to release the lock. Instead it should use 1 for unlocked and 2 for locked since zeroing values in storage can use more gas. This implementation of noRenter is more gas efficient:

    uint256 internal _locked = 1;
...
    modifier noReenter() {
        require(_locked == 1, "LOCKED");
        _locked = uint256(2);
        _;
        _locked = uint256(1);
    }

This is similar to OpenZeppelin's ReentrancyGuard implementation

See the rationale there for why 1 and 2 are more efficient than 0 and 1.

Proof of Concept

In my tests (code snippet below), the more efficient reentrancy guard uses 11130 less gas

pragma solidity ^0.8.0;

contract GasTest {
    uint256 internal _locked = 0;
    uint256 internal _locked2 = 1;

    modifier noReenter() {
        require(_locked == 0, "LOCKED");
        _locked = uint256(1);
        _;
        _locked = uint256(0);
    }
    function y() public noReenter returns (uint256) {
        return address(this).balance;
    }

    modifier noReenter2() {
        require(_locked2 == 1, "LOCKED");
        _locked2 = uint256(2);
        _;
        _locked2 = uint256(1);
    }
    function y2() public noReenter2 returns (uint256) {
        return address(this).balance;
    }
}

Tools Used

N/A

Recommended Mitigation Steps

Use the more efficient noRenter implementation recommended above.

[deluca-mike]

Agreed. While not always cheaper, it is cheaper in this case for batch unlocks or relocks. Will implement this.

[deluca-mike]

Duplicate #1