search

code-423n4 / 2022-01-sherlock-findings

0 stars 0 forks source link

Pausable paused() is not enforced to be present #273

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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

jack-the-pug commented 2 years ago

Good catch!