0xSmartContract - If the `renounceOwnership` authorization is used, the project becomes unavailable

[github-actions[bot]]

0xSmartContract

medium

If the renounceOwnership authorization is used, the project becomes unavailable

Summary

onlyOwner has another secret import (from OwnableUpgradeable.sol) privilege: renounceOwnership() They can use this authority whenever he wants, there is no restriction. If he uses this authority, the very important functions detailed below will not be available, updated or added.

Vulnerability Detail

We see the use of Openzeppelin in OwnableUpgradeable.sol in many contracts and owners can renounceOwnership() like this project, which is sometimes a positive as it reduces the risk of rugpull but the situation is a bit different here, Owner is constantly needed (For example Pause/ Unpause) , so security risk is high

DnGmxJuniorVault.sol#L14

Impact

Key powers of OnlyOwner;

21 results - 3 files

contracts/vaults/DnGmxBatchingManager.sol:
  141      /// @dev allowance is granted while vault is added via addVault, this is only failsafe if that allowance is exhausted
  142:     function grantAllowances() external onlyOwner {

  151      /// @param _keeper address of keeper
  152:     function setKeeper(address _keeper) external onlyOwner {

  158      /// @param _slippageThresholdGmxBps slippage (in bps)
  159:     function setThresholds(uint256 _slippageThresholdGmxBps) external onlyOwner {

contracts/vaults/DnGmxJuniorVault.sol:
  129      /// @dev to be called once the vault is deployed
  130:     function grantAllowances() external onlyOwner {

  173          uint16 withdrawFeeBps
  174:     ) external onlyOwner {
  175          if (withdrawFeeBps > MAX_BPS) revert InvalidWithdrawFeeBps();

  203          uint128 partialEthHedgeUsdcAmountThreshold
  204:     ) external onlyOwner {
  205          if (slippageThresholdSwapBtcBps > MAX_BPS) revert InvalidSlippageThresholdSwapBtc();

  239          uint16 rebalanceHfThresholdBps
  240:     ) external onlyOwner {
  241          if (rebalanceTimeThreshold > 3 days) revert InvalidRebalanceTimeThreshold();

  262          IRewardsController aaveRewardsController
  263:     ) external onlyOwner {
  264          if (targetHealthFactor > 20_000) revert InvalidTargetHealthFactor();

  281      /// @notice pause deposit, mint, withdraw and redeem
  282:     function pause() external onlyOwner {
  283          _pause();

  286      /// @notice unpause deposit, mint, withdraw and redeem
  287:     function unpause() external onlyOwner {
  288          _unpause();

  293      /// @param _feeRecipient recipient address for protocol fees and protocol esGmx
  294:     function setFeeParams(uint16 _feeBps, address _feeRecipient) external onlyOwner {
  295          if (state.feeRecipient != _feeRecipient) {

  313      /// @notice unstakes and vest protocol esGmx to convert it to Gmx
  314:     function unstakeAndVestEsGmx() external onlyOwner {
  315          // unstakes the protocol esGMX and starts vesting it

  324      /// @dev when esGmx is vested some GlP tokens are locked on a pro-rata basis, in case that leads to issue in withdrawal this function can be called
  325:     function stopVestAndStakeEsGmx() external onlyOwner {
  326          // stops vesting and stakes the remaining esGMX

  335      /// @dev vested esGmx gets converted to GMX every second, so whatever amount is vested gets claimed
  336:     function claimVestedGmx() external onlyOwner {
  337          // stops vesting and stakes the remaining esGMX

contracts/vaults/DnGmxSeniorVault.sol:
  101      /// @dev to be called once the vault is deployed
  102:     function grantAllowances() external onlyOwner {
  103          address aavePool = address(pool);

  119      /// @dev depositCap = limit on the asset amount (usdc) that can be deposited into the vault
  120:     function setDepositCap(uint256 _newDepositCap) external onlyOwner {
  121          depositCap = _newDepositCap;

  126      /// @param _leveragePool: updated deposit cap
  127:     function setLeveragePool(IBorrower _leveragePool) external onlyOwner {
  128          leveragePool = _leveragePool;

  133      /// @param _dnGmxJuniorVault: updated deposit cap
  134:     function setDnGmxJuniorVault(IBorrower _dnGmxJuniorVault) external onlyOwner {
  135          dnGmxJuniorVault = _dnGmxJuniorVault;

  141      /// @param _maxUtilizationBps: updated max utilization bps
  142:     function setMaxUtilizationBps(uint256 _maxUtilizationBps) external onlyOwner {
  143          if (_maxUtilizationBps > MAX_BPS) revert InvalidMaxUtilizationBps();

  155      /// @param cap: new cap for the borrower
  156:     function updateBorrowCap(address borrowerAddress, uint256 cap) external onlyOwner {
  157          if (borrowerAddress != address(dnGmxJuniorVault) && borrowerAddress != address(leveragePool))

  172      /// @param _feeStrategy: new fee strategy
  173:     function updateFeeStrategyParams(FeeSplitStrategy.Info calldata _feeStrategy) external onlyOwner {
  174          feeStrategy = _feeStrategy;

Proof of Concept 1 - OnlyOwner does renounceOwnership() based on her authority in the OwnableUpgradeable.sol contract 2 - The project must be 'pause' due to a fatal error or compromised users' vaults 3 - Unfortunately this cannot be done

Code Snippet

OwnableUpgradeable.sol#L66

    function renounceOwnership() public virtual onlyOwner {
        _transferOwnership(address(0));
    }

Tool used

Manual Review

Recommendation

Instead of directly importing the OwnableUpgradeable.sol contract, a project-specific Ownable.sol should be used by removing the renounceOwnership() function, which is the subject of the above-mentioned potential problem.