`ReentryProtection.sol` Switching between 1, 2 instead of increasing `lockCounter` is more gas efficient

[code423n4]

Handle

WatchPug

Vulnerability details

The current implementation of ReentryProtection will increase lockCounter by one every time it's used.

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/shared/Reentry/ReentryProtection.sol#L7-L18

modifier noReentry {
    // Use counter to only write to storage once
    LibReentryProtectionStorage.RPStorage storage s =
        LibReentryProtectionStorage.rpStorage();
    s.lockCounter++;
    uint256 lockValue = s.lockCounter;
    _;
    require(
        lockValue == s.lockCounter,
        "ReentryProtectionFacet.noReentry: reentry detected"
    );
}

A more gas efficient implementation would be switching between 1, 2.

By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/86bd4d73896afcb35a205456e361436701823c7a/contracts/security/ReentrancyGuard.sol#L29-L33

Recommendation

Change to:

modifier noReentry {
    // Use counter to only write to storage once
    LibReentryProtectionStorage.RPStorage storage s =
        LibReentryProtectionStorage.rpStorage();
    s.lockCounter = 2;
    _;
    require(
        s.lockCounter == 2,
        "ReentryProtectionFacet.noReentry: reentry detected"
    );
    s.lockCounter = 1;
}