iamandreiski - Executor can steal all funds from public vault when a new module is set

[sherlock-admin2]

iamandreiski

high

Executor can steal all funds from public vault when a new module is set

Summary

After a new module has been whitelisted (which is a timelocked process), the next step is for the executor role of the given public vault to call setModule() on the ArrakisStandardManager in order to set the new whitelisted module. The problems which cause this attack scenario to be possible are:

• setModule() being called by the executor isn't a timelocked operation, i.e. it will be executed immediately.
• The payload which the executor supplies as a function argument isn't validated anywhere against the already approved payload, and can be malicious and used to steal and transfer funds to an executor operated address.

Vulnerability Detail

When the vault owner wants to whitelist a new module, the step-by-step flow would be:

• Calling whitelistModules on the public vault:

  
  function whitelistModules(
      address[] calldata beacons_,
      bytes[] calldata data_
  ) external onlyOwnerCustom {
      uint256 len = beacons_.length;
      if (len != data_.length) revert ArrayNotSameLength();
  
      address[] memory modules = new address[](len);
      for (uint256 i; i < len; i++) {
          address _module = IModuleRegistry(moduleRegistry)
              .createModule(address(this), beacons_[i], data_[i]);
  
          modules[i] = _module;
  
          _whitelistedModules.add(_module);
      }
  
      emit LogWhiteListedModules(modules);
  }

- When `createModule()` is called, it's a timelocked operation, i.e. there will be a two day delay in which the governance of the project can take action if something is suspicious. 
- After the timelock passes and this is executed, it will check if the beacons are approved when calling `createModule()`and if they are it will continue to add the module in the whitelisted modules.
- The step following next, would be for the executor to call `setModule()` on the Manager contract which proceeds to call `setModule` on the public vault.
- The problem which arises from here is that the executor's actions are no timelocked, i.e. this function will be executed immediately:

```solidity

    function setModule(
        address vault_,
        address module_,
        bytes[] calldata payloads_
    ) external whenNotPaused onlyWhitelistedVault(vault_) {
        if (vaultInfo[vault_].executor != msg.sender) {
            revert NotExecutor();
        }

        IArrakisMetaVault(vault_).setModule(module_, payloads_);

        emit LogSetModule(vault_, module_, payloads_);
    }

• The address of the module will have to be a whitelisted one for the given vault, but the payloads can be arbitrary data, as its legitimacy is never checked:

  function setModule(
        address module_,
        bytes[] calldata payloads_
    ) external onlyManager nonReentrant {
        // store in memory to save gas.
        IArrakisLPModule _module = module;

        if (address(_module) == module_) revert SameModule();
        if (!_whitelistedModules.contains(module_)) {
            revert NotWhitelistedModule(module_);
        }

        module = IArrakisLPModule(module_);

        // #region withdraw manager fees balances.

        _withdrawManagerBalance(_module);

        // #endregion withdraw manager fees balances.

        // #region move tokens to the new module.

        /// @dev we transfer here all tokens to the new module.
        _module.withdraw(module_, BASE);

        // #endregion move tokens to the new module.

        // #region check if the module is empty.

        // #endregion check if the module is empty.

        uint256 len = payloads_.length;
        for (uint256 i = 0; i < len; i++) {
            (bool success,) = module_.call(payloads_[i]);
            if (!success) revert CallFailed();
        }
        emit LogSetModule(module_, payloads_);
    }

• After setModule() on the Manager calls the setModule() function on the public vault, it will check if the module address is whitelisted, and if it is, it will withdraw the manager balance and transfer all funds from the old module to the new one.
• After the above step is completed, it will start executing the executor-provided payloads (as a function argument) on the new module.
• The problem is that payloads can be arbitrary data which is malicious and can instruct it to withdraw all balances to an executor owned and controlled address.

Impact

Executor can steal all public vault funds when a new module is being set.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ArrakisMetaVault.sol#L97-L134 https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisStandardManager.sol#L429-L441

Tool used

Manual Review

Recommendation

Either check the payload data before its executed to make sure that it's not malicious, and/or timelock the executor role as well.

Duplicate of #50

[iamandreiski]

@0xffff11 Hi, I believe this is a duplicate of #50 as well, together with the #21 as it refers to the same issue.

[0xjuaan]

Escalate

This is a valid dupe of #50

[sherlock-admin3]

Escalate

This is a valid dupe of #50

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

Agree with escalation and planning to duplicate with #50

[WangSecurity]

Result: High Duplicate of #50

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: accepted