Dopex - kutugu's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L201

Vulnerability details

Impact

Enforced equality checks can always be broken. The attacker can always preemptively transfer 1 wei token to DOS PerpetualAtlanticVault.settle.

Proof of Concept

diff --git a/tests/perp-vault/Integration.t.sol b/tests/perp-vault/Integration.t.sol
index 3bfed98..0632ad8 100644
--- a/tests/perp-vault/Integration.t.sol
+++ b/tests/perp-vault/Integration.t.sol
@@ -129,6 +129,15 @@ contract Integration is ERC721Holder, Setup {
     // Cannot settle if not ITM
 
     priceOracle.updateRdpxPrice(0.010 gwei);
+
+    // DOS
+    address attacker = makeAddr("Attacker");
+    deal(address(weth), attacker, 1 wei);
+    vm.startPrank(attacker);
+    weth.transfer(address(vaultLp), 1 wei);
+
+    vm.startPrank(address(this), address(this));
+    vm.expectRevert("Not enough collateral was sent out");
     (uint256 ethAmount, uint256 rdpxAmount) = vault.settle(tokenIds);
     vm.stopPrank();

Tools Used

Foundry

Recommended Mitigation Steps

Use the inequality sign instead of strict equality

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L669 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L605

Vulnerability details

Impact

rdpxRequiredInWeth / rdpxAmountInWeth calculation should use division instead of multiplication.

Proof of Concept

    uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8;

    uint256 rdpxAmountInWeth = (_rdpxAmount * getRdpxPrice()) / 1e8;

    /**
    * @notice Returns the price of rDPX against ETH
    * @dev    Price is in 1e8 Precision
    * @return rdpxPriceInEth rDPX price in ETH
    **/
    function getRdpxPrice() public view returns (uint256) {
      return
        IRdpxEthOracle(pricingOracleAddresses.rdpxPriceOracle)
          .getRdpxPriceInEth();
    }

getRdpxPrice returns the rDPX/WETH price. Calculating the rDPX quantity corresponding to the WETH quantity should be rDPXAmount * 1e8 / getRdpxPrice() instead of _rdpxAmount * getRdpxPrice() / 1e8. Errors in calculations result in meaningless bondAmount result and disrupt internal accounting systems. During the transfer, the wrong rdpxAmountInWeth will be calculated again.

Tools Used

Manual review

Recommended Mitigation Steps

Use the correct calculation formula

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L228-L237

Vulnerability details

Impact

ReLPContract.reLP uses the state of the uniswap pool instead of internal state when calculating lpToRemove, which is wrong. Users may directly inject funds into the pool to make the market, or malicious users may frontrun to inject funds, which may lead to reLP DOS.

Proof of Concept

    (uint256 reserveA, uint256 reserveB) = UniswapV2Library.getReserves(
      addresses.ammFactory,
      tokenASorted,
      tokenBSorted
    );

    // get tokenA reserves
    tokenAInfo.tokenAReserve = IRdpxReserve(addresses.tokenAReserve)
      .rdpxReserve(); // rdpx reserves

    tokenAInfo.tokenALpReserve = addresses.tokenA == tokenASorted
      ? reserveA
      : reserveB;

    uint256 baseReLpRatio = (reLPFactor *
      Math.sqrt(tokenAInfo.tokenAReserve) *
      1e2) / (Math.sqrt(1e18)); // 1e6 precision

    uint256 tokenAToRemove = ((((_amount * 4) * 1e18) /
      tokenAInfo.tokenAReserve) *
      tokenAInfo.tokenALpReserve *
      baseReLpRatio) / (1e18 * DEFAULT_PRECISION * 1e2);

    uint256 totalLpSupply = IUniswapV2Pair(addresses.pair).totalSupply();

    uint256 lpToRemove = (tokenAToRemove * totalLpSupply) /
      tokenAInfo.tokenALpReserve;

Analyzing from a mathematical formula, it is assumed that users directly add and double the liquidity to the pool. The reserveA and reserveB (tokenALpReserve) will be doubled, tokenAReserve remains unchanged,baseReLpRatio remains unchanged, tokenAToRemove will be doubled, and totalLpSupply will be doubled, so lpToRemove will be doubled. That is, if the user directly injects funds into the pool, the calculated lpToRemove will become larger. When lpToRemove is greater than the lp actually owned by amo, the reLP execution will revert. Below is the POC used for testing:

diff --git a/tests/rdpxV2-core/Periphery.t.sol b/tests/rdpxV2-core/Periphery.t.sol
index aed7de4..ff2e285 100644
--- a/tests/rdpxV2-core/Periphery.t.sol
+++ b/tests/rdpxV2-core/Periphery.t.sol
@@ -7,8 +7,8 @@ import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721H
 import { Setup } from "./Setup.t.sol";
 
 // Contracts
-import { UniV2LiquidityAMO } from "contracts/amo/UniV2LiquidityAMO.sol";
-import { UniV3LiquidityAMO } from "contracts/amo/UniV3LiquidityAMO.sol";
+import { IUniswapV2Router, UniV2LiquidityAMO } from "contracts/amo/UniV2LiquidityAmo.sol";
+import { UniV3LiquidityAMO } from "contracts/amo/UniV3LiquidityAmo.sol";
 
 // Interfaces
 import { IUniswapV3Factory } from "contracts/uniswap_V3/IUniswapV3Factory.sol";
@@ -49,6 +49,19 @@ contract Periphery is ERC721Holder, Setup {
 
     rdpxV2Core.setIsreLP(true);
 
+    // @audit 1. Users are optimistic about the trading volume of this pool, and directly inject funds into the pool for market
+    // @audit 2. Malicious users intentionally inject funds and DOS bond
+    // Current pool state: 1e22 rdpx / 2e21 weth
+    address alice = makeAddr("Alice");
+    vm.startPrank(alice);
+    deal(address(rdpx), alice, 1e22);
+    deal(address(weth), alice, 2e21);
+    rdpx.approve(address(router), type(uint256).max);
+    weth.approve(address(router), type(uint256).max);
+    IUniswapV2Router(router).addLiquidity(address(rdpx), address(weth), 1e22, 2e21, 0, 0, alice, block.timestamp);
+    vm.stopPrank();
+
+    vm.expectRevert();
     rdpxV2Core.bond(1 * 1e18, 0, address(this));
     uint256 lpBalance2 = pair.balanceOf(address(uniV2LiquidityAMO));
     uint256 rdpxBalance2 = rdpx.balanceOf(address(rdpxV2Core));
@@ -56,10 +69,10 @@ contract Periphery is ERC721Holder, Setup {
     uint256 reLpRdpxBalance = rdpx.balanceOf(address(reLpContract));
     uint256 reLpLpBalance = pair.balanceOf(address(reLpContract));
 
-    assertEq(lpBalance2, 1422170988183415261);
-    assertEq(rdpxBalance2, 53886041379169834724);
-    assertEq(reLpRdpxBalance, 0);
-    assertEq(reLpLpBalance, 0);
+    // assertEq(lpBalance2, 1422170988183415261);
+    // assertEq(rdpxBalance2, 53886041379169834724);
+    // assertEq(reLpRdpxBalance, 0);
+    // assertEq(reLpLpBalance, 0);
   }
 
   function testV2Amo() public {

Tools Used

Foundry

Recommended Mitigation Steps

The contract should maintain the state of lp internally instead of reading the pool directly

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L273-L280

Vulnerability details

Impact

ReLPContract.reLP swapExactTokensForTokens slippage caculation error, which allows malicious attackers to steal large amounts of rDPX with a sandwich attack.

Proof of Concept

    // get rdpx price
    tokenAInfo.tokenAPrice = IRdpxEthOracle(addresses.rdpxOracle)
      .getRdpxPriceInEth();

    // @audit The calculation here is correct. Using tokenA to calculate tokenB requires multiplication.
    uint256 mintokenBAmount = ((tokenAToRemove * tokenAInfo.tokenAPrice) /
      1e8) -
      ((tokenAToRemove * tokenAInfo.tokenAPrice) * liquiditySlippageTolerance) /
      1e16;

    // calculate min amount of tokenA to be received
    // @audit The calculation here is wrong. Using tokenB to calculate tokenA requires division.
    mintokenAAmount =
      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];

The specific reasons have been explained in the code comments above. For specific POC we can run known test code:

forge test --match-test testReLpContract -vvvv

You can find obvious error logs in the running results:

    │   │   ├─ [324] MockRdpxEthPriceOracle::getRdpxPriceInEth() [staticcall]
    │   │   │   └─ ← 20000000 [2e7]

    │   │   ├─ [26705] 0x1b02dA8Cb0d097eB8D57A175b88c7D8b47997506::swapExactTokensForTokens(362643377803882543 [3.626e17], 72166032182972626 [7.216e16], [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, 0x2e234DAe75C793f67A35089C9d99245E1C58470b], ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 1662864926 [1.662e9])
    │   │   │   ├─ [517] 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E::getReserves() [staticcall]
    │   │   │   │   └─ ← 0x00000000000000000000000000000000000000000000021df5a4816dc6fc691c00000000000000000000000000000000000000000000006c7a7fb552d0760a4100000000000000000000000000000000000000000000000000000000631d4e14
    │   │   │   ├─ [3801] MockToken::transferFrom(ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E, 362643377803882543 [3.626e17])
    │   │   │   │   ├─ emit Transfer(from: ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], to: 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E, value: 362643377803882543 [3.626e17])
    │   │   │   │   └─ ← true
    │   │   │   ├─ [14017] 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E::swap(1806007704320917659 [1.806e18], 0, ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0x)
    │   │   │   │   ├─ [3315] MockToken::transfer(ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 1806007704320917659 [1.806e18])
    │   │   │   │   │   ├─ emit Transfer(from: 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E, to: ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], value: 1806007704320917659 [1.806e18])
    │   │   │   │   │   └─ ← true
    │   │   │   │   ├─ [583] MockToken::balanceOf(0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E) [staticcall]
    │   │   │   │   │   └─ ← 9995582950916509247617 [9.995e21]
    │   │   │   │   ├─ [583] MockToken::balanceOf(0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E) [staticcall]
    │   │   │   │   │   └─ ← 2001437976500394261104 [2.001e21]
    │   │   │   │   ├─ emit Sync(: 9995582950916509247617 [9.995e21], : 2001437976500394261104 [2.001e21])
    │   │   │   │   ├─ emit Swap(param0: 0x1b02dA8Cb0d097eB8D57A175b88c7D8b47997506, param1: 0, param2: 362643377803882543 [3.626e17], param3: 1806007704320917659 [1.806e18], param4: 0, param5: ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211])
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000005085e3b1225f02f00000000000000000000000000000000000000000000000019103a703daac09b

tokenAPrice = 2e7, amountB / 2 = 3.626e17, swap result = 1.806e18 wrong mintokenAAmount = amountB / 2 * tokenAPrice = 7.216e16 correct mintokenAAmount = amountB / 2 / tokenAPrice = 1.813e18 Maximum capital loss up to 96% due to slippage calculation error

The above are the specific reasons for the vulnerability, and the specific utilization method is a very common sandwich attack:

1. The attacker frontrun to swap rdpx in the pool to push up the price of rdpx. Due to slippage errors, the price can be pushed up to a high level
2. Call reLP to get very little rdpx, and swap will continue to push up the price of rdpx
3. Attacker backrun to swap weth, get a lot of weth

Tools Used

Manual review

Recommended Mitigation Steps

Use correct slippage calculations

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L286-L295

Vulnerability details

Impact

addLiquidity will not use up all funds, and the remaining funds will remain in the contract and cannot be withdrawn.

Proof of Concept

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,
      address(this),
      block.timestamp + 10
    );

    // transfer the lp to the amo
    IERC20WithBurn(addresses.pair).safeTransfer(addresses.amo, lp);
    IUniV2LiquidityAmo(addresses.amo).sync();

    // transfer rdpx to rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
    );
    IRdpxV2Core(addresses.rdpxV2Core).sync();

