KupiaSec - The rebalance executor can take large amounts of vault shares even without any underlying assets

[sherlock-admin3]

KupiaSec

high

The rebalance executor can take large amounts of vault shares even without any underlying assets

Summary

A malicious rebalance behavior can enable a vault's rebalance executor to acquire large amounts of vault shares.

Vulnerability Detail

Let's consider the following scenario:

1. The current state:
   • Pool: reserve0 = z, reserve1 = 2z
   • Vault: totalSupply = s
   • Oracle: price = 2
2. The malicious rebalance executor of the vault calls the ArrakisStandardManager.rebalance() function with the selector of the ValantisHOTModule.swap() function. In the ValantisHOTModule.swap() function, it is expected that the executor withdraws all assets from the pool, swaps some tokens through the router_ contract, and deposits all assets back into the pool.

            (bool success,) = router_.call(payload_);

3. The parameter router_ is supposed to be the RouterSwapExecutor contract, but the executor can set it to a malicious contract. In the malicious contract, the executor doesn't swap the tokens and instead drains an amount z of token1 and deposits it through the ArrakisPublicVaultRouter.swapAndAddLiquidity() function (swapping z/2 of token1 to z/4 of token0). As a result, during the swap router call, the state becomes:
   • Pool: reserve0 = z/4, reserve1 = z/2
   • Executor's share: s/4
   • Vault: totalSupply = s + s/4 = 5s/4
4. At the end of the ValantisHOTModule.swap() function, the state will become:
   • Pool: reserve0 = z/4 + z = 5z/4, reserve1 = z/2 + z = 3z/2
   • Executor's share: s/4
   • Vault: totalSupply = 5s/4

Through this rebalancing scenario, the executor can unfairly acquire vault shares, and the exchange rate of the pool tokens has changed from 1:2 to 5/4:3/2.

The scenario includes two slippage checks:

1. The maxDeviation check for the price:
   • During the withdraw and deposit operations, the _ammState of the HOT contract is not modified, so the price of the pool remains unchanged, and the maxDeviation check passes.
2. The maxSlippagePIPS check for the total underlying of the vault:
   • The drained token1 is deposited back into the pool by the ArrakisPublicVaultRouter.swapAndAddLiquidity() function, and there is no change in the price, so the total underlying value of the vault remains unchanged.

Impact

A malicious rebalance executor can inequitably acquire a portion of vault shares.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisStandardManager.sol#L322-L421

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L326-L416

Tool used

Manual Review

Recommendation

It is recommended to include a check for the router_ parameter to be the RouterSwapExecutor contract, as well as a check to verify the final exchange rate of the pool's tokens at the end of the ValantisHOTModule.swap() function execution.

[0xjuaan]

Escalate

This issue and #76 do not show sufficient proof to be considered dupes of #44

Summary of vulnerability for reference

Rebalancing has 3 steps: withdraw all funds -> swap -> deposit all funds back

However, instead of swapping in step 2, a malicious executor can use an extremely cheap share price to mint a large amount of shares, which they can withdraw for nearly 100% of the vault's liquidity after step 3 of rebalance() occurs.

Key constraints

There are a few key constraints to this attack:

1. In the middle of the swap() function (after the liquidity has been withdrawn), the attacker must increase the reserves of the pool to make it non-zero. Otherwise, depositing liquidity to mint cheap shares will revert due to SovereignPool__depositLiquidity_zeroTotalDepositAmount

2. In order for the attacker to be able to do the above, at least one of the tokens must be a rebase token, since otherwise the reserves are calculated via storage variables and cannot be updated by sending dust amounts of tokens.

3. expectedMinReturn_ tokens have to be sent into the contract between step1 and 3. And due to _checkMinReturn, amountIn for the swap has to be non-zero, with the lowest possible value being 1. This means the lowest possible value for expectedMinReturn_ is 1 wei, so in between step1,3- the attacker has to send 1 wei worth of tokens into the module to ensure that the check passes.

