Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sakshamguruji - Not The Entire Token Amounts Provided To AMM Might Get Consumed Leading To Incorrect Accounting Of LV Tokens #171

Closed
sherlock-admin2 opened this issue Sep 10, 2024 · 6 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Sep 10, 2024

sakshamguruji

Medium

Not The Entire Token Amounts Provided To AMM Might Get Consumed Leading To Incorrect Accounting Of LV Tokens

Summary

When RA is deposited into the Vault then a part of that RA is used to to mint CT (and DS) and then RA + CT are added as liquidity in the AMM , this is done by first calculating the ratio and then providing assets in that ratio , but it is possible that assets are not provided in the exact ratio which means not all of the assets will be used by UniV2 to be added as liquidity , therefore there will be some left out assets . But these left out assets are not accounted.

Vulnerability Detail

1.) When RA is deposited in the vault , liquidity is added to the AMM in the calculated ratio ->

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/libraries/VaultLib.sol#L204

function __provideLiquidityWithRatio(
        State storage self,
        uint256 amount,
        IDsFlashSwapCore flashSwapRouter,
        address ctAddress,
        IUniswapV2Router02 ammRouter
    ) internal returns (uint256 ra, uint256 ct) {
        uint256 dsId = self.globalAssetIdx;

        uint256 ctRatio = __getAmmCtPriceRatio(self, flashSwapRouter, dsId);

        (ra, ct) = MathHelper.calculateProvideLiquidityAmountBasedOnCtPrice(amount, ctRatio);

        __provideLiquidity(self, ra, ct, flashSwapRouter, ctAddress, ammRouter, dsId);
    }

And inside calculateProvideLiquidityAmountBasedOnCtPrice ->

function calculateProvideLiquidityAmountBasedOnCtPrice(uint256 amountra, uint256 priceRatio)
        external
        pure
        returns (uint256 ra, uint256 ct)
    {
        ct = (amountra * 1e18) / (priceRatio + 1e18);
        ra = (amountra - ct);

        assert((ct + ra) == amountra);
    }

Therefore if amountRa = 33333333333333333333 and ratio was 50e18 , there will be some rounding in the calculated amounts of CT and RA , also the (+ 1e18) in the denominator can will cause some results which will be not necessarily follow the ratio(that's because we must convert the RA to CT, imagine if it would be 50 RA 50 CT but we only had 50 RA, and it's costs us 50 RA to make 50 CT in the PSM. so that's why we add 1 to the equation)

2.) Therefore when adding liquidity to the AMM ->

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/libraries/VaultLib.sol#L74

Due to above mentioned reasons there will be some unutilised amount of RA/CT since the amounts were not provided in the actual ratio , and by time these amounts will grow larger .

  1. These are unaccounted since we assume all of the RA/CT got consumed and mint LV ->

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/libraries/VaultLib.sol#L205

The amount of LV minted is the amount of tokens provided to AMM but as discussed not all of the token amounts will be added as liquidity therefore the LV minted will always be a bit higher.

Impact

Not The Entire Token Amounts Provided To AMM Might Get Consumed Leading To Incorrect Accounting Of Ra/Ct and LV Tokens

Code Snippet

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/libraries/VaultLib.sol#L74

Tool used

Manual Review

Recommendation

Only account for the tokens actually added as liquidity in the AMM.

Duplicate of #240

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A Medium severity issue. labels Sep 14, 2024
@sherlock-admin3 sherlock-admin3 changed the title Jumpy Lime Oyster - Not The Entire Token Amounts Provided To AMM Might Get Consumed Leading To Incorrect Accounting Of LV Tokens sakshamguruji - Not The Entire Token Amounts Provided To AMM Might Get Consumed Leading To Incorrect Accounting Of LV Tokens Sep 25, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Sep 25, 2024
@SakshamGuruji3
Copy link

Escalate

This should be a dupe of #240 , not #155

@sherlock-admin3
Copy link

Escalate

This should be a dupe of #240 , not #155

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.

@cvetanovv
Copy link
Collaborator

I agree it is dupe of #240.

@WangSecurity
Copy link

Planning to accept the escalation and duplicate with #240

@WangSecurity
Copy link

Result:
Medium
Duplicate of #240

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Oct 3, 2024
@sherlock-admin3 sherlock-admin3 added the Escalation Resolved This issue's escalations have been approved/rejected label Oct 3, 2024
@sherlock-admin4
Copy link
Contributor

Escalations have been resolved successfully!

Escalation status:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

6 participants