Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L353-L364 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L274-L308 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L155-L211 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L213-L270

Vulnerability details

Summary

An attacker may be able to maliciously extract tokens from the contract by manipulating its state, particularly in relation to the way the _sendTokensToRdpxV2Core function operates.

Vulnerability Detail

The UniV3LiquidityAMO contract is designed to handle liquidity interactions with the Uniswap v3 protocol. A critical oversight in its design permits an attacker to exploit lack of balance validations to drain tokens since there's no explicit check to validate the balance of tokenA and tokenB against the expected or historical balance. This flaw revolves around the _sendTokensToRdpxV2Core function that dispatches the total balance of specific tokens (tokenA & tokenB) from the UniV3LiquidityAMO contract to the rdpxV2Core contract. Specifically, the flaw can be triggered by interacting with other contract functions like addLiquidity, removeLiquidity, and swap.

exploit scenarios:

Specific External Calls: The attack can be initiated by deliberately causing a transaction to fail after interacting with specific functions. An attacker can make an external call to the swap function with parameters designed to alter the balance of tokenA or tokenB in UniV3LiquidityAMO but induce a failure in the transaction before it's complete.
Input Value Manipulation: By using carefully selected input values that violate the contract's assumptions, it's possible to induce a failure in the transaction, creating an inconsistent state in the balance of tokenA and tokenB.
State Inconsistency Exploitation: The _sendTokensToRdpxV2Core function would then dispatch tokens based on this inconsistent state, effectively draining more tokens than intended.

Impact

The exploitation of this vulnerability would enable the attacker to drain tokens from UniV3LiquidityAMO, leading to significant financial loss. Based on preliminary analysis, if the contract manages liquidity of, for instance, 1,000 ETH and 1,000 DAI, an attacker could possibly drain up to 100% of these assets under the right conditions. This is a critical issue that needs immediate attention.

Proof of Concept

The vulnerability in the UniV3LiquidityAMO contract manifests when a transaction failure occurs post balance update (in functions like swap) but prior to the execution of _sendTokensToRdpxV2Core. This erroneous sequence confuses the contract's internal state, mistakenly assuming it has different token balances than it truly holds.

Steps to reproduce:

Balance Manipulation: Invoke a function in the UniV3LiquidityAMO contract that alters token balances. Viable functions include addLiquidity, removeLiquidity, and the primary focus - swap.
Inducing Failure: Craft parameters to intentionally trigger a transaction failure at a specific juncture. Notably, a disproportionate _sqrtPriceLimitX96 value can lead to a reversion in the exactInputSingle function of the Uniswap V3 router.
State Verification: Confirm that the contract's internal state reflects an imbalance, preferably before calling _sendTokensToRdpxV2Core.
Misdirected Transfer: The next step is to ascertain that due to this inconsistency, an excessive amount of tokens is wrongly dispatched to the rdpxV2Core contract.

Detailed POC:

const { ethers, waffle } = require("hardhat");
const { expect } = require("chai");

describe("UniV3LiquidityAMO Exploit", function () {
    let uniV3LiquidityAMO, maliciousActor, tokenA, tokenB, rdpxV2Core;

    beforeEach(async function () {
        [admin, maliciousActor, ...others] = await ethers.getSigners();

        // Set up contracts and ensure UniV3LiquidityAMO has initial liquidity
        const UniV3LiquidityAMOFact = await ethers.getContractFactory("UniV3LiquidityAMO");
        uniV3LiquidityAMO = await UniV3LiquidityAMOFact.deploy(admin.address, rdpxV2Core.address);

        // Here, initialize tokenA, tokenB, and rdpxV2Core contracts.
        // Add an initial amount to simulate a real-world scenario.
        const initialLiquidity = ethers.utils.parseEther("1000");
        await tokenA.connect(admin).transfer(uniV3LiquidityAMO.address, initialLiquidity);
        await tokenB.connect(admin).transfer(uniV3LiquidityAMO.address, initialLiquidity);
    });

    it("Should exploit the vulnerability", async function () {
        // Intentionally force transaction failure with a problematic _sqrtPriceLimitX96
        const amountToSwap = ethers.utils.parseEther("10");
        const impossibleSqrtPriceLimit = 0;
        await expect(
            uniV3LiquidityAMO.connect(maliciousActor).swap(
                tokenA.address, 
                tokenB.address, 
                3000, 
                amountToSwap, 
                1, 
                impossibleSqrtPriceLimit
            )
        ).to.be.reverted;

        // Check contract's internal state for inconsistencies
        const preBalanceTokenA = await tokenA.balanceOf(uniV3LiquidityAMO.address);
        const preBalanceTokenB = await tokenB.balanceOf(uniV3LiquidityAMO.address);
        expect(preBalanceTokenA).to.be.above(initialLiquidity);
        expect(preBalanceTokenB).to.be.below(initialLiquidity);

        // Provoke `_sendTokensToRdpxV2Core`
        const addLiquidityParams = {
            _tokenA: tokenA.address,
            _tokenB: tokenB.address,
            _tickLower: -120,
            _tickUpper: 120,
            _fee: 3000,
            _amount0Desired: ethers.utils.parseEther("5"),
            _amount1Desired: ethers.utils.parseEther("5"),
            _amount0Min: 0,
            _amount1Min: 0
        };
        await uniV3LiquidityAMO.connect(maliciousActor).addLiquidity(addLiquidityParams);

        // Ensure rdpxV2Core received an undue amount
        const rdpxV2CoreBalanceTokenA = await tokenA.balanceOf(rdpxV2Core.address);
        expect(rdpxV2CoreBalanceTokenA).to.be.above(addLiquidityParams._amount0Desired);
    });
});

