KupiaSec - The `VaultLib.__liquidateUnchecked()` function unnecessarily reorders the already correctly ordered values of `raReceived` and `ctReceived` from `UniswapV2Router`

[sherlock-admin2]

KupiaSec

High

The VaultLib.__liquidateUnchecked() function unnecessarily reorders the already correctly ordered values of raReceived and ctReceived from UniswapV2Router

Summary

The __liquidateUnchecked() function removes liquidity and receives the corresponding RA and CT from UniswapV2, returning the amounts of RA and CT received. When removing liquidity, the removeLiquidity() function of the UniswapV2Router returns raReceived and ctReceived, which are the exact amounts of RA and CT corresponding to the removed liquidity, regardless of the order of tokens RA and CT in the Uniswap pool. However, in the __liquidateUnchecked() function, these values are reordered as if they represent the received amounts of token0 and token1, assuming that token0 = CT and token1 = RA.

Vulnerability Detail

As noted in line 282 of the __liquidateUnchecked() function, the ammRouter.removeLiquidity() function returns two values: raReceived and ctReceived. These values represent the exact amounts of RA and CT received from the Uniswap pool, even in the case where token0 = CT and token1 = RA (see UniswapV2Router02.sol). Therefore, there is no need to reorder these values. However, at line 284, the function reorders them, assuming token0 = CT and token1 = RA. As a result, when token0 = CT and token1 = RA, the values of raReceived and ctReceived will be interchanged. This leads to incorrect behavior in the _liquidatedLp() function, which processes expired states when issuing new DS. Consequently, an incorrect RA amount is reserved in the reserve pool, potentially leading to a loss of RA for users.

VaultLib.sol

    function __liquidateUnchecked(
        State storage self,
        address raAddress,
        address ctAddress,
        IUniswapV2Router02 ammRouter,
        IUniswapV2Pair ammPair,
        uint256 lp
    ) internal returns (uint256 raReceived, uint256 ctReceived) {
        ammPair.approve(address(ammRouter), lp);

        // amountAMin & amountBMin = 0 for 100% tolerence
        (raReceived, ctReceived) =
282         ammRouter.removeLiquidity(raAddress, ctAddress, lp, 0, 0, address(this), block.timestamp);

284     (raReceived, ctReceived) = MinimalUniswapV2Library.reverseSortWithAmount224(
            ammPair.token0(), ammPair.token1(), raAddress, ctAddress, raReceived, ctReceived
        );

        self.vault.config.lpBalance -= lp;
    }

----------------------------

