Skip to content

Conversation

@kinrezC
Copy link
Collaborator

@kinrezC kinrezC commented Oct 27, 2025

Description

What does this PR do? (Brief summary of the changes)

Motivation

Why are these changes needed? (Problem being solved or goal being achieved)

Changes

What specific modifications were made? (List of files, features, or components affected)

int256 liquidityDelta = params.liquidityDelta;

if (liquidityDelta != 0) {
if (storedPosition.salt == bytes32(0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since getPosition[poolId] has its salt set as _fullRangeSalt(poolId) in _afterInitialize, this should always return false. Same with a few other similar checks throughout this contract. In general, note that this pattern means that we assume there's only ever one full range liquidity position and if any other positions are added, they will be included in the same storedPosition since getPosition only tracks one position per poolId

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kinrezC wanna clarify, do we intend to just have one position here? Or should we allow multiple and then maybe do a 3D mapping or something for getPosition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be one position per poolId

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this condition should still always return false. Should be able to remove this whole if block

position.salt = _fullRangeSalt(poolId);
}

getFees[poolId] = Fees({ fees0: 0, fees1: 0, customFee: 3000 });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we currently set a customFee of 3000 for every pool so it could be optimized by using a constant instead. But if we expect to make it dynamic later this makes sense

if (zeroForOne && guess > fees0) return simulation;
if (!zeroForOne && guess > fees1) return simulation;

try quoter.quoteSingle(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kinrezC where is this quoteSingle function from?

Copy link
Collaborator Author

@kinrezC kinrezC Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured this would be better since we're effectively ignoring the before/afterswap in the call context here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know whether it's been audited? Would be good to get it reviewed if it hasn't been yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it has - we can probably swap it out for the official view quoter from uniswap

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you mean this one, note that the readme says it hasn't been audited yet: https://github.com/Uniswap/view-quoter-v3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wrote the v3 one and i believe i fuzzed it against the current quoter: https://github.com/Uniswap/view-quoter-v3/blob/master/test/Quoter.t.sol. the surface area is mostly copying the internals of v3

probably want to audit it tho cause its a useful gas optimization when simming trades onchain.

Comment on lines +348 to +350
if (!best.success || _score(sim.excess0, sim.excess1) < _score(best.excess0, best.excess1)) {
best = sim;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach for determining the best sim doesn't always work. For example, if we swap enough tokens in one sim that we end up with a larger excess in the other token, just comparing the quantities will not work since the tokens have different values

I think the best option would be to use the price to ensure that both tokens have the same denomination, but I still need to think about this further to consider whether there may be any side effects

} else {
if (low == guess) {
if (high <= guess + 1) break;
low = guess;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant to set low = guess here since they're already equal

Same for the other low == guess conditional below

Comment on lines 70 to 71
// TODO: factor in decimals haha 1e6 maybe for 18 decimals? idk haha
uint128 constant EPSILON = 1e6;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just flagging this to point to: #445 (comment)

Deciding on a good EPSILON value is going to be very market specific. Not only does it depend on the decimals of the tokens, but also the value. It might even be necessary to have a different EPSILON for each token

If it's too high then it can be sandwiched, but if it's too low then we swap dust amounts excessively frequently


if (shouldSwap && swapAmountIn > 0) {
Currency outputCurrency = zeroForOne ? key.currency1 : key.currency0;
if (IERC20(Currency.unwrap(outputCurrency)).balanceOf(address(poolManager)) > swapAmountOut) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of this condition can be a bit misleading as there may be sufficient tokens in the pool manager, but that amount might be spread across multiple pools so it doesn't necessarily validate that we have enough tokens in the pool

This should be redundant regardless though since the swap simulation logic should be accurate except for maybe a rounding error and we don't have a risk of revert since we don't enforce the output amount

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some weird accounting error in all of this -- when i remove this check i cannot get the thing to work. I think there's an issue where on the first swap there isn't liquidity to execute the swap in the desired direction. So if I have a test where i execute multiple swaps in a row everything works, but if its just one swap there's a false positive because this component of the hook never executes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has to do with the settlement lifecycle and when tokens actually land in the poolmanager.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this makes sense, we try to take the tokens before the user performing the swap necessarily settled the tokens, so the pool manager tries to swap tokens that it doesn't hold, right?

Will think a bit more about whether there's a cleaner solution to this

Comment on lines +428 to +434
uint128 liquidityDelta = LiquidityAmounts.getLiquidityForAmounts(
sqrtPriceX96,
TickMath.getSqrtPriceAtTick(position.tickLower),
TickMath.getSqrtPriceAtTick(position.tickUpper),
amount0,
amount1
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've had issues with computing liquidity amounts where the actual amount is insufficient when we go to deposit due to the fact that modifyLiquidity will round up the amounts

Would recommend decrementing amount0/amount1 by one (assuming they're non-zero) to be safe

* addition in a Uniswap V4 pool
* @custom:security-contact [email protected]
*/
contract UniswapV4MulticurveRehypeInitializerHook is BaseHook {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to note for this contract, which I figure is probably intentional, is that any rehypothecated liquidity cannot be removed since it's owned by the hook contract and there's no logic to withdraw it

@clemlak clemlak added the 💡 feature Any extra missing or new features label Nov 14, 2025
poolKey.toId(),
asset,
numeraire,
address(1), // temp hardcode to address(1) for testing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder to update this before finalizing

Comment on lines +657 to +660
Currency.wrap(getPoolInfo[poolId].asset)
.transfer(INITIALIZER, isToken0 ? hookFees.beneficiaryFees0 : hookFees.beneficiaryFees1);
Currency.wrap(getPoolInfo[poolId].numeraire)
.transfer(INITIALIZER, isToken0 ? hookFees.beneficiaryFees1 : hookFees.beneficiaryFees0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would recommend validating that the amount to send is non-zero as some tokens will revert on zero value transfers, which will block the other token from getting transferred

uint256 balance1 = getHookFees[poolId].fees1;

if (balance0 <= EPSILON && balance1 <= EPSILON) {
return (BaseHook.afterSwap.selector, delta.amount0());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since afterSwapReturnDelta is set to false in getHookPermissions, we'll always default to a zero delta here

@kinrezC what's the intention behind including this return delta?

if (numeraireBuybackAmount > 0) {
Currency outputCurrency = isToken0 ? key.currency1 : key.currency0;
// must check poolManager balance or else this will revert on failed transfer due to missing tokens
if (IERC20(Currency.unwrap(outputCurrency)).balanceOf(address(poolManager)) > numeraireBuybackAmount) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we check if the pool manager balance of the numeraire token is greater than the numeraireBuybackAmount, but the numeraireBuybackAmount is an amount of asset tokens being input for the swap and not the amount of numeraire tokens we expect to get out

(uint160 sqrtPriceX96,,,) = poolManager.getSlot0(poolId);

Position storage position = getPosition[poolId];
_initializePositionIfNeeded(position, key, poolId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be redundant since we initialized the position already in _afterInitialize

MigrationMath.computeDepositAmounts(fees0, fees1, sqrtPriceX96);

if (depositAmount0 > fees0) {
(, depositAmount1) = MigrationMath.computeDepositAmounts(fees0, depositAmount1, sqrtPriceX96);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think recomputing the deposit amount here is redundant since depositAmount1 only depends on fees0/balance0:

depositAmount1 = balance0.mulDiv(ratioX128, 1 << 128);

Same for computing depositAmount0 in the branch below

Comment on lines +327 to +331
getHookFees[poolId].beneficiaryFees0 += uint128(balance0);
getHookFees[poolId].beneficiaryFees1 += uint128(balance1);

getHookFees[poolId].fees0 = 0;
getHookFees[poolId].fees1 = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to consider is that using this pattern of doing buybacks before rehypothecating and the collecting the remainder as the beneficiary fees means that depending on the params used we might rehypothecate very rarely. To elaborate, any time the fees exceed EPSILON, we do buybacks before rehypothecating only if we still have enough excess fees, then if we don't rehypothecate, we just collect the remainder as beneficiary fees. So we only ever actually rehypothecate if we exceed the EPSILON amount in one swap by enough that we can cover buybacks and still have enough remaining to also swap/rehype

return (BaseHook.afterSwap.selector, 0);
}

function _rebalanceFees(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing a binary search to determine the amount to swap, could we use the current price to compute 50% of the value of the excess amount and just use that as the swap? I would expect that we could get quite accurate doing this

Price impact will be relevant but it should be quite limited considering how small the amounts we're swapping are

I think this could be a much better approach given the gas cost and risk surface of having all this extra logic

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about the exitLiquidity state makes this a bit more interesting in terms of price impact though. @kinrezC, should the liquidity from the curve that's exited go back into the same pool as a full range position? Because if not then there will be very little liquidity and price impact will be significant

Comment on lines +322 to +323
balance0 = zeroForOne ? balance0 - (amount0Added + swapAmountIn) + remainder0 : balance0 + remainder0;
balance1 = zeroForOne ? balance1 + remainder1 : balance1 - (amount1Added + swapAmountIn) + remainder1;
Copy link
Collaborator

@kadenzipfel kadenzipfel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an implicit assumption in this logic that the swap output token did not have any balance prior to the swap. To elaborate, for the swap input token we subtract by (amount0Added + swapAmountIn), removing both the amount added as liquidity and the amount swapped from the accounted balance, but for the swap output token we don't do this. I would expect that the swap output token would subtract (amount0Added - swapAmountOut), which is zero only if the amounts were equal, i.e. if lpAmount0 was zero before adding swapAmountOut

Suggested change
balance0 = zeroForOne ? balance0 - (amount0Added + swapAmountIn) + remainder0 : balance0 + remainder0;
balance1 = zeroForOne ? balance1 + remainder1 : balance1 - (amount1Added + swapAmountIn) + remainder1;
balance0 = zeroForOne ? balance0 - (amount0Added + swapAmountIn) + remainder0 : balance0 - (amount0Added - swapAmountOut) + remainder0;
balance1 = zeroForOne ? balance1 - (amount1Added - swapAmountOut) + remainder1 : balance1 - (amount1Added + swapAmountIn) + remainder1;

This will lead to insolvency due to accounting too high of an amount to be collected by the beneficiaries, which either blocks fee claiming or takes from another pool's token balance

Would recommend including an invariant test that the contract balances are always greater than or equal to the sum of beneficiaryFees0/1 and fees0/1 for all pools

Comment on lines +463 to +480
(BalanceDelta balanceDelta,) = poolManager.modifyLiquidity(
key,
IPoolManager.ModifyLiquidityParams({
tickLower: position.tickLower,
tickUpper: position.tickUpper,
liquidityDelta: int256(uint256(liquidityDelta)),
salt: position.salt
}),
new bytes(0)
);

_settleDelta(key, balanceDelta);
_collectDelta(key, balanceDelta);

position.liquidity += liquidityDelta;

amount0Added = uint256(uint128(-balanceDelta.amount0()));
amount1Added = uint256(uint128(-balanceDelta.amount1()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the pool has a non-zero LP fee, which we allow in the initializer, any fees accrued are only collected in this function and are reduced from the balanceDelta. In most cases this doesn't pose an issue as the fees will be deducted from amount0/1Added and ultimately will just be paid out to the beneficiaries. But in case the accrued fees are ever larger than the balanceDelta, for either token, we'll have a positive balanceDelta.amount0/1 value which will cause a revert when we try to negate and cast it to a uint

Worth considering also whether we want all LP fees to go to the beneficiaries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💡 feature Any extra missing or new features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants