Sherlock contest - Fitraldys's results

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);
  }

Handle

Fitraldys

Vulnerability details

Impact

in line https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/SherlockClaimManager.sol#L285 and line https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/SherlockClaimManager.sol#L417 is cheaper use negation (!) require

Proof of Concept

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

Handle

Fitraldys

Vulnerability details

Impact

In the https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L545 the initialStake() function do the transfer then calculate the stakeShares however the stakeShares is calculated with the totalTokenBalanceStakers() before the transfer happened by subtract it with _amount, this additional subtraction is expensive than do the calculation of stakeShared first then do the transfer.

Proof of Concept

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L545

Recommended Mitigation

uint256 stakeShares_;
    uint256 totalStakeShares_ = totalStakeShares;
    // _amount of tokens divided by the "before" total amount of tokens, multiplied by the "before" amount of stake shares
    if (totalStakeShares_ != 0)
      stakeShares_ = (_amount * totalStakeShares_) / totalTokenBalanceStakers() ;
      // If this is the first stake ever, we just mint stake shares equal to the amount of USDC staked
    else stakeShares_ = _amount;

    token.safeTransferFrom(msg.sender, address(this), _amount);