UniswapV2Router02.sol

    function removeLiquidity(
        address tokenA,
        address tokenB,
        uint liquidity,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline
    ) public virtual override ensure(deadline) returns (uint amountA, uint amountB) {
        address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
        IUniswapV2Pair(pair).transferFrom(msg.sender, pair, liquidity); // send liquidity to pair
        (uint amount0, uint amount1) = IUniswapV2Pair(pair).burn(to);
        (address token0,) = UniswapV2Library.sortTokens(tokenA, tokenB);
@>      (amountA, amountB) = tokenA == token0 ? (amount0, amount1) : (amount1, amount0);
        require(amountA >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT');
        require(amountB >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
    }

Impact

Loss of RA for users.

Code Snippet

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

https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L116

Tool used

Manual Review

Recommendation

Eliminate the reordering.

    function __liquidateUnchecked(
        State storage self,
        address raAddress,
        address ctAddress,
        IUniswapV2Router02 ammRouter,
        IUniswapV2Pair ammPair,
        uint256 lp
    ) internal returns (uint256 raReceived, uint256 ctReceived) {
        ammPair.approve(address(ammRouter), lp);

        // amountAMin & amountBMin = 0 for 100% tolerence
        (raReceived, ctReceived) =
            ammRouter.removeLiquidity(raAddress, ctAddress, lp, 0, 0, address(this), block.timestamp);

-       (raReceived, ctReceived) = MinimalUniswapV2Library.reverseSortWithAmount224(
-           ammPair.token0(), ammPair.token1(), raAddress, ctAddress, raReceived, ctReceived
-       );

        self.vault.config.lpBalance -= lp;
    }

[ziankork]

yes this is valid and have been fixed prior to our trading competition. will provide links later

[sherlock-admin4]

Escalate, This is gas optimization issue at best. The reverseSortWithAmount224() function has the following implementation:

    function reverseSortWithAmount224(
        address token0,
        address token1,
        address ra,
        address ct,
        uint256 token0Amount,
        uint256 token1Amount
    ) internal pure returns (uint256 raAmountOut, uint256 ctAmountOut) {
        if (token0 == ra && token1 == ct) {
            raAmountOut = token0Amount;
            ctAmountOut = token1Amount;
        } else if (token0 == ct && token1 == ra) {
            raAmountOut = token1Amount;
            ctAmountOut = token0Amount;
        } else {
            revert InvalidToken();

You've deleted an escalation for this issue.

[KupiaSecAdmin]

@AtanasDimulski, I think you misunderstood something.

The issue is simply due to the unnecessary application of the reverseSortWithAmount224() function.

That function reorders the amounts, which were already well-ordered.

[AtanasDimulski]

@KupiaSecAdmin you are correct, my mistake. Nice catch!

[0xsimao]

escalate

This issue is invalid because it requires the ra address to be bigger than the ct address. Ra and ct are picked by the admin, in which ct results from the create opcode, so the admin has complete control on them. Place where the admin picks ra. Ct is created here, which uses the create opcode, so the admin can create dummy ra,ct pairs just to get a ct address bigger than ra. As such, it should pick values that do not cause issues.

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

[sherlock-admin3]

escalate

This issue is invalid because it requires the ra address to be bigger than the ct address. Ra and ct are picked by the admin, in which ct results from the create opcode, so the admin has complete control on them. Place where the admin picks ra. Ct is created here, which uses the create opcode, so the admin can create dummy ra,ct pairs just to get a ct address bigger than ra. As such, it should pick values that do not cause issues.

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

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.

[KupiaSecAdmin]

@0xsimao

I don't agree with your insistence.

1. ct is not a variable that the admin needs to monitor closely to avoid any issues. If the admin consistently chooses ct to be greater than ra, there is no need to implement mechanisms to sort raAmount and ctAmount.

so the admin can create dummy ra,ct pairs just to get a ct address bigger than ra.

2. Whenever ct is created, a Uniswap V2 pool (ra, ct) is established, which incurs significant costs. Therefore, creating dummy entries until ct > ra is not a feasible action.

3. Additionally, when issuing new (ds, ct), there is an expiration time that prevents immediate issuance of new (ds, ct).

[0xsimao]

@KupiaSecAdmin still, the admin picks the ra and ct address, so according to the rule, it is trusted to pick values that do not cause any issues.

[KupiaSecAdmin]

@0xsimao

I totally disagree with your insistence.

3. Additionally, when issuing new (ds, ct), there is an expiration time that prevents immediate issuance of new (ds, ct).

When issuing a new (ds, ct), the previous (ds, ct) must be expired. Therefore, once a new (ds, ct) is issued, it cannot be immediately canceled, even if ct < ra. How can the admin pick the ct address?

This is a clear bug, and the sponsor has also confirmed it.

So, I believe the judge will make the correct decision.

[0xsimao]

It does not have to cancel it, it can create dummy ra, ct pairs. ct is created via the create() opcode, so we can always find ct addresses bigger than ra. The address used for the ra in the dummy can be another, just to increase the creation nonce of the contract and change the ct address.

Additionally, the admin can order the issuance of ra, ct pairs according to their addresses. If we have ra1 = 5, ra2 = 3, ct1 = 4, ct2 = 6, the admin can do pairs ra2,ct1 and ra1,ct2. If the ra address is bigger than the next ct, and the admin really wants to use this ra, he can create other pairs to increase the ct address and then create the pair with the correct ct.

Another option for the admin is to create a wrapper for ra that has a smaller address, if needed.

I am not saying the bug does not exist, it does, but it is invalid according to the sherlock rules, as it is picked solely by the admin and the admin is trusted to use values that do not cause issues.

[sherlock-admin2]

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

[KupiaSecAdmin]

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

@0xsimao,

Your assertions do not align with the Sherlock rule you mentioned.

1. The method by which the admin controls CT, as mentioned in your earlier comment, is not intended and appears to be an overestimation of the protocol design to force it to align with the Sherlock rule.

It does not have to cancel it, it can create dummy ra, ct pairs. ct is created via the create() opcode, so we can always find ct addresses bigger than ra. The address used for the ra in the dummy can be another, just to increase the creation nonce of the contract and change the ct address.

Additionally, the admin can order the issuance of ra, ct pairs according to their addresses. If we have ra1 = 5, ra2 = 3, ct1 = 4, ct2 = 6, the admin can do pairs ra2,ct1 and ra1,ct2. If the ra address is bigger than the next ct, and the admin really wants to use this ra, he can create other pairs to increase the ct address and then create the pair with the correct ct.

Another option for the admin is to create a wrapper for ra that has a smaller address, if needed.

I am not saying the bug does not exist, it does, but it is invalid according to the sherlock rules, as it is picked solely by the admin and the admin is trusted to use values that do not cause issues.

Your method does not focus on controlling specific function variables; rather, it centers on managing the create opcode nonce by modifying the protocol design. Therefore, it does not align with the Sherlock rule.

2. The variable values of the issueNewDs() function are determined by the protocol strategy, not to prevent CT < RA.

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

I believe you were pointing to the issueNewDs() function as access restricted. This function has the following parameters:

    function issueNewDs(Id id, uint256 expiry, uint256 exchangeRates, uint256 repurchaseFeePrecentage)

These parameters do not have arbitrary values; each has a specific meaning. Therefore, the admin does not set these values to generate a CT greater than RA. Consequently, the Sherlock rule you mentioned does not apply to my issue.

[0xsimao]

The admin is trusted to use the right ra and ct values, and they are fully in their control. There are ra and ct values that the admin can use that do not cause any issues. Hence, the rule applies.

The id in the function points to a specific ra, so it can choose the ra value. The ct value comes from the nonce, as explained earlier, and can also be picked to not cause any issues.

[KupiaSecAdmin]

The admin is trusted to use the right ra and ct values, and they are fully in their control. There are ra and ct values that the admin can use that do not cause any issues. Hence, the rule applies.

CT is not under the admin's control; it is clearly an internal address that the admin disregards.

The id in the function points to a specific ra, so it can choose the ra value. The ct value comes from the nonce, as explained earlier, and can also be picked to not cause any issues.

Id denotes (ra, pa). Yes, we are concentrating on a specific fixed Id.

For a given Id, your method of controlling the generated CT is not feasible. It could only be possible if the admin creates a fake Id to increase the nonce. This approach does not involve controlling function variables; rather, it stems from an overestimation of the protocol design.

Therefore, the rule does not apply. I believe there may have been a misunderstanding.

Once again, I trust that the judge will make the right decision.

[0xsimao]

CT is not under the admin's control; it is clearly an internal address that the admin disregards. For a given Id, your method of controlling the generated CT is not feasible. It could only be possible if the admin creates a fake Id to increase the nonce. This approach does not involve controlling function variables; rather, it stems from an overestimation of the protocol design.

The admin would disregard it if it was not trusted. But he is trusted, so he should do the correct thing and use proper ra and ct values, which is possible, as mentioned before.

As per the argument that the admin should not create dummy ct addresses to get one higher than ra and prevent this issue, this is the reason #180 was invalidated, as the admin could do this to create another pool. So here the admin must also be able to use a proper ct and mitigate this issue.