sherlock-admin4 commented 1 month ago

vinica_boy

High

Wrong accounting of locked RA when repurchasing DS+PA with RA

Summary

Users have the option to repurchase DS + PA by providing RA to the PSM. A portion of the RA provided is taken as a fee, and this fee is used to mint CT + DS for providing liquidity to the AMM pair.

Vulnerability Detail

NOTE: Currently PsmLib.sol incorrectly uses lockUnchecked() for the amount of RA provided by the user. As discussed with sponsor it should be lockFrom() in order to account for the RA provided.

After initially locking the RA provided, part of this amount is used to provide liquidity to the AMM via VaultLibrary.provideLiquidityWithFee()

function repurchase(
        State storage self,
        address buyer,
        uint256 amount,
        IDsFlashSwapCore flashSwapRouter,
        IUniswapV2Router02 ammRouter
    ) internal returns (uint256 dsId, uint256 received, uint256 feePrecentage, uint256 fee, uint256 exchangeRates) {
        DepegSwap storage ds;

        (dsId, received, feePrecentage, fee, exchangeRates, ds) = previewRepurchase(self, amount);

        // decrease PSM balance
        // we also include the fee here to separate the accumulated fee from the repurchase
        self.psm.balances.paBalance -= (received);
        self.psm.balances.dsBalance -= (received);

        // transfer user RA to the PSM/LV
        // @audit-issue shouldnt it be lock checked, deposit -> redeemWithDs -> repurchase - locked would be 0
        self.psm.balances.ra.lockUnchecked(amount, buyer);

        // transfer user attrubuted DS + PA
        // PA
        (, address pa) = self.info.underlyingAsset();
        IERC20(pa).safeTransfer(buyer, received);

        // DS
        IERC20(ds._address).transfer(buyer, received);

        // Provide liquidity
        VaultLibrary.provideLiquidityWithFee(self, fee, flashSwapRouter, ammRouter);
    }

provideLiqudityWithFee internally uses __provideLiquidityWithRatio() which calculates the amount of RA that should be used to mint CT+DS in order to be able to provide liquidity.

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);
    }

__provideLiquidity() uses PsmLibrary.unsafeIssueToLv() to account the RA locked.

function __provideLiquidity(
        State storage self,
        uint256 raAmount,
        uint256 ctAmount,
        IDsFlashSwapCore flashSwapRouter,
        address ctAddress,
        IUniswapV2Router02 ammRouter,
        uint256 dsId
    ) internal {
        // no need to provide liquidity if the amount is 0
        if (raAmount == 0 && ctAmount == 0) {
            return;
        }

        PsmLibrary.unsafeIssueToLv(self, ctAmount);

        __addLiquidityToAmmUnchecked(self, raAmount, ctAmount, self.info.redemptionAsset(), ctAddress, ammRouter);

        _addFlashSwapReserve(self, flashSwapRouter, self.ds[dsId], ctAmount);
    }

RA locked is incremented with the amount of CT tokens minted.

function unsafeIssueToLv(State storage self, uint256 amount) internal {
        uint256 dsId = self.globalAssetIdx;

        DepegSwap storage ds = self.ds[dsId];

        self.psm.balances.ra.incLocked(amount);

        ds.issue(address(this), amount);
    }

Consider the following scenario: For simplicity the minted values in the example may not be accurate, but the idea is to show the wrong accounting of locked RA.

PSM has 1000 RA locked.
Alice repurchase 100 DS+PA with providing 100 RA and we have fee=5% making the fee = 5
PSM will have 1100 RA locked and ra.locked would be 1100 also.
In __provideLiquidity() let say 3 of those 5 RA are used to mint CT+DS.
PsmLibrary.unsafeIssueToLv() would add 3 RA to the locked amount, making the psm.balances.ra.locked = 1103 while the real balance would still be 1100.

Impact

Wrong accounting of locked RA would lead to over-distribution of rewards for users + after time last users to redeem might not be able to redeem as there wont be enough RA in the contract due to previous over-distribution. This breaks a core functionality of the protocol and the likelihood of this happening is very high, making the overall severity High.

Code Snippet

PsmLib.repurchase(): https://github.com/sherlock-audit/2024-08-cork-protocol/blob/db23bf67e45781b00ee6de5f6f23e621af16bd7e/Depeg-swap/contracts/libraries/PsmLib.sol#L293

Tool used

Manual Review

Recommendation

Consider either not accounting for the fee used to mint CT+DS as it is already accounted or do not initially account for the fee when users provide RA.

ziankork commented 1 month ago

This is a valid issue, will fix by not accounting the fee when users provide RA

sherlock-admin2 commented 1 month ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/Cork-Technology/Depeg-swap/pull/77

spdimov commented 1 month ago

All the selected duplicates describe different issues:

57, #153, #179 describe the issue in the redeem RA with DS case

171 describes the issue regarding return amounts when providing liquidity to the AMM

219 is a duplicate of #119

40 is describing something completely different

cvetanovv commented 1 month ago

Escalate above comment

sherlock-admin2 commented 1 month ago

Escalate above comment

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.

0xsimao commented 1 month ago

57, #153, #179 are valid dups. They all mention the fee used to provide liquidity is not decreased from the balance of the psm.

171 is a dup of #240.

219 is a dup of #119.

40 is invalid, explanation is in #40 itself.

spdimov commented 1 month ago

I will agree with the previous comment. #57, #153, #179 really describe the same root cause issue and based on other groupings of issues in this contest, maybe they have to be counted as one with the issue described here. I thought that when two similar issues happen in different places in the code, they should be separated as fixing one would not fix the other.

AtanasDimulski commented 1 month ago

I know this is not part of the original escalation, but since the issue is escalated, I believe it should be of high severity, as it essentially results in the first users that withdraw stealing funds from the last users that withdraw.

WangSecurity commented 1 month ago

I agree with the duplication explained in this comment and plan to apply it. About the severity, as I understand, there are indeed no limitations on this issue and a couple of last users could end up with no rewards at all. Hence, planning to accept the escalation, apply the duplication explained here and increase the severity to high.