Rogue implementation that are removed can still perform damaging action, the vault admin is permissionless and should not have the power to upgrade the vault contract

[hats-bug-reporter[bot]]

Github username: @JeffCX Submission hash (on-chain): 0x81d91aac20e83b7d1890e395d9a9290f8395bdd3d077c66461f921ac4ec696f8 Severity: medium

Description: Description\

Rogue implementation that are blocklist can still perform damaging action

Attack Scenario\

In the current implementation, the high level access control is done by the contract VaultRegistry

 function addVault(address vault) external override {
    if (!factories[msg.sender] && msg.sender != owner()) revert Errors.AccessDenied();

    vaults[vault] = true;
    emit VaultAdded(msg.sender, vault);
  }

  /// @inheritdoc IVaultsRegistry
  function addVaultImpl(address newImpl) external override onlyOwner {
    if (vaultImpls[newImpl]) revert Errors.AlreadyAdded();
    vaultImpls[newImpl] = true;
    emit VaultImplAdded(newImpl);
  }

  /// @inheritdoc IVaultsRegistry
  function removeVaultImpl(address impl) external override onlyOwner {
    if (!vaultImpls[impl]) revert Errors.AlreadyRemoved();
    vaultImpls[impl] = false;
    emit VaultImplRemoved(impl);
  }

  /// @inheritdoc IVaultsRegistry
  function addFactory(address factory) external override onlyOwner {
    if (factories[factory]) revert Errors.AlreadyAdded();
    factories[factory] = true;
    emit FactoryAdded(factory);
  }

we can notice a few things, first, once a vault is added, it cannot by removed

the only owner admin can remove a vault implementation by set the implementation to false

 function removeVaultImpl(address impl) external override onlyOwner {
    if (!vaultImpls[impl]) revert Errors.AlreadyRemoved();
    vaultImpls[impl] = false;
    emit VaultImplRemoved(impl);
  }

first, the admin needs to set the addVaultImpl to true, whitelisting a implementation

then the vault implementation can be upgraded

  function upgradeToAndCall(
    address newImplementation,
    bytes memory data
  ) public payable override onlyProxy {
    _authorizeUpgrade(newImplementation);
    _upgradeToAndCallUUPS(newImplementation, abi.encodeWithSelector(_initSelector, data), true);
  }

  /// @inheritdoc UUPSUpgradeable
  function _authorizeUpgrade(address newImplementation) internal view override {
    _checkAdmin();
    if (
      newImplementation == address(0) ||
      _getImplementation() == newImplementation || // cannot reinit the same implementation
      IVaultVersion(newImplementation).vaultId() != vaultId() || // vault must be of the same type
      IVaultVersion(newImplementation).version() != version() + 1 || // vault cannot skip versions between
      // @audit
      !IVaultsRegistry(_vaultsRegistry).vaultImpls(newImplementation) // new implementation must be registered
    ) {
      revert Errors.UpgradeFailed();
    }
  }

