Sherlock contest - ye0lde's results

Handle

ye0lde

Vulnerability details

Impact

Redundant arithmetic underflow/overflow checks can be avoided when an underflow/overflow cannot happen.

Proof of Concept

The "unchecked" keyword can be applied here since there is an if statement before to ensure the arithmetic operations would not cause an integer underflow or overflow. https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/SherlockProtocolManager.sol#L327-L336

Change the code at 327 to:

   unchecked { 

        if (claimablePremiumError > lastClaimablePremiumsForStakers_) {
          insufficientTokens = claimablePremiumError - lastClaimablePremiumsForStakers_;
          lastClaimablePremiumsForStakers = 0;
        } else {
          // If the error is not bigger than the claimable premiums, then we just decrease claimable premiums
          // By the amount that was added in error (error) and insufficientTokens = 0
          lastClaimablePremiumsForStakers =
            lastClaimablePremiumsForStakers_ -
            claimablePremiumError;
        }
    }

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Add the "unchecked" keyword as shown above.

Handle

ye0lde

Vulnerability details

Impact

Gas savings and code clarity

Proof of Concept

Below are several occurrences of comparison with literal boolean values of true or false: https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/SherBuy.sol#L89 https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/SherBuy.sol#L126

https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/SherClaim.sol#L79

https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/Manager.sol#L52

https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/SherlockClaimManager.sol#L285 https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/SherlockClaimManager.sol#L417 https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/SherlockClaimManager.sol#L502

https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/SherlockProtocolManager.sol#L677

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Use the compared expression itself in place of comparison with a boolean literal. The expression can be replaced as is when the expression is expected to evaluate to true and negation can be used when the expression is expected to have a false value.

For example:

if (able == false) revert InvalidConditions();

can be replaced with

if (!able) revert InvalidConditions();

Handle

ye0lde

Vulnerability details

Impact

Removing redundant code can reduce gas usage and improve code clarity.

Proof of Concept

The _isCleanupState function: https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/SherlockClaimManager.sol#L160-L164

I suggest this refactoring:

    function _isCleanupState(State _oldState) internal pure returns (bool) { 
    return _oldState == State.SpccDenied || _oldState == State.SpccPending ? true : false;
  }

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

See POC for details.

Handle

ye0lde

Vulnerability details

Impact

A protocol can be removed when the premium is not zero or there is a misleading comment for function protocolRemove.

Proof of Concept

A comment for function protocolRemove at: https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/contracts/managers/SherlockProtocolManager.sol#L598

states

  /// @dev Before removing a protocol the premium must be 0

But the function does not check that the premium is zero and calls _forceRemoveProtocol which eventually "forces" the premium to zero without any checking.

If the comment is correct then a check for premiums_[_protocol] == 0 is needed. While onlyOwner can call this function, it still verifies that onlyOwner does not pass in a non-existent protocol. It seems it should also check that premium is zero.

On the other hand, if the comment is incorrect I suggest that it should be rewritten to something like this:

  /// @dev Premium will be forced to 0 before the protocol is removed.

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

See the POC