search

sherlock-audit / 2023-01-derby-judging

4 stars 1 forks source link

rvierdiiev - Vault.blacklistProtocol can revert in emergency #128

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

rvierdiiev

high

Vault.blacklistProtocol can revert in emergency

Summary

Vault.blacklistProtocol can revert in emergency, because it tries to withdraw underlying balance from protocol, which can revert for many reasons after it's hacked or paused.

Vulnerability Detail

Vault.blacklistProtocol is created for emergency cases and it's needed to remove protocol from protocols that allowed to deposit. https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L477-L483

  function blacklistProtocol(uint256 _protocolNum) external onlyGuardian {
    uint256 balanceProtocol = balanceUnderlying(_protocolNum);
    currentAllocations[_protocolNum] = 0;
    controller.setProtocolBlacklist(vaultNumber, _protocolNum);
    savedTotalUnderlying -= balanceProtocol;
    withdrawFromProtocol(_protocolNum, balanceProtocol);
  }

The problem is that this function is trying to withdraw all balance from protocol. This can create problems as in case of hack, attacker can steal funds, pause protocol and any other things that can make withdrawFromProtocol function to revert. Because of that it will be not possible to add protocol to blacklist and as result system will stop working correctly.

Impact

Hacked or paused protocol can't be set to blacklist.

Code Snippet

Provided above

Tool used

Manual Review

Recommendation

Provide needToWithdraw param to the blacklistProtocol function. In case if it's safe to withdraw, then withdraw, otherwise, just set protocol as blacklisted. Also you can call function with true param again, once it's safe to withdraw. Example of hack situation flow: 1.underlying vault is hacked 2.you call setProtocolBlacklist("vault", false) which blacklists vault 3.in next tx you call setProtocolBlacklist("vault", true) and tries to withdraw

mister0y commented 1 year ago

Disagree with severity because it's unlikely to result in (extra) loss of funds

Theezr commented 1 year ago

Fix: https://github.com/derbyfinance/derby-yield-optimiser/pull/207