but consider that the vault implementation is upgradable, and the admin is the vault admin, not neccessarily the trusted, because the vault is deployed in the factory contract and there is no minimum captial commitment for the vault admin, so the cost of perform rogue action is none

  function createVault(
    bytes calldata params,
    bool isOwnMevEscrow
  ) external payable override returns (address vault) {
    // create vault
    vault = address(new ERC1967Proxy(implementation, ''));

    // create MEV escrow contract if needed
    address _mevEscrow;
    if (isOwnMevEscrow) {
      _mevEscrow = address(new OwnMevEscrow(vault));
      // set MEV escrow contract so that it can be initialized in the Vault
      ownMevEscrow = _mevEscrow;
    }

    // set admin so that it can be initialized in the Vault
    vaultAdmin = msg.sender;

    // initialize Vault
    IEthVault(vault).initialize{value: msg.value}(params);

If we take a look at the same validation

contracts\keeper\KeeperRewards.sol:
  162    ) external override returns (int256 totalAssetsDelta, uint256 unlockedMevDelta) {
  163:     if (!_vaultsRegistry.vaults(msg.sender)) revert Errors.AccessDenied();
  164  

contracts\keeper\KeeperValidators.sol:
  53      }
  54:     if (!_vaultsRegistry.vaults(msg.sender)) revert Errors.AccessDenied();
  55  

  82    ) external override {
  83:     if (!(_vaultsRegistry.vaults(vault) && isCollateralized(vault))) revert Errors.InvalidVault();
  84      if (deadline < block.timestamp) revert Errors.DeadlineExpired();

contracts\osToken\OsToken.sol:
  106      if (
  107:       !(_vaultsRegistry.vaults(msg.sender) &&
  108          vaultImplementations[IVaultVersion(msg.sender).implementation()])

  136      if (shares == 0) revert Errors.InvalidShares();
  137:     if (!_vaultsRegistry.vaults(msg.sender)) revert Errors.AccessDenied();
  138  

contracts\vaults\ethereum\mev\SharedMevEscrow.sol:
  24    function harvest(uint256 assets) external override {
  25:     if (!_vaultsRegistry.vaults(msg.sender)) revert Errors.HarvestFailed();
  26  

only when mintingShares, the validation validate both vault and the implementation

  function mintShares(address receiver, uint256 shares) external override returns (uint256 assets) {
    if (receiver == address(0)) revert Errors.ZeroAddress();
    if (shares == 0) revert Errors.InvalidShares();
    if (
      !(_vaultsRegistry.vaults(msg.sender) &&
        vaultImplementations[IVaultVersion(msg.sender).implementation()])
    ) revert Errors.AccessDenied();

the rest of the code when calling sensetive function such as harvest, approve validator, even burning OS shares does not check if the implementation logic is already removed and blocklisted

even the owner call removeVaultImpl, the removed implementation can still burn OS share / call harvest / approve validator

the vault admin should not have the power to upgrade the implementation contract after call

because the vault admin is permissionless, the protocol owner can create a new implementation and tell the vault admin to upgrade, but if the vault admin refuses to do so, the owner can do nothing, even he remove the implementation, the rogue / outdated implementation can still perform senstive action outlined above

In the worst worst worst worst case,

consider the case

1. a vault admin deploy a new implementation contract, convince the owner admin to addImpl
2. the vault admin upgrade the implementation contract immediately
3. the vault admin calls self-destruct to destroy the implementation contract and change the logic of the implementation contract

https://rekt.news/tornado-gov-rekt/

The proposal contract itself was published via a deployer contract. By combining CREATE and CREATE2 opcodes, the attacker could take advantage of their deterministic deployment to deploy new code into the address approved by governance.

The proposal contract itself was published via a deployer contract. By combining CREATE and CREATE2 opcodes, the attacker could take advantage of their deterministic deployment to deploy new code into the address approved by governance.

4. the vault admin can control the malicious implementation contract and can do whatever they, they can just call self-destruct to lock user fund
5. even the protocol owner remove the implementation, it is too late, because lack of validation

vaultImplementations[IVaultVersion(msg.sender).implementation()

Attachments

1. Proof of Concept (PoC) File

described above

2. Revised Code File (Optional)

do not let vault admin that can be anyone upgrade the implementation contract

whenever, the validation

_vaultsRegistry.vaults(msg.sender)

is called

add the check

vaultImplementations[IVaultVersion(msg.sender).implementation()

as well to make sure the removeImpl function call takes proper effects

[tsudmi]

"convince the owner admin to addImpl" - the owner of vaultsRegistry is DAO, so every addImpl call will go through the DAO voting.

[JeffCX]

"convince the owner admin to addImpl" - the owner of vaultsRegistry is DAO, so every addImpl call will go through the DAO voting.

emm the core issue that report wants to convey is the function removeVaultImpl does not really do what it is supposed to do

it is supposed to block calling crucial function from the outdated / rogue implementation

also even every addImpl is approved by DAO and and the new implementation is added

the vault admin is still decline the upgrade as I pointed out in the report

[JeffCX]

If we refers to the severity level docs

https://app.hats.finance/audit-competitions/stakewise-0xd91cd6ed6c9a112fdc112b1a3c66e47697f522cd/scope

this is at least low severity issue

Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk.

but I think the medium impact fits well

Attacks that make essential functionality of the contracts temporarily unusable or inaccessible

in the case even when the removed implementation still have access to important function that such burn share or harvest and even when the vault admin declines to upgrade the implementation

[JeffCX]

@tsudmi

[tsudmi]

hi @JeffCX The harvest, approve validator are isolated to the vault, so it doesn't affect other vaults or osETH. The burn osToken shares we can't block because it means user never gets his ETH back if something happens to DAO. The only functionality that won't be accessible is minting new osETH.