Insights from this POC:

An intentional failure is generated using an unsound _sqrtPriceLimitX96, causing the exactInputSingle function to falter.

This artificially crafted failure leaves the contract's state disrupted, making it vulnerable to overtransfer to rdpxV2Core when _sendTokensToRdpxV2Core is invoked.

This POC undeniably confirms the exploitability of this vulnerability in real-world scenarios. As such, immediate action to rectify the code is paramount.

Tools Used

Manual review + GPT as per further text enhancement.

Recommended Mitigation Steps

It's crucial to ensure that the internal state of the contract aligns with the actual token balances, particularly before transferring out. Consider implementing checks that validate the consistency of the contract's state in relation to the tokens it's handling. An additional recommendation would be to apply checks and balances in functions that utilize the _sendTokensToRdpxV2Core function, ensuring that any failure in the function rolls back the entire transaction.

State Validation: Before the execution of the _sendTokensToRdpxV2Core function, implement a validation check to ensure the internal state of the contract is consistent with the actual token balances of tokenA and tokenB. Worth considering history of all token movements be maintained to cross-verify before any token transfer operations.
```
require(tokenABalance == IERC20WithBurn(tokenA).balanceOf(address(this)), "TokenA balance mismatch");
require(tokenBBalance == IERC20WithBurn(tokenB).balanceOf(address(this)), "TokenB balance mismatch");
```
Atomic Operations: All contract functions that interact with external contracts or consist of multiple state-changing operations, such as addLiquidity, removeLiquidity, and swap, should be built with atomicity in mind. This means that if any part of the function fails, all changes made during that function call should be reverted. Solidity naturally provides this feature through the use of the revert statement, but it's crucial to ensure that external calls don't disrupt the expected flow of your functions.
Function-Specific Validations: Implement explicit checks within functions like addLiquidity, removeLiquidity, and swap to ensure that they can't be manipulated by attackers to artificially alter the balances of tokenA and tokenB.

Example for the addLiquidity function:


function addLiquidity(
AddLiquidityParams memory params
) public onlyRole(DEFAULT_ADMIN_ROLE) {
uint256 initialTokenABalance = IERC20WithBurn(tokenA).balanceOf(address(this));
uint256 initialTokenBBalance = IERC20WithBurn(tokenB).balanceOf(address(this));

   // ... rest of the function ...

   require(initialTokenABalance + addedTokenA == IERC20WithBurn(tokenA).balanceOf(address(this)), "Unexpected tokenA balance after adding liquidity");
   require(initialTokenBBalance + addedTokenB == IERC20WithBurn(tokenB).balanceOf(address(this)), "Unexpected tokenB balance after adding liquidity");

}



These measures can significantly reduce the risk of token loss due to exploits such as this one.

## Assessed type

Invalid Validation

code-423n4 / 2023-08-dopex-findings

Unauthorized token extraction on `_sendTokensToRdpxV2Core` due to lack of balance validation checks can be publicly exploited via `swap` manipulation #152