cu5t0mPe0 - setModule is not compatible with ValantisHOTModulePublic.

[sherlock-admin3]

cu5t0mPe0

high

setModule is not compatible with ValantisHOTModulePublic.

Summary

After resetting the new module, since notFirstDeposit is false, the manager will mistakenly treat the amount in the pool as the dust amount. Additionally, new users will use incorrect reserve values to add liquidity.

Vulnerability Detail

When setting a new module, the executor first transfers the amount from the pool to the new module and when module created initlize notFirstDeposit to false. Then it calls initializePosition to transfer the amount to the pool, but notFirstDeposit remains false.

This results in the module mistakenly treating the amount in the pool as the dust amount when a user makes a deposit. The module will incorrectly send the amount to the manager. If the amount in the pool exceeds the _init amount, a user can acquire a large number of shares with a very small deposit.

Even if the manager later redistributes the amount based on shares or reinvests it into the pool, the attacker can still profit.

Consider the following scenario:

1. The malicious executor calls setModule to reset to a new module, then calls initializePosition to reinvest the funds into the SovereignPool.

2. The malicious executor immediately calls ArrakisMetaVaultPublic.mint to initiate a deposit. Since notFirstDeposit is false, ValantisModulePublic.deposit will assume the amounts of token0 and token1 in the SovereignPool are _init0 and _init1, respectively.

3. If the total amount in the SovereignPool is much greater than _init0 and _init1, the attacker can acquire a large number of shares with a very small deposit. At this point, even if the attacker mints shares equal to twice the supply, the amount spent will only be 2 * _init0/_init1.

4. Additionally, ValantisModulePublic.deposit will withdraw all amounts from the pool and send them to the manager.

              (uint256 _amt0, uint256 _amt1) = pool.getReserves();
   
              if (!notFirstDeposit) {
                  if (_amt0 > 0 || _amt1 > 0) {
                      // #region send dust on pool to manager.
   
                      address manager = metaVault.manager();
   
                      alm.withdrawLiquidity(_amt0, _amt1, manager, 0, 0);
   
                      // #endregion send dust on pool to manager.
                  }
   
                  _amt0 = _init0;
                  _amt1 = _init1;
                  notFirstDeposit = true;
              }

5. Now, the manager has discovered that there is a significant increase in funds in the account and has identified the reason. They want to distribute it back to users based on their share ratio or return it to the pool. Attackers with a large share can thus gain a corresponding amount of money.

Impact

Everyone loses their amount and the attacker can profit from it

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L133-L151

Tool used

Manual Review

Recommendation

Override setModule in ValantisHOTModulePublic.sol to set notFirstDeposit to true before calling super.setModule.

Duplicate of #27

[cu5t0mPeo]

escalate The state variable notFirstDeposit is not considered to be set to true when setModule is called, which can lead to users losing significant amounts of funds. Many other Watsons have also noticed this issue.

[sherlock-admin3]

escalate The state variable notFirstDeposit is not considered to be set to true when setModule is called, which can lead to users losing significant amounts of funds. Many other Watsons have also noticed this issue.

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.

[cu5t0mPeo]

dups https://github.com/sherlock-audit/2024-03-arrakis-judging/issues/25

[0xjuaan]

Escalate

I believe this and #27 should be duped with #25, with either this submission or #27 being the primary issue.

The main root cause in the 3 submissions (#14, #25, #27) is that notFirstDeposit is not set to true when setModule and initialisePosition is called.

[sherlock-admin3]

Escalate

I believe this and #27 should be duped with #25, with either this submission or #27 being the primary issue.

The main root cause in the 3 submissions (#14, #25, #27) is that notFirstDeposit is not set to true when setModule and initialisePosition is called.

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]

This is indeed a duplicate of #27. If #27 will be accepted and a new issue family is made, then this will also be accepted and duplicated with #27. If not, and this ends up invalid, then the escalation will be rejected.

[WangSecurity]

@cu5t0mPeo @0xjuaan to clarify:

The malicious executor calls setModule to reset to a new module, then calls initializePosition to reinvest the funds into the SovereignPool

This is about malicious executor calling rebalance with malicious payloads to call setModule?

[WangSecurity]

no wait, it's malicious executor calling setModule with malicious payloads?

[cu5t0mPeo]

no wait, it's malicious executor calling setModule with malicious payloads?

No The key issue here is not malicious payloads. The critical problem is that in setModule, notFirstDeposit does not change the state variable after calling initializePosition, therefore the issue persists. initializePosition is a function that needs to be called in the normal flow of setModule and it is responsible for transferring the balance from the old pool to the new pool.

[0xjuaan]

@WangSecurity no malicious payload. It's a mistake in the code which leads to all funds incorrectly sent to manager after setModule is called.

[WangSecurity]

Thank you for these clarifications. The decision remains the same, for a detailed answer you can see here

Planning to accept the escalation and duplicate with #27 with high severity.

[WangSecurity]

Result: High Duplicate of #27

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cu5t0mPeo: accepted
• 0xjuaan: accepted