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

0 stars 0 forks source link

Vault: Reduce reliance on manual rebalances from strategy to vault #149

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

hickuphh3

Vulnerability details

Impact

Withdrawals are processed solely with funds that are held by the vault. Should there be insufficient liquidity (Eg. many withdrawals in a short time), users have to rely on a trusted party (operator) to move funds from the investment strategy to the vault.

As long as one operator is compromised, that operator can remove all other authorized parties to make himself to be the sole operator. Funds are then held hostage by the hacker (refuse to call withdrawToVault() on the strategy until a ransom is paid, for instance).

Recommended Mitigation Steps

Strategies (BaseStrategy) have a withdrawToVault() function. This should be utilised to attempt to withdraw any shortfall between a user’s withdrawal amount and the underlying token balance of the vault.

function _withdraw(
  address _to,
  uint256[] memory _ids,
  bool _force
) internal {
    ...
    uint256 currentBalance = underlying.balanceOf(address(this));
    if (amount > currentBalance) {
        // attempt to withdraw shortfall to vault
        strategy.withdrawToVault(amount - currentBalance);
    }
    underlying.safeTransfer(_to, amount);
}

// TODO: do likewise for _unsponsor
naps62 commented 2 years ago

The overall issue described here is of an operator being compromised, and removing authorization from other operators. That's a duplicate of #132