search

sherlock-audit / 2023-06-real-wagmi-judging

3 stars 2 forks source link

stopthecap - Divergence in calculating lp amounts will cause an imbalance depositing and withdrawing #166

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

stopthecap

high

Divergence in calculating lp amounts will cause an imbalance depositing and withdrawing

Summary

Divergence in calculating lp amounts will cause an imbalance depositing and withdrawing

Vulnerability Detail

The interaction between the multipool contract and the dispacher to withdraw and deposit the lp tokens from the pool contract has a faulty logic that causes the wrong amount of lp tokens being subtracted from the users balance.

To send their lp tokens to the dispacher, users have to first deposit independently in the multipool. The calculation of the lp tokens that they receive is as follows:

They multiply the amounts by the total supply and divide by the reserves of each token, then the lp amount of tokens minted, will be the smaller of both

         uint256 l0 = (amount0Desired * _totalSupply) / reserve0; 
           uint256 l1 = (amount1Desired * _totalSupply) / reserve1; 
           lpAmount = l0 < l1 ? l0 : l1;

---------------------------------
_mint(msg.sender, lpAmount);

Once a user deposits on the lp tokens on the dispacher, the calculation of the lp tokens to burn, it is different to the one from the multipool. causing an error in the accounting:

lpAmount = _estimateWithdrawalLp(reserve0, reserve1, _totalSupply, fee0, fee1);
function _estimateWithdrawalLp(
        uint256 reserve0,
        uint256 reserve1,
        uint256 _totalSupply,
        uint256 amount0,
        uint256 amount1
    ) private pure returns (uint256 shareAmount) {
  shareAmount =
            ((amount0 * _totalSupply) / reserve0 + (amount1 * _totalSupply) / reserve1) / 2;
}

multiplying both supplies by the amounts, dividing by their reserves, and dividing by 2.

Therefore, you will mint the smallest amount of both calculations of lp tokens and you will burn the average amount of both calculations.

Impact

Loss of lp tokens due to an error accounting for their amounts in between dispacher and multipool

Code Snippet

Tool used

Manual Review

Recommendation

Use the exact same calculation to determine the shares to mint/burn and substract the users

0xffff11 commented 1 year ago

I think the report clearly shows how the calculations differ from each other and shows the impact of it. This should be a high imo.

huuducsc commented 1 year ago

Escalate

I believe this issue is invalid and it is a mislead of the Watson.

  1. The Multipool.deposit function mints an lpAmount that represents the minimum LP shares from amount0Desired and amount1Desired. However, they are obtained from the _optimizeAmounts() function, which calculates them based on the ratio of reserve0 and reserve1. This implies that the difference between both LP shares (l0 and l1) is <= 1 (rounding error).

    (amount0Desired, amount1Desired) = _optimizeAmounts(
amount0Desired,
amount1Desired,
amount0Min,
amount1Min,
reserve0,
reserve1
);
// MINIMUM
uint256 l0 = (amount0Desired * _totalSupply) / reserve0;
uint256 l1 = (amount1Desired * _totalSupply) / reserve1;
lpAmount = l0 < l1 ? l0 : l1;

  2. The Dispatcher.deposit function only withdraws the growth fees of the user from the Multipool contract by converting these fee tokens (2 tokens in the UniswapV3 pool) to LP shares. _estimateWithdrawalLp is used to convert these fee tokens to LP shares. These fee amounts are not optimized as the Multipool.deposit function. Therefore, it is not related to the depositing process or the calculation of the Multipool.deposit function. They are different mechanisms and should use different calculations.

    if (user.shares > 0) {
  uint256 lpAmount;
  {
      (uint256 fee0, uint256 fee1) = _calcFees(feesGrow, user);
      lpAmount = _estimateWithdrawalLp(reserve0, reserve1, _totalSupply, fee0, fee1);
  }
  user.shares -= lpAmount;
  _withdrawFee(pool, lpAmount, reserve0, reserve1, _totalSupply, deviationBP);
}

Additionally, there is another issue regarding _estimateWithdrawalLp (issue #142), but it is not related to this issue, as this issue does not highlight any problem in _estimateWithdrawalLp

sherlock-admin commented 1 year ago

Escalate

I believe this issue is invalid and it is a mislead of the Watson.

  1. The Multipool.deposit function mints an lpAmount that represents the minimum LP shares from amount0Desired and amount1Desired. However, they are obtained from the _optimizeAmounts() function, which calculates them based on the ratio of reserve0 and reserve1. This implies that the difference between both LP shares (l0 and l1) is <= 1 (rounding error).

    (amount0Desired, amount1Desired) = _optimizeAmounts(
amount0Desired,
amount1Desired,
amount0Min,
amount1Min,
reserve0,
reserve1
);
// MINIMUM
uint256 l0 = (amount0Desired * _totalSupply) / reserve0;
uint256 l1 = (amount1Desired * _totalSupply) / reserve1;
lpAmount = l0 < l1 ? l0 : l1;

  2. The Dispatcher.deposit function only withdraws the growth fees of the user from the Multipool contract by converting these fee tokens (2 tokens in the UniswapV3 pool) to LP shares. _estimateWithdrawalLp is used to convert these fee tokens to LP shares. These fee amounts are not optimized as the Multipool.deposit function. Therefore, it is not related to the depositing process or the calculation of the Multipool.deposit function. They are different mechanisms and should use different calculations.

    if (user.shares > 0) {
  uint256 lpAmount;
  {
      (uint256 fee0, uint256 fee1) = _calcFees(feesGrow, user);
      lpAmount = _estimateWithdrawalLp(reserve0, reserve1, _totalSupply, fee0, fee1);
  }
  user.shares -= lpAmount;
  _withdrawFee(pool, lpAmount, reserve0, reserve1, _totalSupply, deviationBP);
}

Additionally, there is another issue regarding _estimateWithdrawalLp (issue #142), but it is not related to this issue, as this issue does not highlight any problem in _estimateWithdrawalLp

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.

fann95 commented 1 year ago

There is no loss of tokens since when burning, the user receives exactly as many of both tokens as he should. But the function for determining the number of tokens to burn has been completely updated to provide a more correct calculation of the required number of LP https://github.com/RealWagmi/concentrator/blob/fbccf1caf28edbb23db8c7e0409d0c40c3a56461/contracts/Dispatcher.sol#L268

ctf-sec commented 1 year ago

@0xffff11

JeffCX commented 1 year ago

Based on the above discussion, I have to agree with sponsor's comment unless further argument from both side are provided

hrishibhat commented 1 year ago

There is no loss of tokens since when burning, the user receives exactly as many of both tokens as he should.

@ctf-sec Is this a non issue?

hrishibhat commented 1 year ago

Result: Low Unique Considering this a low as there is no loss of funds or breaking functionality in this case, although there is a difference in amounts calculated. the function works as expected. There is no medium impact here.

There is no loss of tokens since when burning, the user receives exactly as many of both tokens as he should

Additional Sponsor comment:

Yes, the function works as expected. The ratio of the accumulated fees in both tokens may not match the ratio of the tokens in the multipool, respectively, in order to accurately calculate the liquidity that needs to be burned, you need to exchange part of one of the tokens for some part of the other to bring them to the ratio in the multipool and accurately calculate the LP tokens for burning.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: