Guardian can cause temporary DoS on withdrawals in the MainVault contract

Summary

A known issue about not validating the input when setting a value in basis points. When working with percentages the setter should not be allowed to set the fee bps to more than the maximum percentage 100% or 10_000 basis points.

Vulnerability Detail

The guardian can set the governanceFee to more than 10_000 which will cause transferFunds function to take fee greater than the withdrawn amount to send. The more major problem is that the transferFunds will even revert because of underflow exception when calculating the left amount to send to the recipient since govFee will be > _value:

  function transferFunds(address _receiver, uint256 _value) internal {
    uint256 govFee = (_value * governanceFee) / 10_000;

    vaultCurrency.safeTransfer(getDao(), govFee);
    vaultCurrency.safeTransfer(_receiver, _value - govFee);
  }

Impact

Temporary DoS on withdrawals in the MainVault.

Code Snippet

  function setGovernanceFee(uint16 _fee) external onlyGuardian {
    governanceFee = _fee;
  }

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/MainVault.sol#L457-L459

  uint256 govFee = (_value * governanceFee) / 10_000;

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/MainVault.sol#L185

  vaultCurrency.safeTransfer(_receiver, _value - govFee);

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/MainVault.sol#L188

Tool used

Manual Review

Recommendation

Restrict the guardian from setting a value higher than the maximum basis points 10_000.

  function setGovernanceFee(uint16 _fee) external onlyGuardian {
+   require(_fee < 10_000, "Fee too high");
    governanceFee = _fee;
  }

sherlock-audit / 2023-01-derby-judging

gogo - Guardian can cause temporary DoS on withdrawals in the MainVault contract #401