[Lines of code](https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L335) # Vulnerability details ## Impact Protocol currently uses 2 levels of callbacks for burning Pool liquidity: **Inner callback** \- timeswapV2PoolBurnChoiceCallback function in [Line 438 of Pool.sol]() allows user to specify long0 & long1 amount such that long0 + long1 < longAmount. longAmount here refers to the amount of long tokens calculated by Constant Product AMM to be returned to user for burning a specifc amount of pool liquidity. This check is done by checkEnough function in line 450. Purpose of this callback is to provide users a choice to decide composition of long0 & long1 (protocol only cares about the sum of tokens & not individual levels). **Outer callback** \- the long0, long1, short tokens returned by pool.mint function are then passed to second callback in [Line 343 of TimeswapV2Pool.sol](). Note that pool also transfers long0 and long1 tokens to the user. Problem here is that if user passes a lower number of long0 and long1 tokens such that long0 + long1 < long, proportion of long tokens returned to the user will be inconsistent with the liquidity burnt by the pool. While the liquidity burnt is consistent with the constant AMM calculation corresponding to long amount, actual tokens sent by the pool are the lower numbers chosen by user in inner callback. This inconsistent leads to a direct loss of tokens for users when they choose lower values of long0 & long1 tokens. Not all users will be sophisticated to calculate the exact amount of tokens - a conservative estimate by users will result in excess burn of liquidity tokens. Due to a potential loss of user funds, I've marked this as HIGH risk ## Proof of Concept * Bob requests a burn of 100 LP tokens. Constant Product AMM shows pool needs to send 50 Long tokens & 2 Short tokens back to user * Bob chooses 40 Long1 tokens, 0 Long2 tokens -> this satisfies the callback sanity check (long1 + long0 (40) < long(50)) * Pool then expects Bob to transfer 100 LP tokens for burning * Pool returns Bob 40 Long0, 0 Long1 & 2 Short tokens Notice that while 100 LP tokens are burnt, only 40 Long0 tokens are received back. 10 Long0 tokens belonging to Bob are never received. ## Tools Used Manual ## Recommended Mitigation Steps Liquidity calculated in the Pool.burn function should be scaled down proportionately if lesser sum of long0 + long1 tokens are sent by the user. Just like liquidity, shortAmount should be adjusted downwards proportionate to the shortfall This will ensure user does not lose LP tokens without receiving proportionate long tokens in return Recommend the following code block after line 540 in Pool.sol Error.checkEnough(longAmount, StrikeConversion.combine(long0Amount, long1Amount, param.strike, true)); //************ RECOMMEND THIS CODE BLOCK ************** uint256 longSum = StrikeConversion.combine(long0Amount, long1Amount, param.strike, false); uint256 shortFall = longAmount - longSum; uint256 scalingFactor; if(excess > 0){ scalingFactor = FullMath.mulDiv(shortFall, uint256(1) << 16, longAmount); liquidityAmount = FullMath.mulDiv(liquidityAmount, (uint256(1) << 16).unsafeSub(scalingFactor), uint256(1) << 16 ); shortAmount = FullMath.mulDiv(shortAmount, (uint256(1) << 16).unsafeSub(scalingFactor), uint256(1) << 16 ); } //*********************************************** if (long0Amount != 0) pool.long0Balance -= long0Amount; if (long1Amount != 0) pool.long1Balance -= long1Amount; pool.liquidity -= liquidityAmount; --- The text was updated successfully, but these errors were encountered: All reactions