Without any of these, the attack would fail.

You can find all these constraints detailed in #44

Sherlock rules

Now, the sherlock rules state the following:

PoC is required for all issues falling into any of the following groups:

• non-obvious ones with complex vulnerabilities/attack paths
• issues for which there are non-trivial limitations/constraints on inputs, to show that the attack is possible despite those

Clearly, this vulnerability meets the two above criteria, since it is a complex attack with several requirements for success.

Explanation of how #76 and #55 do NOT show how the attack is possible despite the various constraints:

However, this submission and #76 are lacking the following key features:

1. They do not explain that the attacker must increase the reserves of the pool during the callback so that the minting of shares does not revert
2. They do not explain that at least one of the tokens must be rebase in order for this attack
3. They do not explain that during the callback, expectedMinReturn tokens must be sent into the module during the callback.
4. They do not include a PoC as required by the sherlock rules for a complex vulnerability with various non-trivial limitations and constraints.

Conclusion

Based on the above evidence (lack of explaining key constraints, lack of PoC, incorrect attack paths), it is clear that #76 and #55 are insufficient reports based on sherlock's rules and cannot be duped with #44.

Note that #44 explains ALL the non-trivial limitations/constraints of the issue, and shows how the attack is possible despite those. It also provides a coded PoC that demonstrates 100% of the vault's funds being stolen. On the other hand, the attack paths provided by #76 and #55 only show a smaller portion of the funds being stolen.

[sherlock-admin3]

Escalate

This issue and #76 do not show sufficient proof to be considered dupes of #44

Summary of vulnerability for reference

Rebalancing has 3 steps: withdraw all funds -> swap -> deposit all funds back

However, instead of swapping in step 2, a malicious executor can use an extremely cheap share price to mint a large amount of shares, which they can withdraw for nearly 100% of the vault's liquidity after step 3 of rebalance() occurs.

Key constraints

There are a few key constraints to this attack:

1. In the middle of the swap() function (after the liquidity has been withdrawn), the attacker must increase the reserves of the pool to make it non-zero. Otherwise, depositing liquidity to mint cheap shares will revert due to SovereignPool__depositLiquidity_zeroTotalDepositAmount

2. In order for the attacker to be able to do the above, at least one of the tokens must be a rebase token, since otherwise the reserves are calculated via storage variables and cannot be updated by sending dust amounts of tokens.

3. expectedMinReturn_ tokens have to be sent into the contract between step1 and 3. And due to _checkMinReturn, amountIn for the swap has to be non-zero, with the lowest possible value being 1. This means the lowest possible value for expectedMinReturn_ is 1 wei, so in between step1,3- the attacker has to send 1 wei worth of tokens into the module to ensure that the check passes.

Without any of these, the attack would fail.

You can find all these constraints detailed in #44

Sherlock rules

Now, the sherlock rules state the following:

PoC is required for all issues falling into any of the following groups:

• non-obvious ones with complex vulnerabilities/attack paths
• issues for which there are non-trivial limitations/constraints on inputs, to show that the attack is possible despite those

Clearly, this vulnerability meets the two above criteria, since it is a complex attack with several requirements for success.

Explanation of how #76 and #55 do NOT show how the attack is possible despite the various constraints:

However, this submission and #76 are lacking the following key features:

1. They do not explain that the attacker must increase the reserves of the pool during the callback so that the minting of shares does not revert
2. They do not explain that at least one of the tokens must be rebase in order for this attack
3. They do not explain that during the callback, expectedMinReturn tokens must be sent into the module during the callback.
4. They do not include a PoC as required by the sherlock rules for a complex vulnerability with various non-trivial limitations and constraints.

Conclusion

Based on the above evidence (lack of explaining key constraints, lack of PoC, incorrect attack paths), it is clear that #76 and #55 are insufficient reports based on sherlock's rules and cannot be duped with #44.

Note that #44 explains ALL the non-trivial limitations/constraints of the issue, and shows how the attack is possible despite those. It also provides a coded PoC that demonstrates 100% of the vault's funds being stolen. On the other hand, the attack paths provided by #76 and #55 only show a smaller portion of the funds being stolen.

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.

[KupiaSecAdmin]

Hi, @0xjuaan. Let me clarify your 3 key constraints:

Key constraints

There are a few key constraints to this attack:

1. In the middle of the swap() function (after the liquidity has been withdrawn), the attacker must increase the reserves of the pool to make it non-zero. Otherwise, depositing liquidity to mint cheap shares will revert due to SovereignPool__depositLiquidity_zeroTotalDepositAmount
2. In order for the attacker to be able to do the above, at least one of the tokens must be a rebase token since otherwise the reserves are calculated via storage variables and cannot be updated by sending dust amounts of tokens.
3. expectedMinReturn_ tokens have to be sent into the contract between step1 and 3. And due to _checkMinReturn, amountIn for the swap has to be non-zero, with the lowest possible value being 1. This means the lowest possible value for expectedMinReturn_ is 1 wei, so in between step1,3- the attacker has to send 1 wei worth of tokens into the module to ensure that the check passes.

Without any of these, the attack would fail.

1. The SovereignPool__depositLiquidity_zeroTotalDepositAmount error occurs when one of the deposit amounts is 0, not when the pool's reserves are 0. In my issue, the deposit amounts are not 0. Therefore, this error doesn't occur.
2. When minting, if the pool's reserves are 0, then the share calculation will be based on the default reserve values (init0, init1). Therefore, there is no requirement for the tokens to be rebase tokens, nor is there a need to send dust amounts.
3. In my issue, it mentioned that the malicious router doesn't swap tokens. However, this doesn't mean the amountIn is zero. The malicious executor can set amountIn to a non-zero value and expectedMinReturn_ to 0, allowing it to bypass the _checkMinReturn function. Additionally, it is possible to set the malicious router not to swap tokens even when amountIn is non-zero.

Given these clarifications, I'd like to reiterate that my issue is valid.

[0xjuaan]

Hi @KupiaSecAdmin all 3 of the points provided in the above message are incorrect and I will explain why.

Point 1

You have stated that SovereignPool__depositLiquidity_zeroTotalDepositAmount when one of the deposit amounts is 0, not when the pool's reserves are 0. However if you make the following change in my Coded PoC from #44:

-token0.transfer(pool, 1 wei);
-token1.transfer(pool, 1 wei);
+//token0.transfer(pool, 1 wei);
+//token1.transfer(pool, 1 wei);

This now ensures that no tokens are sent to the pool by the attacker, so the reserves remain as zero.

However now, the PoC fails with the error SovereignPool__depositLiquidity_zeroTotalDepositAmount

Trace

```bash [14694] HOT::depositLiquidity(0, 0, 0, 0) │ │ │ │ │ │ │ │ │ ├─ [1110] MockChainlinkOracle::latestRoundData() [staticcall] │ │ │ │ │ │ │ │ │ │ └─ ← 1, 500000000000000 [5e14], 1694984663 [1.694e9], 1694984663 [1.694e9], 0 │ │ │ │ │ │ │ │ │ ├─ [1110] MockChainlinkOracle::latestRoundData() [staticcall] │ │ │ │ │ │ │ │ │ │ └─ ← 1, 1000000 [1e6], 1694984663 [1.694e9], 1694984663 [1.694e9], 0 │ │ │ │ │ │ │ │ │ ├─ [279] MockChainlinkOracle::decimals() [staticcall] │ │ │ │ │ │ │ │ │ │ └─ ← 18 │ │ │ │ │ │ │ │ │ ├─ [279] MockChainlinkOracle::decimals() [staticcall] │ │ │ │ │ │ │ │ │ │ └─ ← 6 │ │ │ │ │ │ │ │ │ ├─ [4244] SovereignPool::depositLiquidity(0, 0, BeaconProxy: [0xbc5FD75F8Fb428645b8f1eC8A11B75Ae960439Ab], 0x, 0x) │ │ │ │ │ │ │ │ │ │ └─ ← SovereignPool__depositLiquidity_zeroTotalDepositAmount() │ │ │ │ │ │ │ │ │ └─ ← SovereignPool__depositLiquidity_zeroTotalDepositAmount() │ │ │ │ │ │ │ │ └─ ← SovereignPool__depositLiquidity_zeroTotalDepositAmount() │ │ │ │ │ │ │ └─ ← SovereignPool__depositLiquidity_zeroTotalDepositAmount() │ │ │ │ │ │ └─ ← SovereignPool__depositLiquidity_zeroTotalDepositAmount() │ │ │ │ │ └─ ← SovereignPool__depositLiquidity_zeroTotalDepositAmount() ```

This is because the amounts to deposit are calculated via:

amount0 = FullMath.mulDivRoundingUp(proportion_, _amt0, BASE);
amount1 = FullMath.mulDivRoundingUp(proportion_, _amt1, BASE);

But the reserves are 0, so the amount to deposit to the ALM is 0. Hence, the attack is impossible when pool reserves are 0 which is contrary to your claim.

Point 2

This is simply wrong, just read the logic of ValantisHOTModulePublic::deposit()

The init values are only used on the first deposit. Otherwise, when pool reserves are 0, the deposit amounts are calculated to be zero, leading to the revert, causing the attack path in this submission to fail (proven in the PoC of my submission).

Point 3

You have stated

The malicious executor can set amountIn to a non-zero value and expectedMinReturn_ to 0

However this is impossible. Even if the smallest value of amountIn=1 was selected, the _checkMinReturn() will require expectedMinReturn_ to be 1 at minimum. Hence, it is a requirement that the attacker sends these expectedMinReturn_ tokens back into the module during the attack, otherwise the attack will 100% fail. This requirement is not expressed in the report.

Summary

All key constraints that I explained in my escalation and in the issue #44 are critical for this issue, and have not been explained the watson.

Sherlock rules state:

PoC is required for all issues falling into any of the following groups:

non-obvious ones with complex vulnerabilities/attack paths
issues for which there are non-trivial limitations/constraints on inputs, to show that the attack is possible despite those
...
Also, Watsons must outline all constraints of the issue being triggered and specify in which situations these constraints may trigger the issue.

The watson has not outlined the various key constraints and situations in which the issue can be triggered. Hence, this submission should be invalidated and not duped with #44 which properly outlines all the constraints and provides a coded PoC.

[KupiaSecAdmin]

@0xjuaan I see what you mean.

However, the main points of my issue are the malicious router contract and lack of a price slippage check in the withdrawLiquidity and depositLiquidity functions, as detailed in #44 and #76. Since steps like sending dust amounts are not essential main points in this attack, my report could still be considered a valid one.

[0xjuaan]

Since steps like sending dust amounts are not essential main points in this attack,

Sending tokens to the pool mid-attack to inflate reserves, and sending the expectedMinReturn_ tokens to the module ARE essential points in the attack, since the attack is impossible without those actions by the attacker.

Since those non-trivial constraints (and more) were not included in the issue, I believe it should be invalidated according to the sherlock rules provided in my previous comment

[WangSecurity]

In one of the above comments, @0xjuaan quoted a very correct line from POC requirements:

Also, Watsons must outline all constraints of the issue being triggered and specify in which situations these constraints may trigger the issue.

This issue falls into the following two groups:

non-obvious ones with complex vulnerabilities/attack paths issues for which there are non-trivial limitations/constraints on inputs, to show that the attack is possible despite those

Hence, as clearly seen in the rules, all the constraints have to be outlined, doesn't matter if they're essential or not. This report lacks all the constraints for the attack to go through.

Planning to accept the escalation to invalidate this report.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: accepted