ReLPContract will transfer the remaining tokenA to rdpxV2Core, but the remaining tokenB will remain in the contract. The contract does not provide a withdraw method, and the tokenB used for the next reLP comes from removeLiquidity, which means that the remaining tokenB of addLiquidity can never be used. Specific POC can run tests:

forge test --match-test testReLpContract -vvvv

    │   │   ├─ [67389] 0x1b02dA8Cb0d097eB8D57A175b88c7D8b47997506::addLiquidity(MockToken: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1806007704320917659 [1.806e18], 362643377803882543 [3.626e17], 0, 0, ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 1662864926 [1.662e9])
    │   │   │   ├─ [644] 0xc35DADB65012eC5796536bD9864eD8773aBc74C4::getPair(MockToken: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
    │   │   │   │   └─ ← 0x000000000000000000000000ba3647555a53e16eb88ae9e7e9cfce034c96277e
    │   │   │   ├─ [517] 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E::getReserves() [staticcall]
    │   │   │   │   └─ ← 0x00000000000000000000000000000000000000000000021ddc9446fd8951a88100000000000000000000000000000000000000000000006c7f88138de29bfa7000000000000000000000000000000000000000000000000000000000631d4e14
    │   │   │   ├─ [3801] MockToken::transferFrom(ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E, 1806007704320917659 [1.806e18])
    │   │   │   │   ├─ emit Transfer(from: ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], to: 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E, value: 1806007704320917659 [1.806e18])
    │   │   │   │   └─ ← true
    │   │   │   ├─ [3801] MockToken::transferFrom(ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E, 361620970285555063 [3.616e17])
    │   │   │   │   ├─ emit Transfer(from: ReLPContract: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211], to: 0xBA3647555A53E16eb88AE9E7e9cfCe034C96277E, value: 361620970285555063 [3.616e17])
    │   │   │   │   └─ ← true

The addLiquidity inputs 1.806e18 tokenA and 3.626e17 tokenB and uses 1.806e18 tokenA and 3.616e17 tokenB, left 0.01 tokenB, which means that 100 calls will result in a loss of 1 ether WETH(tokenB).

Tools Used

Manual review

Recommended Mitigation Steps

1. Like other contracts, provide a method to withdraw funds
2. reLP should use all tokenB of this contract, not just those generated by removeLiquidity

Assessed type

Context