The balances of assets in RdpxV2Core are not being updated when adding or removing liquidity using UniV2LiquidityAMO

[code423n4]

Lines of code

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

Vulnerability details

Impact

The UniV2LiquidityAMO provides three functions, addLiquidity, removeLiquidity and swap, which the admin can use to add and remove liquidity from Uniswap, or swap assets. The addLiquidity and swap requires assets from *RdpxV2Core, and the assets obtained from removeLiquidity and swap are stored in RdpxV2Core. However, UniV2LiquidityAMO does not update the data of ReserveAssets in RdpxV2Core, which can lead to inconsistencies between the data recorded in ReserveAsset and the actual data of the stored assets.

  function _sendTokensToRdpxV2Core() internal {
    uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf(
      address(this)
    );
    uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf(
      address(this)
    );
    // transfer token A and B from this contract to the rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      tokenABalance
    );
    IERC20WithBurn(addresses.tokenB).safeTransfer(
      addresses.rdpxV2Core,
      tokenBBalance
    );

    emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);
  }

Proof of Concept

In the following test code, there are 50 rDPX and 11 WETH in RdpxV2Core. The addLiquidity function is used to add liquidity to Uniswap by adding 5 rDPX and 1 WETH. After adding liquidity, the balances of rDPX and WETH in RdpxV2Core will decrease.

  function testV2Amo() 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);

    rdpxV2Core.sync();
    (, uint256 rdpxToken1,) = rdpxV2Core.getReserveTokenInfo("RDPX");
    (, uint256 wethToken1,) = rdpxV2Core.getReserveTokenInfo("WETH");

    // add liquidity
    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);

    (, uint256 rdpxToken2,) = rdpxV2Core.getReserveTokenInfo("RDPX");
    (, uint256 wethToken2,) = rdpxV2Core.getReserveTokenInfo("WETH");

    assertLt(rdpxToken2, rdpxToken1);
    assertLt(wethToken2, wethToken1);
  }

The test results indicate that after calling addLiquidity to add liquidity to Uniswap, the balances of rDPX and WETH recorded in the ReserveAsset of RdpxV2Core did not decrease.

[nian@localhost 2023-08-dopex]$ forge  test --match-test testV2Amo  -vv
[⠆] Compiling...
[⠘] Compiling 2 files with 0.8.19
[⠊] Solc 0.8.19 finished in 6.49s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Assertion failed.] testV2Amo() (gas: 2333888)
Logs:
  Error: a < b not satisfied [uint]
    Value a: 50000000000000000000
    Value b: 50000000000000000000
  Error: a < b not satisfied [uint]
    Value a: 11000000000000000000
    Value b: 11000000000000000000

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.59s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Assertion failed.] testV2Amo() (gas: 2333888)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

In the function _sendTokensToRdpxV2Core(), call the sync() function of RdpxV2Core to update the data in ReserveAsset.

diff --git a/contracts/amo/UniV2LiquidityAmo.sol.orig b/contracts/amo/UniV2LiquidityAmo.sol
index ce92d43..dc8dad2 100644
--- a/contracts/amo/UniV2LiquidityAmo.sol.orig
+++ b/contracts/amo/UniV2LiquidityAmo.sol
@@ -149,6 +149,8 @@ contract UniV2LiquidityAMO is AccessControl {
       token.safeTransfer(msg.sender, token.balanceOf(address(this)));
     }
 
+    IRdpxV2Core(addresses.rdpxV2Core).sync();
+
     emit LogEmergencyWithdraw(msg.sender, tokens);
   }
 

Performing the test again, you should observe that after adding liquidity to Uniswap, the assets balances recorded in ReserveAsset are reduced.

[nian@localhost 2023-08-dopex]$ forge  test --match-test testV2Amo  -vv
[⠢] Compiling...
[⠒] Compiling 2 files with 0.8.19
[⠑] Solc 0.8.19 finished in 7.23s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[PASS] testV2Amo() (gas: 2339509)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.54s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as duplicate of #798

[c4-pre-sort]

bytes032 marked the issue as duplicate of #269

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[GalloDaSballo]

Description is slightly better and POCs are equivalent

[c4-judge]

GalloDaSballo marked the issue as selected for report

[c4-judge]

GalloDaSballo marked issue #269 as primary and marked this issue as a duplicate of 269

[c4-judge]

GalloDaSballo marked the issue as satisfactory