cergyk - ArrakisMetaVault::setModule Malicious executor can drain the vault by calling withdraw after initializePosition

[sherlock-admin3]

cergyk

high

ArrakisMetaVault::setModule Malicious executor can drain the vault by calling withdraw after initializePosition

Summary

A malicious executor, after a new module is whitelisted by an admin, can drain the vault by calling ArrakisStandardManager.setModule() with a malicious payloads_.

Vulnerability Detail

The setModule() function of the ArrakisStandardManager contract allows an executor to set a new module for a vault: ArrakisStandardManager.sol#L438.

It calls ValantisModule.withdraw(), to withdraw the funds from the pool and transfer to the new module: ValantisHOTModule.sol#L235.

Once the vault's funds are in the new module, the first malicious payloads_ is executed and calls ValantisModule::initializePosition to deposit the liquidity back into the pool: ValantisHOTModule.sol#L148.

Lastly, the second payloads_ of the malicious executor gets executed and calls ValantisModulePublic::withdraw which calls ValantisModule::withdraw on the new module and withdraws all funds to an arbitrary address: ValantisHOTModule.sol#L203.

Impact

Theft of all funds in the vault.

Scenario

1. Owner whitelists a legitimate new module with ArrakisMetaVault::whitelistModules.
2. Malicious public vault executor calls ArrakisStandardManager::setModule with a malicious payloads_.
   1. It calls ArrakisMetaVault::setModule.
   2. It calls ValantisModulePublic::withdraw which calls _module.withdraw(module_, BASE) to withdraw all vault's funds from old to new module.
   3. Malicious executor has provided two payloads
      1. The first one calls ValantisModule::initializePosition to deposit liquidity into the new pool.
      2. Then the second malicious payloads_ is executed and contains a call to ValantisModule::withdraw with attacker as the receiver

Please note that instead of calling withdraw on the new module, enabling direct theft of funds, the executor can also call withdrawManagerBalance which would transfer all the balance of the module to the manager. Or also not call any function at all (payloads is an empty array), and leave the balances in the module.

Code Snippet

• ArrakisMetaVault.sol#L130

Tool used

Manual Review

Recommendation

Check that the reserves in the new pool are correct after setting the new module, similarly to what is currently done at the end of ArrakisStandardManager::rebalance: https://github.com/sherlock-audit/2024-03-arrakis/blob/d7946ee784ca8df3246d723e8b92529447e23bb7/arrakis-modular/src/ArrakisStandardManager.sol#L391-L414

[0xjuaan]

Hi @0xffff11, #45 is a completely different issue and should not be duped with this one

[WangSecurity]

Tagging here the Watsons who submitted and escalated their findings about malicious executor exploiting the protocol via setModules in ArrakisStandardManager:

@CergyK @0xjuaan @cu5t0mPeo @iamandreiski (I assume there might be more, please tag everyone I've missed)

Here're the escalated findings:

• 35

• 21

• 27

• 14

• 45

• if there are any missing please also send them in this discussion.

Gentlemen, I've got you all here, as we all know in the README it says the executor for Arrakis Standard Manager (let's call it ASM) is RESTRICTED during rebalance actions. These vulnerabilities above are about exploiting setModules, so as I understand, the executor in that case is TRUSTED. So I've got three questions here:

1. Is there any evidence that the sponsors agree they're untrusted and restricted for setModules or it's only for rebalance indeed?
2. Are your submissions possible via rebalance or only via setModules?

3. 27 and #14 are dups of each other and as I understand they're about the problem with new modules being incorrectly set, but both mention malicious executor, which is trusted in that case. Are they possible with a TRUSTED executor and without a mistake?

Let's keep the discussions here and then, depending on the answers to these questions, we could move the talks to issues individually.

[cu5t0mPeo]

@WangSecurity Hi, here is the sponsor's explanation about the executor in my private thread: "When the executor is calling setModule, he will interact with the module through the vault. The module functions exposed to the vault should be safely implemented. In this case, the executor isn't trusted, so the module's exposed function to setModule should be safe."

[0xjuaan]

Furthermore, it does not make sense for an actor to be trusted to use one function non-maliciously, but untrusted for a different function.

Sponsor's intention is confirmed here:

[0xjuaan]

Also @WangSecurity, #14 and #27 do not involve any special malicious actions. Its a mistake in the code. Whenever setModule is called (with perfectly correct payloads), all the funds are sent to the ArrakisStandardManager incorrectly.

[WangSecurity]

Thank you for these clarifications.

Furthermore, it does not make sense for an actor to be trusted to use one function non-maliciously, but untrusted for a different function.

Fair enough, still wanted to get a confirmation. Let's proceed to each individual issue.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/ArrakisFinance/arrakis-modular/pull/88

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.