Looping through storage is expensive

[code423n4]

Handle

Fitraldys

Vulnerability details

Impact

In the addCallback() this function will check by loop through all of the _callback that was available inside claimCallbacks, to check if the _callback that will be add is already added or not, it cheaper to save the claimCallback.length to a variable then use that variable in the loop.

Proof of Concept

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/SherlockClaimManager.sol#L234

Recommended Mitigation

function addCallback(ISherlockClaimManagerCallbackReceiver _callback)
    external
    onlyOwner
    nonReentrant
  {
    uint256 callbackLength = claimCallbacks.length;
    if (address(_callback) == address(0)) revert ZeroArgument();
    // Checks to see if the max amount of callback contracts has been reached
    if (callbackLength == MAX_CALLBACKS) revert InvalidState();
    // Checks to see if this callback contract already exists
    for (uint256 i; i < callbackLength; i++) {
      if (claimCallbacks[i] == _callback) revert InvalidArgument();
    }

    claimCallbacks.push(_callback);
    emit CallbackAdded(_callback);
  }

[jack-the-pug]

Dup #231