Sherlock contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

safeApprove will fail if the current approval is > 0 but < amount:

  if (want.allowance(address(this), address(lp)) < amount) {
    want.safeApprove(address(lp), type(uint256).max);
  }

This condition is unlikely to happen in practice as you approve the max value which should in theory last forever, but nevertheless a better option would be to reset the approval before setting it once again:

  if (want.allowance(address(this), address(lp)) < amount) {
    want.safeApprove(address(lp), 0);
    want.safeApprove(address(lp), type(uint256).max);
  }

Handle

pauliax

Vulnerability details

Impact

You can consider adding a slippage parameter to the execute function of SherBuy (or a boolean flag, e.g. _acceptRemaining) so that users can opt-in if they accept to receive the remaining amount in case there are not enough SHER tokens left to cover the whole _sherAmountWant. This can happen when a user queries viewCapitalRequirements and is satisfied with the expected results but when he actually executes the purchase, the state could have drifted to the point where he has to pay for less than intended to receive.

Recommended Mitigation Steps

I propose adding a slippage control parameter, e.g. uint256 _sherAmountWantMin or bool _acceptRemaining.

Handle

pauliax

Vulnerability details

Impact

When pausing the Sherlock contract, it also tries to pause other related contracts (similar situation is with unpause):

    if (!Pausable(address(yieldStrategy)).paused()) yieldStrategy.pause();
    // sherDistributionManager can be 0, pause isn't needed in that case
    if (
      address(sherDistributionManager) != address(0) &&
      !Pausable(address(sherDistributionManager)).paused()
    ) {
      sherDistributionManager.pause();
    }
    if (!Pausable(address(sherlockProtocolManager)).paused()) sherlockProtocolManager.pause();
    if (!Pausable(address(sherlockClaimManager)).paused()) sherlockClaimManager.pause();

Theoretically, there is no enforcement that these contracts comply with the Pausable interface, e.g. yieldStrategy is defined as IStrategyManager, and neither this interface nor its parent IManager contains a function paused() that is used to check whether these contracts should be paused or not. While in practice these functions are present, it is always a good practice to enforce this, as if this function is not present, the execution will cause a runtime exception.

Recommended Mitigation Steps

Because all these contracts one way or another inherit from IManager, a straightforward solution would be to declare this paused function there, or a more complicated solution would be to use ERC165 introspection: https://docs.openzeppelin.com/contracts/3.x/api/introspection#ERC165Checker

Handle

pauliax

Vulnerability details

Impact

The condition could be inclusive >= to avoid the useless computation:

  if (debt > balance) return 0;
  return balance - debt;

Handle

pauliax

Vulnerability details

Impact

    ILendingPool lp = getLp();
    if (balanceOf() == 0) {
      return 0;
    }

Better check the balance and only then get the lending pool. When the balance is 0 it will skip the external call.

Handle

pauliax

Vulnerability details

Impact

The unchecked directive can be applied in the following lines of code since there are statements before to ensure the arithmetic operations would not cause an integer underflow or overflow:

  if (_amount > mainBalance) {
    yieldStrategy.withdraw(_amount - mainBalance);
  }

  if (debt > balance) return 0;
  return balance - debt;

  if (debt > balance) {
    uint256 error = debt - balance;

  if (claimablePremiumError > lastClaimablePremiumsForStakers_) {
    insufficientTokens = claimablePremiumError - lastClaimablePremiumsForStakers_;

  if (_amount > balance) revert InsufficientBalance(_protocol);
  nonStakersClaimableByProtocol[_protocol] = balance - _amount;

  if (_amount > currentBalance) revert InsufficientBalance(_protocol);
  activeBalances[_protocol] = currentBalance - _amount;

  if (balance < amount) sherlockCore.payoutClaim(receiver, amount - balance);