Liquidity Providers would lose their deposits during withdrawals

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L292

Vulnerability details

Impact

Detailed description of the impact of this finding. When a user wants to the withdraw an amount of shares, the user is expecting to receive XTotalDeposit+XTotal Amount of fees accrued(where x=Shares/TotalShares).When the function withdraw() is called the callflow goes like this:withdraw(X)->decreaseLiquidity(), after decrease liquidity is called the token0 and Token1 are sent to the contract, but the issue here is that there is no implementation that sends the received tokens to the caller, the user only receives Xfees.This would lead to loss of funds because picture a case where the Tokens are being redeemed without distribution to the owners, it would put LP's at heavy lossess. function withdraw( uint256 lp, uint256 amount0Min, uint256 amount1Min ) external nonReentrant returns (uint256 removed0, uint256 removed1) { claimFee(); uint removedLiquidity = (uint(liquidity) lp) / totalSupply(); (removed0, removed1) = POS_MGR.decreaseLiquidity( INonfungiblePositionManager.DecreaseLiquidityParams({ tokenId: tokenId, liquidity: uint128(removedLiquidity), amount0Min: amount0Min, amount1Min: amount1Min, deadline: block.timestamp }) //@audit-issue Liquidity is burnt but the base Amount and quote amount isn't returned back to the user ); liquidity = uint128(uint256(liquidity) - removedLiquidity); //@audit-issue :The approach being used in calculating //A better approach would be to collect all fees and distribute the fees based on the amount of liquidity to the totalFees accrued POS_MGR.collect( INonfungiblePositionManager.CollectParams({ tokenId: tokenId, recipient: msg.sender, amount0Max: uint128(removed0), amount1Max: uint128(removed1) }) ); // Handle uncompounded fees if (fee0 > 0) { TOKEN0.token.safeTransfer(msg.sender, (fee0 lp) / totalSupply()); removed0 += (fee0 lp) / totalSupply(); fee0 -= (fee0 lp) / totalSupply(); } if (fee1 > 0) { TOKEN1.token.safeTransfer(msg.sender, (fee1 lp) / totalSupply()); removed1 += (fee1 lp) / totalSupply(); fee1 -= (fee1 lp) / totalSupply(); } _burn(msg.sender, lp); emit Withdraw(msg.sender, lp); }

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used:Deus Vult

Recommended Mitigation Steps:Add mechanism to transfer the amount of Tokens back to the LP

i.e TOKEN0.token.safeTransfer(msg.sender,removed0)
TOKEN1.token.safeTransfer(msg.sender,removed1)

Assessed type

Token-Transfer

[c4-pre-sort]

141345 marked the issue as low quality report

[141345]

invalid

collect() did the transfer

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Insufficient quality